Tschipp / CarryOn

Carry On mod for Minecraft
GNU Lesser General Public License v3.0
86 stars 51 forks source link

Dupe bug with Functional Storage (also reported to Functional Storage) #668

Open Penrosian opened 3 months ago

Penrosian commented 3 months ago

Expected Behavior

Cabinet will de-link from controller or the controller will pull from the moved cabinet.

Actual Behavior

Cabinet moves while link remains linked to nothing, allowing items to be pulled from the controller without decreasing the number in the actual cabinet

Steps to Reproduce

Place down a storage controller and a cabinet of any type Link the cabinet to the storage controller Use carry on to move the cabinet Pull items out of the controller with hoppers/item cables/etc.

Version of Minecraft, Carry On, Forge/Fabric

Minecraft 1.19.2, Carry On 2.1.2.23, Functional Storage 1.1.10, Forge 43.3.9

Screenshots encouraged

https://youtu.be/K83Hsuzq2Jo video of dupe

Penrosian commented 1 month ago

To quote @richie3366 on discord:

"The issue is occurring most likely because Carry On removes the BlockEntity first, here, before removing the block in the next line of code.

And the problem is: since Functional Storage uses an intermediary class to improve performance (by not accessing/checking block entities every time an item has to be added/removed), it doesn't know the tile entity has been removed because it relies on Block.onRemove and checks for BlockEntity inside, but at the time the method is called, it's already too late: the tile entity was removed just earlier.

In 1.20 / 1.21 versions of Functional Storage, it stills does the same thing but in another class (Drawer instead of DrawerBlock), but it behaves the same way whatsoever.

I wouldn't know for sure which mod is "misbehaving" since Carry On is one mod of its kind, but I'd be more inclined to suggest Functional Storage to also handle removal inside the BlockEntity.setRemoved method by overriding it in their extending class(es). It's the best way to fix the issue once & for all. I think Carry On couldn't have done a better job than what they did, but that's my non-expert opinion."

Penrosian commented 1 month ago

Another quote from the same guy: Although! Now that I look at the MC code, it seems that calling level.removeBlock subsequently calls level.removeBlockEntity inside the BlockBehavior.onRemove code (note: Block extends BlockBehavior) shown in the joined screenshot. That means Carry On could remove this line without any side-effect (that I know of), and that would solve the dupe issue as well. It would also possibly solve other related issues with other mods relying only on Block.onRemove to detect & handle block/tile removal.