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

Create a System Builder Syntax instead of using System Descriptors #2736

Closed
wants to merge 40 commits into from
Closed

Create a System Builder Syntax instead of using System Descriptors #2736

wants to merge 40 commits into from

Conversation

thechubbypanda
Copy link

@thechubbypanda thechubbypanda commented Aug 27, 2021

Objective

Closes #1978

Solution

Working around @cart's [design](https://github.com/bevyengine/bevy/issues/1978#issuecomment-825337056], implement a builder syntax for systems such that we can do something like:

App::new().add_system(
    foo.add_to_stage(Stage::Label)
        .before(Stage::Label2)
        .startup()
)

Please do watch this pr and comment on the changes made :)
Made with love and significant help from @Ratysz XD

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Aug 27, 2021
@alice-i-cecile alice-i-cecile self-requested a review August 27, 2021 15:06
@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 and removed S-Needs-Triage This issue needs to be labelled labels Aug 27, 2021
@thechubbypanda
Copy link
Author

thechubbypanda commented Sep 4, 2021

@alice-i-cecile, could you please clarify what you mean by

main docs for each configuration method

and also

point to the configuration methods for each relevant concept

from where?

Keval Kapdee and others added 3 commits September 5, 2021 19:51
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@alice-i-cecile
Copy link
Member

main docs for each configuration method

So, this is "the method to add a system to a stage should point to the Stage docs" and so on.

point to the configuration methods for each relevant concept

from where?

From e.g. the Stage docs. The idea here is that we want bidirectional links between "the thing we're configuring" and "the tool used to configure it".

@thechubbypanda
Copy link
Author

Documentation checklist

I believe all are now complete bar the book :)

@IceSentry
Copy link
Contributor

IceSentry commented Sep 9, 2021

Could this also be ported to SystemSet? That would allow things like this:
Nevermind, it's already done.

I'm also curious what happens if a system in a SystemSet specifies a stage like so

.add_system_set(
	SystemSet::new()
		.stage(Stage::Label)
		.with_system(example_system.stage(Stage::OtherLabel)))

@Ratysz
Copy link
Contributor

Ratysz commented Sep 9, 2021

.add_system_set_to_stage() shouldn't be a thing anymore. If it is, that's an oversight in the PR.

@IceSentry
Copy link
Contributor

@Ratysz yeah, sorry that was copy pasted from the first part of my question before I noticed it was removed and I removed that part of my comment. It's not in the code anymore.

@Ratysz
Copy link
Contributor

Ratysz commented Sep 9, 2021

Right. I don't remember if it came up during implementation, but that should crash, the same as with extra run criteria.

@thechubbypanda
Copy link
Author

We don't actually throw any errors when a system has it's own stage and is also inside a set. On baking the SystemSet, we either overwrite or add to data in the systems.

@Ratysz
Copy link
Contributor

Ratysz commented Sep 11, 2021

Well... I think both the system and the set it's in having a stage label should be a hard error, because it's code that doesn't have an obvious correct pathway. See here for similar discussion about run criteria.

Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

It's not particularly important to fix this, but some of the impl block have inconsistent spacing between functions. MOst of the codebase has a space between each function.

Overall, I really like this new api.

crates/bevy_app/src/app.rs Show resolved Hide resolved
crates/bevy_app/src/app.rs Show resolved Hide resolved
@@ -229,6 +229,23 @@ impl App {
}

pub fn add_system_set(&mut self, set: SystemSet) -> &mut Self {
fn ensure_no_conflicts(set: &SystemSet) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could keep a reference to the conflicting stage name and add that to the panic message.

Small suggestion, you could use this instead conflict = set.systems.iter().any(|system| system.config().stage != set.config().stage);, but that's entirely up to preference.

@alice-i-cecile alice-i-cecile added the S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged label Sep 22, 2021
@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Apr 22, 2022
@thechubbypanda thechubbypanda closed this by deleting the head repository Jan 27, 2023
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Feb 6, 2023

Fixed by #7267.

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 S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged X-Controversial There is active debate or serious implications around merging this PR
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Extensible builder syntax for system insertion
4 participants