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

[Bukkit] Fix entities with persist=false copied in an invalid state #2721

Open
wants to merge 1 commit into
base: version/7.3.x
Choose a base branch
from

Conversation

brickmonster
Copy link
Contributor

Remove a redundant passenger check, as entity.save() returns false in that case.

This also causes leash knots to not be copied. I don't think this is a problem because:

  • They would not be saved to disk, it's misleading for users that they appear.
  • Pasted leashed mobs still think they're leashed to the original position and get unleashed* - no change in behaviour.
    * Unless they're pasted close enough to the original position, in which case this has better behaviour because they create their own leash_knot entity.

Fixes #2719

Remove a redundant passenger check, as entity.save() returns false in that case.

This also causes leash knots to not be copied. I don't think this is a problem because:
- They would not be saved to disk, it's misleading for users that they appear.
- Pasted leashed mobs still think they're leashed to the original position and get unleashed* - no change in behaviour.
    \* Unless they're pasted close enough to the original position, in which case this has better behaviour because they create their own leash_knot entity.
@brickmonster brickmonster requested a review from a team as a code owner February 24, 2025 19:41
@me4502
Copy link
Member

me4502 commented Feb 26, 2025

Thanks for this, does this issue also affect the other platforms? (NeoForge, Fabric, Sponge)

@brickmonster
Copy link
Contributor Author

I don't think they have the same persist flag, it was added by bukkit not mojang.
It looks like fabric is also disregarding ignoring when entities ask not to be saved, but maybe modded has different expectations? I only use bukkit so I don't know. Didn't check forge.

@me4502
Copy link
Member

me4502 commented Feb 26, 2025

my understanding of this is that it's fixing a wider bug, where the persist tag was basically one of the symptoms on Bukkit. given worldedit is a cross platform mod, generally if something is fixed/changed in one platform it's best to keep it consistently applied across all

@wizjany
Copy link
Collaborator

wizjany commented Feb 26, 2025

I'm a bit perplexed by the minecraft behavior here. the save method returns a bool, but still fills the tag in? doesn't this mean that if, for example, there's extra data on a leash entity, it will get lost after a save/load? does the leashed animal recreate a new one each time?

@brickmonster
Copy link
Contributor Author

brickmonster commented Feb 26, 2025

Using fabric as a reference here just because I have it open:

Minecraft calls saveSelfNbt and returns false if it's a passenger or if it's not allowed to save that entity type.

WorldEdit instead calls a deeper function, writeNbt, fetching the entity type itself and checking if it's a passenger, but does not check if it's allowed to save the entity type.

And yes the leashed animal creates the leash on spawn.

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

Successfully merging this pull request may close these issues.

Non-persistent entities are pasted incorrectly
3 participants