Skip to content

Convert to fallible system in IntoSystemConfigs #17051

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Dec 31, 2024
Merged

Convert to fallible system in IntoSystemConfigs #17051

merged 12 commits into from
Dec 31, 2024

Conversation

@hymm
Copy link
Contributor

@hymm hymm commented Dec 30, 2024

Objective

  • Fallible systems #16589 added an enum to switch between fallible and infallible system. This branching should be unnecessary if we wrap infallible systems in a function to return Ok(()).

Solution

  • Create a wrapper system for System<(), ()>s that returns Ok on the call to run and run_unsafe. The wrapper should compile out, but I haven't checked.
  • I removed the impl IntoSystemConfigs for BoxedSystem<(), ()> as I couldn't figure out a way to keep the impl without double boxing.

Testing

  • ran many_foxes example to check if it still runs.

Migration Guide

  • IntoSystemConfigs has been removed for BoxedSystem<(), ()>. Either use InfallibleSystemWrapper before boxing or make your system return bevy::ecs::prelude::Result.

@hymm hymm changed the title Fallible systems trait based Convert to fallible system in IntoSystemConfigs Dec 30, 2024
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Dec 30, 2024
@alice-i-cecile
Copy link
Member

@Neo-Zhixing how do you feel about this implementation instead?

alice-i-cecile
alice-i-cecile approved these changes Dec 30, 2024
View reviewed changes
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 prefer this design: I think it's clearer and reducing the branching is good.

@NthTensor
Copy link
Contributor

Seems great! Have you tested logging and panics to see if this mangles the source tracking, as was the issue in #8705?

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times C-Code-Quality A section of code that is hard to understand or change S-Needs-Testing Testing must be done before this is safe to merge labels Dec 30, 2024
@hymm
Copy link
Contributor Author

hymm commented Dec 30, 2024

Have you tested logging and panics to see if this mangles the source tracking, as was the issue in #8705?

I'll check, but it should be fine. This should just add one more line to the backtrace.

NthTensor
NthTensor approved these changes Dec 31, 2024
View reviewed changes
Copy link
Contributor

@NthTensor NthTensor left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is clearly a much better approach. Thanks for tidying up after me :).

@NthTensor NthTensor added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Testing Testing must be done before this is safe to merge labels Dec 31, 2024
@hymm
Copy link
Contributor Author

hymm commented Dec 31, 2024 •
edited
Loading

Panics seem ok.

thread '<unnamed>' panicked at examples/stress_tests/many_foxes.rs:116:5:
testing panics backtraces
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Encountered a panic in system `many_foxes::setup`!
Encountered a panic in system `bevy_app::main_schedule::Main::run_main`!
error: process didn't exit successfully: `target\debug\examples\many_foxes.exe` (exit code: 101)

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 31, 2024
Merged via the queue into bevyengine:main with commit ac43d5c Dec 31, 2024
29 checks passed
@PixelDust22
Copy link
Contributor

Oops I'm a bit late to the discussions, but this is clearly a much better approach.

ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
@hymm @ecoskey
Convert to fallible system in IntoSystemConfigs (bevyengine#17051)
0017ab0 
# Objective

- bevyengine#16589 added an enum to switch between fallible and infallible system.
This branching should be unnecessary if we wrap infallible systems in a
function to return `Ok(())`.

## Solution

- Create a wrapper system for `System<(), ()>`s that returns `Ok` on the
call to `run` and `run_unsafe`. The wrapper should compile out, but I
haven't checked.
- I removed the `impl IntoSystemConfigs for BoxedSystem<(), ()>` as I
couldn't figure out a way to keep the impl without double boxing.

## Testing

- ran `many_foxes` example to check if it still runs.

## Migration Guide

- `IntoSystemConfigs` has been removed for `BoxedSystem<(), ()>`. Either
use `InfallibleSystemWrapper` before boxing or make your system return
`bevy::ecs::prelude::Result`.
mrchantey pushed a commit to mrchantey/bevy that referenced this pull request Feb 4, 2025
@hymm @mrchantey
Convert to fallible system in IntoSystemConfigs (bevyengine#17051)
f3c6c35 
# Objective

- bevyengine#16589 added an enum to switch between fallible and infallible system.
This branching should be unnecessary if we wrap infallible systems in a
function to return `Ok(())`.

## Solution

- Create a wrapper system for `System<(), ()>`s that returns `Ok` on the
call to `run` and `run_unsafe`. The wrapper should compile out, but I
haven't checked.
- I removed the `impl IntoSystemConfigs for BoxedSystem<(), ()>` as I
couldn't figure out a way to keep the impl without double boxing.

## Testing

- ran `many_foxes` example to check if it still runs.

## Migration Guide

- `IntoSystemConfigs` has been removed for `BoxedSystem<(), ()>`. Either
use `InfallibleSystemWrapper` before boxing or make your system return
`bevy::ecs::prelude::Result`.
@Nilirad Nilirad mentioned this pull request Feb 4, 2025
Open
JeanMertz added a commit to dcdpr/bevy that referenced this pull request Feb 7, 2025
@JeanMertz
feat(ecs): implement fallible observer systems
20e04dd 
This commit builds on top of the work done in bevyengine#16589 and bevyengine#17051, by
adding support for fallible observer systems.

As with the previous work, the actual results of the observer system
are suppressed by default, but the intention is to provide a way to
handle errors in a global way.

Until then, you can use a `PipeSystem` to manually handle results.

Signed-off-by: Jean Mertz <[email protected]>
JeanMertz added a commit to dcdpr/bevy that referenced this pull request Feb 7, 2025
@JeanMertz
feat(ecs): implement fallible observer systems
2f4b47f 
This commit builds on top of the work done in bevyengine#16589 and bevyengine#17051, by
adding support for fallible observer systems.

As with the previous work, the actual results of the observer system
are suppressed by default, but the intention is to provide a way to
handle errors in a global way.

Until then, you can use a `PipeSystem` to manually handle results.

Signed-off-by: Jean Mertz <[email protected]>
@JeanMertz JeanMertz mentioned this pull request Feb 7, 2025
Merged
JeanMertz added a commit to dcdpr/bevy that referenced this pull request Feb 9, 2025
@JeanMertz
feat(ecs): configurable error handling for fallible systems
4d1cb90 
You can now configure error handlers for fallible systems. These can be
configured on several levels:

- Globally via `App::set_systems_error_handler`
- Per-schedule via `Schedule::set_error_handler`
- Per-system via a piped system (this is existing functionality)

The "fallible_systems" example demonstrates the new functionality.

This builds on top of bevyengine#17731, bevyengine#16589, bevyengine#17051.

Signed-off-by: Jean Mertz <[email protected]>
@JeanMertz JeanMertz mentioned this pull request Feb 9, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request Feb 11, 2025
@JeanMertz
feat(ecs): configurable error handling for fallible systems (#17753)
fd67ca7 
You can now configure error handlers for fallible systems. These can be
configured on several levels:

- Globally via `App::set_systems_error_handler`
- Per-schedule via `Schedule::set_error_handler`
- Per-system via a piped system (this is existing functionality)

The default handler of panicking on error keeps the same behavior as
before this commit.

The "fallible_systems" example demonstrates the new functionality.

This builds on top of #17731, #16589, #17051.

---------

Signed-off-by: Jean Mertz <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Feb 11, 2025
@JeanMertz
feat(ecs): implement fallible observer systems (#17731)
7d8504f 
This commit builds on top of the work done in #16589 and #17051, by
adding support for fallible observer systems.

As with the previous work, the actual results of the observer system are
suppressed for now, but the intention is to provide a way to handle
errors in a global way.

Until then, you can use a `PipeSystem` to manually handle results.

---------

Signed-off-by: Jean Mertz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

+1 more reviewer

@NthTensor NthTensor NthTensor approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Performance A change motivated by improving speed, memory usage or compile times 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

Milestone

0.16

Development

Successfully merging this pull request may close these issues.

4 participants

@hymm @alice-i-cecile @NthTensor @PixelDust22