Adds StorageEntry to Storage for convenience #274

Aceeri · 2017-09-27T06:04:40Z

Fixes #268

Adds StorageEntry to allow for in place insertions, also some other methods for dealing with the occupied and vacant entries, similar to stds HashMap Entry.

Still need to add some tests for further verification (I'll do this tomorrow).

torkleyy · 2017-09-27T09:20:53Z

src/storage/mod.rs

@@ -203,6 +317,20 @@ where
        }
    }

+    /// Returns an entry to the component associated to the entity.
+    ///
+    /// Note: This does not immediately error if the entity is dead.


I think we should use Result here.

Are we sure? That would make this require a bit more boilerplate for this stuff then:

if let Ok(entry) = storage.entry(entity) { entry.or_insert(component); }

At least until we get try to work outside of just functions.

And now the boilerplate is at another location

if let Some(component) = storage.entry(entity).or_insert(...) { ... }

I'm somewhat worried about enforcing that Entry is only created with a live Entity, is there a case where an entity could be deleted after you create an entry?

It can be killed atomically. But atomically killed enities are considered alive until maintenance.

True, I messed around with it and couldn't find an easy way to kill the entity while having an entry, so I guess it should be fine.

zakarumych · 2017-09-27T21:29:16Z

src/storage/mod.rs

+    pub fn or_insert_with<F>(self, component: F) -> Option<&'a mut T>
+        where F: FnOnce() -> T,
+    {
+        self.or_insert(component())


The whole idea of the method is not to call closure unless component is required.
Instead or_insert should be implemented as or_insert_with(|| component).

torkleyy

In addition to the comments below, could you please add tests which

explicitly match the enum and
insert by using an entry and create another entry checking if it's now occupied

torkleyy · 2017-10-04T10:58:51Z

src/storage/mod.rs

@@ -203,6 +317,25 @@ where
        }
    }

+    /// Returns an entry to the component associated to the entity.
+    ///


Please add a usage example here.

torkleyy · 2017-10-04T10:59:43Z

src/storage/mod.rs

+    }
+
+    /// Returns the entity of the current entry.
+    pub fn entity(&self) -> &Entity {


Is there a reason for us to give a reference to the entity?

torkleyy · 2017-10-04T11:00:25Z

src/storage/mod.rs

+        self.or_insert_with(|| component)
+    }
+
+    /// Inserts a component using a default function.


"using a default function"? Why "default"?

Default should probaby be "lazily called function"

torkleyy · 2017-10-04T11:00:58Z

src/storage/mod.rs

+    Vacant(VacantEntry<'a, 'b, T, D>),
+}
+
+pub enum EntryError {


This belongs into the error module. Also, we should not have enums with just a single variant.

torkleyy · 2017-10-04T11:01:56Z

src/storage/mod.rs

+
+    /// Inserts a value into the storage and returns the old one.
+    ///
+    /// Return value can be `None` if the entity passed in was dead.


Is this still true? Given that we now return an error, shouldn't this return just T?

This isn't true, thought I removedid that line.

torkleyy · 2017-10-04T11:02:13Z

src/storage/mod.rs

+    /// Removes the component from the storage and returns it.
+    ///
+    /// Return value can be `None` if the entity passed in was dead.
+    pub fn remove(&mut self) -> Option<T> {


torkleyy · 2017-10-04T11:02:28Z

src/storage/mod.rs

+{
+    /// Inserts a value into the storage and returns the old one.
+    ///
+    /// Return value can be `None` if the entity passed in was dead.


torkleyy · 2017-10-06T12:22:08Z

src/error.rs

+
+/// Storage entry error when the request entity was dead.
+#[derive(Debug)]
+pub struct EntryIsDead(pub Entity);


Why aren't you just using WrongGeneration? And in case this actually needs its own error, it should still be representable with specs::error::Error (see WrongGeneration; the idea is that every error can be converted to that enum).

torkleyy · 2017-10-06T12:22:54Z

src/storage/mod.rs

+          D: Deref<Target = MaskedStorage<T>>,
+{
+    /// Get a reference to the component associated with the entity.
+    pub fn get(&self) -> Option<&T> {


If the entry is occupied, how could this call return None?

Ah, missed that one when removing all the others.

torkleyy · 2017-10-06T12:23:25Z

src/storage/mod.rs

+    pub fn get_mut(self) -> &'a mut T {
+        match self.storage.get_mut(self.entity) {
+            Some(component) => component,
+            None => { panic!("`OccupiedEntry` on an entity without a component."); },


No need for { and }

I know why the coverage goes down: coveralls thinks that this is an uncovered line, but that's not the case. Speaking of which, this should use unreachable!.

torkleyy · 2017-10-06T12:24:26Z

src/storage/mod.rs

+    pub fn remove(self) -> T {
+        match self.storage.remove(self.entity) {
+            Some(old) => old,
+            None => { panic!("`OccupiedEntry` on an entity without a component."); },


Meh, the whole sense of Entry API is that you usually don't do any checks twice which is the case for all these methods :(

torkleyy · 2017-10-06T12:24:50Z

src/storage/mod.rs

+    /// Inserts a value into the storage.
+    pub fn insert(self, component: T) -> &'a mut T {
+        match self.storage.insert(self.entity, component) {
+            InsertResult::Updated(_) => { panic!("`VacantEntry` on an entity with a component."); },


No need for { and }

torkleyy · 2017-10-06T12:24:58Z

src/storage/mod.rs

+                    None => { panic!("Storage insertion into entity {:?} failed on `VacantEntry`", self.entity); },
+                }
+            },
+            InsertResult::EntityIsDead(_) => { panic!("`VacantEntry` on a dead entity: {:?}.", self.entity); },


torkleyy · 2017-10-06T12:25:32Z

src/storage/mod.rs

+    ///
+    /// Behaves somewhat similarly to `std::collections::HashMap`'s entry api.
+    ///
+    ///## Example


missing space

I had them previously and it makes them not render correctly for whatever reason.

That's strange; our other examples have spaces, too.

Alright, seems to work now?

torkleyy · 2017-10-06T12:26:58Z

src/storage/mod.rs

+    ///## Example
+    /// 
+    /// ```rust
+    ///# extern crate specs;


missing space in this and the following lines

torkleyy · 2017-10-17T11:05:23Z

Are you still interested in this feature? I'm not sure now if I should be waiting for it, that's why I'm asking.

Aceeri · 2017-10-17T19:53:56Z

I'll finish it up, just kind of got carried away with other stuff

kvark · 2017-10-22T02:31:23Z

src/storage/mod.rs

+
+    /// Inserts a value into the storage and returns the old one.
+    pub fn insert(&mut self, mut component: T) -> T {
+        std::mem::swap(&mut component, unsafe { self.storage.data.inner.get_mut(self.entity.id()) });


could use self.get_mut()

Annoyingly I can't figure out the lifetimes that would work for get_mut that allows just &mut self, so I can't get that to work properly.

kvark · 2017-10-22T02:32:46Z

src/storage/mod.rs

+    }
+
+    /// Removes the component from the storage and returns it.
+    pub fn remove(self) -> T {


random impractical thought: would be fun to return (T, VacantEntry<...>)

kvark · 2017-10-22T02:35:44Z

src/storage/mod.rs

+}
+
+impl<'a, 'b, T, D> StorageEntry<'a, 'b, T, D>
+    where T: Component,


is this an actual formatting of rustfmt-nightly?

No, I don't think so

kvark · 2017-10-22T02:37:14Z

src/storage/mod.rs

+
+    /// Inserts a component using a lazily called function that is only called
+    /// when inserting the component.
+    pub fn or_insert_with<F>(self, component: F) -> &'a mut T


I think the F-typed argument should be named something other than component? stdlib uses default

kvark · 2017-10-22T02:38:01Z

src/storage/mod.rs

+    ///  }
+    /// # }
+    /// ```
+    pub fn entry<'a>(&'a mut self, e: Entity) -> Result<StorageEntry<'a, 'e, T, D>, ::error::WrongGeneration>


any reason we need to use a global type path here?

kvark · 2017-10-22T02:39:23Z

src/storage/mod.rs

+            let gen = self.entities
+                .alloc
+                .generations.get(e.id() as usize)
+                .map(|gen| *gen)


nit: could be cloned?

torkleyy · 2017-11-02T08:15:04Z

Only conflicts left to resolve 👍 Once that's done, feel free to merge.

bors delegate=@Aceeri

bors · 2017-11-02T08:15:05Z

✌️ Aceeri can now approve this pull request

torkleyy

Looks good!

…is now a single struct.

… the entry's

Aceeri · 2017-11-02T10:22:55Z

bors r+

274: Adds StorageEntry to Storage for convenience r=Aceeri a=Aceeri Fixes #268 Adds `StorageEntry` to allow for in place insertions, also some other methods for dealing with the occupied and vacant entries, similar to `std`s HashMap Entry. Still need to add some tests for further verification (I'll do this tomorrow).

Aceeri · 2017-11-02T10:31:01Z

Should probably add this to the changelog.

bors r-

bors · 2017-11-02T10:31:02Z

Canceled

Aceeri · 2017-11-02T10:34:56Z

bors r+

274: Adds StorageEntry to Storage for convenience r=Aceeri a=Aceeri Fixes #268 Adds `StorageEntry` to allow for in place insertions, also some other methods for dealing with the occupied and vacant entries, similar to `std`s HashMap Entry. Still need to add some tests for further verification (I'll do this tomorrow).

bors · 2017-11-02T11:14:46Z

Build succeeded

continuous-integration/travis-ci/push

torkleyy reviewed Sep 27, 2017

View reviewed changes

zakarumych reviewed Sep 27, 2017

View reviewed changes

zakarumych mentioned this pull request Oct 2, 2017

Add an ability to save and load entities. #275

Merged

torkleyy requested changes Oct 4, 2017

View reviewed changes

Aceeri force-pushed the entry branch 3 times, most recently from a4d3065 to 72efbc5 Compare October 6, 2017 05:25

torkleyy reviewed Oct 6, 2017

View reviewed changes

Aceeri force-pushed the entry branch 4 times, most recently from 5c093ae to 904003e Compare October 20, 2017 04:50

torkleyy approved these changes Oct 20, 2017

View reviewed changes

torkleyy added the ready label Oct 21, 2017

kvark approved these changes Oct 22, 2017

View reviewed changes

Aceeri force-pushed the entry branch from 2043241 to 7fb7c2a Compare November 2, 2017 10:02

torkleyy approved these changes Nov 2, 2017

View reviewed changes

Aceeri added 5 commits November 2, 2017 03:08

Adds Entry to Storage for convenience

df4adbe

Move erroring to getting Entry, tests for StorageEntry

9652791

Clean up methods, use substruct methods for consistency, entry error …

b7406bf

…is now a single struct.

Use unchecked versions for methods

be8016d

Formatting, minor changes

30a009d

Aceeri force-pushed the entry branch 2 times, most recently from 6a8a53f to 55c6180 Compare November 2, 2017 10:10

Add into_mut method for bounding by the storages lifetime rather than…

55c6180

… the entry's

Add to changelog

313a0a8

bors bot merged commit 313a0a8 into amethyst:master Nov 2, 2017

Adds StorageEntry to Storage for convenience #274

Adds StorageEntry to Storage for convenience #274

Conversation

Aceeri commented Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zakarumych Sep 27, 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

zakarumych Sep 27, 2017 • edited Loading

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

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 Oct 6, 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

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 Oct 17, 2017

Aceeri commented Oct 17, 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

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 2, 2017

bors bot commented Nov 2, 2017

torkleyy left a comment

Choose a reason for hiding this comment

Aceeri commented Nov 2, 2017

Aceeri commented Nov 2, 2017

bors bot commented Nov 2, 2017

Canceled

Aceeri commented Nov 2, 2017

bors bot commented Nov 2, 2017

Build succeeded

Aceeri commented Sep 27, 2017 •

edited

Loading

zakarumych Sep 27, 2017 •

edited

Loading

zakarumych Sep 27, 2017 •

edited

Loading

torkleyy Oct 6, 2017 •

edited

Loading

Aceeri commented Oct 17, 2017 •

edited

Loading