Add a Baseline Mapping to Preserve Relationships

[bushrat011899]

Objective

• Fixes Adding non-rollbacked children to rollbacked entities break the hierarchy #63

Solution 1 (original)

Before populating the EntityMap used during rollback with tracked entities, all entities within the World are added as no-op mappings:

for entity in world.iter_entities() {
    let entity = entity.id();
    entity_map.insert(entity, entity); // no-op mapping
}

This does add additional overhead, so it may be worth pursuing alternate strategies, perhaps through a PR to Bevy itself.

Solution 2 (current; suggestion from @johanhelsing)

Reverse the mappings for any dead entities, and apply the map again:

// At this point, the map should only contain rollback entities
debug_assert_eq!(entity_map.len(), rollback_entities.len());

// For every type that reflects `MapEntities`, map the entities so that they reference the
// new IDs after applying the snapshot.
for reflect_map_entities in type_registry
    .iter()
    .filter_map(|reg| reg.data::<ReflectMapEntities>())
{
    reflect_map_entities.map_all_entities(world, &mut entity_map)
}

// If the entity map is now larger than the set of rollback entities, then dead entities were created
if entity_map.len() > rollback_entities.len() {
    // Reverse dead-mappings, no-op correct mappings
    for original_entity in entity_map.keys().collect::<Vec<_>>() {
        let mapped_entity = entity_map.remove(original_entity).unwrap();

        if rollback_entities.remove(&original_entity) {
            // Rollback entity was correctly mapped; no-op
            entity_map.insert(mapped_entity, mapped_entity);
        } else {
            // An untracked bystander was mapped to a dead end; reverse
            entity_map.insert(mapped_entity, original_entity);
        }
    }

    // Map entities a second time, fixing dead entities
    for reflect_map_entities in type_registry
        .iter()
        .filter_map(|reg| reg.data::<ReflectMapEntities>())
    {
        reflect_map_entities.map_all_entities(world, &mut entity_map)
    }
}

This avoids the allocation of an EntityMap the size of all entities in the World, but does require running map_all_entities twice.

[johanhelsing]

I was considering iterating over Query<Children, With<Rollback>>, and adding only those... It would solve the specific problem of hierarchies, but not the more fundamental general problem.

[bushrat011899]

I was considering iterating over Query<Children, With<Rollback>>, and adding only those... It would solve the specific problem of hierarchies, but not the more fundamental general problem.

Agreed. I think it makes more sense to get a working but possibly slow solution now, instead of trying to get a partially correct but faster solution later. Premature optimization is the root of all evil, etc.

[johanhelsing]

Okay, I think we have the following options:

1. Add identity mappings for all entities (this pr)
2. Add identity mappings only for child/parent relationships, leave other relationships to non-rollbacked entities broken
3. Revert Map Entity IDs When Writing Snapshot to World #31 (which introduced the hierarchy-destroying regression), do a holistic non-regressive change for adding relationship support.
4. Try to get API into bevy for a version of entity mapping that preserves unknown entities.

There are also rumors that Bevy will fundamentally change how they do relationships.

Currently, I'm leaning towards 1, possibly with a way to opt out, and just wait to see what bevy comes up with.

Curious to hear if @gschup has any thoughts on this.

[johanhelsing]

Had an idea for a possible horrible hack:

1. Run entity mapping as we currently do
2. Create a new entity map
3. Iterate through old entity map (that has now gotten entries for the dead entities), add reverse mappings for any dead entries in the old map to the new map. Add identity mappings for all others.
4. Run entity mapping again with the second entity map

[bushrat011899]

Had an idea for a possible horrible hack:

1. Run entity mapping as we currently do
2. Create a new entity map
3. Iterate through old entity map (that has now gotten entries for the dead entities), add reverse mappings for any dead entries in the old map to the new map. Add identity mappings for all others.
4. Run entity mapping again with the second entity map

Interesting idea! I'll give that a try and see how it goes.

[bushrat011899]

Ok so I've pushed up an alteration to this PR which implements the concept @johanhelsing proposed. Instead of creating one large EntityMap containing all entities, and then updating the entries tracked for rollback, this now:

1. Creates the original EntityMap that only contains rollback entities, and a HashSet of the rollback entities for later use.
2. Apply the map.
3. Goes through each entry in the map and either replaces it with a no-op (A -> B becomes B -> B) if it was a rollback entity, or reverses the mapping if it was not (A -> B becomes B -> A). The reversal will only apply to dead entities, since they are the only type that could be in the map, but not in the set.
4. Apply the map a second time.

It is possible to avoid using the HashSet by checking if an entity is dead (they are supposed to have a specific signature to their inner values, making them identifiable), however I avoided adding that complexity because I don't like the idea of investigating the index and generation of an entity to determine if it's dead, since we can easily construct a list of known-living entities instead.

It seems to work, but I'd like @johanhelsing to test it with a project that was previously broken to confirm it actually resolves the issue.

Also, I don't know if it's faster than just making a big entity map at the beginning. I couldn't see an obvious problem in the box_example sync_test, but that's hardly demanding.

[gschup]

On first glance, this seems like a good initial working solution. I wonder if there is a smarter way that avoids going through the map twice. In any case, we can figure optimizing this once it is no longer broken.

[bushrat011899]

On first glance, this seems like a good initial working solution. I wonder if there is a smarter way that avoids going through the map twice. In any case, we can figure optimizing this once it is no longer broken.

As you were typing this I noticed that we can get a slight optimisation by just checking if the entity map grew after applying it the first time. If it didn't, then no dead entities were created, so a second mapping is not required.

[johanhelsing]

It seems to work, but I'd like @johanhelsing to test it with a project that was previously broken to confirm it actually resolves the issue.

I'll give it a go. There might be other rollback issues in my game, though, bevy 0.10 -> 0.11 has been a tough one for me.

Code looks sensible to me. I agree this is a good baseline. Should be correct, and adds relatively minimal (constant time) overhead. Not a bad deal to fix a serious regression.

[johanhelsing]

Seems to be working like a charm. No more broken parent-child relationships in my game 🎉 ...and particle effects work correctly in synctest sessions.

My game is still a bit broken due to #71, I get a bunch of false positive (i think at least) desync errors so not a perfect test case yet. But still think it should be good to merge this once the conflicts are resolved 👍

[johanhelsing]

Hmm... There is a quite noticeable performance drop in synctest sessions, though (in debug mode at least)

[johanhelsing]

Hmm... There is a quite noticeable performance drop in synctest sessions, though (in debug mode at least)

lol. scratch that. I'm now pretty sure it was just my hierarchy sanity checking system that was running a lot more efficiently when hierarchies were actually broken

[gschup]

For now, I like the solution. The code in world_snapshot.rs is very monolithic, but that is something we can maybe approach once it works properly! Thanks for doing the work!

[bushrat011899]

For now, I like the solution. The code in world_snapshot.rs is very monolithic, but that is something we can maybe approach once it works properly! Thanks for doing the work!

No problems! I look forward to helping out more on the future 😁

[mikeder]

FWIW I found that parent->child transform propagation is still different/broken compared to before the bevy 0.11 updates.

This does not work:

.
└── parent
    └── child1 <- follows parent just fine
        └── child2 <- does not follow either parent unless with .add_rollback()

This does work:

.
└── parent
    ├── child1
    └── child2

[johanhelsing]

bevy_ggrs does not do anything to propagate transforms, i don't think it did before, and I'm not sure it should either...

This pr was about the entity relations (what entity have what children) which I think should be ok now.

[mikeder]

@johanhelsing understood, just commenting as I found my original implementation broken by the Bevy 0.11 upgrade and thought it to be related. It is curious that using .add_rollback() in the first example above resulted in the desired behavior again, despite not doing anything to propagate the transforms.

[johanhelsing]

Gotcha. If you think something is still broken wrt hierarchies, it's probably best to open a new issue.