Fix entities with persist=false copied in an invalid state #2721
Conversation
|
Thanks for this, does this issue also affect the other platforms? (NeoForge, Fabric, Sponge) |
|
I don't think they have the same persist flag, it was added by bukkit not mojang. |
|
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 |
|
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? |
|
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. |
18152fa to
4db029b
Compare
|
Applied patch to other targets. Sponge does not expose the correct function, so emulate it. |
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.
4db029b to
4b12b6b
Compare
| CompoundTag tag = new CompoundTag(); | ||
| entity.saveWithoutId(tag); | ||
| if (!entity.save(tag)) { |
There was a problem hiding this comment.
is changing this away from the saveWithoutId function definitely correct here (and in NF)?
There was a problem hiding this comment.
The only thing paper does differently is return false when their added persist flag is false.
When the EntityType has noSave() set (or the equivalent mapping name on other platforms) they will no longer be copied, but like with bukkit's flag they were being copied in a default state. Given that they're set to not save, it's implied that their lifetime is tied to some other entity or block, whose responsibility it is to recreate them (like a leash).
Any behaviour change observed by mods is in theory their fault or a workaround for WorldEdit's existing behaviour.
Let me know if you think of a case I might not have considered.
There was a problem hiding this comment.
sorry i was meaning going from saveWithoutId -> save. i don't have the decompiled source in front of me right now, but from memory there was a reason we were using the WithoutId variant here
There was a problem hiding this comment.
Oh yeah I do remember it being different in the past but it's really simple now.
The getState function in sponge is now a 1:1 copy of what nms does except nms also checks if the entity is removed. I didn't add that because I assumed this codepath wouldn't be hit if it was removed.
There was a problem hiding this comment.
Oh and they add the id tag
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:
* 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