Add serializer to EntitySnapshot #7509
Conversation
| public EntitySnapshot deserialize(Fields fields) throws StreamCorruptedException, NotSerializableException { | ||
| EntityFactory factory = Bukkit.getEntityFactory(); | ||
| String input = fields.getPrimitive("snapshot", String.class); | ||
| return factory.createEntitySnapshot(input); |
There was a problem hiding this comment.
My ONLY worry here is that the server (CraftBukkit/Paper/Whatever), doesn't handle data changes for entity snapshots.
Mojang lately has been changing NBT fields on entities quite a bit.
A user could save their snapshot in MC 1.21.4, and then load it in a future version (let's say 1.21.5), and any changed tags will get ignored.... the returned entity snapshot would have different values than the user originally had "saved", and they may have no clue what is going on.
Exhibit A:
(from a 1.21.5 snapshot)
In this instance, if you saved a Zombie with some cool armor (in 1.21.4), he's going to load (in 1.21.5) as a boring old, unprotected Zombie.
There was a problem hiding this comment.
To add onto this. As per the javadocs, it states that the NBT string provided by getAsString should NOT be relied be upon for serialization.
There was a problem hiding this comment.
My assumption would probably be because exactly this, Bukkit/CB/Paper doesn't run this thru DataFixerUpper (maybe it should).
sovdeeth
added
the
enhancement
sovdeeth
added
the
up for debate
|
Given the concerns raised by Tud and Shane about the string not being reliable for serialization, I don't think this PR should proceed as is. |
Description
Add serializer to EntitySnapshot
Target Minecraft Versions: 1.20+
Requirements: none
Related Issues: none