Saveload overhaul #337

torkleyy · 2018-01-13T09:44:23Z

TODO

Commit messages need to be improved
There is some weird lifetime error that I don't really understand

Motivation for this PR

Rename some items to make the code easier to understand
Remove a couple of unnecessary bounds
Allow passing references of storages (previously you could not reuse the WriteStorage)

Fixes #138
Fixes #282

This change is

torkleyy · 2018-01-13T09:45:28Z

This is the error I'm getting:

error[E0623]: lifetime mismatch
  --> src/saveload/de.rs:47:13
   |
35 |         &'b mut self,
   |         ------------ these two types are declared with different lifetimes...
36 |         entities: &'b EntitiesRes,
37 |         markers: &'b mut WriteStorage<'a, M>,
   |                          -------------------
...
47 |             markers,
   |             ^^^^^^^ ...but data from `self` flows into `markers` here

zakarumych · 2018-01-13T18:43:44Z

@torkleyy the problem is in "de.rs" line 129

torkleyy · 2018-01-13T18:48:23Z

OMG this is such a stupid mistake... Thanks for your help!

zakarumych · 2018-01-13T18:52:59Z

The error message is confusing. I've spotted it by chance.

zakarumych · 2018-01-14T10:43:06Z

src/saveload/de.rs

+struct DeserializeEntity<'a: 'b, 'b, 's, E, M: Marker, S: 's> {
+    allocator: &'b mut M::Allocator,
+    entities: &'b EntitiesRes,
+    storages: &'s mut S,


Do you really need different lifetime here?

zakarumych · 2018-01-14T10:45:37Z

src/saveload/de.rs

-    fn expecting(&self, formatter: &mut Formatter) -> fmt::Result {
-        write!(formatter, "Sequence of serialized entities")
-    }
+    /// Error may occur during serialization or deserialization of component


during deserialization.

torkleyy · 2018-02-02T17:33:37Z

@Xaeroxe is going to continue this PR, so here's what I planned:

It turned out that the current code doesn't really allow multiple allocators and has some weird interaction with the marker component storage.

Therefore, I've been thinking that it would make sense to put the entity -> marker and marker -> entity mapping both into the allocator (also see these messages on Gitter). You'll likely be using a BitSet in order to allow joining over the markers.

That's all I remember so far, please feel free to ask if something doesn't make sense.

Xaeroxe · 2018-02-10T09:17:18Z

Currently looking into simplifying the marker type by stripping away what I feel may be metadata not appropriate to store here. Some examples:

Marker::Identifier and Marker::id(): This feels extraneous, a marker's primary purpose should be to identify, rather than it being just one property of them. We're cloning these things all the time so allowing additional metadata to be associated with them may result in confusing behavior as users lose track of where their metadata actually is.
Marker::update(): Similar to the above, this feels excessive and easy to get confused on. This kind of meta data also doesn't exactly fulfill its proposed use case imo as the actual update signature doesn't provide enough context to give a meaningful audit of changes. In addition to this, the constant cloning of these markers means this data might even be misleading in some instances.

I've removed the Component impl for the Marker as well, since it was preventing use of multiple allocators with the same marker type for varying purposes. I'm also making the MarkerAllocator trait able to map in the opposite direction in order to make up for this removal.

torkleyy · 2018-02-10T09:19:05Z

@Xaeroxe I was also confused by this at the beginning, but @omni-viral said that it can be useful for e.g. a generational id or something. I'm not quite sure about it.

Xaeroxe · 2018-02-10T09:31:25Z

This metadata may be useful in the right contexts, I just don't think this is the right place to be trying to store and generate it. It should be handled by something with a more clear picture of what exactly it's trying to track. Typically the user, with their own data structures.

torkleyy · 2018-02-10T09:32:55Z

I'm also in favor of simplifying it, I just left it as is because @omni-viral told me so. Let's wait for them to respond.

zakarumych · 2018-02-10T10:21:02Z

I'm all for simplifying. But would it be possible to implement otherwise?

Xaeroxe · 2018-02-10T10:22:42Z

Can you clarify what you're referring to when you say "it"?

zakarumych · 2018-02-10T10:25:16Z

Network synchronization that checks for sequence id and rejects if entity in packet comes with less sequence id then one already in the world.

Xaeroxe · 2018-02-10T10:26:59Z

Oh, yes that will definitely be possible. That kind of meta data could be stored in another component that the network system itself queries and writes to. It can exist completely separate from the marker system.

zakarumych · 2018-02-10T11:16:23Z

Ok. And how deserializer would know how to deal with it?

Xaeroxe · 2018-02-13T16:31:12Z

I don't believe it's the deserializer's job to perform that kind of filtering. It should be handled in the network layer.

zakarumych · 2018-02-13T21:24:00Z

Actually it's not deserializer. It's loader.
And if loader will overwrite components with outdated data you won't be able to restore them.

Xaeroxe · 2018-03-17T04:11:48Z

I'm finally getting back to this. I see your point now @omni-viral .

If a marker's type is meant to be a semantic detail that controls how the underlying identifier is used during loading I'm no longer sure what we gain from making these not components @torkleyy . Can you please provide a use case where we needed a single marker type to have multiple storages?

Xaeroxe · 2018-03-17T05:56:26Z

Rebased, review comments addressed, example fixed, formatted with rustfmt 0.3.4-nightly. I think this is ready for review.

torkleyy · 2018-03-17T06:44:43Z

src/common.rs

@@ -161,8 +161,7 @@ impl Errors {
 #[derive(Derivative)]
 #[derivative(Default(bound = ""))]
 pub struct Merge<F> {
-    #[derivative(Default(value = "PhantomData"))]
-    future_type: PhantomData<F>,
+    #[derivative(Default(value = "PhantomData"))] future_type: PhantomData<F>,


Oh, is this some new rustfmt setting again?

Is this attribute necessary at all?

Yes, otherwise F would have to implement Default.

No. PhantomData implements default for any type parameter.

It does now but I think it was a rather recent fix which I don't want to rely on yet.

It does at least since 1.0.0

Ahhh, the attribute on PhantomData isn't required, but we need derivative here.

torkleyy

LGTM. @Xaeroxe Please squash the commits.

torkleyy · 2018-03-17T07:12:27Z

I think it's good as-is, so feel free to merge @WaDelma

WaDelma

Looking good!

WaDelma · 2018-03-17T09:19:48Z

examples/saveload.rs

+                &mut ser,
+            ).unwrap_or_else(|e| eprintln!("Error: {}", e));
+            // TODO: Specs should return an error which combines serialization
+            // and component errors.


Is this TODO to be left to future PR?

WaDelma · 2018-03-17T09:20:56Z

examples/saveload.rs

+            ReadStorage<'a, U64Marker>,
+        );
+
+        fn run(&mut self, (ents, pos, vel, markers): Self::SystemData) {


s/vel/mass/

WaDelma · 2018-03-17T09:26:39Z

examples/serialize.rs

@@ -48,7 +48,7 @@ fn main() {
            let entity_list: Vec<_> = (&*data.entities).join().collect();

            // Remove all components
-            for (entity, _) in (&*data.entities, &data.comp.check()).join() {
+            for (entity, _) in (&*data.entities, data.comp.mask().clone()).join() {


Can't you use drain here?

WaDelma · 2018-03-17T09:40:21Z

src/saveload/marker.rs

    /// Allocator for this `Marker`
    type Allocator: MarkerAllocator<Self>;

-    /// Get this marker internal id
+    /// Get this marker internal id.
+    /// This value may never change.


As the update method says that changes in return value doesn't cause UB, I think the wording should be relaxed.
Maybe something like: "The value of this method should be constant".

WaDelma · 2018-03-17T09:47:44Z

src/saveload/marker.rs

+    fn mark<'m>(&mut self, entity: Entity, storage: &'m mut WriteStorage<M>) -> (&'m M, bool) {
+        let mut new = false;
+
+        let marker = storage.entry(entity).unwrap().or_insert_with(|| {


What is with this unwrap here? Is it expected to not panic? I would change it to expect that contains explanation.

WaDelma · 2018-03-17T09:52:07Z

src/saveload/ser.rs

+            serseq.serialize_element(&EntityData::<M, Self::Data> {
+                marker: marker.clone(),
+                components: self.serialize_entity(entity, &ids)
+                    .map_err(ser::Error::custom)?, // TODO: revise


What does the TODO here mean? If it's still relevant, it should be made more informative.

WaDelma · 2018-03-17T09:56:46Z

src/saveload/ser.rs

+                    })?;
+                }
+            }
+            to_serialize = add;


This code is bit hard to decipher, but I don't know how to improve it.
Also you could reuse to_serialize and only do two allocations, but I don't know if it's worth it.

WaDelma · 2018-03-17T09:59:07Z

src/saveload/storages.rs

+
+    //    fn join(&'b mut self) -> Self::Join {
+    //        self
+    //    }


Remove commented code

WaDelma · 2018-03-17T09:59:32Z

src/saveload/storages.rs

+
+    //    fn join(&'c mut self) -> Self::Join {
+    //        &mut **self
+    //    }


Remove commented code

torkleyy · 2018-03-18T18:08:40Z

Not sure why I'm doing this everywhere then.. I'll check later.

UserAB1236872 · 2018-04-01T00:16:20Z

I had an issue with saveload that crashed on deserialize with NullStorage, any chance the overhaul fixes this? I was going to open a separate issue, but figured this may be a better place to report it since it's a pretty big overhaul.

torkleyy · 2018-04-01T06:52:54Z

@LinearZoetrope What kind of crash was that? I never experienced any crashes, so can't say if I fixed them.

UserAB1236872 · 2018-04-04T22:10:10Z

I'll have to retest, but if I recall correctly, it was claiming anything stored in NullStorage wasn't present when deserializing.

torkleyy · 2018-04-10T12:59:37Z

@LinearZoetrope I'll check it and add a test to ensure it's working.

Xaeroxe · 2018-04-10T21:01:51Z

@torkleyy I believe this is ready to be merged.

Xaeroxe · 2018-04-16T03:35:34Z

Ok I've just rebased this, it should be ready to merge. Recommend one more review prior to merging, as the rebase kind of aggressively preferred to keep code from this PR where there were conflicts.

WaDelma

LGTM, but would like another pair of eyes to go over it.

WaDelma · 2018-04-16T07:40:24Z

src/lib.rs

@@ -1,4 +1,4 @@
-#![deny(missing_docs)]
+#![warn(missing_docs)]


So there are missing documentations?

WaDelma · 2018-04-16T07:48:00Z

src/saveload/ser.rs

+    type Data: Serialize;
+
+    /// Serialize the components of a single entiy using a entity -> marker mapping.
+    fn serialize_entity<F>(&self, entity: Entity, ids: F) -> Result<Self::Data, E>


s/entiy/entity/

torkleyy · 2018-04-16T08:11:51Z

Errors for missing docs make it rather hard to experiment with new features.

torkleyy

Thanks!

bors r+

337: Saveload overhaul r=torkleyy a=torkleyy ## TODO * [x] Commit messages need to be improved * [x] There is some weird lifetime error that I don't really understand ## Motivation for this PR * Rename some items to make the code easier to understand * Remove a couple of unnecessary bounds * Allow passing references of storages (previously you could not reuse the `WriteStorage`) --- Fixes #138 Fixes #282  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/slide-rs/specs/337)  Co-authored-by: torkleyy <torkleyy@gmail.com> Co-authored-by: Jacob Kiesel <kieseljake@gmail.com>

bors · 2018-04-16T16:53:20Z

Build succeeded

continuous-integration/travis-ci/push

torkleyy added enhancement help wanted in progress labels Jan 13, 2018

torkleyy force-pushed the saveload branch from 61086bf to e599d66 Compare January 13, 2018 09:46

torkleyy removed the help wanted label Jan 14, 2018

zakarumych reviewed Jan 19, 2018

View reviewed changes

torkleyy mentioned this pull request Jan 24, 2018

API to create entities with Id and to retrieve them via Id #338

Closed

Xaeroxe force-pushed the saveload branch from c4859e6 to 573e29b Compare March 17, 2018 05:54

torkleyy commented Mar 17, 2018

View reviewed changes

torkleyy added ready and removed in progress labels Mar 17, 2018

torkleyy commented Mar 17, 2018

View reviewed changes

WaDelma reviewed Mar 17, 2018

View reviewed changes

torkleyy mentioned this pull request Mar 19, 2018

Networking with specs? #360

Closed

torkleyy and others added 12 commits April 15, 2018 20:16

Saveload overhaul

91f24ca

Finish deserialization overhaul

ef3d8e1

Revert formatting changes to make review easier

db66d78

Fix silly lifetime mistake

fbd2469

Address review comments and fix warnings

5d6d35e

Fix example

64e38b9

Formatting pass

7119258

Fix doc and remove TODO

4cc12af

Remove obsolete test again

bb56e33

Fix deprecation warnings and doc test

6068471

Cleanup

b73c659

Cleanup rebase work

297abd4

Xaeroxe force-pushed the saveload branch from c60d706 to 297abd4 Compare April 16, 2018 03:30

WaDelma reviewed Apr 16, 2018

View reviewed changes

torkleyy commented Apr 16, 2018

View reviewed changes

bors bot merged commit 297abd4 into amethyst:master Apr 16, 2018

torkleyy unassigned WaDelma Oct 20, 2019

		@@ -1,4 +1,4 @@
		#![deny(missing_docs)]
		#![warn(missing_docs)]

Saveload overhaul #337

Saveload overhaul #337

Conversation

torkleyy commented Jan 13, 2018 • edited Loading

TODO

Motivation for this PR

torkleyy commented Jan 13, 2018 • edited Loading

zakarumych commented Jan 13, 2018

torkleyy commented Jan 13, 2018

zakarumych commented Jan 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torkleyy commented Feb 2, 2018 • edited Loading

Xaeroxe commented Feb 10, 2018

torkleyy commented Feb 10, 2018

Xaeroxe commented Feb 10, 2018

torkleyy commented Feb 10, 2018

zakarumych commented Feb 10, 2018

Xaeroxe commented Feb 10, 2018

zakarumych commented Feb 10, 2018

Xaeroxe commented Feb 10, 2018

zakarumych commented Feb 10, 2018

Xaeroxe commented Feb 13, 2018

zakarumych commented Feb 13, 2018 • edited Loading

Xaeroxe commented Mar 17, 2018

Xaeroxe commented Mar 17, 2018

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

zakarumych Mar 18, 2018 • edited Loading

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

torkleyy commented Mar 17, 2018

WaDelma 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

torkleyy commented Mar 18, 2018 via email

UserAB1236872 commented Apr 1, 2018

torkleyy commented Apr 1, 2018

UserAB1236872 commented Apr 4, 2018

torkleyy commented Apr 10, 2018

Xaeroxe commented Apr 10, 2018

Xaeroxe commented Apr 16, 2018

WaDelma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torkleyy commented Apr 16, 2018 via email

torkleyy left a comment

Choose a reason for hiding this comment

bors bot commented Apr 16, 2018

Build succeeded

torkleyy commented Jan 13, 2018 •

edited

Loading

torkleyy commented Jan 13, 2018 •

edited

Loading

torkleyy commented Feb 2, 2018 •

edited

Loading

zakarumych commented Feb 13, 2018 •

edited

Loading

zakarumych Mar 18, 2018 •

edited

Loading