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

Convert to fallible system in IntoSystemConfigs #17051

Merged
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?

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.

Copy link
Contributor

@NthTensor NthTensor left a comment

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

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
@Neo-Zhixing
Copy link
Contributor

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

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-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
Development

Successfully merging this pull request may close these issues.

4 participants