Expand on entity serialization API

[SoSeDiK]

Makes an entity serializable via API no matter what. Previously you'd get an empty component (and an error later when trying to spawn the deserialized entity) in some cases:

• When the entity is marked as non-persistent.
• When the entity is a passenger.
• When the entity is not saveable (misc entities like lightning, knot, etc.).
• When the entity is "invalid" (dead, unloaded, etc.).

Now there's no problem serializing & deserializing & spawning entities in those cases.

---

Edit: see the comment below for the new flags explanation

[Owen1212055]

The issue I see is that wouldn't this allow for the serialization of entities in an invalid state and then their state will be "reset"? Not sure if that's a good thing.

[SoSeDiK]

The issue I see is that wouldn't this allow for the serialization of entities in an invalid state and then their state will be "reset"? Not sure if that's a good thing.

Yes, it would, and that's partially the goal. Before you'd get a byte[] of invalid data, and now you'll have the last captured state of the entity, which can be manually "reset" (spawned) into a new valid entity.

Personally, I don't think this is a problem for API which does nothing by itself and is even placed in Unsafe. I'd say it's a dev's responsibility if they want to take into account such cases. After all, it was them who tried to serialize a dead entity, for example (which is considered invalid too).

[Owen1212055]

Well, in my opinion, if you try serializing an invalid entity I wouldn't expect it to magically be deserialized as a valid entity? It's just tricky now since you will have to account for odd cases where "the entity will be serialized as a valid entity, despite being invalid"

[SoSeDiK]

serializing an invalid entity I wouldn't expect it to magically be deserialized as a valid entity

I think it depends. My wording might've not been the best, but I'll try to explain.

Upon deserialization, you'll get the same set of data as before serialization. So while the entity might be considered invalid, its data is valid and shouldn't be prevented from deserialization via API.

For example, if you serialize an entity with 0 health you'll also get an entity with 0 health upon deserialization, and nothing will happen if you try to spawn it, because, well, it's "invalid". Of course, you can change the health via API and spawn now a valid entity.

One other example of an "invalid" state is when the entity was despawned. It's invalid because it's not present in the world (someone called entity.remove() or a chunk was unloaded, for instance). This, by game's means !entity.isValid() state, shouldn't prevent you from serialization & deserialization & manual spawn of an entity too, because the entity's data is valid.

[lynxplay]

Generally after looking at this, I think it might be worth hiding these "bypasses" behind some form of flag system similar to relative teleportation. E.g. if I as a plugin wants to save an entity that is a vehicle, it could actively define that by passing some EntitySerialisazionCheckBypassFlag.PASSENGER to the method to indicate that specific check should be ignored.

I do not really want to end up with completely invalid entities being saved when all I want to do is passenger saving. Alternative would be to just allow a flag to toggle on passenger/non-persist saving and never allow non-serialisable and invalid entities to be saved (knots etc).

[lynxplay]

Do you have anything interest in moving forward with this PR ? Generally seems like a worth change in my opinion :+1:

[SoSeDiK]

Yes, I do! :) However, I'm currently not at home having exams at uni, and due to the situation in Ukraine I have lack of electricity most of the spare time. I'll revisit in about a week.

As for the feedback, I guess flags might be a neat solution 🤔

1. What else besides PASSENGER would there be?

My proposals are: MISC (serialize misc entities like knot/lightning) / FORCE (serialize dead/unloaded)?

2. And as for PASSENGER, do you mean that it'll allow including passengers?

Because I think that not being able to save a passenger is more of a bug in this case, and it shouldn't be prevented. Vanilla does that bacause that entity is already serialized as part of its vehicle, so no need to save the passenger separately once again.

I believe in our case we want to serialize the passenger specifically since we don't care about the vehicle.

3. If this is the case (PASSENGER is for including passengers), do we want to go through all serialized entities chain (vehicle -> passengers) and spawn them upon deserializing?

4. Also, what should we do when someone tries to serialize "invalid" entity without passing the bypass flag?

Previously you'd end up with corrupted data. Should we have the inner checks and raise IllegalArgumentException? So, for example, if we have !isValid() entity and no FORCE flag then throw?

4. One more caveat I thought of is de/serializing Players. Should we prevent that altogether?

[SoSeDiK]

Alright, rebased & introduced flags & added some javadocs. Feedback would be appreciated!

For now, this PR:

1. Introduces EntitySerializationFlags:

• FORCE - serializing invalid (dead, unloaded) entities.
  • This way you can "validate" data on Entity later after deserializing it (for example, set health, so the entity is no longed considered dead).
• MISC - serializing misc entities (those with !EntityType#canSerialize (since there's no need in vanilla to, for example, save a lightning bolt)).
• PASSENGERS - if present, the serialized data will include serialized passengers as well.
• PLAYER - if anyone for whatever reason wants to save/look at serialized player's data.
  • Vanilla always skips entity with Player's id, so deserializing such data will lead to IllegalArgumentException.

2. UnsafeValues#serializeEntity method now has additional checks:

• If !entity.isValid() then there must be the FORCE flag.
• If !#canSerialize then there must be the MISC flag.
• If serializing a Player then there must be the PLAYER flag.
• If the PASSENGERS flag is present then the passengers are included in the serialized data.

3. Overview of behaviors with this patch:

• Non-persistent entities can now be serialized since we do not save the data to the world folder like vanilla but rather the user manually requested the serialized data.
• For the same reason, entities that ride other entities can now be serialized too (e.g. you can serialize a skeleton riding a spider and then deserialize/spawn the skeleton only; previously you'd have a corrupted data in this and the previous case).
• Spawning invalid entities does nothing, they are rejected. The data on Entity instance must be "validated" for the entity to appear in the world.
  • E.g. serializing a dead entity is possible with the FORCE flag, but to spawn it in the world after deserializing you have to set its health to be valid.
• While it is possible to serialize Players with a flag, it's not possible to deserialize them.
• preserveUUID boolean controls preserving uuid for the entity and its passengers.
• #deserializeEntity now accepts the new preservePassengers boolean. If passed true, Entity instances will be created for passengers as well, and upon calling #spawnAt the entity and its passengers will be spawned together.

5. Currently, all the flags affect only the passed entity and do not apply to its passengers. So, for example, even if FORCE and PASSENGERS are passed, invalid passengers will not be present in serialized data. ~~Also, while serializing allows valid non-persistent entities, due to the inner CraftBukkit's checks non-persistent entities will be skipped in passenger serializing. This can be worked around if needed, but will require extra work.~~

   • Non-persistent entities can now be serialized too.

6. Bukkit's EntityType enum currently does not expose #canSerialize boolean*. Should this be introduced (in this PR in a separate patch / different PR) or should this be left to the user to figure out? Currently, leash knots, lightning bolts, players and fishing bobbers are not serializable. Player has its own flag, and others require the misc flag.

   • * There's SpawnCategory#MISC, so it's fine.

[lynxplay]

I like the flags chosen :+1: player seems a bit weird, but I guess there could be some really weird point ? I have not yet had the time to look at the impl, but API wise, while the methods in there are deprecated, I don't think we have to break the API if not needed here ?

UnsafeValues#serialiseEntity(Entity) is now gone. While programatically the same, this is an ABI break to the new method signature UnsafeValues#serialiseEntity(Entity, FlagType...). A simple default method that passes the previous default in regards to flags (I think none ?) would keep this working for older plugins and cross versions :+1:

I'll look at the impl of things once I make it out of the train :)

[SoSeDiK]

Here's an example of serialized player data: https://pastebin.com/CtZFh0Gz (same as what vanilla saves into the uuid.dat file) I thought maybe someone wants to read that data (to display some info about the player somewhere) or take snapshots of player's state, etc., so decided to leave it as a possibility.

Note: player normally falls under MISC category (!EntityType#canSerialize), but since it can't be deserialized into world later, there's a separate flag because the behaviour differs. That's also the reason for that extra instanceof Player check in the implementation.

[SoSeDiK]

Rebased. Allowed non-persistent passengers to be serialized, but that required a little extra diff. ~~Left that change as a separate commit for easier changes overview and in case someone has a better solution or if it's deemed unsupported.~~

[SoSeDiK]

Rebased, but haven't gotten to test whether everything works yet. Future me: tested, working as described.

[SoSeDiK]

Will revisit once 1.21 builds are ready. Edit: done.

[lynxplay]

Needs some testing on my end, but does look good otherwise. Will get to on monday :+1: