Make scene handling of entity references robust

[Illiux]

Objective

• Handle dangling entity references inside scenes
• Handle references to entities with generation > 0 inside scenes
• Fix a latent bug in Parent's MapEntities implementation, which would, if the parent was outside the scene, cause the scene to be loaded into the new world with a parent reference potentially pointing to some random entity in that new world.
• Fixes DynamicScenes contain invalid Entity references #4793 and addresses Handling of Entity in Scenes & Serialization #7235

Solution

• DynamicScenes now identify entities with a Entity instead of a u32, therefore including generation
• World exposes a new reserve_generations function that despawns an entity and advances its generation by some extra amount.
• MapEntities implementations have a new get_or_reserve function available that will always return an Entity, establishing a new mapping to a dead entity when the entity they are called with is not in the EntityMap. Subsequent calls with that same Entity will return the same newly created dead entity reference, preserving equality semantics.
• As a result, after loading a scene containing references to dead entities (or entities otherwise outside the scene), those references will all point to different generations on a single entity id in the new world.

---

Changelog

Changed

• In serialized scenes, entities are now identified by a u64 instead of a u32.
• In serialized scenes, components with entity references now have those references serialize as u64s instead of structs.

Fixed

• Scenes containing components with entity references will now deserialize and add to a world reliably.

Migration Guide

• MapEntities implementations must change from a &EntityMap parameter to a &mut EntityMapper parameter and can no longer return a Result. Finally, they should switch from calling EntityMap::get to calling EntityMapper::get_or_reserve.

[alice-i-cecile]

Very much like the technical elements of the changes, but this is rather tricky, so I'm being strict on code quality / docs.

[alice-i-cecile]

Much better docs, and I really like the closure based API. Why not bake this in directly to EntityMap? It's unclear when you would be okay to forgo the guarantees provided here.

I don't feel strongly on u64 vs Entity, so we'll see what others think.

[Illiux]

Much better docs, and I really like the closure based API. Why not bake this in directly to EntityMap? It's unclear when you would be okay to forgo the guarantees provided here.

I'm not sure I see a good way to do that that both preserves safety and doesn't make the API more awkward. Baked into EntityMap we either have an extra bit of state (has a dead_start or doesn't, making dead_start have to be Option<Entity>) or we push the initialization and saving out to code setting up the EntityMap. In the first case, the result would be get_or_alloc either panicking or returning Option<Entity> instead of entity. In the second we can't use the closure approach to ensure safety anymore. Also in the first case, it becomes possible to bypass with_mapper.

[cart]

Making this controversial because this is taking scene ids in a new-ish direction. Scenes should have "virtual self contained identity spaces" that don't (necessarily) correspond to real world entities. Introducing generation (especially for "top level" entities in the scene) makes the scene format longer / harder to read and compose (just look at the diffs in the scene files). Generation isn't a useful part of the of "scene mental model". Introducing generation also encourages people to think about "runtime ids" as the same thing as scene ids.

There is definitely an issue in the current approach when it comes to uniqueness. ( id: 4, gen: 0) and (id: 4, gen: 1) should not be considered the same thing. And dangling ids should be accounted for.

Some alternative ideas to consider:

• Serialize scene entity ids as u64s (laid out as [id_u32][generation_u32]). Treat that as just a simpler encoding of (id: u32, generation: u32)
• When serializing, just create a brand new Entity->SceneId mapping and allocate new SceneIds as new entities are encountered.

Curious what the reflection SMEs think: @MrGVSV @jakobhellermann

[Illiux]

• When serializing, just create a brand new Entity->SceneId mapping and allocate new SceneIds as new entities are encountered.

This is very difficult to do because we currently can't look inside components with entity references when serializing, and so we can't allocate SceneIds for dangling references nor serialize component entity references as SceneIds. It would be a much, much larger change to start doing that. The entities in dangling references are only encountered when adding a deserialized scene to the world and no sooner (after both serialization and deserialization).

• Serialize scene entity ids as u64s (laid out as [id_u32][generation_u32]). Treat that as just a simpler encoding of (id: u32, generation: u32)

Amusingly, this is actually what the initial version of this PR did (well, almost: it was [generation_u32][id_u32] since it used the existing Entity::to_bits()). Check out the now-resolved thread off the first comment alice left on dynamic_scene.rs.

I think it makes sense for Entity to serialize like this everywhere, so that both component entity references and the top-level entity IDs serialize as u64s. Though, I was going to put that into a separate PR.

[cart]

(moved back to the 0.10 milestone, but it might be too late at this point so no promises!)

[james7132]

Moving this to 0.11 as it seems like we're going to be shipping 0.10 early today, and this isn't ready yet for a merge.

[Shatur]

and this isn't ready yet for a merge.

But looks like comments were addressed. Or you mean that the approach is controversial?
I just think scene loading can't be broken more. This PR fixes a pretty serious bug with scene loading.

[nicopap]

Need to resolve conflicts and this seems ready for a final review/merge, right?

[alice-i-cecile]

@Illiux once this is rebased I think we can merge this ASAP

[Shatur]

Added a few very minor suggestions.

[Shatur]

@Illiux I started using this new approach and found mutable world access inconvenient for the MapEntities trait in practice. And it causes huge impact on performance if you map many components.
Mapping entities is useful for networking. Yes, I can create my own trait that just returns error on incorrect mapping (to avoid world acess), but this mean that I have to create two derives for component/events: one for networking and one for scene serialization.