Skip to content

Don't create errors for ignored failed commands #19718

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

Merged
merged 6 commits into from
Jun 29, 2025

Conversation

brianreavis
Copy link
Contributor

@brianreavis brianreavis commented Jun 18, 2025

Objective

  1. Reduce overhead from error handling for ECS commands that intentionally ignore errors, such as try_despawn. These commands currently allocate error objects and pass them to a no-op handler (ignore), which can impact performance when many operations fail.

  2. Fix a hang when removing ChildOf components during entity despawning. Excessive logging of these failures can cause significant hangs (I'm noticing around 100ms).

image

Solution

  • Added a ignore_error method to the HandleError trait to use instead of handle_error_with(ignore). It swallows errors and does not create error objects.
  • Replaced remove::<ChildOf> with try_remove::<ChildOf> to suppress expected (?) errors and reduce log noise.

Testing

  • I ran these changes on a local project.

@brianreavis brianreavis changed the title Don't create errors for ignored failed commands Don't create errors for ignored failed commands + Jun 18, 2025
@brianreavis brianreavis changed the title Don't create errors for ignored failed commands + Don't create errors for ignored failed commands Jun 18, 2025
@brianreavis brianreavis marked this pull request as draft June 18, 2025 15:43
@brianreavis brianreavis added A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 18, 2025
@brianreavis brianreavis marked this pull request as ready for review June 18, 2025 16:28
fn handle_error_with(self, error_handler: ErrorHandler) -> impl Command;
///
/// If `None` is provided, the error will be completely ignored.
fn handle_error_with(self, error_handler: Option<ErrorHandler>) -> impl Command;
Copy link
Contributor

Choose a reason for hiding this comment

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

As someone who often fails to read documentation, I would probably have expected .handle_error_with(None) to be the same as .handle_error(), but one does "ignore" and the other does "default".

It looks like we always know at compile time whether we want Some or None here. Would it work to make a separate method like fn ignore_error(self) -> impl Command? That might even remove a runtime branch.

(If we do want a runtime switch between the behaviors, then maybe it could be something like enum ErrorHandler { Default, Ignore, Custom(fn(BevyError, ErrorContext)) } so that it can cover handle_error, as well.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call! Implemented the former.

Copy link
Member

@janhohenheim janhohenheim left a comment

Choose a reason for hiding this comment

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

Beyond the performance, it makes sense to me that this is an API hole. try_ is great, let's make it a more general principle for commands in general!

@janhohenheim janhohenheim added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it D-Straightforward Simple bug fixes and API improvements, docs, test and examples C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 29, 2025
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.

Very clean design, and an important fix. Nice stuff.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 29, 2025
Merged via the queue into bevyengine:main with commit 795e273 Jun 29, 2025
38 checks passed
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-Performance A change motivated by improving speed, memory usage or compile times C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Entity does not exist" warning triggered by DespawnOnExitState for all child components Warning: missing IDs in observer_propagation
4 participants