FlaggedStorage #93

Aceeri · 2017-01-20T05:40:48Z

A basic implementation for a TrackedStorage (#89).

This version of the TrackedStorage does not implement a way to compare the past version and the modifies version of the components. Usage requires unwrapping the storage to the base TrackedStorage which implements Join for flagged components.

Alternative design choices:

Implement TrackedStorage similar to how MaskedStorage works.

Pros:

Would make it possible to have the Join of the storage only work on the flagged components without calling any additional methods.
Might open up a path for a generic storage wrapper like UnprotectedStorage but instead as a container for how the Join gets the components necessary for the loop (this could be interesting since it would allow for different methods other than bitsets, but would also be pretty hard to get right).

Cons:

Difficult to get all the components instead of just the flagged ones as there is no current way to directly get access to the container storage.
Might be unintuitive at a glance as you would expect to get all the components when joining the storage.

Compare modifications for more accurate flags

Pros:

Removes a large concern for if the user iterates over a mutable tracked storage.

Cons:

Performance benefits of the tracked storage decreases.
Difficult to implement. Requires a way to directly compare the new "modified" version with the past version.

kvark

Sorry if this has already been discussed (and I forgot), but I feel that comps.get_mut(entity) is something we can do better at

kvark · 2017-02-06T01:22:54Z

src/storage.rs

+///         // Instead do something like:
+///         for (entity, comp) in (&entities, &comps.check()).iter() {
+///             if true { // check whether you should modify this component or not.
+///                 let mut comp = comps.get_mut(entity);


Hmm, we should be able to make this nicer. Asking for get_mut on an entity that we are currently iterating on would be slower than just giving the value right away. Perhaps, the check() could iterate over some nice wrapper aka std::hashmap::Entry, so that actually accessing the data becomes straightforward (in terms of performance)?

I'm assuming the main problem is just that calling get_mut means it is checking again whether the entity has the component, even though we already know?

kvark · 2017-02-06T01:24:21Z

src/storage.rs

+
+impl<C: Component, T: UnprotectedStorage<C>> TrackedStorage<C, T> {
+    /// All components will be cleared of being flagged.
+    pub fn clear(&mut self) {


needs to be renamed to clear_flags

Aceeri · 2017-02-06T04:11:25Z

Also, this runs into the same problem as the Join implementation for a &'a mut Storage. Maybe it might be better to have something like a Join and JoinMut?

kvark · 2017-02-06T16:16:59Z

I'm assuming the main problem is just that calling get_mut means it is checking again whether the entity has the component, even though we already know?

Yes.

Maybe it might be better to have something like a Join and JoinMut?

I don't think JoinMut would solve that. It would require all the iterated things to be mutable, while we need to mix and match them.

torkleyy · 2017-06-01T20:45:27Z

src/storage.rs

+    }
+    unsafe fn get_mut(&mut self, id: Index) -> &mut C {
+        // calling `.iter()` on a mutable reference to the storage will flag everything
+        self.mask.add(id);


I imagine that such a TrackedStorage would be best used in the following way:

// pos is a component storage with a `TrackedStorage` for (vel, pos) in (&vel, &mut pos).join() { *pos += vel * delta_time; }

In another system:

for (pos_handle, matrix) in (pos.check(), &mut mvp).join() { if pos.flagged(pos_handle) { // Recalc } else { // Use cached version } }

It's inconvenient to do whatever checks and often too complicated, instead we might want a method on UnprotectedStorage which is called after every iterated component, where we check for equality. This would also solve the lifetime problem of a guard.

What would a cached version be? Wouldn't the matrix just stay the same? Although good idea of using Entry<'a> for TrackedStorage.

@Aceeri Yeah, the matrix would stay just the same. The comment above is just not very clear about that.

torkleyy

Great, thanks!

torkleyy · 2017-06-06T04:26:28Z

src/storage/storages.rs

+///
+/// **Note: Never use `.iter()` on a mutable component storage that uses this.**
+///
+///# Example Usage:


torkleyy · 2017-06-06T04:31:58Z

In case you don't want to add or change anything, I'd say you can merge it after squashing.

bors delegate=Aceeri

Not sure if bors knows "delegate", otherwise I'll merge it.

Documentation about common pitfalls and usage. Join implementations for only iterating over flagged components.

Aceeri · 2017-06-06T05:58:02Z

bors r+

bors · 2017-06-06T05:58:02Z

🔒 Permission denied

torkleyy · 2017-06-06T05:58:54Z

src/storage/storages.rs

+/// }
+///# fn main() { }
+/// ```
+pub struct TrackedStorage<C, T> {


I just had an idea:

Given that both this storage and ChangeStorage track the mutation of their components, maybe we should rename this one to FlaggedStorage?

torkleyy · 2017-06-06T06:06:22Z

Nice we finally have a FlaggedStorage 🎉
Thanks @Aceeri!

bors r+

93: FlaggedStorage r=torkleyy A basic implementation for a `TrackedStorage` (#89). This version of the `TrackedStorage` does not implement a way to compare the past version and the modifies version of the components. Usage requires unwrapping the storage to the base `TrackedStorage` which implements `Join` for flagged components. ### Alternative design choices: **Implement `TrackedStorage` similar to how `MaskedStorage` works.** _Pros:_ - Would make it possible to have the `Join` of the storage only work on the flagged components without calling any additional methods. - Might open up a path for a generic storage wrapper like `UnprotectedStorage` but instead as a container for how the `Join` gets the components necessary for the loop (this could be interesting since it would allow for different methods other than bitsets, but would also be pretty hard to get right). _Cons:_ - Difficult to get all the components instead of just the flagged ones as there is no current way to directly get access to the container storage. - Might be unintuitive at a glance as you would expect to get all the components when joining the storage. **Compare modifications for more accurate flags** _Pros:_ - Removes a large concern for if the user iterates over a mutable tracked storage. _Cons:_ - Performance benefits of the tracked storage decreases. - Difficult to implement. Requires a way to directly compare the new "modified" version with the past version.

bors · 2017-06-06T06:13:27Z

Build succeeded

continuous-integration/travis-ci/push

kvark self-requested a review January 20, 2017 20:42

Aceeri force-pushed the tracked branch from 300195c to e978d4a Compare January 21, 2017 02:08

kvark requested a review from a user February 6, 2017 01:20

kvark requested changes Feb 6, 2017

View reviewed changes

torkleyy reviewed Jun 1, 2017

View reviewed changes

Aceeri force-pushed the tracked branch from e978d4a to f99fb2f Compare June 5, 2017 11:56

kvark approved these changes Jun 6, 2017

View reviewed changes

torkleyy self-requested a review June 6, 2017 03:58

torkleyy approved these changes Jun 6, 2017

View reviewed changes

Aceeri added 2 commits June 5, 2017 22:51

TrackedStorage structure

a244932

Documentation about common pitfalls and usage. Join implementations for only iterating over flagged components.

Rename clear, use Entry for checking if flagged

490a3a7

Aceeri force-pushed the tracked branch from d036944 to 7f8b4e4 Compare June 6, 2017 05:51

torkleyy reviewed Jun 6, 2017

View reviewed changes

Add tests, implementations of EntityIndex for references

302e9e3

Aceeri force-pushed the tracked branch from 7f8b4e4 to 302e9e3 Compare June 6, 2017 06:04

torkleyy changed the title ~~TrackedStorage~~ FlaggedStorage Jun 6, 2017

bors bot merged commit 302e9e3 into amethyst:master Jun 6, 2017

This was referenced Jun 8, 2017

TrackedStorage (flagged component storage) #89

Closed

ChangedStorage #170

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FlaggedStorage #93

FlaggedStorage #93

Aceeri commented Jan 20, 2017

kvark left a comment

kvark Feb 6, 2017

Aceeri Feb 6, 2017

kvark Feb 6, 2017

Aceeri commented Feb 6, 2017 •

edited

Loading

kvark commented Feb 6, 2017

torkleyy Jun 1, 2017

Aceeri Jun 5, 2017 •

edited

Loading

torkleyy Jun 5, 2017

torkleyy left a comment

torkleyy Jun 6, 2017

torkleyy commented Jun 6, 2017 •

edited

Loading

Aceeri commented Jun 6, 2017

bors bot commented Jun 6, 2017

torkleyy Jun 6, 2017

torkleyy commented Jun 6, 2017

bors bot commented Jun 6, 2017

FlaggedStorage #93

FlaggedStorage #93

Conversation

Aceeri commented Jan 20, 2017

Alternative design choices:

kvark left a comment

Choose a reason for hiding this comment

kvark Feb 6, 2017

Choose a reason for hiding this comment

Aceeri Feb 6, 2017

Choose a reason for hiding this comment

kvark Feb 6, 2017

Choose a reason for hiding this comment

Aceeri commented Feb 6, 2017 • edited Loading

kvark commented Feb 6, 2017

torkleyy Jun 1, 2017

Choose a reason for hiding this comment

Aceeri Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

torkleyy Jun 5, 2017

Choose a reason for hiding this comment

torkleyy left a comment

Choose a reason for hiding this comment

torkleyy Jun 6, 2017

Choose a reason for hiding this comment

torkleyy commented Jun 6, 2017 • edited Loading

Aceeri commented Jun 6, 2017

bors bot commented Jun 6, 2017

torkleyy Jun 6, 2017

Choose a reason for hiding this comment

torkleyy commented Jun 6, 2017

bors bot commented Jun 6, 2017

Build succeeded

Aceeri commented Feb 6, 2017 •

edited

Loading

Aceeri Jun 5, 2017 •

edited

Loading

torkleyy commented Jun 6, 2017 •

edited

Loading