Adjust strategy for refreshing hut inventory handlers for better compatibility with AE2 and other mods (fixes #10670)

[jpenilla]

Closes #10670

Changes proposed in this pull request

Instead of invalidating the item handler capability every tick, we only invalidate it when the external inventories have changed. This fixes issues with mods that attempt to operate with a capability over multiple ticks. For example, the AE2 storage bus requires 2 ticks for some operations, and will restart the operation on tick 1 if the capability is different on tick 2, leading to breakage when we recreate the capability every tick. There's room for improvement here on the AE2 side, but in general, I don't think we need to invalidate the capability every tick and this resolves the issue.

Testing

• Yes I tested this before submitting it.
• I also did a multiplayer test.

Review please

[CLAassistant]

All committers have signed the CLA.

[Thodor12]

I think a whole new class for this is overkill. Just leave the dirty flag in the parenting class, less changes that way.
(All new code must have javadoc)

[jpenilla]

If we don't add a new class we won't be able to remove the invalidate calls, as the actual handler instance returned to other mods will change. Alternatively, I could modify CombinedItemHandler, but the diff would be larger than this class and it's used in other places that will not benefit from the changes.

[jpenilla]

Another solution could be tracking whether the external inventories changed and only then calling invalidate and recreating the CombinedItemHandler. I didn't go with this originally to try and keep the same general mechanism as before, but it seems like it might be cleaner.

[Thodor12]

Another solution could be tracking whether the external inventories changed and only then calling invalidate and recreating the CombinedItemHandler. I didn't go with this originally to try and keep the same general mechanism as before, but it seems like it might be cleaner.

I was discussing this with someone else of the team, I figured that would be the better solution

[jpenilla]

I've updated the PR to use that strategy and re-tested. Also updated the description and added Javadoc.

[Thodor12]

I'm still not sure of the amount of complexity this takes, it should be a lot easier to be able to do this.

AbstractBuildingContainer.java could contain the flag whether the container list has changed, along with a setDirty method to change this value.

The tile entity could on tick check if this dirty flag in the building is true, and invalidate the caps + recreate the item handler.
Right now it would still be checking the whole process, listing every container and requiring all chunks to be loaded each tick, which is quite a bit of effort.

[jpenilla]

I was under the assumption that the code is also guarding against players breaking/placing containers manually at the defined positions, basing updates on the position list changing wouldn't catch this. I agree it's not ideal to retrieve the tile entities each tick, but this is already happening in the current code (see the debugger screenshots in the linked issue).

[Thodor12]

All the positions in the container list are always added by ourselves to any rack location, there is no way to insert custom positions (short of code) in there.

[jpenilla]

I understand that, I'm referring to if a player constructs for example a warehouse, and then breaks some of the racks placed by the builder, and then later places them again. I'm not super familiar with the NeoForge API, but maybe we can register capability invalidation listeners for the external inventories to catch this, and have some period to re-check for missing inventories?

[Thodor12]

You could leave the ticking bit in that removes the containers if a tile entity is no longer present, then immediately invalidate the capability as well.
Just to ensure no unknown inventories remain in the list.
Adding any inventory post construction is something we don't want to have anyway, they can only be added by builders, so this is fine

[jpenilla]

I've updated the PR to use a flag in AbstractBuildingContainer as suggested, along with capability invalidation listeners for external inventories to trigger hut capability invalidation (to handle players breaking racks).

edit: needs some revision, but the concept should be good

[Thodor12]

I'd probably move the else block to the tick method, continuously reading each container position to see if it's loaded and is a rack, removing if not, setting the building pos if it is.

[jpenilla]

Removals should already be handled through the capability invalidation listener and the getOrCreateCombinedInv call in tick.

In the case where the player has broken a rack and later re-places it, it won't get picked up until a chunk reload, building upgrade, or removal of another rack. The goal of using the listeners was to avoid querying the block entities every tick, so maybe we should check for re-placed racks every X (100?) ticks?

[Thodor12]

The thing is, I don't want to see another solution using yet another different system.
If this is not directly crucial to be removed asap, then we are better off moving this check to the building directly using the onColonyTick method. Which is a tick happening about every 30 seconds.

[jpenilla]

Putting the check in onColonyTick seems reasonable then. But I think we should keep the capability listeners, they seem to be a tool built for this purpose. Without it there could be errors if something tries to query the inventory of a broken rack before the next colony tick.

[someaddons]

I'm not sure this is needed as other mods need to check if a capability is still valid, only those who do not do that properly run into issues. Also this may cause some fun unintended side effect due to spagetthi code in minecolonies

[jpenilla]

AE2 is properly checking for validity, using a capability invalidation listener. As mentioned in the description the problem is when an operation wants to use the same item handler across multiple ticks. Since AE2 is listening for invalidation, it notices that the item handler is different each tick, and aborts any multi-tick operations designed to work with the same item handler and gets stuck in a retry loop. I plan to look further into the AE2 side eventually, since I have doubts that these operations truly need to take place across multiple ticks. But even if this was fixed from the AE2 side, it's not ideal to be invalidating and recreating the handler every tick (including reading all of the block entities).

[uecasm]

I understand that, I'm referring to if a player constructs for example a warehouse, and then breaks some of the racks placed by the builder, and then later places them again. I'm not super familiar with the NeoForge API, but maybe we can register capability invalidation listeners for the external inventories to catch this, and have some period to re-check for missing inventories?

We do expect that players should be able to break an existing rack and replace it "quickly" without it losing validity. If left for too long (e.g. next slow colony tick) it will become invalid and they need to repair the building to re-add racks.

I'm not sure this is needed as other mods need to check if a capability is still valid, only those who do not do that properly run into issues. Also this may cause some fun unintended side effect due to spagetthi code in minecolonies

I agree that we should not be re-creating the capability unless actually needed.

[Thodor12]

We do expect that players should be able to break an existing rack and replace it "quickly" without it losing validity. If left for too long it will become invalid and they need to repair the building to re-add racks.

Shouldn't we move it to building > onColonyTick then to begin with, then there's no need for back & forth from the building to the tile entity and back, it's all contained within the building in that case.

[uecasm]

Shouldn't we move it to building > onColonyTick then to begin with, then there's no need for back & forth from the building to the tile entity and back, it's all contained within the building in that case.

Yes, that's what I was meaning.

[jpenilla]

I've implemented the intended behavior but I'm not sure exactly what should be moved to onColonyTick and how.

[uecasm]

Why do you need the capability invalidation listener at all? Just have the tick only invalidate when the container list changed.

Regarding using the colony tick, the way to do that would be to make the building itself own the combined inventory (created as needed or invalidated in the building's colony tick) and have the TE simply query it off it. I haven't thought through if there's any reason that wouldn't be a good idea.

[jpenilla]

Currently building doesn't store a reference to the block entity (needed to build the combined inventory), presumably to avoid managing clean up of that state.

The capability listener is needed as otherwise a broken rack may lead to undefined behavior until the next rebuild of the hut's capability. Once a rack is broken we want to immediately stop trying to insert or extract anything from it, as depending on the external inventory's implementation it may throw an error or void inserted items.

[uecasm]

You could pass that in when the TE requests the combined inventory. Or let it recover it internally, since the building knows the level and pos of the TE, just as it knows the container positions.