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

[Merged by Bors] - Add system sets and run criteria example #1909

Closed
wants to merge 4 commits into from
Closed

[Merged by Bors] - Add system sets and run criteria example #1909

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 13, 2021

If accepted, fixes #1694 .

I wanted to explore system sets and run criteria a bit, so made an example expanding a bit on the snippets shown in the 0.5 release post.

Shows a couple of system sets, uses system labels, run criterion, and a use of the interesting RunCriterion::pipe functionality.

@mockersf mockersf added A-ECS Entities, components, systems, and events C-Examples An addition or correction to our examples labels Apr 13, 2021
@ghost
Copy link
Author

ghost commented Apr 13, 2021

I think I'm CI blocked until this is in: #1906

// This shows that we can modify existing run criteria results.
// Here we create a _not done_ criteria by piping the output of
// the `done` system and inverting the output.
.with_run_criteria(RunCriteria::pipe("done", inverse.system())),
Copy link
Contributor

@MinerSebas MinerSebas Apr 13, 2021

Choose a reason for hiding this comment

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

The "done" string realy confused me here, as i thought it was somehow finding the correct function through a string alone. After reading further I realised that this string is actualy a label.
To reduce this confusion, I suggest to use another Type that derives SystemLabel instead.

Copy link
Author

Choose a reason for hiding this comment

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

I varied names slightly to differentiate better.

I wanted to have an example of using the string literal for creating a SystemLabel since my intuition says there are probably users who would like to uses labels but not create extra types for it.

Hopefully the new names and added comment makes things clearer.

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

This looks good to me. Part of me wants to avoid "piping" run criteria at all as its a pretty niche feature and makes the example harder to follow. But this does do a good job of illustrating its use so I'm down to keep it.

.label(PostPhysics)
// `collision` and `sfx` are not ordered with respect to
// each other, and may run in any order
.with_system(collision.system())
Copy link
Member

Choose a reason for hiding this comment

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

Small nit: I like to define systems last in system sets as a general pattern. This creates a nice SetConfig -> Systems order.

Copy link
Author

Choose a reason for hiding this comment

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

Ok! I'm all for consistency. Updated.

@@ -51,6 +51,11 @@ fn main() {
App::build()
.add_plugins(DefaultPlugins)
.init_resource::<Done>()
// Note that the system sets added in this example sets their run criteria explicitly.
Copy link
Member

Choose a reason for hiding this comment

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

nit: "Note that the system sets added in this example set their run criteria explicitly."

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

* Typo: sets -> set
* Reorder system sets to add systems last

Signed-off-by: Torstein Grindvik <[email protected]>
@ghost
Copy link
Author

ghost commented Apr 23, 2021

This looks good to me. Part of me wants to avoid "piping" run criteria at all as its a pretty niche feature and makes the example harder to follow. But this does do a good job of illustrating its use so I'm down to keep it.

Yes the pipe usage was a bit shoehorned in.
But I didn't really get what is was for/how to use it until I found some use of it in the wild, so I think it has some value being used somewhere.

@cart
Copy link
Member

cart commented Apr 23, 2021

bors r+

bors bot pushed a commit that referenced this pull request Apr 23, 2021
If accepted, fixes #1694 .

I wanted to explore system sets and run criteria a bit, so made an example expanding a bit on the snippets shown in the 0.5 release post.

Shows a couple of system sets, uses system labels, run criterion, and a use of the interesting `RunCriterion::pipe` functionality.
@bors bors bot changed the title Add system sets and run criteria example [Merged by Bors] - Add system sets and run criteria example Apr 23, 2021
@bors bors bot closed this Apr 23, 2021
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
If accepted, fixes bevyengine#1694 .

I wanted to explore system sets and run criteria a bit, so made an example expanding a bit on the snippets shown in the 0.5 release post.

Shows a couple of system sets, uses system labels, run criterion, and a use of the interesting `RunCriterion::pipe` functionality.
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-Examples An addition or correction to our examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SystemSets and run criteria need an example
4 participants