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.

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

Replace IntoSystemSetConfig with IntoSystemSetConfigs #9247

Merged
merged 11 commits into from
Sep 5, 2023
Merged

Replace IntoSystemSetConfig with IntoSystemSetConfigs #9247

merged 11 commits into from
Sep 5, 2023

Conversation

geieredgar
Copy link
Contributor

@geieredgar geieredgar commented Jul 23, 2023 •
edited
Loading

Objective

Solution

  • Changed the (Into)SystemSetConfigs traits and structs be more like the (Into)SystemConfigs traits and structs.
  • Replaced uses of IntoSystemSetConfig with IntoSystemSetConfigs
  • Added generic ItemConfig and ItemConfigs types.
  • Changed SystemConfig(s) and SystemSetConfig(s) to be type aliases to ItemConfig(s).
  • Added generic process_configs to ScheduleGraph.
  • Changed configure_sets_inner and add_systems_inner to reuse process_configs.

Changelog

  • Added run_if to IntoSystemSetConfigs
  • Deprecated Schedule::configure_set and App::configure_set
  • Removed IntoSystemSetConfig

Migration Guide

  • Use App::configure_sets instead of App::configure_set
  • Use Schedule::configure_sets instead of Schedule::configure_set

@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

@geieredgar geieredgar force-pushed the configure-sets branch 4 times, most recently from 2ad6fc9 to 1ed6a01 Compare July 23, 2023 11:54
@mockersf mockersf added the A-ECS Entities, components, systems, and events label Jul 23, 2023
@JoJoJet JoJoJet self-requested a review July 26, 2023 19:48
@alice-i-cecile alice-i-cecile added this to the 0.12 milestone Aug 10, 2023
@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

alice-i-cecile
alice-i-cecile requested changes Aug 14, 2023
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 really like the refactor; this is way cleaner and easier to review. I think we can make the doc strings nicer, but as long as you explicitly work in the concept of "chaining == system ordering" I'll be happy to approve.

@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

4 similar comments
@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

hymm
hymm reviewed Aug 31, 2023
View reviewed changes
crates/bevy_ecs/src/schedule/config.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/schedule/schedule.rs Outdated Show resolved Hide resolved
@hymm hymm 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 Sep 5, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 5, 2023
@alice-i-cecile alice-i-cecile added M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Sep 5, 2023
Merged via the queue into bevyengine:main with commit 118509e Sep 5, 2023
25 checks passed
@Nilirad Nilirad mentioned this pull request Sep 6, 2023
Merged
@cart cart mentioned this pull request Oct 13, 2023
Merged
43 tasks
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
@geieredgar @alice-i-cecile
Replace IntoSystemSetConfig with IntoSystemSetConfigs (bevyengine…
86d8fdc 
…#9247)

# Objective

- Fixes bevyengine#9244.

## Solution


- Changed the `(Into)SystemSetConfigs` traits and structs be more like
the `(Into)SystemConfigs` traits and structs.
- Replaced uses of `IntoSystemSetConfig` with `IntoSystemSetConfigs`
- Added generic `ItemConfig` and `ItemConfigs` types.
- Changed `SystemConfig(s)` and `SystemSetConfig(s)` to be type aliases
to `ItemConfig(s)`.
- Added generic `process_configs` to `ScheduleGraph`.
- Changed `configure_sets_inner` and `add_systems_inner` to reuse
`process_configs`.

---

## Changelog

- Added `run_if` to `IntoSystemSetConfigs`
- Deprecated `Schedule::configure_set` and `App::configure_set`
- Removed `IntoSystemSetConfig`

## Migration Guide

- Use `App::configure_sets` instead of `App::configure_set`
- Use `Schedule::configure_sets` instead of `Schedule::configure_set`

---------

Co-authored-by: Alice Cecile <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hymm hymm hymm approved these changes

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

@JoJoJet JoJoJet Awaiting requested review from JoJoJet

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-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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.12
Development

Successfully merging this pull request may close these issues.

Deprecate configure_set & make configure_sets work for single params.
4 participants
@geieredgar @hymm @alice-i-cecile @mockersf