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

Easier mutation of schedules within the Schedule resource #13185

Closed
lee-orr opened this issue May 2, 2024 · 4 comments · Fixed by #13193
Closed

Easier mutation of schedules within the Schedule resource #13185

lee-orr opened this issue May 2, 2024 · 4 comments · Fixed by #13193
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes X-Contentious There are nontrivial implications that should be thought through

Comments

@lee-orr
Copy link
Contributor

lee-orr commented May 2, 2024

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

Right now, if I want to add systems to a schedule to the schedules resource directly (not via an App), I have to manually request it, check if it exists and if not create and insert a new schedule.

Something like this:

        match schedules.get_mut(startup) {
            Some(schedule) => {
                schedule.add_systems(|world: &mut World| {
                    let _ = world.try_run_schedule(StateTransition);
                });
            }
            None => {
                let mut schedule = Schedule::new(startup);

                schedule.add_systems(|world: &mut World| {
                    let _ = world.try_run_schedule(StateTransition);
                });

                schedules.insert(schedule);
            }
        }

Unlike HashMap, for example, which has the .entry(&mut self, key: &K).or_insert( ... ) pattern that lets you work though it as a single step.

What solution would you like?

  • Add a Schedules::add_systems(impl ScheduleLabel, impl IntoSystem) function - essentially moving the abstraction for adding systems further into the schedules resource from the sub_app (still using sub_app to access it as well).

What alternative(s) have you considered?

  • Adding a Schedules::get_or_insert_mut(&mut self, label: impl ScheduleLabel) -> &mut Schedule method that allows you to get a schedule at a certain label - and creates it if it doesn't exist. I feel that this is less ergonomic than the above suggestion, but might add some additional capabilities in limited situations - so it could still be worth adding.

  • An approach similar to HashMap - where you use Schedules::get(..._).or_insert(...). For "HashMap" - we don't know the exact structure of a new Value, but since here we do - I think the above two options work better on the whole.

Additional context

Brought up by @MiniaczQ in #11426 (comment)

@lee-orr lee-orr added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels May 2, 2024
@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 X-Uncontroversial This work is generally agreed upon D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes X-Contentious There are nontrivial implications that should be thought through and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled X-Uncontroversial This work is generally agreed upon labels May 2, 2024
@alice-i-cecile
Copy link
Member

Yep, I think I'm on board with this!

@lee-orr
Copy link
Contributor Author

lee-orr commented May 3, 2024

Can you assign this to me please?

@lee-orr
Copy link
Contributor Author

lee-orr commented May 3, 2024

@alice-i-cecile Would it be controversial if I added "ignore_ambiguity" & "configer_sets" to the Schedules resource as well? They have the same basic access pattern, so it feels like a good fit.

@alice-i-cecile
Copy link
Member

No objections at all. I think that's the right choice, and is a really nice UX feature for those using bevy_ecs on its own.

github-merge-queue bot pushed a commit that referenced this issue May 3, 2024
# Objective

Resolves #13185 

## Solution

Move the following methods from `sub_app` to the `Schedules` resource,
and use them in the sub app:

- `add_systems`
- `configure_sets`
- `ignore_ambiguity`

Add an `entry(&mut self, label: impl ScheduleLabel) -> &mut Schedule`
method to the `Schedules` resource, which returns a mutable reference to
the schedule associated with the label, and creates one if it doesn't
already exist. (build on top of the `entry(..).or_insert_with(...)`
pattern in `HashMap`.

## Testing

- Did you test these changes? If so, how? Added 4 unit tests to the
`schedule.rs` - one that validates adding a system to an existing
schedule, one that validates adding a system to a new one, one that
validates configuring sets on an existing schedule, and one that
validates configuring sets on a new schedule.
- I didn't add tests for `entry` since the previous 4 tests use
functions that rely on it.
- I didn't test `ignore_ambiguity` since I didn't see examples of it's
use, and am not familiar enough with it to know how to set up a good
test for it. However, it relies on the `entry` method as well, so it
should work just like the other 2 methods.
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 D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants