search

SpongePowered / SpongeForge

A Forge mod that implements SpongeAPI
http://www.spongepowered.org/
MIT License
1.14k stars 306 forks source link

Duplication bug with Soul Bound (EnderIO) #2892

Open runescapejon opened 5 years ago

runescapejon commented 5 years ago

I am currently running

1) You would need to spawn a anvil (vanilla minecraft), Soul Bound enchantment book (EnderIO). Then get an armor piece for example boots. Soul Bound is when the player dies, any item enchanted with Soulbound will be kept in inventory.

2) put boots (example) in the anvil then the Soul Bound enchantment book, then enchant it.

2) Make sure you have permission to nucleus.inventory.keepondeath from Nucleus You can type on a clean test server lp user {name} permission set nucleus.inventory.keepondeath

3) Then Die, you will have the boots in your inventory armor slots and you will have an extra boot duplication glitch in your hotbar.

This seem to be relate to https://github.com/SpongePowered/SpongeForge/issues/2874 EDIT: Make sure on your test server you have gamerule keepinventory false

gabizou commented 5 years ago

Pretty sure I may have fixed this more recently, can you re-test with latest?

runescapejon commented 5 years ago

I can still able to reproduce it using spongeforge-1.12.2-2838-7.1.7-RC3870 which is the current latest version

jinkhya commented 5 years ago

I'm also having this issue with soulbound from enderio and cofh. The items get duped. Server is on Nucleus 1.13.2 and Sponge RC3903.

runescapejon commented 4 years ago

Still an issue on spongeforge-1.12.2-2838-7.1.9-RC3988

Spontini commented 4 years ago

Having this issue with Spongeforge-1.12.2-2838-7.1.10

ZeroCool5254 commented 4 years ago

I'm very late to this bug, Having the same issue. Not quite sure about the versions, but we're running FTB Revelation with the latest sponge and nucleus with the keep inventory perms.

HenryLoenwind commented 4 years ago

There is a vanilla gamerule for "keep inventory on death", which we need to check to avoid duping stuff. nucleus probably uses the same mechanism to preserve the inventory, but doesn't set the gamerule (because it's not global).

If possible, I'd recommend to temporarily enable the gamerule while firing Forge events about the death when the dying player has that permission. Then Forge mods will have a chance of knowing about it.

dualspiral commented 4 years ago

If possible, I'd recommend to temporarily enable the gamerule while firing Forge events about the death when the dying player has that permission. Then Forge mods will have a chance of knowing about it.

That's already what happens.

HenryLoenwind commented 4 years ago

I tried to follow the program flow there, but just on github the code is a bit too fragmented. Could you please look up if both net.minecraftforge.event.entity.player.PlayerDropsEvent and net.minecraftforge.event.entity.player.PlayerEvent.Clone are fired inside that context?

PS: To understand the flow:

PlayerDropsEvent is called first. It gets a player object "old" with an empty inventory and a list of items to be dropped. Ender IO goes through the list and moves items to the inventory. At no point any itemstack exists in two locations at the same time (i.e. we don't dupe stacks here).

PlayerEvent.Clone is called second. It gets the "old" player object and a new cloned player object. Both inventories are empty by default, the "old" player object and inventory will be discarded after the event is done, the "new" player object will be put into the world. With Ender IO, the "old" inventory will have the soulbound items. We then move them from the old to the new inventory. Again we move them, not copy them.

Please note again that we never copy an item---we only move them around. If they get duped, that means that the calling code copies them from its own data storage. Which, honestly, it should not do. If an item is to not be dropped, it should show up in the "old" inventory and not the list for the first event and in the "new" inventory but not the "old" one for the second event. (I'm quite sure vanilla code also copies stuff, but we're quite used to vanilla code being unnecessarily convoluted (especially after Forge's events are patched in) and don't need to copy all its flaws...)

ZeroCool5254 commented 4 years ago

I have tested and initially, I set the "gamerule KeepInventory true" which didn't work for some reason and that's why I ended up using the nucleus perm. I later noticed that keepinventory didn't work because of the case sensitivity. It should be "gamerule keepInventory true". OFC with sponge it's a per-world permission but that fixed the dupe bug for me.

HenryLoenwind commented 3 years ago

There's an additional bug here:

When the vanilla gamerule keepInventory is enabled, SpongeForge still fires the PlayerDropsEvent. That event should not be fired if the player doesn't actually drop stuff (Forge doesn't fire it with keepInventory).

While Ender IO's Soulbound is protected against this, COFH's Soulbound isn't and will happily dupe items. But people will still report issues with COFH's Soulbound to us (Ender IO), so I just wasted a couple of hours on finding this bug. I'm a teensy tiny bit annoyed with the random weird incompatible behaviour SpongeForge introduces and am contemplating blacklisting it in Ender IO. If you cannot play nicely with others, just play alone.

Faithcaio commented 3 years ago

What we're doing is allowing plugins to set KEEP_INVENTORY for an individual user on death.

So the bug is probably this then: https://github.com/SpongePowered/Sponge/blob/3e618117627c3557e29e32ed35681033b3d07cc9/src/main/java/org/spongepowered/common/event/tracking/phase/entity/EntityDeathState.java#L126 Calling the Sponge event always causes the Forge event to also fire.