[Merged by Bors] - Use default serde impls for Entity #6194
Conversation
alice-i-cecile
added
C-Usability
|
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. |
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
This change makes sense to me. I don't really understand what not serializing generation does. You already can't do anything with an I see no harm in allowing an 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 In that case, not including the generation can actually create invalid aliasing. If you had serialized a component that was holding onto a stale |
|
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'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 |
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 |
|
Like @Shatur says, this is not about spawning entities with specific
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 Unrelated to this networking usage, I think this change should be made for correctness. If (Edit: whole paragraph) I'm not familiar with Edit: #6325 suggests mapping living |
|
Thinking about it more, maybe we should return an error if we find an invalid Then it would be OK for scenes to only serialize an ID/index, to save space, not for privacy or "safety". But yeah, dropping the generation while components can store invalid Once |
There was a problem hiding this comment.
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).
|
bors r+ |
# 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](https://github.com/dollisgame/dollis/blob/master/src/core/network/entity_serde.rs) 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 `Entity`s 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](https://docs.rs/bevy/latest/bevy/scene/struct.DynamicEntity.html) 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`.
|
Pull request successfully merged into main. Build succeeded:
|
bors
bot
changed the title
mockersf
added
the
hacktoberfest-accepted
# 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](https://github.com/dollisgame/dollis/blob/master/src/core/network/entity_serde.rs) 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 `Entity`s 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 bevyengine#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](https://docs.rs/bevy/latest/bevy/scene/struct.DynamicEntity.html) 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`.
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 (usingEntityMap) to those received from the server (to know which one matches which). And ifgenerationfield is missing, this mapping can be broken. Example:Entity{ id: 2, generation: 1}with components.idandgeneration. Let's call itEntity{ id: 12, generation: 4}.Entity{ id: 12, generation: 4}. To do so, it maps local entity to the one from server. Butgenerationfield is 0 because it was omitted for serialization on the server. So it maps toEntity{ id: 2, generation: 0}.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
Entityover a customNetworkIdalso have the following advantages:MapEntitiestrait to mapEntitys in replicated components.Entity <-> NetworkIdandEntity <-> NetworkId, we map entities only on client.Closes #6143.
Solution
Use default serde impls. If anyone want to avoid wasting memory on
generation, they can create a new type that holdsu32. 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
generationfield.Migration Guide
id, as it was before, you can create a new type that wrapsu32.