Inconsistencies with inventories that have result slots that were created from different sources

[fhnaumann]

Expected behavior

No matter how an inventory is created, it should behave identical regarding methods like isEmpty. For example, setting the result slot in an anvil should always cause isEmpty on that inventory to return false.

Observed/Actual behavior

Depending on how the inventory was created, these methods behave differently. Inventories that were created using open... (e.g. openAnvil) create an instance of CraftInventoryAnvil. This class (and every other subclass of CraftResultInventory) do not override getContents or getStorageContents and therefore isEmpty in CraftInventory do not work properly. Inventories that were created using createInventory always create inventories of type CraftCustomInventory which holds a single container for all slots in the inventory and does not hold a container specifically for the result. Therefore isEmpty checks all slots, including the result slot.

Steps/models to reproduce

Player player = Bukkit.getOnlinePlayers().stream().findAny().get();
Inventory anvilInv = Bukkit.createInventory(null, InventoryType.ANVIL);
anvilInv.setItem(2, new ItemStack(Material.STONE));
player.openInventory(anvilInv);


Bukkit.getScheduler().runTaskLater(this, () -> {
    System.out.println(anvilInv.isEmpty()); // false
    System.out.println(player.getOpenInventory().getTopInventory().isEmpty()); // false
    player.closeInventory();

    InventoryView view = player.openAnvil(player.getLocation(), true);
    view.getTopInventory().setItem(2, new ItemStack(Material.STONE));

    Bukkit.getScheduler().runTaskLater(this, () -> {
        System.out.println(view.getTopInventory().isEmpty()); // true
        System.out.println(player.getOpenInventory().getTopInventory().isEmpty()); // true
    }, 2L);
}, 2L);

Plugin and Datapack List

None

Paper version

This server is running Paper version git-Paper-496 (MC: 1.20.4) (Implementing API version 1.20.4-R0.1-SNAPSHOT) (Git: 7ac24a1 on ver/1.20.4)
You are running the latest version
Previous version: git-Paper-398 (MC: 1.20.4)

Other

No response

[fhnaumann]

This is probably also related to #10695

[Machine-Maker]

I don't think it relates to that issue. This issue is more valid in my eyes. I explained in the other issue why I think the behavior is works-as-intended. But the javadocs for isEmpty specifically say "any slot of this inventory".

[fhnaumann]

Both issues fundamentally use getContents. isEmpty was just an example highlighted here. getContents returns only the 2 slots in the code above. Fixing this would automatically change the behaviour of the all method for example as well to include the entire inventory

[Machine-Maker]

I don't think we should change what getStorageContents returns for these "result" inventories. I think the javadocs of getStorageContents suggest that it shouldn't include result slots which often have special restrictions on placing items there. We can make getContents return all the slots tho.

None of this should affect how player inventories work to be clear, so the #all method in that case still isn't going to check armor/offhand.

[fhnaumann]

But it already has inconsistencies built within. I made a comment here explaining them. In the current state getStorageContents is not consistent within different inventories. For CraftInventoryCrafting, which is an inventory with a result slot, it is handled accordingly and getStorageContents returns the entire inventory, including the result slot. For CraftFurnace, which has a result slot, getStorageContents returns the entire inventory as well, because despite having a result slot, it has no own result container internally.

Honestly it seems like the overriding of CraftInventory methods in the CraftInventoryCrafting should be moved into the CraftResultInventory class and CraftInventoryCrafting should extend CraftResultInventory. Furthermore, every other inventory with a result slot that does not already extend CraftResultInventory should do so. Because otherwise I'm not really sure how this should be handled on CraftCustomInventory to account for all these special cases.

[electronicboy]

The entire issue here is that the inventory you create with createInventory is a CraftCustomInventory which has 0 understanding or representation of the underlying inventory in use here. Combine that with the fact that the inventory API is generally in a weird state of rot and hot potato, and it's created the situation where the contracts of various parts of the API are somewhat unclear, especially as you can fairly easily spend all day debating on if "it says X" means that you include Y,Z into specific methods

[Machine-Maker]

We can't just make furnance inventories use CraftResultInventory. CraftResultInventory is for an inventory that is made up of 2 nms Containers. Furnaces aren't, its just one container (the BaseContainerBlockEntity itself). We can however introduce logic to exclude it's result slot in the same way that CraftResultInventory does. I think that will provide the most consistency. result slots and player extras won't be included in getStorageContents

[Machine-Maker]

@fhnaumann I'd appreciate if you tried out the server build linked in this PR and see if things function as expected now.

[fhnaumann]

@Machine-Maker This partially fixes the issue. The isEmpty check works as the documentation describes it. However, there are still inconsistencies between inventories created from createInventory and open....

Player player = Bukkit.getOnlinePlayers().stream().findAny().get();
Inventory anvilInv = Bukkit.createInventory(null, InventoryType.ANVIL); // CraftInventoryCustom
anvilInv.setItem(2, new ItemStack(Material.STONE));
player.openInventory(anvilInv);

Bukkit.getScheduler().runTaskLater(this, () -> {
    System.out.println(Arrays.toString(anvilInv.getContents())); // [null, null, stone]
    System.out.println(Arrays.toString(anvilInv.getStorageContents())); // [null, null, stone]
    player.closeInventory();

    InventoryView view = player.openAnvil(player.getLocation(), true); // CraftInventoryAnvil
    view.getTopInventory().setItem(2, new ItemStack(Material.STONE));

    Bukkit.getScheduler().runTaskLater(this, () -> {
        System.out.println(Arrays.toString(view.getTopInventory().getContents())); // [null, null, stone]
        System.out.println(Arrays.toString(view.getTopInventory().getStorageContents())); // [null, null]
    }, 2L);
}, 2L);

The storage contents don't match between these two inventories. As @electronicboy mentioned, I don't know if this is really fixable.

My problem with that is, that I am writing a mock implementation of the inventories for MockBukkit and in the current scenario I'd have to check what type of instance the inventory is (Custom or not) and mimic the behaviour.

I'm not too familiar with the paper internals, but before researching into this I always thought createInventory create instances of the respective classes (e.g. CraftInventoryAnvil). Is there a reason why this isn't the case?

[fhnaumann]

This inconsistency seems to occur for all inventories that have a result slot, but don't have their own converter implementation in CraftInventoryCreator. For example, creating a furnace via createInventory creates an instance of CraftInventoryFurnace because a custom converter is applied that delegates to CraftInventoryFurnace. In this case everything works as expected (because they are from the same class).

Player player = Bukkit.getOnlinePlayers().stream().findAny().get();
Inventory furnaceInv = Bukkit.createInventory(null, InventoryType.FURNACE); // class is CraftInventoryFurnace, NOT CraftInventoryCustom
furnaceInv.setItem(2, new ItemStack(Material.STONE));
player.openInventory(furnaceInv);

Bukkit.getScheduler().runTaskLater(this, () -> {
    System.out.println(Arrays.toString(furnaceInv.getContents())); // [null, null, stone]
    System.out.println(Arrays.toString(furnaceInv.getStorageContents())); // [null, null]
    player.closeInventory();
}

[Machine-Maker]

@fhnaumann yeah, the issue about create... vs open... is really a whole separate thing. I have an open PR that kinda tries to do something about it. It's the same cause that makes anvils not work with create.... I did recently have another idea for a better way to address that issue that I'll try to work on here. It's baby steps to make this inventory mess acceptable. Not gonna all happen in one PR.

[Machine-Maker]

So I did start up on #10732, which is my new idea for handling this. Some basic testing with the 2 inventory types I implemented seemed to suggest they function correctly (aka not randomly deleting items and actually letting you craft in them). With the other changes in the other PR, that should fix both issues (for result inventories). There are still other issues with non-result inventories.

[fhnaumann]

Alright, thanks for the work. I don't think I have enough knowledge with the minecraft internals to help you with that, but I'll eagerly await the changes