Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fully serialize entities #6143

Closed
Shatur opened this issue Oct 2, 2022 · 5 comments
Closed

Fully serialize entities #6143

Shatur opened this issue Oct 2, 2022 · 5 comments
Labels
A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use

Comments

@Shatur
Copy link
Contributor

Shatur commented Oct 2, 2022

What problem does this solve or what need does it fill?

Currently for entities we serialize only id:

serializer.serialize_u32(self.id())

But this is not very expected behavior. For example, if you serialize an entity, send it over the network, and map it to an entity on another machine, this will not work correctly, because generation is skipped for serialization.

What solution would you like?

Fully serialize entities. It may only be redundant for scenes, but Bevy use DynamicEntity for them anyway.

@Shatur Shatur added C-Enhancement A new feature S-Needs-Triage This issue needs to be labelled labels Oct 2, 2022
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR and removed C-Enhancement A new feature S-Needs-Triage This issue needs to be labelled labels Oct 2, 2022
@alice-i-cecile
Copy link
Member

From what I've heard from @cart, this is a deliberate decision. Entity identifiers are intended to be opaque, and should not be used for serialization over the network. This is because other incidental entities (such as UI elements or particles) may be spawned, taking up the entity identifier you were planning to use.

Instead, the suggested solution is to use a secondary identifier for network synchronization.

@Shatur
Copy link
Contributor Author

Shatur commented Oct 2, 2022

Entity identifiers are intended to be opaque, and should not be used for serialization over the network. This is because other incidental entities (such as UI elements or particles) may be spawned, taking up the entity identifier you were planning to use.

No, no, it's not for reusing the same identifiers. It is needed for mapping.
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.

@alice-i-cecile
Copy link
Member

Ah, okay! I've seen a lot of people trying to reuse identifiers, so I was a bit suspicious. I'm okay with this change now that you've explained the pattern you're using now; feel free to open a PR.

@alice-i-cecile alice-i-cecile removed the X-Controversial There is active debate or serious implications around merging this PR label Oct 2, 2022
@james7132
Copy link
Member

I'm curious why a secondary index (i.e. a NetworkIdentity/NetworkId) is insufficient here. Both bevy_backroll and bevy_ggrs utilize a secondary and network-wide unique identifier that is handled separately from how the ECS world ID's entities, which makes the ID assignment deterministic and isolated from the rest of the engine.

@Shatur
Copy link
Contributor Author

Shatur commented Oct 7, 2022

It's easier:

  • I do not need to handle uniqueness.
  • I map it only on client. Server sends its state as usual and client maps server entity to its own entity. With a secondary index I would need to first map it on server and then on client.
  • When you send a component with entities, you need to map its entities too. Currently I just re-use MapEntities trait that Bevy use for scenes on client. With secondary index I will need to send the whole server resource with network mappings to let client decode it. Or you an intermediate component with network identifiers instead.

The only use case I know for entity serialization is scenes, but Bevy already uses DynamicEntity for it to avoid storing an extra u32 for generation. This is why I suggested this change, if anyone want to avoid storing an extra u32, they can just create a new type.

@bors bors bot closed this as completed in 71f8b4a Oct 28, 2022
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
# 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`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants