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

Add configure_schedules to App and Schedules to apply ScheduleBuildSettings to all schedules #9514

Merged
merged 4 commits into from
Aug 28, 2023

Conversation

DevinLeamy
Copy link
Contributor

@DevinLeamy DevinLeamy commented Aug 20, 2023

Objective

Solution

  • Adds
fn configure_schedules(&mut self, schedule_build_settings: ScheduleBuildSettings)

to Schedules, and App to simplify applying ScheduleBuildSettings to all schedules.


Migration Guide

  • No breaking changes.
  • Adds Schedule::get_build_settings() getter for the schedule's ScheduleBuildSettings.
  • Can replaced manual configuration of all schedules:
// Old 
for (_, schedule) in app.world.resource_mut::<Schedules>().iter_mut() {
    schedule.set_build_settings(build_settings);
}

// New
app.configure_schedules(build_settings);

@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 A-App Bevy apps and plugins labels Aug 20, 2023
@DevinLeamy DevinLeamy changed the title Fixes [#9508]: Add configure_schedules to App and Schedules to apply a ScheduleBuildSettings to all schedules Add configure_schedules to App and Schedules to apply ScheduleBuildSettings to all schedules Aug 20, 2023
@alice-i-cecile
Copy link
Member

Thanks!

@alice-i-cecile alice-i-cecile requested a review from hymm August 20, 2023 19:20
Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

I worry a little that there isn't a way to turn on say ambiguity detection for all schedules, but leave the rest of the settings untouched. But I think the predominant usage will be to just set all the schedules the same anyways, so going to approve and see what happens.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Aug 21, 2023
@alice-i-cecile
Copy link
Member

That can still be done manually, using the workaround in the linked issue plus struct update syntax :) If that turns out to be a common pattern in practice though we can add a new method.

@hymm
Copy link
Contributor

hymm commented Aug 21, 2023

There's no getter for the current build settings, so you can't use the struct update syntax.

@@ -198,6 +205,11 @@ impl Schedule {
self
}

/// Returns the schedule's current `ScheduleBuildSettings`.
pub fn get_build_settings(&self) -> ScheduleBuildSettings {
self.graph.settings.clone()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clone because it's a light struct and we're not mutating the return value.

@DevinLeamy
Copy link
Contributor Author

DevinLeamy commented Aug 22, 2023

Makes sense to add the ability to chain configure_schedules() and #9526 to allow for:

  App::new()
        // ...
        .configure_schedules(ScheduleBuildSettings {
            ambiguity_detection: LogLevel::Error,
            ..schedule.get_build_settings()
        })
       // ...
       .run()

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 28, 2023
Merged via the queue into bevyengine:main with commit a8dc835 Aug 28, 2023
@DevinLeamy DevinLeamy deleted the dl/configure-all-schedules branch August 28, 2023 19:29
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
…ettings` to all schedules (bevyengine#9514)

# Objective

- Fixes: bevyengine#9508 
- Fixes: bevyengine#9526 

## Solution

- Adds
```rust 
fn configure_schedules(&mut self, schedule_build_settings: ScheduleBuildSettings)
``` 
to `Schedules`, and `App` to simplify applying `ScheduleBuildSettings`
to all schedules.

---

## Migration Guide
- No breaking changes.
- Adds `Schedule::get_build_settings()` getter for the schedule's
`ScheduleBuildSettings`.
- Can replaced manual configuration of all schedules:
```rust
// Old 
for (_, schedule) in app.world.resource_mut::<Schedules>().iter_mut() {
    schedule.set_build_settings(build_settings);
}

// New
app.configure_schedules(build_settings);
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-App Bevy apps and plugins A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use 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
3 participants