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

add example of creating systems via closures #2231

Closed
wants to merge 1 commit into from

Conversation

cab
Copy link

@cab cab commented May 21, 2021

Prompted by this discord discussion.

This example shows how to pass an arbitrary argument to a closure, and how to then use that closure as a bevy_ecs::system::System.

@mockersf
Copy link
Member

could you also add the example to https://github.com/bevyengine/bevy/blob/main/examples/README.md?

@mockersf
Copy link
Member

mockersf commented May 21, 2021

To pass parameters to your systems, you can use a Local parameter and config it rather than using move to capture arg:

Box::new(|mut cmd: Commands, arg: Local<String>| {
    info!("this system uses the moved arg: {:?}", arg);
    let id = cmd.spawn().id();
    info!("also it spawned an entity {:?}", id);
})
.system()
.config(|config| config.1 = Some("hello".to_string()))

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

cab commented May 21, 2021

Thanks for the review!

To pass parameters to your systems, you can use a Local parameter and config it rather than using move to capture arg:

Box::new(|mut cmd: Commands, arg: Local<String>| {
    info!("this system uses the moved arg: {:?}", arg);
    let id = cmd.spawn().id();
    info!("also it spawned an entity {:?}", id);
})
.system()
.config(|config| config.1 = Some("hello".to_string()))

Interesting, I didn't know about this. Do you think this PR is still useful as an example? (cc @alice-i-cecile)

@mockersf
Copy link
Member

Do you think this PR is still useful as an example?

Yes! We don't show how to pass closures as system, so that's valuable. And it would be even more with the Local/config way, as we don't have an example on that yet

@cab cab force-pushed the cab/closure-example branch from bfe9413 to 26e29fa Compare May 21, 2021 14:08
examples/README.md Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
examples/README.md Outdated Show resolved Hide resolved
examples/ecs/system_closures.rs Outdated Show resolved Hide resolved
info!("this system uses an argument: {:?}", arg);
let id = cmd.spawn().id();
info!("also it spawned an entity {:?}", id);
}).system().config(|config| config.1 = Some("hello".to_string())))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}).system().config(|config| config.1 = Some("hello".to_string())))
}).system())

Unless the .config() is completely necessary here, I feel like it's just adding unnecessary noise.

Copy link
Author

@cab cab May 21, 2021

Choose a reason for hiding this comment

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

I think it is necessary -- that's what sets the Local<String>.

examples/ecs/system_closures.rs Outdated Show resolved Hide resolved
@cab cab force-pushed the cab/closure-example branch from 26e29fa to 7e19ff1 Compare May 21, 2021 17:00
@alice-i-cecile
Copy link
Member

The content itself looks solid, it but could use a bit more motivating text at the start, explaining why you might want to do this.

@NathanSWard NathanSWard self-requested a review May 22, 2021 05:53
@cab
Copy link
Author

cab commented May 23, 2021

The content itself looks solid, it but could use a bit more motivating text at the start, explaining why you might want to do this.

Just to make sure I'm understanding -- should I add a comment/text to the example, or do you mean add more explanation to the PR description?

@alice-i-cecile
Copy link
Member

The former please; it's much more important that end users can understand the motivation. And the reviewers can check the comments in the code ;)

@cab
Copy link
Author

cab commented Jun 30, 2021

sorry for dropping this! will try to wrap it up by the end of this week.

@alice-i-cecile
Copy link
Member

FYI, this has been substantially improved by #2398 and #2403. Check those out and try again using the new impl!

@mockersf
Copy link
Member

mockersf commented Jul 1, 2021

as seen on Discord, .system() removal may not yet work with .config(), we should do a full pass on Bevy first

@Ratysz
Copy link
Contributor

Ratysz commented Jul 1, 2021

#2422 should help with that.

@mockersf mockersf added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 16, 2021
@alice-i-cecile
Copy link
Member

Hey @cab I'm interested in merging this. Are you interested in refreshing this?

If so, please comment in #2373 and refresh this to main :)

@alice-i-cecile
Copy link
Member

Closing due to no response on relicensing.

@alice-i-cecile alice-i-cecile removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Mar 20, 2022
@alice-i-cecile
Copy link
Member

Closing to let someone else pick this work up :) Please credit the author of this PR if you build off it.

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.

5 participants