Skip to content

Conversation

Axyss
Copy link
Contributor

@Axyss Axyss commented Aug 19, 2025

Resolves #12928

This issue was introduced on 1.21.2 when Mojang refactored AbstractContainerMenu#removed() and moved some of the logic to AbstractContainerMenu.dropOrPlaceInInventory(). When they did that, they inverted the logic of the flag that checked wether the player was dead or not and stopped using !player.isAlive() in favor of player.isRemoved().

On vanilla this works fine because by the time AbstractContainerMenu.dropOrPlaceInInventory() is executed the player is already removed, but this is not the case in paper — here's some examples:

// SPIGOT-943 - only call if they have an inventory open
if (this.containerMenu != this.inventoryMenu) {
    this.closeContainer(org.bukkit.event.inventory.InventoryCloseEvent.Reason.DEATH); // Paper - Inventory close reason
} // From ServerPlayer#die()
// Paper end - Configurable container update tick rate
if (this.containerMenu != this.inventoryMenu && (this.isImmobile() || !this.containerMenu.stillValid(this))) { // Paper - Prevent opening inventories when frozen
    this.closeContainer(org.bukkit.event.inventory.InventoryCloseEvent.Reason.CANT_USE); // Paper - Inventory close reason
    this.containerMenu = this.inventoryMenu;
} // From ServerPlayer#tick()

@Axyss Axyss requested a review from a team as a code owner August 19, 2025 16:02
@github-project-automation github-project-automation bot moved this to Awaiting review in Paper PR Queue Aug 19, 2025
@jpenilla
Copy link
Member

The change was in 24w37a https://www.minecraft.net/en-us/article/minecraft-snapshot-24w37a

@jpenilla
Copy link
Member

jpenilla commented Aug 19, 2025

Would moving the closeContainer call in ServerPlayer#die up to right after

        if (this.isRemoved()) {
            return;
        }
        // <-- here

also solve this? That would mean the cursor item becomes part of the player's death drops if the inventory has room (not certain if that's desirable from an API standpoint, but it seems reasonable?).

@Axyss
Copy link
Contributor Author

Axyss commented Aug 20, 2025

I've just tested it, and I can confirm that works too. A little caveat I've found though, is that this approach wouldn't prevent the hovering item from poping into the player's hotbar while they are in the death screen.

Copy link
Member

@jpenilla jpenilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change (reverting the 24w37a diff) seems fine to me then, but I am weary of any edge cases this might uncover given we don't know the exact purpose of the original 24w37a diff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Awaiting review
Development

Successfully merging this pull request may close these issues.

Lose carried items when dying with a container GUI open.
2 participants