Skip to content

Fix unnecessary item handler invalidation #10677

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

Open
wants to merge 11 commits into
base: version/1.21
Choose a base branch
from

Conversation

Thodor12
Copy link
Contributor

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

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

Review please

someaddons
someaddons previously approved these changes Mar 16, 2025
@Thodor12
Copy link
Contributor Author

Thodor12 commented Mar 16, 2025

You mean like this @Raycoms e317346

@TrevorCow
Copy link

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.

@someaddons
Copy link
Contributor

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

Copy link
Member

@Nightenom Nightenom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants