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

Schedule resource mutation #13193

Merged
merged 9 commits into from
May 3, 2024

Conversation

lee-orr
Copy link
Contributor

@lee-orr lee-orr commented 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.

…dd systems to either an existing or new schedule with a call to "add_systems"
note - no test was added since there wasn't an example of using it, and I am not familiar enough with it's effects to know how to set up that test
@lee-orr lee-orr marked this pull request as draft May 3, 2024 00:49
@lee-orr lee-orr marked this pull request as ready for review May 3, 2024 01:00
@alice-i-cecile alice-i-cecile self-requested a review May 3, 2024 01:03
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels May 3, 2024
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 agree with moving these up a level. Nice and simple and a more reasonable API.

@alice-i-cecile alice-i-cecile added the X-Contentious There are nontrivial implications that should be thought through label May 3, 2024
Copy link
Contributor

@Trashtalk217 Trashtalk217 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 a good change. There was very little reason for these methods to be exlusive to bevy_app.

Copy link
Contributor

@MiniaczQ MiniaczQ left a comment

Choose a reason for hiding this comment

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

LGTM

@Jondolf Jondolf 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 May 3, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 3, 2024
Merged via the queue into bevyengine:main with commit b9455af May 3, 2024
32 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-Usability A simple quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Easier mutation of schedules within the Schedule resource
5 participants