Metadata #301

Aceeri · 2017-11-15T06:06:56Z

Moves the wrapper storages into their own separate "observing" structures. This is a breaking change due to no default associated types, but I think it might be better for ergonomics.

Checks won't pass right now, still need to update the docs and such. Somewhat of a proof of concept right now.

fixes #232

This change is

WaDelma · 2017-11-15T07:28:57Z

Tracking issue for associated type defaults rust-lang/rust#29661

torkleyy · 2017-11-17T15:05:00Z

specs-derive/src/lib.rs

@@ -3,14 +3,16 @@
 //!
 //! [sp]: https://slide-rs.github.io/specs-website/



Didn't review this file yet

torkleyy · 2017-11-17T15:06:29Z

src/storage/flagged.rs

 ///             // ...
 ///         }
 ///
 ///         // Clears the tracked storage every frame with this system.
-///         (&mut comps).open().1.clear_flags();
+///         comps.find_mut::<Flagged>().clear_flags();


Inconsistent with the above style

torkleyy · 2017-11-17T15:08:24Z

src/storage/meta.rs

+///
+/// Useful for things like tracking modifications to components, storing sorted lists related
+/// to the storage, etc.
+pub trait Metadata<T>: Default {


When you have a TrackedStorage, doesn't that allow to do basically 99% of the meta stuff you need?

Sorted lists could be useful as well, as well as wanting to use something other than TrackedStorage.

Sorted lists can be built using TrackedStorage, that's one of my main motivations for adding it.

torkleyy · 2017-11-17T15:09:46Z

src/storage/meta.rs

+///
+/// Useful for if you are using a tuple metadata and don't want to index each
+/// sub-metadata using `metadata.0`, `metadata.1`, etc. as well as helping with
+/// modelling generic APIs where something needs a specific metadata structure from


Not sure about that, since the type does not really have a logical meaning.

Also, it's probably a bit inefficient

I doubt it is that inefficient, all it does is forward to self.field which is very likely to be inlined.

Sorry, you're right, I misunderstood first. I think the identifier find confused me.

torkleyy · 2017-11-17T15:11:07Z

src/storage/mod.rs

@@ -148,6 +150,62 @@ impl<T: Component> Drop for MaskedStorage<T> {
    }
 }

+#[derive(Derivative)]
+#[derivative(Default(bound = ""))]
+/// Associates the component's storage with its metadata.


Please place docs before attribs

torkleyy · 2017-11-17T15:11:52Z

src/storage/mod.rs

+#[derive(Derivative)]
+#[derivative(Default(bound = ""))]
+/// Associates the component's storage with its metadata.
+pub struct WrappedStorage<T: Component> {


I don't see why we need such a type.

I think it is better for implementation, otherwise I'd have to repeat myself for every usage of get, get_mut, insert, remove, etc.

torkleyy · 2017-11-17T15:12:18Z

src/storage/mod.rs

+        &self.data.wrapped.meta
+    }
+
+    pub fn find<M>(&self) -> &M


Any reason why this is immutable-only?

There is a mutable version find_mut

torkleyy · 2017-11-17T15:14:14Z

src/storage/tracked.rs

@@ -118,6 +118,7 @@ where
    /// impl Component for Comp {
    ///     // Will use `DenseVecStorage` by default.
    ///     type Storage = TrackedStorage<Self>;
+    ///     type Metadata = ();


So TrackedStorage doesn't work with Metadata?

It could work, I just haven't ported it over like the FlaggedStorage yet.

I don't think we should proceed until TrackedStorage is ported

…ta methods on Storage

kvark

Thanks @Aceeri , great work!

I don't have a good feel of the possible metadata use-cases here. Assuming there are some, moving that definition out is a great step for clarity.

Also, ideally, I'd like to not have to derive Metadata, since it involves more magic code for us.

kvark · 2017-11-22T03:38:01Z

specs-derive/src/lib.rs

+    };
+
+    let count = field.iter().count();
+    let name_list = (0..count).map(|_| name).cloned().collect::<Vec<_>>();


would vec![name; count] work instead?

kvark · 2017-11-22T03:42:25Z

src/lib.rs

@@ -1,5 +1,6 @@
-#![deny(missing_docs)]
+//#![deny(missing_docs)]


any reason this is commented out?

Just commented it out because I was removing/adding different structures often, I'll uncomment and document stuff once I figure out the final design for it.

kvark · 2017-11-22T03:42:59Z

src/lib.rs

@@ -216,10 +220,10 @@ pub use shred::{Dispatcher, DispatcherBuilder, Fetch, FetchId, FetchIdMut, Fetch
 pub use shred::AsyncDispatcher;

 pub use storage::{BTreeStorage, Change, ChangeEvents, DenseVecStorage, DistinctStorage, Entry,
-                  FlaggedStorage, HashMapStorage, InsertResult, MaskedStorage, NormalRestriction,
+                  Flagged, HashMapStorage, HasMeta, InsertResult, MaskedStorage, Metadata, NormalRestriction,


uh, that formatting doesn't work good for lots of uses

kvark · 2017-11-22T03:44:43Z

src/storage/meta.rs

+
+use Index;
+
+/// Observes interactions with the component's storage and stores state alongside it.


I think Metadata is too broad for this. Perhaps, something like Hook would do better? Or maybe Observer.

kvark · 2017-11-22T03:45:55Z

src/storage/meta.rs

+/// a component.
+pub trait HasMeta<M> {
+    /// Immutably gets the sub-metadata.
+    fn find(&self) -> &M;


can we rename the methods to meta and meta_mut? Will need to be adjusted if Metadata is renamed too.

kvark · 2017-11-22T03:46:24Z

src/storage/meta.rs

+/// sub-metadata using `metadata.0`, `metadata.1`, etc. as well as helping with
+/// modelling generic APIs where something needs a specific metadata structure from
+/// a component.
+pub trait HasMeta<M> {


Actually, wouldn't this effectively be AsRef + AsMut?

I suppose so yes, I was initially worried about it being confusing that you got the inner portions through .as_ref() but I guess that doesn't matter for a majority of usecases anyways.

Actually, we kind of need this, I'm changing it very slightly though so it isn't really identical to AsRef/AsMut anymore.

kvark · 2017-11-22T03:47:00Z

src/storage/mod.rs

@@ -401,6 +473,19 @@ where
            data: &mut self.data,
        }
    }
+
+    /*
+    pub fn meta_mut(&mut self) -> &mut T::Metadata {


any reason this is commented out?

Oop, that was a mistake, I wanted to make sure I changed most of the documentation to use find::<T>();.

kvark · 2017-11-22T03:47:37Z

src/storage/tracked.rs

@@ -118,6 +118,7 @@ where
    /// impl Component for Comp {
    ///     // Will use `DenseVecStorage` by default.
    ///     type Storage = TrackedStorage<Self>;
+    ///     type Metadata = ();


I don't think we should proceed until TrackedStorage is ported

torkleyy · 2017-11-22T09:10:51Z

I think it would be sufficient if there would be three "levels" for observing:

flagged
tracked
changed (meaning the tracked would only check insertions, changed stuff moved out)

where each item includes the information provided by the former. If there's anything we can't do with this, please tell me.

Aceeri · 2017-12-06T06:55:21Z

I'll close this, since I like the Tracked api and think it is going to work out the way we want.

torkleyy reviewed Nov 17, 2017

View reviewed changes

Aceeri force-pushed the side branch 2 times, most recently from 1be3ff6 to c6f94ad Compare November 20, 2017 07:02

Aceeri added 5 commits November 19, 2017 23:08

SideStorage basics

8c059c2

Replace FlaggedStorage with Flagged, rename to Metadata, implement me…

9170b1e

…ta methods on Storage

Metadata structures

551c985

HasMeta for derived structures

51aec49

Fix errors with tests, impl HasMeta for Self

dc68af0

Aceeri force-pushed the side branch from c6f94ad to dc68af0 Compare November 20, 2017 07:12

Fix benchmarks

036d72c

kvark reviewed Nov 22, 2017

View reviewed changes

torkleyy mentioned this pull request Dec 3, 2017

[WIP] Tracked Interface #305

Closed

Aceeri closed this Dec 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metadata #301

Metadata #301

Aceeri commented Nov 15, 2017 •

edited by torkleyy

Loading

WaDelma commented Nov 15, 2017

torkleyy Nov 17, 2017

torkleyy Nov 17, 2017

Aceeri Nov 20, 2017

torkleyy Nov 17, 2017

Aceeri Nov 18, 2017

torkleyy Nov 18, 2017

torkleyy Nov 17, 2017

torkleyy Nov 17, 2017

Aceeri Nov 18, 2017

torkleyy Nov 18, 2017

torkleyy Nov 17, 2017

torkleyy Nov 17, 2017

Aceeri Nov 20, 2017

torkleyy Nov 17, 2017

Aceeri Nov 18, 2017

torkleyy Nov 17, 2017

Aceeri Nov 20, 2017

kvark Nov 22, 2017

kvark left a comment

kvark Nov 22, 2017

kvark Nov 22, 2017

Aceeri Nov 22, 2017

kvark Nov 22, 2017

kvark Nov 22, 2017

kvark Nov 22, 2017

kvark Nov 22, 2017

Aceeri Nov 22, 2017

Aceeri Nov 22, 2017

kvark Nov 22, 2017

Aceeri Nov 22, 2017

kvark Nov 22, 2017

torkleyy commented Nov 22, 2017

Aceeri commented Dec 6, 2017

		@@ -3,14 +3,16 @@
		//!
		//! [sp]: https://slide-rs.github.io/specs-website/

		@@ -1,5 +1,6 @@
		#![deny(missing_docs)]
		//#![deny(missing_docs)]


		use Index;

		/// Observes interactions with the component's storage and stores state alongside it.

Metadata #301

Metadata #301

Conversation

Aceeri commented Nov 15, 2017 • edited by torkleyy Loading

WaDelma commented Nov 15, 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

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

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

Choose a reason for hiding this comment

kvark 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

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

Choose a reason for hiding this comment

torkleyy commented Nov 22, 2017

Aceeri commented Dec 6, 2017

Aceeri commented Nov 15, 2017 •

edited by torkleyy

Loading