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

fallback to remove components one by one when failing to remove a bundle #719

Merged
merged 10 commits into from
Nov 15, 2020

Conversation

mockersf
Copy link
Member

fixes #698
when failing to remove a bundle, issue a warn log and fallback to remove components from bundle one by one instead of panicking.

see #710 for discussion on fallback

@Moxinilian Moxinilian added A-ECS Entities, components, systems, and events C-Enhancement A new feature labels Oct 22, 2020
use std::collections::hash_map::Entry;

self.flush();
let loc = self.entities.get_mut(entity)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retrieving the location here and in remove() is redundant work. I'd prefer it if we found a way to only do this once (ex: pass the location into remove_bundle_internal)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was not possible, as location needs to be an &mut that references self, which also needs to be an &mut to call remove_bundle_internal.

I was able to remove getting the location from remove by retrieving the bundle value in remove_bundle_internal. When coming from remove_one_by_one, this will not return the expected value as we actually can't get it, that's why there is a parameter check_presence set to false when coming from this function...

@cart
Copy link
Member

cart commented Nov 15, 2020

rebased / updated to master (by using tracing instead of log). I also reverted the "only get location once in normal path" changes. Looking up locations is just an array lookup. I don't think its worth the added branching (and return complexity) to remove it.

We can always just duplicate code if we need to, but in this case its all probably in the noise anyway.

I also removed the uber unsafe block in favor of slightly more scoped blocks.

@cart cart merged commit 02f543e into bevyengine:master Nov 15, 2020
@mockersf mockersf deleted the no-panic-remove branch April 27, 2021 23:50
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-Enhancement A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

removing a bundle from an entity when one of the component has already been removed crashes
3 participants