[Merged by Bors] - Added example of creating a system from a closure #4327

bytemuck · 2022-03-25T01:47:47Z

examples/README.md

alice-i-cecile · 2022-03-25T03:22:33Z

Title suggestion for you @bytemuck (they show up in our commit history, so it's not just bikeshedding): "Added example of creating a system from a closure"

alice-i-cecile

Cool! Overall, I think this could use a bit more explanation about what's going on: for a lot of readers, this will be their first introduction to closures at all.

I would also like to see a non-capturing closure demonstrated too, just to show that you don't have to use a Local.

bytemuck · 2022-03-25T03:38:01Z

Awesome! I'll take a look for this tomorrow. I gotta sleep, I have school tomorrow. 👍

Co-authored-by: Alice Cecile <[email protected]>

alice-i-cecile

Much clearer!

alice-i-cecile · 2022-04-03T23:47:56Z

@bevyengine/docs-team can I get a second review here?

mockersf · 2022-04-04T13:07:37Z

examples/ecs/system_closure.rs

+    App::new()
+        .add_plugin(LogPlugin)
+        // we can use a non-capturing closure as a system
+        .add_startup_system(non_capturing_closure)


why did you use startup systems?

I used a startup system because I wanted it to be called only once.

as you didn't add a scheduler, the app will only run once anyway 🙂

Oh, that's good to know!

mockersf · 2022-04-04T13:08:05Z

examples/ecs/system_closure.rs

+    };
+
+    // create a capturing closure, with a Local value.
+    let capturing_closure = |value: String| {


this is confusing as this is not a capturing closure

I think I miss understood what's a capturing closure, I'll reread about them and change that when I come back after school.

mockersf · 2022-04-04T13:10:30Z

examples/ecs/system_closure.rs

+        .add_startup_system(move |mut arg: Local<String>| {
+            *arg = outside_variable.clone();


why use local to demonstrate capturing? the point of capturing is to not need a local at all

You're right. I don't know why I did that, I'll review my entire PR and recommit when I get home. I think I tried to demonstrate Local and capturing at the same time, and it is just confusing at the end.

alice-i-cecile · 2022-04-13T02:09:37Z

examples/ecs/system_closure.rs

+                outside_variable
+            );
+            // you can use outside_variable, or any other variables inside this closure.
+            // there states will be saved.


Suggested change

// there states will be saved.

a) typo "their", not "there" b) I don't think this line adds much :)

You are probably right about this. I'll remove it tomorrow 🙂.

IceSentry

I agree with alice comment other than that I had no idea this was even possible so that's good to know.

mockersf · 2022-04-13T22:18:14Z

examples/ecs/system_closure.rs

+    // create a closure, with a Local value to be initialized.
+    let complex_closure = |value: String| {
+        move |mut arg: Local<String>| {
+            *arg = value.clone();
+            info!("Hello from a complex closure! {:?}", arg);
+
+            // 'arg' will be saved between calls.
+            // you could also use an outside variable like presented in then inlined closures
+        }
+    };


This is probably not working the way you want it to. Having a Local in a capturing closure is useless. Here you're cloning value to the local at every system execution, so whatever value would the local have would always be replaced by a fresh clone from value. arg would indeed be saved between calls, but the value wouldn't actually be available to the next call.

Instead you can directly use value. The outside closure put it in current scope as mutable, the inner closure capture it and actually modify it as every system execution.

Suggested change

// create a closure, with a Local value to be initialized.

let complex_closure = |value: String| {

move |mut arg: Local<String>| {

*arg = value.clone();

info!("Hello from a complex closure! {:?}", arg);

// 'arg' will be saved between calls.

// you could also use an outside variable like presented in then inlined closures

}

};

let complex_closure = |mut value: String| {

move || {

info!("Hello from a complex closure! {:?}", value);

value = format!("{}o", value);

}

};

This could probably use more comments to explain what's happening...

With some explanation, It makes sense. I should have tested this more aggressively. oops

jondo2010 · 2022-04-27T14:40:25Z

Wouldn't it be much more useful if you showed receiving a resource or entity query?

alice-i-cecile · 2022-04-27T18:01:29Z

Wouldn't it be much more useful if you showed receiving a resource or entity query?

Without the ability to add systems dynamically, this isn't exactly feasible. None of the world's data exists at the time of system initialization. However, once #279 is solved, we should update this example to include that :)

alice-i-cecile · 2022-04-27T18:01:47Z

bors r+

Fixes #4262

bytemuck · 2022-04-27T18:02:57Z

Wouldn't it be much more useful if you showed receiving a resource or entity query?

Maybe it would be more useful, but I think we should use simplicity over utility in this context, because usually, users will already have knowledge about how the engine works. It will most like not be the first example they will read.

bors · 2022-04-27T18:31:56Z

Pull request successfully merged into main.

Build succeeded:

mockersf · 2022-04-27T19:23:43Z

Without the ability to add systems dynamically, this isn't exactly feasible. None of the world's data exists at the time of system initialization. However, once #279 is solved, we should update this example to include that :)

it should already work, you can use as a system: |query: Query<(Entity, &Transform)>| {... whatever}

alice-i-cecile · 2022-04-27T19:29:35Z

Ah; I'd interpreted the request as "capture data from a resource, and then generate a new system that stores that data". But your read is much more reasonable.

Fixes bevyengine#4262

bytemuck added 2 commits March 24, 2022 21:34

added system_closure example

35d07ee

updated examples/README.md

c7e23e1

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 25, 2022

bytemuck added 3 commits March 24, 2022 21:55

updated examples/system_closure.rs

8940d16

formatted code

03a968c

added another example of closure + fixed closures themselves

823c8c4

alice-i-cecile added A-ECS Entities, components, systems, and events C-Examples An addition or correction to our examples and removed S-Needs-Triage This issue needs to be labelled labels Mar 25, 2022

alice-i-cecile reviewed Mar 25, 2022

View reviewed changes

examples/README.md Outdated Show resolved Hide resolved

alice-i-cecile requested changes Mar 25, 2022

View reviewed changes

bytemuck changed the title ~~Added Closure Example~~ Added example of creating a system from a closure Mar 25, 2022

bytemuck and others added 2 commits March 25, 2022 14:42

added non-capturing closure example and inlined closured

1ffc8de

Update examples/README.md

1002d25

Co-authored-by: Alice Cecile <[email protected]>

bytemuck requested a review from alice-i-cecile March 25, 2022 19:00

alice-i-cecile approved these changes Mar 25, 2022

View reviewed changes

tim-blackbird approved these changes Apr 4, 2022

View reviewed changes

mockersf reviewed Apr 4, 2022

View reviewed changes

bytemuck added 2 commits April 4, 2022 20:33

modified the closure, and added comment thoughts in complex_closure

6880dfc

ran cargo fmt

fca997c

alice-i-cecile reviewed Apr 13, 2022

View reviewed changes

IceSentry approved these changes Apr 13, 2022

View reviewed changes

mockersf reviewed Apr 13, 2022

View reviewed changes

fixed typo + clarified comments + removed Local<T> from closure

0905e64

alice-i-cecile approved these changes Apr 14, 2022

View reviewed changes

mockersf approved these changes Apr 27, 2022

View reviewed changes

mockersf 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 Apr 27, 2022

bors bot pushed a commit that referenced this pull request Apr 27, 2022

Added example of creating a system from a closure (#4327)

f1aae38

Fixes #4262

bors bot changed the title ~~Added example of creating a system from a closure~~ [Merged by Bors] - Added example of creating a system from a closure Apr 27, 2022

bors bot closed this Apr 27, 2022

exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022

Added example of creating a system from a closure (bevyengine#4327)

1083fc8

Fixes bevyengine#4262

bytemuck deleted the closure branch June 24, 2022 14:53

wilk10 mentioned this pull request Aug 5, 2022

Add a system closure example #4262

Closed

ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023

Added example of creating a system from a closure (bevyengine#4327)

67e89cd

Fixes bevyengine#4262

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Added example of creating a system from a closure #4327

[Merged by Bors] - Added example of creating a system from a closure #4327

bytemuck commented Mar 25, 2022

alice-i-cecile commented Mar 25, 2022

alice-i-cecile left a comment

bytemuck commented Mar 25, 2022

alice-i-cecile left a comment

alice-i-cecile commented Apr 3, 2022

mockersf Apr 4, 2022

bytemuck Apr 4, 2022 •

edited

Loading

mockersf Apr 4, 2022

bytemuck Apr 4, 2022

mockersf Apr 4, 2022

bytemuck Apr 4, 2022 •

edited

Loading

mockersf Apr 4, 2022

bytemuck Apr 4, 2022

alice-i-cecile Apr 13, 2022

bytemuck Apr 13, 2022

IceSentry left a comment

mockersf Apr 13, 2022

bytemuck Apr 13, 2022

jondo2010 commented Apr 27, 2022

alice-i-cecile commented Apr 27, 2022

alice-i-cecile commented Apr 27, 2022

bytemuck commented Apr 27, 2022

bors bot commented Apr 27, 2022

mockersf commented Apr 27, 2022

alice-i-cecile commented Apr 27, 2022

		.add_startup_system(move \|mut arg: Local<String>\| {
		*arg = outside_variable.clone();

[Merged by Bors] - Added example of creating a system from a closure #4327

[Merged by Bors] - Added example of creating a system from a closure #4327

Conversation

bytemuck commented Mar 25, 2022

alice-i-cecile commented Mar 25, 2022

alice-i-cecile left a comment

Choose a reason for hiding this comment

bytemuck commented Mar 25, 2022

alice-i-cecile left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Apr 3, 2022

Choose a reason for hiding this comment

bytemuck Apr 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bytemuck Apr 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IceSentry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jondo2010 commented Apr 27, 2022

alice-i-cecile commented Apr 27, 2022

alice-i-cecile commented Apr 27, 2022

bytemuck commented Apr 27, 2022

bors bot commented Apr 27, 2022

mockersf commented Apr 27, 2022

alice-i-cecile commented Apr 27, 2022

bytemuck Apr 4, 2022 •

edited

Loading

bytemuck Apr 4, 2022 •

edited

Loading