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

EntityCommands should have methods to fail gracefully #3845

Open
alice-i-cecile opened this issue Feb 2, 2022 · 6 comments · May be fixed by #15929
Open

EntityCommands should have methods to fail gracefully #3845

alice-i-cecile opened this issue Feb 2, 2022 · 6 comments · May be fixed by #15929
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Design This issue requires design work to think about how it would best be accomplished

Comments

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Feb 2, 2022

What problem does this solve or what need does it fill?

Commands failing due to an entity being despawned is one of the most common and frustrating sources of panics in Bevy at the moment.

What solution would you like?

  1. Enable better backtrace behavior to enable debugging. In particular, we need to know the list of other commands that operated on this entity, and which systems they were issued in.
  2. A tool to enable entity commands to fail gracefully (silently ignore / log / warn) rather than panicking.

What alternative(s) have you considered?

Try

For each of the existing methods on EntityCommands, unwrap this option. Create a try_X variant for each method.

This creates significant API bloat and doesn't capture the way that entity commands are chained: after the first operation has succeeded, the rest of the operations in the chain are infallible.

Verified entity flag

Store a verified_entity_exists Option flag on EntityCommands. Each of the existing methods on EntityCommands should attempt to set this flag by unwrapping it to Some(true) if it is None`, and be skipped if it is false.

Then, create a few methods on EntityCommands that set the value of the verification flag with varying behavior:

  1. commands.entity(entity).silently_fail_if_missing().insert(Foo)
  2. commands.entity(entity).log_if_missing().insert(Foo)
  3. commands.entity(entity).warn_if_missing().insert(Foo)

This doesn't work though, because the check is occuring at command-issuing time.

Additional context

#2241 proposes a much more general approach to this, but has significant complexity and overhead.

#3757 aims for a similar goal, but doesn't actually create a reliable API: entities can be despawned after we check for their existence, resulting in the same panic.

@EmiOnGit expressed interest in this problem in #3840.

@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 labels Feb 2, 2022
@alice-i-cecile alice-i-cecile changed the title EntityCommands should store an Option<Entity>, and methods should exist to fail gracefully EntityCommands should have methods to fail gracefully Feb 2, 2022
@alice-i-cecile
Copy link
Member Author

The critical challenge here is that we need to check evaluation at the time of Command evaluation, not command issuing.

@alice-i-cecile alice-i-cecile added the S-Needs-Design This issue requires design work to think about how it would best be accomplished label Feb 2, 2022
@mockersf
Copy link
Member

mockersf commented Feb 3, 2022

  • Enable better backtrace behavior to enable debugging. In particular, we need to know the list of other commands that operated on this entity, and which systems they were issued in.

I tried to solve this with:

  • A tool to enable entity commands to fail gracefully (silently ignore / log / warn) rather than panicking.

As said on discord, I dislike this option as I worry people would jump on ignoring failures instead of fixing their system ordering, potentially leaving their game in an unexpected state

@ndarilek
Copy link
Contributor

ndarilek commented Feb 3, 2022

I want to respectfully push back against this idea, or at least my perception of this idea, that those of us experiencing this just need to reorder our systems already. :)

There really aren't any good tools for doing this at scale, without introducing all sorts of brittle cross-plugin dependencies. So say I have a damage/health system that despawns any entities with < 0 damage. That means I...run every single system before this destroy system? Unless I'm misunderstanding, that's a lot of .before(DESTROY_LABEL).before(DESTROY_LABEL).before(...) all over my game, and now every plugin in my game has to know about this one very specific point in an unrelated plugin. That, when I as the game designer know that 90% of these current panics can fail without consequence, and the 10% that can't aren't worth adding lots of extra overhead to the other 90% because it's more academically correct. :) I'm sure someone's going to suggest how I can fix this by using events, tagging entities about to despawn, etc. but why? If I as the game designer know these systems can fail to do their thing, then please don't make me have to care about one more thing that doesn't seem strictly necessary. Writing a game is complicated enough without having to fight the engine, says the guy used to fighting engines. :)

To be clear, I'm fine with methods like .silently_fail_if_missing() and similar. Opting in is fine. But right now there's no opt either way, and these crashes are outpacing others for me by a long shot. I just tend to see a lot more understanding for the "your commands don't make sense!" perspective than I do the "Building a game out of emergent complexity is hard enough already!" perspective.

@cart
Copy link
Member

cart commented Feb 3, 2022

Some other solutions that might help reduce these problems:

  • (optional) entity refcounting (if you have a strong reference, the entity is guaranteed to still be alive)
  • "hard system dependencies": force SystemA commands to be applied before running dependent SystemB (this is the biggest feature I want from the "stageless" scheduler we're designing now). This would make it possible for SystemB to directly respond to the effects from SystemA, rather than needing a second data channel to "anticipate" changes. Note that this is already possible by converting your systems to exclusive systems.

@mockersf
Copy link
Member

mockersf commented Feb 5, 2022

  • (optional) entity refcounting (if you have a strong reference, the entity is guaranteed to still be alive)

in the cases that panics now, there are two commands to apply on the same entity: a despawn, then a component insert. How would it work with refcounting? would it be the despawn that would panic?

@ChristopherBiscardi
Copy link
Contributor

One graceful method (try_insert) was added in #9844

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 S-Needs-Design This issue requires design work to think about how it would best be accomplished
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants