[Merged by Bors] - Make `StartupSet` a base set #7574

TimJentzsch · 2023-02-08T21:23:09Z

Objective

Make StartupSet a base set

Solution

Add #[system_set(base)] to the enum declaration
Replace .in_set(StartupSet::...) with .in_base_set(StartupSet::...)

Note: I don't really know what I'm doing and what exactly the difference between base and non-base sets are. I mostly opened this PR based on discussion in Discord. I also don't really know how to test that I didn't break everything. Your reviews are appreciated!

Changelog

StartupSet is now a base set

Migration Guide

StartupSet is now a base set. This means that you have to use .in_base_set instead of .in_set:

Before

app.add_system(foo.in_set(StartupSet::PreStartup))

After

app.add_system(foo.in_base_set(StartupSet::PreStartup))

alice-i-cecile

This is almost the correct set of changes, assuming that you didn't end up missing any uses :)

Base sets are intended to be "broad organization tools for schedules" that try to warn users about free-floating systems. I think this makes sense for both of these sets: running startup systems or rendering systems that are not ordered relative to these sets is likely to result in very surprising ambiguities.

To finish this up, just define a default base set for the CoreSchedule::Startup. I don't think the RenderSchedule should have one: there's no clear default.

…dule

TimJentzsch · 2023-02-08T21:40:30Z

To finish this up, just define a default base set for the CoreSchedule::Startup.

Done.

I think there is also an opportunity to make this more readable with system chaining, but the diff is quite large already so I'll do that in a follow-up PR.

TimJentzsch · 2023-02-08T22:11:25Z

Whoops, I didn't mean to remove the review requests, and I also can't add them back. Sorry about that

maniwani · 2023-02-13T18:32:35Z

crates/bevy_render/src/lib.rs

        let mut schedule = Schedule::new();



Does the render schedule have a default base set?

No, I went by Alice's comment:

I don't think the RenderSchedule should have one: there's no clear default.

But I can add one if needed :)

Looking at how rendering systems were defined and ordered, it wasn't at all clear what the correct "default" would be.

Are there any errors that pop up if you add a system to a schedule that has base sets but doesn't pick one as default?

How would I best test that?

It should be as simple as trying render_schedule.add_system(my_system). ~~I'm pretty sure that should error with something like "my_system needs to be put into a base set", but worth checking.~~

edit: tested this and it doesn't error. Feels like it should or why bother changing them to be base sets.

It might make sense to change this to only change the startup schedule to use base sets, but move changing the render schedule to a different pr. Feels like there's a missing configuration option to require that a base set be set, but not set a default set.

Ok, I reverted the RenderSet changes and updated the PR description.

This reverts commit a01a0f0.

alice-i-cecile · 2023-02-14T21:29:28Z

Totally fine with splitting out the RenderSet changes: they're much less important. Remember to update the title and description.

TimJentzsch · 2023-02-14T21:30:06Z

Done.

TimJentzsch · 2023-02-14T21:35:03Z

I "tested" the changes by running some random examples, it doesn't seem to crash.

Question: With it being a base set, can .in_base_set(StartupSet::Startup) be removed/replaced somehow because StartupSet::Startup is the default now?

alice-i-cecile · 2023-02-14T21:45:50Z

Question: With it being a base set, can .in_base_set(StartupSet::Startup) be removed/replaced somehow because StartupSet::Startup is the default now?

This should be removed now :)

TimJentzsch · 2023-02-14T22:23:09Z

Done, .in_base_set(StartupSet::Startup) calls have been removed.

alice-i-cecile · 2023-02-19T03:09:49Z

bors r+

# Objective Closes #7573 - Make `StartupSet` a base set ## Solution - Add `#[system_set(base)]` to the enum declaration - Replace `.in_set(StartupSet::...)` with `.in_base_set(StartupSet::...)` **Note**: I don't really know what I'm doing and what exactly the difference between base and non-base sets are. I mostly opened this PR based on discussion in Discord. I also don't really know how to test that I didn't break everything. Your reviews are appreciated! --- ## Changelog - `StartupSet` is now a base set ## Migration Guide `StartupSet` is now a base set. This means that you have to use `.in_base_set` instead of `.in_set`: ### Before ```rs app.add_system(foo.in_set(StartupSet::PreStartup)) ``` ### After ```rs app.add_system(foo.in_base_set(StartupSet::PreStartup)) ```

bors · 2023-02-19T03:26:03Z

Pull request successfully merged into main.

Build succeeded:

build-and-install-on-iOS
build-android
build (macos-latest)
build (ubuntu-latest)
build-wasm
build (windows-latest)
build-without-default-features (bevy)
build-without-default-features (bevy_ecs)
build-without-default-features (bevy_reflect)
check-compiles
check-doc
check-missing-examples-in-docs
ci
markdownlint
msrv
run-examples
run-examples-on-wasm
run-examples-on-windows-dx12

# Objective Closes bevyengine#7573 - Make `StartupSet` a base set ## Solution - Add `#[system_set(base)]` to the enum declaration - Replace `.in_set(StartupSet::...)` with `.in_base_set(StartupSet::...)` **Note**: I don't really know what I'm doing and what exactly the difference between base and non-base sets are. I mostly opened this PR based on discussion in Discord. I also don't really know how to test that I didn't break everything. Your reviews are appreciated! --- ## Changelog - `StartupSet` is now a base set ## Migration Guide `StartupSet` is now a base set. This means that you have to use `.in_base_set` instead of `.in_set`: ### Before ```rs app.add_system(foo.in_set(StartupSet::PreStartup)) ``` ### After ```rs app.add_system(foo.in_base_set(StartupSet::PreStartup)) ```

TimJentzsch added 2 commits February 8, 2023 22:12

Make StartupSet a base set

bf8b87b

Make RenderSet a base set

a01a0f0

alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use labels Feb 8, 2023

alice-i-cecile added this to the 0.10 milestone Feb 8, 2023

alice-i-cecile requested changes Feb 8, 2023

View reviewed changes

alice-i-cecile requested a review from cart February 8, 2023 21:29

alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Feb 8, 2023

alice-i-cecile requested a review from maniwani February 8, 2023 21:29

Define StartupStage::Startup as default base set for the Startup sche…

8639bee

…dule

TimJentzsch requested review from alice-i-cecile and removed request for cart and maniwani February 8, 2023 22:07

alice-i-cecile requested review from cart and maniwani February 8, 2023 22:11

alice-i-cecile approved these changes Feb 8, 2023

View reviewed changes

maniwani reviewed Feb 13, 2023

View reviewed changes

Revert "Make RenderSet a base set"

7ffe2c5

This reverts commit a01a0f0.

TimJentzsch changed the title ~~Make StartupSet and RenderSet base sets~~ Make StartupSet a base set Feb 14, 2023

alice-i-cecile removed the X-Controversial There is active debate or serious implications around merging this PR label Feb 14, 2023

Remove .in_base_set(StartupSet::Startup) because it's redundant

4484f6c

hymm approved these changes Feb 14, 2023

View reviewed changes

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 Feb 14, 2023

bors bot changed the title ~~Make StartupSet a base set~~ [Merged by Bors] - Make StartupSet a base set Feb 19, 2023

bors bot closed this Feb 19, 2023

TimJentzsch deleted the 7573-startupset-renderset-base-sets branch February 19, 2023 09:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Make `StartupSet` a base set #7574

[Merged by Bors] - Make `StartupSet` a base set #7574

TimJentzsch commented Feb 8, 2023 •

edited

alice-i-cecile left a comment

TimJentzsch commented Feb 8, 2023

TimJentzsch commented Feb 8, 2023

maniwani Feb 13, 2023 •

edited

TimJentzsch Feb 13, 2023

alice-i-cecile Feb 13, 2023

maniwani Feb 13, 2023 •

edited

TimJentzsch Feb 13, 2023

hymm Feb 13, 2023 •

edited

hymm Feb 13, 2023 •

edited

TimJentzsch Feb 14, 2023

alice-i-cecile commented Feb 14, 2023

TimJentzsch commented Feb 14, 2023

TimJentzsch commented Feb 14, 2023

alice-i-cecile commented Feb 14, 2023

TimJentzsch commented Feb 14, 2023

alice-i-cecile commented Feb 19, 2023

bors bot commented Feb 19, 2023

[Merged by Bors] - Make StartupSet a base set #7574

[Merged by Bors] - Make StartupSet a base set #7574

Conversation

TimJentzsch commented Feb 8, 2023 • edited

Objective

Solution

Changelog

Migration Guide

Before

After

alice-i-cecile left a comment

Choose a reason for hiding this comment

TimJentzsch commented Feb 8, 2023

TimJentzsch commented Feb 8, 2023

maniwani Feb 13, 2023 • edited

Choose a reason for hiding this comment

TimJentzsch Feb 13, 2023

Choose a reason for hiding this comment

alice-i-cecile Feb 13, 2023

Choose a reason for hiding this comment

maniwani Feb 13, 2023 • edited

Choose a reason for hiding this comment

TimJentzsch Feb 13, 2023

Choose a reason for hiding this comment

hymm Feb 13, 2023 • edited

Choose a reason for hiding this comment

hymm Feb 13, 2023 • edited

Choose a reason for hiding this comment

TimJentzsch Feb 14, 2023

Choose a reason for hiding this comment

alice-i-cecile commented Feb 14, 2023

TimJentzsch commented Feb 14, 2023

TimJentzsch commented Feb 14, 2023

alice-i-cecile commented Feb 14, 2023

TimJentzsch commented Feb 14, 2023

alice-i-cecile commented Feb 19, 2023

bors bot commented Feb 19, 2023

[Merged by Bors] - Make `StartupSet` a base set #7574

[Merged by Bors] - Make `StartupSet` a base set #7574

TimJentzsch commented Feb 8, 2023 •

edited

maniwani Feb 13, 2023 •

edited

maniwani Feb 13, 2023 •

edited

hymm Feb 13, 2023 •

edited

hymm Feb 13, 2023 •

edited