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 when applying an Insert command #9379

Closed
wants to merge 1 commit into from

Conversation

SludgePhD
Copy link
Contributor

Objective

Solution

  • Remove the delayed panic caused by EntityCommands::insert, by ignoring the command if its entity has been despawned
  • Make all builtin commands behave consistently when acting on an entity that has since been despawned

Changelog

  • The Insert command now no longer panics when its entity no longer exists.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR labels Aug 7, 2023
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I think we should at least warn here: this is pretty important potential bug.

My preferred solution though is to create a class of try_verb commands though, which fail silently.

Copy link
Contributor

@killercup killercup left a comment

Choose a reason for hiding this comment

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

I would at least make it log an error, so that you can still see when this is happening.

@SludgePhD
Copy link
Contributor Author

If we log an error or warning here, we'd basically be indicating that any occurrence of this situation is a bug that should be resolved, and I'm not convinced that that is correct.

I'm currently running into this panic because I am despawning entities in the PostUpdate schedule, and the bevy_sprite::calculate_bounds_2d system is trying to insert the Aabb component there as well.

The panic of course does not happen reliably, because the entity is only rarely despawned on the same frame as the bevy_sprite::calculate_bounds_2d system inserts the Aabb.

From what I can tell, most people running into this panic end up in a similar situation, where the correct resolution is to simply ignore the insert and despawn the entity. It does not really sound like treating this as a bug would be the right thing to do.

Perhaps the "bug" would be that bevy's internals need to use .try_insert instead of .insert pretty much everywhere, but then this strikes me as poor API design: the non-prefixed, shortest method name should ideally be the correct one to use, but here it causes nondeterministic bugs.

@JMS55
Copy link
Contributor

JMS55 commented Sep 10, 2023

This would also fix #9721

@alice-i-cecile
Copy link
Member

This was added as opt-in behavior in #9844.

@SludgePhD
Copy link
Contributor Author

Were bevy's internal systems ever fixed to use it?

@SludgePhD SludgePhD deleted the dont-panic-on-insert branch October 10, 2023 20:08
@alice-i-cecile
Copy link
Member

No, I would appreciate issues for the places where this is the correct choice :) Or a single issue might be fine and the implementor can track them down.

We just ran into one over in #9721, and I suspect that there's more to fix.

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-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Potential surprising "entity does not exist" panic due to asynchronous command execution
4 participants