ItemStackHandler not a direct replacement for IInventory?

[TheGreyGhost]

Howdy all

 

I'm working on a Container based on a TileEntity and on the advice of more experienced folks than myself I have tried to avoid using MyTileEntity implements IInventory.

But I find that IItemHandler (ItemStackHandler) is not a direct replacement for IInventory because IInventory is also used by the Container to communicate with its parent TileEntity 

For example openInventory(), closeInventory(), isUsableByPlayer().

Is there a drop-in Forge replacement for those methods?

I can of course provide these to the Container as lambdas or a callback interface or give an Optional reference to the parent TileEntity directly, but it seems a bit clumsy.

-TGG

 

[Draco18s]

Btw, if you're doing MyTileEntity implements IItemProvider, you're doing it wrong.

https://github.com/Draco18s/ReasonableRealism/blob/1.14.4/src/main/java/com/draco18s/harderores/entity/MillstoneTileEntity.java#L33-L41

[TheGreyGhost]

Hi

Thanks for the replies

Yeah I just realised it's even worse than I thought because SlotItemHandler throws away / ignores the markDirty() notification, which vanilla appears to use to tell the TileEntity it needs to be saved/retransmitted to the client.

 

I think I'll just stick with the vanilla TileEntity implements IInventory, it appears to be simpler to write a few composition wrappers for an itemStackHandler held in my TileEntity than to tightly couple my TileEntity to the container and to write a customised Slot which handles markDirty() properly.

 

-TGG

 

 

 

[Animefan8888]

7 hours ago, TheGreyGhost said:

I can of course provide these to the Container as lambdas or a callback interface or give an Optional reference to the parent TileEntity directly, but it seems a bit clumsy.

If you haven't given your container a reference to the TileEntity I have only one question for you. Why? Also why would it be an Optional? Your container can't exist without the TileEntity right? At least in most cases. You may have a few that are reused.

6 hours ago, TheGreyGhost said:

than to tightly couple my TileEntity to the container and to write a customised Slot which handles markDirty() properly.

You don't need to do that. Just override ItemStackHandler::onContentsChanged to call TileEntity::markDirty(). Then just use SlotItemHandler for all your slot needs that are not limited.

[TheGreyGhost]

Hi Anime

 

Thanks for the suggestions.

2 hours ago, Animefan8888 said:

If you haven't given your container a reference to the TileEntity I have only one question for you. Why? Also why would it be an Optional? Your container can't exist without the TileEntity right? At least in most cases. You may have a few that are reused.

You don't need to do that. Just override ItemStackHandler::onContentsChanged to call TileEntity::markDirty(). Then just use SlotItemHandler for all your slot needs that are not limited.

Re TileEntity can't exist with the TileEntity: yeah, I used to think that too, but after studying the vanilla code a bit more I realised that the TileEntity is only available on the server.  On the client side, the container does not get a TileEntity, it gets a temporary IInventory instead that isn't linked to any TileEntity.

 

Re override ItemStackHandler:  I don't think that ItemStackHandler::onContentsChanged is called every time that the Slot::onSlotChanged is called.  Many places in vanilla call Slot::onSlotChanged directly and it's not clear that ItemStackHandler::onContentsChanged would also be called in those situations (why would a manual call be needed if the other Slot methods had already marked as dirty?).  SlotItemHandler::onSlotChanged just discards the notification without passing it to ItemStackHandler.

 

Maybe it would be cleaner in some ways to separate a "notifications" interface from the more-general manipulation of a group of ItemStacks, or to code an adapter to convert ItemStackHandler to an IInventory with Notifications, but I'm thinking it really isn't worth the extra complexity.

 

-TGG

 

 

[Animefan8888]

11 minutes ago, TheGreyGhost said:

On the client side, the container does not get a TileEntity, it gets a temporary IInventory instead that isn't linked to any TileEntity.

What are you talking about. This is all about how you setup your containers. The client does have the TileEntities in the world so you can just get it from the world. If you create your ContainerType like this you have access to your TileEntity.

IForgeContainerType.create((id, playerInven, buffer) -> new MyContainer(id, inventory.player.world.getTileEntity(buffer.readBlockPos()), inventory)).setRegistryName("modid:my_container");

 

18 minutes ago, TheGreyGhost said:

I don't think that ItemStackHandler::onContentsChanged is called every time that the Slot::onSlotChanged is called.

It is called whenever an ItemStack inside the ItemStackHandler is changed from one to another or insert/extract has been called. However if you instead do something like ItemStackHandler::getStackInSlot().setTag(someNBTTag);

 

It will not be called in this case make sure to call it yourself.

[TheGreyGhost]

Hi Anime

8 minutes ago, Animefan8888 said:

What are you talking about. This is all about how you setup your containers. The client does have the TileEntities in the world so you can just get it from the world. If you create your ContainerType like this you have access to your TileEntity.

IForgeContainerType.create((id, playerInven, buffer) -> new MyContainer(id, inventory.player.world.getTileEntity(buffer.readBlockPos()), inventory)).setRegistryName("modid:my_container");

 

Perhaps this page will explain what I mean better

https://greyminecraftcoder.blogspot.com/2020/04/containers-1144.html

 

I understand what you're saying about giving your client container a copy of the client TileEntity.  The problem is that Vanilla doesn't do that, probably for a good reason (i.e. you then wind up with two independent synchronisation methods trying to fight each other: the vanilla tileentity synchronisation in parallel with the container directly poking around in the client's contents.)  There's also no guarantee that the client side TileEntity will exist if the client and server worlds are slightly out of synch for some reason

 

13 minutes ago, Animefan8888 said:

It is called whenever an ItemStack inside the ItemStackHandler is changed from one to another or insert/extract has been called. However if you instead do something like ItemStackHandler::getStackInSlot().setTag(someNBTTag);

 

It will not be called in this case make sure to call it yourself.

Here's an example of what I mean, from Container::mergeItemStack:

               int maxSize = Math.min(slot.getSlotStackLimit(), stack.getMaxStackSize());
               if (j <= maxSize) {
                  stack.setCount(0);
                  itemstack.setCount(j);
                  slot.onSlotChanged();
                  flag = true;
               } else if (itemstack.getCount() < maxSize) {

It looks to me like this vanilla code is directly manipulating the ItemStack, not via the Slot methods for changing/insertion/deletion.  Hence the reason the vanilla code must call onSlotChanged directly.  Seems to me that relying on ItemStackHandler::onContentsChanged for this situation may lead to data loss or mis-synchronisation because onContentsChanged is not triggered and hence the TileEntity has been altered but not marked as dirty.

 

-TGG

 

 

 

 

[Animefan8888]

1 hour ago, TheGreyGhost said:

There's also no guarantee that the client side TileEntity will exist if the client and server worlds are slightly out of synch for some reason

Then that is a problem in it of itself. Solve the root of the problem not the visible one.

1 hour ago, TheGreyGhost said:

It looks to me like this vanilla code is directly manipulating the ItemStack, not via the Slot methods for changing/insertion/deletion.  Hence the reason the vanilla code must call onSlotChanged directly.

Actually even this is not a problem both experimentally and in the code. Because when a slot is clicked while it does call mergeItemstack later on it still calls Container::detectAndSendChanges on ServerPlayerNethandler 1201.

Edited April 4, 2020 by Animefan8888

[TheGreyGhost]

I guess we'll have to agree to differ then

 

-TGG