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

essentials.keepinv permission stops player loot table from dropping (breaking data packs), unlike /gamerule keepInventory true #5863

Open
GrantGryczan opened this issue Jul 6, 2024 · 9 comments · May be fixed by #6036
Labels
bug: confirmed Confirmed bugs in EssentialsX. bug: upstream Bugs that exist in the Bukkit API or Minecraft itself.

Comments

@GrantGryczan
Copy link

GrantGryczan commented Jul 6, 2024

Type of bug

Compatibility issue, Other unexpected behaviour

/ess dump all output

I'm using EssentialsX 2.21.0-dev+102-fcf6e64 (latest as of writing) on CraftBukkit version 4254-Bukkit-bd4e8c3 (MC: 1.21) (Implementing API version 1.21-R0.1-SNAPSHOT).

/ess dump all currently outputs "Error while creating dump error code: 504." which sounds like an issue on the web server's end. If necessary, I can update this issue with a dump later after that issue is resolved.

Error log (if applicable)

No response

Bug description

Players with the essentials.keepinv permission should keep their inventory on death, but they should still drop their loot table. For example, using the Player Head Drops data pack from Vanilla Tweaks which makes a player killing another player drop their head, the head of a player with the essentials.keepinv permission will never drop.

Other data packs, such as Graves from Vanilla Tweaks, use player loot tables to save data from just before a player dies. It's otherwise impossible to do so (at least without saving the player's NBT data every tick, which would be just about the laggiest thing a data pack can do). A data pack using this method adds to the player loot table an item which copies data from the player onto it via an item modifier, so that the data is persisted and can be read after the player dies. But with this bug, any data packs that rely on this method of saving player data on death (such as items from their inventory or how much XP they had) won't find the loot table item for any players with the essentials.keepinv permission.

Steps to reproduce

  1. On a 1.21 server, install this minimal reproduction data pack to your world by copying it into the server's world > datapacks folder. This adds an unconditional player head to the player's loot table.
  2. Enter /minecraft:reload to load the data pack.
  3. Give your player the essentials.keepinv permission. (Or if testing without EssentialsX for comparison, enter /gamerule keepInventory true.)
  4. Enter /minecraft:kill.

Expected behaviour

Just like using /gamerule keepInventory true, dying with the essentials.keepinv permission should drop your loot table.

Actual behaviour

With EssentialsX, using the essentials.keepinv permission, dying does not drop your loot table.

Additional Information

It's important to emphasize that not only does this bugged behavior deviate from the vanilla gameplay functionality of keepInventory (e.g. players not dropping their heads when killed with a data pack like Player Head Drops), but it also causes incompatibilities with other data packs that rely on player loot tables to preserve player information on death. And again, there isn't really any efficient alternative that works as well as using a player loot table, so data packs have no workaround for this. Using Graves as an example again, this bug currently causes a big warning to show up in chat due to the loot table item not being detected every time a player with this permission dies.

@GrantGryczan GrantGryczan added the bug: unconfirmed Potential bugs that need replicating to verify. label Jul 6, 2024
@GrantGryczan GrantGryczan changed the title essentials.keepinv permission stops player loot table from dropping, unlike /gamerule keepInventory true (breaking data packs) essentials.keepinv permission stops player loot table from dropping (breaking data packs), unlike /gamerule keepInventory true Jul 6, 2024
@GrantGryczan
Copy link
Author

GrantGryczan commented Jan 13, 2025

Just to dispel any perception that this issue isn't that big of a deal, I still deal with people running into this and reporting our Graves data pack not working because of it nearly daily. It's been driving me crazy for months now and makes giving technical support for our data packs significantly more time-consuming.

@JRoy
Copy link
Member

JRoy commented Jan 25, 2025

This is likely an issue in Spigot and Paper. EssentialsX is using the standard API to cancel drops (items not loot tables PlayerDeathEvent#getDrops#clear) and set the player to keep inventory (PlayerDeathEvent#setKeepInventory). I would try opening an issue with either Spigot or Paper because the only thing I would be able to do from the EssX side is manually triggering a loot table drop. Looking at the server code it looks like this was a previous issue with spigot that appears to have resurfaced.

@JRoy JRoy added bug: confirmed Confirmed bugs in EssentialsX. bug: upstream Bugs that exist in the Bukkit API or Minecraft itself. and removed bug: unconfirmed Potential bugs that need replicating to verify. labels Jan 25, 2025
@GrantGryczan
Copy link
Author

GrantGryczan commented Jan 31, 2025

@JRoy From the documentation on getDrops:

Image

This appears to be generic to all entities (EntityDeathEvent), not just players. So if anything, it was originally meant for entity loot tables rather than player inventories. And see the description of the method--clearly, this intentionally includes drops from loot tables, and I'm sure countless plugins rely on it doing so. If I were to submit an upstream bug for this, it would be closed as Works As Intended. If that's the case, there's nothing strictly wrong with this method; EssentialsX merely misuses it.

@JRoy
Copy link
Member

JRoy commented Jan 31, 2025

@JRoy From the documentation on getDrops:

Image

This appears to be generic to all entities (EntityDeathEvent), not just players. So if anything, it was originally meant for entity loot tables rather than player inventories. And see the description of the method--clearly, this intentionally includes drops from loot tables, and I'm sure countless plugins rely on it doing so. If I were to submit an upstream bug for this, it would be closed as Works As Intended.

No it wouldn't since there is no way to distinguish which drops are from a loot table or not.

@GrantGryczan
Copy link
Author

GrantGryczan commented Jan 31, 2025

That doesn't mean the getDrops method has a bug. Again, from the documentation, its intention is to include loot table drops. I won't submit an issue which will be closed as WAI directly based on the documentation.

If a data pack (Graves) can distinguish which drops are from loot tables and which drops are from the player inventory, surely a plugin (EssentialsX) can too.

@JRoy
Copy link
Member

JRoy commented Jan 31, 2025

That doesn't mean the getDrops method has a bug. Again, from the documentation, its intention is to include loot table drops.

I personally would maintain that's unexpected behavior since not even keep inventory in vanilla cancels those drops, however Paper would need to add some method to not cancel them if we were to get this fixed. Otherwise there's nothing I can do from the Essentials side

@JRoy
Copy link
Member

JRoy commented Jan 31, 2025

If a data pack (Graves) can distinguish which drops are from loot tables and which drops are from the player inventory, surely a plugin (EssentialsX) can too.

please point me in the direction of this api (there isn't one)

@GrantGryczan
Copy link
Author

GrantGryczan commented Jan 31, 2025

I personally would maintain that's unexpected behavior since not even keep inventory in vanilla cancels those drops, however Paper would need to add some method to not cancel them if we were to get this fixed. Otherwise there's nothing I can do from the Essentials side

I again emphasize that recreating keepInventory is not strictly the purpose of that method. Based on its description, EssentialsX misuses it for that purpose. Just because Spigot's API doesn't offer a feature doesn't mean an existing API has a bug. A plugin must work within the limitations of the plugin API.

please point me in the direction of this api (there isn't one)

There's no API for this in data packs either. You can distinguish it by checking whether the items were actually in the inventory before the player died. This should be even easier for plugins to check than data packs. I can give more details if you'd like.

@JRoy
Copy link
Member

JRoy commented Jan 31, 2025

I personally would maintain that's unexpected behavior since not even keep inventory in vanilla cancels those drops, however Paper would need to add some method to not cancel them if we were to get this fixed. Otherwise there's nothing I can do from the Essentials side

I again emphasize that recreating keepInventory is not strictly the purpose of that method. Based on its description, EssentialsX misuses it for that purpose. Just because Spigot's API doesn't offer a feature doesn't mean an existing API has a bug. A plugin must work within the limitations of the plugin API.

please point me in the direction of this api (there isn't one)

There's no API for this in data packs either. You can distinguish it by checking whether the items were actually in the inventory before the player died. This should be even easier for plugins to check than data packs. I can give more details if you'd like.

That actually could work, despite it being kind of a hack. Will look into this.

JRoy added a commit to JRoy/Essentials-PR that referenced this issue Feb 7, 2025
This maintains the behavior with the vanilla gamerule, which doesn't keep loot table drops.

Fixes EssentialsX#5863
@JRoy JRoy linked a pull request Feb 7, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug: confirmed Confirmed bugs in EssentialsX. bug: upstream Bugs that exist in the Bukkit API or Minecraft itself.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants