minecolonies icon indicating copy to clipboard operation
minecolonies copied to clipboard

Fix unnecessary item handler invalidation

Open Thodor12 opened this issue 10 months ago • 3 comments

Closes #10671 Closes #10670

Changes proposed in this pull request

  • Move container removal to onColonyTick in the building
  • Item handler capability will only be recalculated if the underlying container list has changed
  • Removed obsolete code in CombinedItemHandler (the nameable has no effect, nor any usages)

Testing

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

Review please

Thodor12 avatar Feb 23 '25 19:02 Thodor12

You mean like this @Raycoms https://github.com/ldtteam/minecolonies/pull/10677/commits/e317346127403fd873a48470fbee3af6f6cc3f42

Thodor12 avatar Mar 16 '25 13:03 Thodor12

Forgive me if this is not helpful, I am not super familiar with this codebase, and I am probably doing close to the most unsupported thing (Trying to fix a bug in the infamous Compatibility addon for Minecolonies), but I digress. I am running a local compiled version of this patch, backported to version/main for 1.20.1, and think that we might be forgetting to add the colony building itself when rebuilding the combinedInv/capability. I had numerous problems with couriers not being able to deliver to guard towers and other "colony block only" buildings. Adding

TileEntityColinyBuilding

if (combinedInv == null)
{
    final List<IItemHandlerModifiable> handlers = new ArrayList<>();
    final Set<BlockPos> newPositions = new HashSet<>();

    final Level world = colony.getWorld();

    for (final BlockPos pos : building.getContainers())
    {
        if (WorldUtil.isBlockLoaded(world, pos) && !pos.equals(this.worldPosition))
        {
            final BlockEntity te = world.getBlockEntity(pos);
            if (te != null)
            {
                if (te instanceof final AbstractTileEntityRack rack)
                {
                    handlers.add(rack.getInventory());
                    newPositions.add(pos);
                }
            }
        }
    }

+    handlers.add(this.getInventory());
+    newPositions.add(this.getPosition());

    combinedInv = LazyOptional.of(() -> new CombinedItemHandler(handlers));
    currentInvPositions = newPositions;

seemed to fix that. I don't know if this is the best option but thought it might be of help.

TrevorCow avatar Mar 27 '25 20:03 TrevorCow

We actually went about this the wrong way a bit, we should change it to add an invalidatin listener to a rack, which when a rack becomes unavailable triggers the reset/invalidation of the combined inventory capability and that way forces a new one to be created

someaddons avatar Apr 20 '25 15:04 someaddons