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.