-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Inconsistencies with inventories that have result slots that were created from different sources #10720
Comments
This is probably also related to #10695 |
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 |
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 |
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 |
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. |
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 |
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 |
@fhnaumann I'd appreciate if you tried out the server build linked in this PR and see if things function as expected now. |
@Machine-Maker This partially fixes the issue. The 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 |
This inconsistency seems to occur for all inventories that have a result slot, but don't have their own converter implementation in 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();
} |
@fhnaumann yeah, the issue about |
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. |
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 |
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 causeisEmpty
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 ofCraftInventoryAnvil
. This class (and every other subclass ofCraftResultInventory
) do not overridegetContents
orgetStorageContents
and thereforeisEmpty
inCraftInventory
do not work properly. Inventories that were created usingcreateInventory
always create inventories of typeCraftCustomInventory
which holds a single container for all slots in the inventory and does not hold a container specifically for the result. ThereforeisEmpty
checks all slots, including the result slot.Steps/models to reproduce
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
The text was updated successfully, but these errors were encountered: