Skip to content
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

Use setInventorySlotContents instead of modifying stackSize directly #511

Closed
copygirl opened this issue Dec 15, 2012 · 9 comments

Comments

Projects
None yet
2 participants
@copygirl
Copy link

commented Dec 15, 2012

When pipes and hoppers put items into another inventory, and there's already an item of the same type in there, I suspect it's modifying the stackSize of that stack, instead of creating another ItemStack and using setInventorySlotContents. Because my Storage Crates only simulate an inventory, their actual internal storage is not accessible, they simply ignore the changed stackSize and the items are lost forever.

Have a look at http://github.com/copyboy/mod_BetterStorage if you want to see some code.

Since Railcraft's Item Unloaders have the same issue I will post the same issue on their tracker.
(I should note that ComputerCraft and Thermal Expansion work just fine with crates, and Thaumcraft will have a fix soon, hopefully.)

@SirSengir

This comment has been minimized.

Copy link
Contributor

commented Dec 16, 2012

Manipulating the itemstack stack size directly is more efficient than reading the old itemstack, creating a new itemstack with a different stacksize (copying all information including NBT from the old stack) and then setting it back with setInventorySlotContents.

If you have a virtual inventory, you can always expose a "pseudo-itemstack" IMO for other mods to manipulate.

After some discussion we don't plan to implement your suggestion, sorry.

EDIT: The obvious solution to your problem is of course to implement ISpecialInventory.

@SirSengir SirSengir closed this Dec 16, 2012

@copygirl

This comment has been minimized.

Copy link
Author

commented Dec 16, 2012

What do you mean by "pseudo-itemstack"? I'm not sure how I'd be able to detect a change in stackSize unless I do something crazy complicated.

@SirSengir

This comment has been minimized.

Copy link
Contributor

commented Dec 16, 2012

See my edit: "The obvious solution to your problem is of course to implement ISpecialInventory."

@copygirl

This comment has been minimized.

Copy link
Author

commented Dec 17, 2012

And what would you say about just adding setInventorySlotContents(), without cloning the stack before?

From my view you're not using IInventory correctly, so I hope you understand why I'm having a such a hard time deciding whether to keep it the way it is (mods stay incompatible), or implement the BuildCraft API.

@SirSengir

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2012

We are using IInventory correctly. The issue is that it was not built for the type of interactions BC or RC perform. Which is why ISpecialInventory was born.

@copygirl

This comment has been minimized.

Copy link
Author

commented Dec 17, 2012

I meant that there's a setInventorySlotContents function in IInventory and you're not using it when just changing the stackSize of a slot. Other mods work fine with my crates, so I guess they're using setInventorySlotContents the way it's supposed to be used ..?

@SirSengir

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2012

setInventorySlotContents != onInventorySlotContentsChanged - though of course we don't know what the method was called originally. As I said IInventory wasn't built for BC or RC interactions. What you are proposing is a work-around I don't feel is warranted. You also cannot expect other mods to call setInventorySlotContents if they don't actually change the ItemStack object, only manipulate it. If I am wrong, CJ or cpw will certainly correct me.

You only need to include ISpecialInventory, not the full BC API.

@copygirl

This comment has been minimized.

Copy link
Author

commented Dec 17, 2012

You also cannot expect other mods to call setInventorySlotContents if they don't actually change the ItemStack object, only manipulate it.

If they don't change it, they don't need to call setInventorySlotContents. If they change the NBT data, they should, in my opinion.

The only reason why I'm still struggling is because if there's a mod that doesn't use ISpecialInventory and doesn't call the function when manipulating the stack, I'd too have to ask them to use it. I think Thaumcraft doesn't use it so that'd be a nice example.

... I guess I'll use it, but only because it'll probably also increase the performance and ... well because now I know I don't have to include anything other than ISpecialInventory (still new to modding). I still believe that this is something you should change.

@copygirl

This comment has been minimized.

Copy link
Author

commented Dec 20, 2012

I'd still be interested in what you think about just using setInventorySlotContents, without copying the ItemStack before.

Also I'd like to note that BuildCraft and Railcraft are the only two mods so far that don't seem to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.