Hopper does not pull items from modded containers that subclass vanilla containers and override their functions without super calls
Noaaan opened this issue · 16 comments
Instructions
This is very similar to issue #417, albeit in a different nature.
Most notably I do not use Fabric Carpet here.
Hoppers with Mythic Metals Decorations do not work properly when Lithium is installed. The issue only seems to happen on extraction rather than inserting into the containers.
I have tried both using Fabrics Item Transfer API (see https://github.com/Noaaan/MythicMetalsDecorations/blob/333319ba22f1d65bf3caa50e168ce05a8cc4d763/src/main/java/nourl/mythicmetalsdecorations/MythicMetalsDecorations.java#L66).
My containers do support LithiumInventory (via https://github.com/Noaaan/MythicMetalsDecorations/blob/1.19/src/main/java/nourl/mythicmetalsdecorations/mixin/MythicChestBlockEntityLithiumCompatMixin.java#L11), yet the hoppers do not respect the chests.
Version Information
Using lithium-fabric-mc1.19.3-0.10.4 and Mythic Metals Decorations 0.5.1+1.19.3 (unreleased), and is reproducible outside of a development environment.
This issue also happens with lithium-fabric-mc1.19.2-0.10.2 and mythicmetals-decorations 0.5.1+1.19.2 (also unreleased)
Turns out this is not the case, my apologies.
Tested it against the backport of lithium-fabric-mc.1.19.2**-0.10.4** and it does NOT work. This might exclusively be an issue with the later versions of Lithium (>0.10.2).
Expected Behavior
The items from the top hopper should fall through to the bottom hopper
Actual Behavior
The items stop inside the chest.
Reproduction Steps
- Place two hoppers, one above and below the chest.
- Place an item in the top hopper
- Check the bottom hopper for items
Other Information
If the state of the hopper changes (for example by updating the stack, placing a block above/below it), then the hopper will start extracting items again.
Full modlist:
TLDR:
- Fabric API
- Mythic Metals Decorations,
- Lithium
- Smooth Boot
- LazyDFU
- their dependencies
To clarify this affects both single and double chests at the moment. Latest release is here: https://www.curseforge.com/minecraft/mc-mods/mythicmetals-decorations/files/4377089
An issue I have had with Lithium is that without the Mixin that applies the LithiumInventory my implementation simply crashes, as it goes out of bounds when iterating over the inventory. This is the main reason I implemented compat in the first place, as well as reaping performance benefits with hoppers while it is installed.
crash-2023-02-08_17.25.56-server.txt
This release has a HopperBlockMixin that changes chests to apply a DoubleInventory, similar to how Vanilla does it with their chests. See https://github.com/Noaaan/MythicMetalsDecorations/blob/1.19/src/main/java/nourl/mythicmetalsdecorations/blocks/chest/MythicChestBlock.java#L301
public class MythicChestBlockEntity extends ChestBlockEntity implements ImplementedInventory, LidOpenable {
private final int size;
private final String name;
private DefaultedList<ItemStack> inventory; //Please remove this field, Just use super.getInvStackList() instead
I think the main issue is that you extends ChestBlockEntity but then override most of the methods from ChestBlockEntity, including EVERYTHING about the inventory.
Just use the existing inventory and use super() calls instead of duplicating the functionality from ChestBlockEntity
Cleaned up a bunch of the Block Entity in Noaaan/MythicMetalsDecorations@e335359, thanks for the comment.
The single chests are now working as intended, and hoppered items are falling through, although double chests are not being properly respected.
Maybe related to #415
You might be able to help me to come to a better understanding why modders do things the way they do it. This should help me adapt lithium's assumptions to fit better to other mods.
Why are you using fabric-api and that "ImplementedInventory" interface to implement Inventories? (My first attempt would be to do whatever the vanilla BarrelBlockEntity does.
Implementing LithiumInventory is NOT needed but it will allow lithium to run some optimizations (IF the insert/extract conditions of the inventory are only changed when the inventory contents change)
After some further reading it is unclear to me why lithium does not work correctly with your mod. I will look into it
Probably fixed in c5cbd18
Tested using the artifact from Actions (https://github.com/CaffeineMC/lithium-fabric/actions/runs/4069499962), and it did not resolve the issue, as the hopper is still not initalized.
Why are you using fabric-api and that "ImplementedInventory" interface to implement Inventories? (My first attempt would be to do whatever the vanilla BarrelBlockEntity does.
The current ImplementedInventory implementation is actually not strictly needed anymore. I think I originally needed it as a previous implementation of this mod was using Ellemes Container Lib at the time. I do not remember if it required the call exposed from it. I am considering dropping it, but I want to do more extensive testing in terms of backwards compatability. Would be a shame to have players lose their items or something by dropping it.
In case this doesn't help, can you link your latest build?
Does it only affect your custom double chests or also the custom single chests?
Lithium handles DoubleInventory quite different from other inventories, so I assume that the code might not expect custom double chests
Personally I am fine with doing something similar to before - Interface injecting into my own mod if Lithium is present, especially if the optimizations are good.
Either way, your proposal would probably be a decent solution for anyone else building containers on top of the vanilla classes
The double chests do not work due Lithium using its own way to retrieve Inventories from the world. The method that vanilla hoppers use combines accessing blocks and entities. Lithium separates those accesses for optimization purposes. I think I cannot really change it on lithium's side.
You can change your mixin to inject into ChestBlock.getInventory instead of HopperBlock.getInventoryAt. But that solution only applies to inventories that subclass ChestBlockEntity, so it is not very gerneralizable.
I will try to think about a better solution