[Merged by Bors] - Use default serde impls for Entity

[Shatur]

Objective

Currently for entities we serialize only id. But this is not very expected behavior. For example, in networking, when the server sends its state, it contains entities and components. On the client, I create new objects and map them (using EntityMap) to those received from the server (to know which one matches which). And if generation field is missing, this mapping can be broken. Example:

1. Server sends an entity Entity{ id: 2, generation: 1} with components.
2. Client puts the received entity in a map and create a new entity that maps to this received entity. The new entity have different id and generation. Let's call it Entity{ id: 12, generation: 4}.
3. Client sends a command for Entity{ id: 12, generation: 4}. To do so, it maps local entity to the one from server. But generation field is 0 because it was omitted for serialization on the server. So it maps to Entity{ id: 2, generation: 0}.
4. Server receives Entity{ id: 2, generation: 0} which is invalid.

In my game I worked around it by writing custom serialization and using serde(with = "..."). But it feels like a bad default to me.

Using Entity over a custom NetworkId also have the following advantages:

1. Re-use MapEntities trait to map Entitys in replicated components.
2. Instead of server Entity <-> NetworkId and Entity <-> NetworkId, we map entities only on client.
3. No need to handling uniqueness. It's a rare case, but makes things simpler. For example, I don't need to query for a resource to create an unique ID.

Closes #6143.

Solution

Use default serde impls. If anyone want to avoid wasting memory on generation, they can create a new type that holds u32. This is what Bevy do for DynamicEntity to serialize scenes. And I don't see any use case to serialize an entity id expect this one.

---

Changelog

Changed

• Entity now serializes / deserializes generation field.

Migration Guide

• Entity now fully serialized. If you want to serialze only id, as it was before, you can create a new type that wraps u32.

[inodentry]

I like this. Simpler, more predictable behavior.

[alice-i-cecile]

For the record: both @mockersf and I are opposed to this, as discussed in the linked issue. This violates one of our most important privacy guarantees, and is not adequately motivated IMO.

[maniwani]

This change makes sense to me. I don't really understand what not serializing generation does. You already can't do anything with an Entity that doesn't exist, nor can you spawn an entity with a specific ID and generation.

I see no harm in allowing an Entity value from one World to be used in a mapping in another.

Presumably, if we were loading a scene, we'd be spawning entirely new entities and just using the serialized values to correctly map the saved Entity values to the new ones.

In that case, not including the generation can actually create invalid aliasing. If you had serialized a component that was holding onto a stale Entity value, now we can't distinguish it from another value with the same ID but "living" generation.

[alice-i-cecile]

I've been convinced, and I agree with @maniwani's position. There's no value in serializing Entity in the current strategy, and simply not serializing Entity will result in a pretty rough UX with no real increase in safety.

EntityMap badly needs better docs and examples, but that's not this PR's problem.

[mockersf]

Entity's generation only make sense in a given world. Outside of their originating world, the only thing that makes sense is the "relations" between different entities when a component refer to another entity. The ID is enough to capture that information.

For me, relying on the generation to keep entities in sync means that entities are not actually synced, as spawn/despawn are not synced otherwise there would be no risk of targeting a wrong entity just by its ID. It also means that the synchronised world will only despawn entities when it notice a synced component is targeting an entity with a same ID/different generation, so that means your synchronised world is actually never in sync as entities are not despawned by themselves

[Shatur]

For me, relying on the generation to keep entities in sync means that entities are not actually synced, as spawn/despawn are not synced otherwise there would be no risk of targeting a wrong entity just by its ID.

We probably misunderstood each other. This PR is not about spawning an entity with the same ID. This is necessary to map server entities to local entities on client. And without generation this mapping won't be correct. I updated the description to provide an example of why it happens. I mean if server receives a command "Move Entity { id: 10, generation: 0} it won't be able to correctly map it to Entity { id: 10, generation: 2}.
We could map back and forth on client and on server using an intermediate representation (like NetworkId). But this have some downsides, I added to the issue description.
Since I don't see why the current behavior could be needed, I created this suggestion and PR.

[maniwani]

Like @Shatur says, this is not about spawning entities with specific Entity values or trying to do worldB.get_entity(worldA_entity). It's about serializing Entity values correctly without losing information.

Entity's generation only make sense in a given world. Outside of their originating world, the only thing that makes sense is the "relations" between different entities when a component refer to another entity. The ID is enough to capture that information.

This isn't true. In the first place, the "originating world" is irrelevant when it comes to scenes and other serialized structures. Not serializing generation only saves space. It does not safeguard against someone trying to use invalid Entity values.

Unrelated to this networking usage, I think this change should be made for correctness. If Entity is serializable and you have a component that stores an Entity value, it can hold a stale reference. We can't prevent that. You can serialize a scene where entity (id 14, gen 2) is alive but some component had a reference to entity (id 14, gen 0).

(Edit: whole paragraph) I'm not familiar with bevy_scene and bevy_reflect and how we currently handle components that store Entity values. But when we save a scene, it seems like either nothing happens to those Entity values or they also lose their generations. And when we load the scene, if those component-stored Entity values were serialized as-is, we could detect the invalid ones and error, similar to how #4793 panics (I don't think we check though). If we didn't serialize their generation or didn't check their validity, aliasing can happen.

Edit: #6325 suggests mapping living Entity values to their iteration order during scene serialization (or similar), e.g. starting from 1 (similar to how pointers are serialized). If we did that, we would ideally update Entity values stored in components too, using the exact Entity value as the key, not just the ID, to prevent aliasing.

[maniwani]

Thinking about it more, maybe we should return an error if we find an invalid Entity reference when saving a scene.

Then it would be OK for scenes to only serialize an ID/index, to save space, not for privacy or "safety".
Outside of scenes, Entity should serialize its full value imo.

But yeah, dropping the generation while components can store invalid Entity values and we don't check is bad. If an invalid Entity value is serialized, it seems like either the value could alias or the scene load will error/panic. Both of those are undesirable.

Once Option<Entity> has the niche optimization, we could make that a convention. Then if the scene serializer sees a component holding a Some(invalid_entity) value, it could replace it with None instead of erroring like it would in the unwrapped case.

[cart]

Ok I'm on board for this. The initial impl was intended to be used for scene formats, but I agree that it is a separate concern.

Entity is a generation + id, which represents a runtime-only id for a specific world. Serializing that should include the generation.

However this should never be used to represent anything other than the current runtime state of a specific World. We should actively discourage using this impl for scenarios like scenes or keeping multiple worlds in sync on the same Entity value. (But @Shatur's ServerEntity -> ClientEntity mapping scenario does seem valid).

[cart]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-and-install-on-iOS
• build-android
• build (macos-latest)
• build (ubuntu-latest)
• build-wasm
• build (windows-latest)
• build-without-default-features (bevy)
• build-without-default-features (bevy_ecs)
• build-without-default-features (bevy_reflect)
• check-compiles
• check-doc
• check-missing-examples-in-docs
• ci
• markdownlint
• run-examples
• run-examples-on-wasm
• run-examples-on-windows-dx12