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

Don't panic, do debug, when failing to remove component(s) #710

Closed
wants to merge 2 commits into from
Closed

Don't panic, do debug, when failing to remove component(s) #710

wants to merge 2 commits into from

Conversation

CleanCut
Copy link
Member

Resolves #698 by never panicking and always debugging when attempting to remove components (or remove_one a component) fails.

@mockersf
Copy link
Member

I'm not sure this is the best way to deal with the unwrap in the remove case, as it will leave dangling components that the user think will be removed, and without notice at all if a logger is not setup...

Anyway I think it should at least be a warning instead of a debug as it leaves those components.
To have the log at debug level I think this components should be removed one by one when removing the bundle fails...

@cart
Copy link
Member

cart commented Oct 21, 2020

Yeah I think falling back to one-by-one removal is a good idea. Its a bit more expensive (each removal is an archetype change), but it shouldn't be a super common case. Producing a debug log in that case should be enough for people to identify those cases when they care.

@CleanCut
Copy link
Member Author

CleanCut commented Oct 22, 2020

Shoot, I missed the fact that remove didn't remove any components if an error occurred.

I spent an hour on this, but I haven't been able to figure out a way to iterate through the bundle's types, though I learned quite a bit about TypeID and Any. Any hints?

More specifically, I see that you can get TypeIDs from T::type_info(), but I can't figure out how to go from a TypeID to an actual type.

@Moxinilian Moxinilian added P-Crash A sudden unexpected crash A-ECS Entities, components, systems, and events labels Oct 22, 2020
@mockersf
Copy link
Member

implemented the fallback in #719

@CleanCut
Copy link
Member Author

Closing in favor of #719

@CleanCut CleanCut closed this Oct 22, 2020
@CleanCut CleanCut deleted the remove-calmly branch October 23, 2020 01:02
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 P-Crash A sudden unexpected crash
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
4 participants