search

bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
35.01k stars 3.43k forks source link

`ReflectMapEntities` on `Children` Component Fails to Complete with Partial `EntityMap`. #6790

Open zicklag opened 1 year ago

zicklag commented 1 year ago

Bevy version

Tested on Bevy v0.8.1, but the code in question hasn't changed in Bevy 0.9.0.

What you did

In the bevy_ggrs plugin we use the ReflectMapEntities data to update all of the Children and Parent component's stored entity ids whenever we restore a world snapshot.

The issue happens when we try to map entities on Children components, but we don't necessarily have every existing entity in the entity map.

The problem is in this line

https://github.com/bevyengine/bevy/blob/e954b8573c085a01c62007c4c6232870e0b5c891/crates/bevy_hierarchy/src/components/children.rs#L26

When we don't have a certain entity in the map, this function returns with an error, but in my scenario, I still need it to map all of the entities that it can, and leave any entities that it can't map the same as what they already were.

For me, patching this so that it only updates the entity if it is in the map, and ignoring it if it isn't fixes my issue. This is similar to what is already done for the Parent component.

I'm not sure if there is a scenario where it's important to report an error or not.

Edit: Opened #6791 to fix.

SkiFire13 commented 1 year ago

This is similar to what is already done for the Parent component.

It seems there was a concern for this but it was never addressed https://github.com/bevyengine/bevy/pull/2424#discussion_r678257338

sQu1rr commented 1 year ago

I have a somewhat related issue with mapping in #6702. The code that fails for you also fails for me, but in my case, it highlights the incorrect behaviour of the write_to_world function. Hopefully, the fix you provided will not make it harder to debug my original issue, as it will silently ignore the entity remapping bug.

zicklag commented 1 year ago

@sQu1rr apparently we can run into issues with an invalid hierarchy if we use my fix ( https://github.com/bevyengine/bevy/pull/6791#pullrequestreview-1197003848 ) so I'm not sure if my fix will work anyway. 🤔

I may just need to make a custom RollbackMapEntities trait for my use-case, that allows partial maps.

The only other thought I have right now is to provide maybe a second function to the MapEntities trait for allowing you do partial maps.