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

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

Open
zicklag opened this issue Nov 29, 2022 · 3 comments
Labels
A-ECS Entities, components, systems, and events A-Hierarchy Parent-child entity hierarchies A-Reflection Runtime information about types C-Bug An unexpected or incorrect behavior

Comments

@zicklag
Copy link
Member

zicklag commented Nov 29, 2022

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

*entity = entity_map.get(*entity)?;

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.

@zicklag zicklag added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Nov 29, 2022
@SkiFire13
Copy link
Contributor

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 #2424 (comment)

@sQu1rr
Copy link
Contributor

sQu1rr commented Nov 29, 2022

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
Copy link
Member Author

zicklag commented Nov 29, 2022

@sQu1rr apparently we can run into issues with an invalid hierarchy if we use my fix ( #6791 (review) ) 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.

@james7132 james7132 added A-ECS Entities, components, systems, and events A-Reflection Runtime information about types A-Hierarchy Parent-child entity hierarchies and removed S-Needs-Triage This issue needs to be labelled labels Dec 4, 2022
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 A-Hierarchy Parent-child entity hierarchies A-Reflection Runtime information about types C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants