LazyUpdate: lazily insert and remove components, execute closures on world #221

eldyer · 2017-07-09T22:49:01Z

Remove a component lazily without having to borrow its storage. To be used like this:

let lazy = world.read_resource::<LazyUpdate>();
lazy.add_removal::<SomeComponentType>(some_entity);

kvark · 2017-07-10T13:27:44Z

Looks ok to me. Thanks for the PR!

torkleyy

Thanks for the PR! I'd prefer not to have a second struct actually, see below.

torkleyy · 2017-07-10T14:42:23Z

src/world.rs

+/// component storages have to be borrowed mutably.
+///
+/// This resource is added to the world by default.
+pub struct LazyRemovals {


There is no reason to have a second struct, actually. Given that LazyInsertions haven't been released yet, let's just rename it to LazyUpdate and have a method there.

torkleyy · 2017-07-10T17:04:01Z

src/world.rs

-pub struct LazyInsertions {
-    stack: TreiberStack<Box<LazyInsertInternal>>
+pub struct LazyUpdate {
+    insert_stack: TreiberStack<Box<LazyInsertInternal>>,


Let's also rename LazyInsertInternal to LazyUpdateInternal so we do not need two stacks.

To elaborate this a bit, LazyInsertInternal can just access the World, so we can just use it for both (after renaming). Just accept entities and a type parameter then, have a private struct with PhantomData and implement the removal, please.

Sounds good, I will try!

torkleyy

Looks pretty good! I'd actually prefer to not have a tuple for removals, but that's up to you. I'll review it completely later.

torkleyy · 2017-07-11T09:10:37Z

src/world.rs

-    fn insert(self: Box<Self>, world: &World);
+struct LazyRemovalMarker<F>(PhantomData<F>);
+
+impl<F> LazyRemovalMarker<F> {


Could we just have a struct containing both the entity and the marker?

torkleyy

Sorry - have to do some more nitpicking, I actually wonder if we should just allow lazy.execute(|world| world.hello()), additionally. But that could also be done in another PR.

torkleyy · 2017-07-11T20:37:38Z

src/world.rs

+
+struct LazyRemovalInfo<C> {
+    entity: Entity,
+    marker: LazyRemovalMarker<C>,


I think we can actually inline that type.

Do you mean using #[inline] on a get function (like with the Entity struct)?

No, just use PhantomData here.

Ah right, of course!

torkleyy · 2017-07-11T20:39:23Z

src/world.rs

+    }
+}
+
+/// Lazy updates can be used after creating a new


I'd say in general, they can be used for stuff that should better happen at the end because it needs to borrow many resources. Dealing with initialization of components could be shown as an example of that.

torkleyy · 2017-07-11T20:40:07Z

src/world.rs

    ///     }
    /// }
    /// ```
-    pub fn add<L>(&self, l: L)
+    pub fn add_insertions<L>(&self, l: L)


Maybe it would be better to do a "lazy insert" - so just pub fn insert? Also, the plural is not always applicable.

torkleyy · 2017-07-11T20:40:21Z

src/world.rs

+
+    /// Adds a removal. Please note that this method takes `&self`
+    /// so there's no need to fetch it mutably.
+    pub fn add_removal<C>(&self, e: Entity)


Then this one would be just remove.

torkleyy · 2017-07-11T20:42:17Z

src/world.rs

        where L: LazyInsert + 'static
    {
        self.stack.push(Box::new(l));
    }
+
+    /// Adds a removal. Please note that this method takes `&self`


It would also be nice to have a usage example somewhere.

eldyer · 2017-07-12T09:24:53Z

Thanks for your feedback and no need to apologize, I agree with all points and I'm working on it!

eldyer · 2017-07-12T15:27:54Z

Also implemented execute. Tell me what you think, I'm happy to get your feedback.

torkleyy · 2017-07-12T15:54:01Z

src/world.rs

-/// entity in a system. This way, none of the actual
-/// component storages have to be borrowed mutably.
-///
+impl LazyUpdateInternal for Box<Fn(&World) + Send + Sync> {


Do we really need Box here?

Not sure, I tried without that Box first, but ran into problems. The compiler suggests to add a 'static bound to F in execute, but the passed in closures apparently don't have 'static lifetime. Any idea how to solve this?

Did you try impl<F> LazyUpdateInternal for F where F: Fn(&World) + Send + Sync + 'static?

torkleyy · 2017-07-12T15:54:52Z

src/world.rs

+        self.stack.push(Box::new(LazyRemovalInfo::<C>(e, PhantomData)));
+    }
+
+    /// Lazily executes a closure with world access.


Could also say that "lazily" means "when calling maintain()".

torkleyy

Thanks for your work. I'm wondering if we need the LazyInsert and the LazyRemoval now that we have execute.

torkleyy · 2017-07-12T16:00:12Z

src/world.rs

+    where
+        C: Component + Send + Sync,
+    {
+        self.stack.push(Box::new(LazyRemovalInfo::<C>(e, PhantomData)));


Actually, this could just use execute now. That way, there's no need for the removal struct.

torkleyy · 2017-07-12T16:02:12Z

src/world.rs

    ///     }
    /// }
    /// ```
-    pub fn add<L>(&self, l: L)
+    pub fn insert<L>(&self, l: L)


We could probably divide this method into insert and insert_all to increase performance for multiple insertions; insert_all could just accept an iterator.

Agreed, will do that.
It would be nice if we could use execute in insert as well, to get rid of LazyInsert. But I think then FnOnce would be required, because the component would have to be moved into the closure, right?

Yes. I'm not sure, however, if executing call_once on Box<FnOnce is permitted / if there's a workaround.

eldyer · 2017-07-12T19:41:02Z

Could you take a look? Still needs cleanup, docs and examples, just wanted to get some feedback at this point.

eldyer · 2017-07-12T19:42:41Z

Oops, forgot to remove LazyInsert..

eldyer · 2017-07-12T19:53:30Z

Hm, would it make sense to renameLazyUpdate into LazyExecute or LazyExecution?

torkleyy · 2017-07-12T19:53:59Z

src/world.rs

-        self.stack.push(Box::new(
-            Box::new(f) as Box<Fn(&World) + Send + Sync>)
-        );
+        self.stack.push(Box::new(f));


Much better 😉

torkleyy

Okay, we're nearly there. Just the few nits below - and please don't forget to update the changelog. Since this partially amends my PR, let's summarize it on one line and mention both PRs in brackets.

Really appreciating your efforts 👍

torkleyy · 2017-07-12T19:54:51Z

src/world.rs

-    fn insert(self: Box<Self>, world: &World) {
-        L::insert(*self, world);
+impl<F> LazyUpdateInternal for F
+    where F: FnOnce(&World) + Send + Sync + 'static {


Formatting: { is supposed to be on a new line.

torkleyy · 2017-07-12T19:55:31Z

src/world.rs

-pub struct LazyInsertions {
-    stack: TreiberStack<Box<LazyInsertInternal>>
+/// Please note that the provided methods take `&self`
+/// so there's no need to fetch `LazyUpdate` mutably.


Let's leave the note that it is added to the world by default.

torkleyy · 2017-07-12T19:56:01Z

src/world.rs

+            }
+        });
+    }
+    ///


Missing new line, docs and example usage.

torkleyy

Thank you! These are great changes 👍

torkleyy · 2017-07-13T11:23:59Z

I guess we can merge this now.

bors r+

221: LazyUpdate: lazily insert and remove components, execute closures on world r=torkleyy Remove a component lazily without having to borrow its storage. To be used like this: `let lazy = world.read_resource::<LazyUpdate>();` `lazy.add_removal::<SomeComponentType>(some_entity);`

bors · 2017-07-13T11:24:58Z

Canceled

torkleyy · 2017-07-13T11:25:01Z

Hold on, rebase is wrong

bors r-

eldyer · 2017-07-13T11:25:29Z

Yes I was just about to fix the Changelog...

torkleyy · 2017-07-13T11:25:35Z

The line in the changelog should be in the 0.9.3 section

eldyer · 2017-07-13T11:25:56Z

Yeah just committed... 😄

torkleyy · 2017-07-13T11:26:36Z

Okay, now it's perfect.

bors r+

221: LazyUpdate: lazily insert and remove components, execute closures on world r=torkleyy Remove a component lazily without having to borrow its storage. To be used like this: `let lazy = world.read_resource::<LazyUpdate>();` `lazy.add_removal::<SomeComponentType>(some_entity);`

eldyer · 2017-07-13T11:30:40Z

Thank you by the way for guiding me through this. It's actually my first PR and I learned a lot of things. I totally like that you want to get all the details right... it's a great thing to learn from such talented developers! 👍

bors · 2017-07-13T11:33:50Z

Build succeeded

continuous-integration/travis-ci/push

schuylerkylstra · 2019-08-29T14:46:04Z

src/world.rs

+    /// ```
+    pub fn execute<F>(&self, f: F)
+    where
+        F: FnOnce(&World) + 'static + Send + Sync,


Why is there a Sync requirement here?

Add LazyRemovals

e853998

torkleyy self-requested a review July 10, 2017 14:23

torkleyy approved these changes Jul 10, 2017

View reviewed changes

Merge LazyInsertions and LazyRemovals into LazyUpdate

0c81409

torkleyy reviewed Jul 10, 2017

View reviewed changes

torkleyy added the in progress label Jul 10, 2017

Use only one stack with LazyUpdateInternal

42d6ae4

eldyer changed the title ~~Add LazyRemovals~~ LazyUpdate for lazy component insertions and removals Jul 11, 2017

torkleyy requested changes Jul 11, 2017

View reviewed changes

Use struct for lazy removal info instead of tuple

13ceaec

torkleyy reviewed Jul 11, 2017

View reviewed changes

Add execute method, cleanup, examples added.

16cc88e

torkleyy reviewed Jul 12, 2017

View reviewed changes

Simplification by internally using execute

941c6f1

torkleyy reviewed Jul 12, 2017

View reviewed changes

Cleanup, docs & examples

6bd7693

eldyer changed the title ~~LazyUpdate for lazy component insertions and removals~~ LazyUpdate: lazily insert and remove components, execute closures on world Jul 13, 2017

eldyer added 2 commits July 13, 2017 13:00

Update Changelog

66582b5

Merge branch 'master' into lazy_remove

0e0db9c

torkleyy approved these changes Jul 13, 2017

View reviewed changes

Update CHANGELOG.md

746da1a

bors bot merged commit 746da1a into amethyst:master Jul 13, 2017

eldyer deleted the lazy_remove branch July 16, 2017 12:12

schuylerkylstra reviewed Aug 29, 2019

View reviewed changes

LazyUpdate: lazily insert and remove components, execute closures on world #221

LazyUpdate: lazily insert and remove components, execute closures on world #221

Conversation

eldyer commented Jul 9, 2017 • edited Loading

kvark commented Jul 10, 2017

torkleyy left a comment

Choose a reason for hiding this comment

torkleyy Jul 10, 2017 • 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

torkleyy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torkleyy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eldyer Jul 12, 2017 • 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

torkleyy Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eldyer commented Jul 12, 2017

eldyer commented Jul 12, 2017

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

torkleyy left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eldyer commented Jul 12, 2017

eldyer commented Jul 12, 2017

eldyer commented Jul 12, 2017

Choose a reason for hiding this comment

torkleyy left a 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

torkleyy left a comment • edited Loading

Choose a reason for hiding this comment

torkleyy commented Jul 13, 2017

bors bot commented Jul 13, 2017

Canceled

torkleyy commented Jul 13, 2017

eldyer commented Jul 13, 2017

torkleyy commented Jul 13, 2017

eldyer commented Jul 13, 2017

torkleyy commented Jul 13, 2017

eldyer commented Jul 13, 2017

bors bot commented Jul 13, 2017

Build succeeded

Choose a reason for hiding this comment

eldyer commented Jul 9, 2017 •

edited

Loading

torkleyy Jul 10, 2017 •

edited

Loading

torkleyy left a comment •

edited

Loading

eldyer Jul 12, 2017 •

edited

Loading

torkleyy Jul 11, 2017 •

edited

Loading

torkleyy left a comment •

edited

Loading