Add an ability to save and load entities. #275

zakarumych · 2017-09-27T20:45:59Z

Add (de)serialization support for entities to specs.
Main parts are:

Marker trait.
Marker is used for several purposes:
- Mark which entities should be serialized.
- It contains ID to replace Entity stored in Components during serialization process.
- It may contain additional data to be used for tracking owner of the component.
MarkerAllocator trait for allocating Markers, storing Marker -> Entity mapping and other stuff.
SerializableComponent trait that is auto implemented by components if they implements Serialize, DeserializeOwned and Copy (later is rather strict, maybe it should be replaced with Clone or removed somehow).
Mainly Component implements SerializableComponent manually when it contains Entity.
Component should replace Entity with provided Marker in SerializableComponent::save function. And replace Marker with Entity in SerializableComponent::load function.
Components trait is implemented for tuples of SerializableComponents
It allows to save all existing and mentioned in tuple components of one entity from ReadStorages into serializable structure. And load it into WriteStorages again.

Helper types WorldSerialize and WorldDeserialize are good example how to use this facility.

torkleyy

Looks very nice! I still have to check some details of the marker system.

torkleyy · 2017-09-28T13:46:49Z

src/saveload/de.rs

+    where
+        A: SeqAccess<'de>,
+    {
+        while let Some(()) = seq.next_element_seed(DeserializeEntity {


style: replace with .is_some.

torkleyy · 2017-09-28T13:47:55Z

src/saveload/de.rs

+
+
+/// `DeerializeSeed` implementation for `World`
+//#[derive(SystemData)]


Why not use derive here? And further, if derive doesn't work here, why did you keep the line?

I think it should start to work. It requires PhantomData: SystemData. So I kept the line as a remainder

torkleyy · 2017-09-28T13:48:51Z

src/saveload/de.rs

+
+/// `DeerializeSeed` implementation for `World`
+//#[derive(SystemData)]
+pub struct WorldDeserialize<'a, M: Marker, E: Error, T: Components<M::Identifier, E>> {


You shouldn't create unnecessary bounds on the struct itself, e.g. E: Error should be removed here.

torkleyy · 2017-09-28T13:50:55Z

src/saveload/details.rs

+/// Used as a placeholder for associated error types if
+/// something cannot fail.
+#[derive(Debug)]
+pub enum NoError {}


Could this go into the errors module?

torkleyy · 2017-09-28T13:51:54Z

src/saveload/details.rs

+
+/// This trait is implemented by any tuple where all elements are
+/// `Component + Serialize + DeserializeOwned`
+pub trait Components<M, E: Error>: for<'a> Storages<'a> {


No need for the Error bound here.

torkleyy · 2017-09-28T13:58:57Z

src/saveload/marker.rs

+/// `maintain` method can be implemented for cleanup and actualization
+/// # Example
+/// see docs for `Marker`
+pub trait MarkerAllocator<M: Marker>: Resource {


Why not make the Marker an associated?

One allocator can implement this trait for different M.
I prefer type parameter when there is no need to fetch for associated type or restrict implementation to only one type.

torkleyy · 2017-09-28T14:01:41Z

src/saveload/ser.rs

+/// with serializer `S`
+/// Doesn't recursively mark referenced entities.
+/// Closure passed in `SerializableComponent::save` returns `None` for unmarked `Entity`
+/// In this case `SerializableComponent::save` may perform workaround


This should be a bit more detailed.

Not sure how 🤷‍♂️

torkleyy · 2017-09-28T14:02:11Z

src/saveload/ser.rs

+/// It may be constructed manually (TODO: Add `new` function)
+/// Or fetched by `System` as `SystemData`
+/// Serializes components in tuple `T` with marker `M`
+pub struct WorldSerialize<'a, M: Marker, E: Error, T: Components<M::Identifier, E>> {


Again, too many bounds.

torkleyy · 2017-09-28T14:02:44Z

src/saveload/ser.rs

+
+/// This type implements `Serialize` so that it may be used in generic environment
+/// where `Serialize` implementer is expected
+/// It may be constructed manually (TODO: Add `new` function)


Please fix the TODO or make it a comment.

torkleyy · 2017-09-28T14:03:17Z

src/saveload/ser.rs

+}
+
+
+impl<'a, M, E, T> SystemData<'a> for WorldSerialize<'a, M, E, T>


Same as before, could this be derived?

torkleyy · 2017-10-02T12:18:12Z

src/saveload/details.rs

@@ -0,0 +1,179 @@
+
+


Extra lines?

torkleyy · 2017-10-02T12:20:29Z

src/saveload/marker.rs

+    ///
+    /// Allowed to panic if `self.id() != update.id()`.
+    /// But usually implementer may ignore `update.id()` value
+    /// as deserialization algorithm ensures `id()`'s match.


should be id()s (without apostrophe)

torkleyy · 2017-10-02T12:21:07Z

src/saveload/marker.rs

+    }
+}
+
+/// This allocator is used with `Marker` trait


Please use full stops here.

torkleyy · 2017-10-02T12:21:28Z

src/saveload/marker.rs

+pub trait MarkerAllocator<M: Marker>: Resource {
+    /// Allocate new `Marker`.
+    /// Stores mapping `Marker` -> `Entity`.
+    /// If _id_ arguemtn is `Some(id)` then new marker will have this `id`.


¯\_(ツ)_/¯

kvark

Great work! I've got a few notes/complains here.
My biggest concern is the ids function. Could you look for a way to make it a part of one of the traits you have? Just anything to simplify it - passing that function around looks a bit hacky, and is certainly not very obvious or simple.

kvark · 2017-10-02T14:16:11Z

src/error.rs

+/// Used as a placeholder for associated error types if
+/// something cannot fail.
+#[derive(Debug)]
+pub enum NoError {}


Should be PhantomError

Should be ! but it isn't stable yet.
Why PhantomError anyway?

Well, first, because you are implementing Error for NoError, which already looks like a hack. Secondly, because the "phantom" is rather established for such things that can't be instantiated, so PhantomError is crystal clear here.

PhantomData is unit type and can be instantiated. And it is an example from std.
IDK any other examples.

I don't have a strong opinion here, @kvark do you insist on this request?

Nope. I think I might have confused a bit the semantics of phantom types. Pretty sure I saw them used in the context where one couldn't create instances (enum MyPhantomType {}), but I don't want to push hard on that here. It's fine to ship as is.

kvark · 2017-10-02T14:16:21Z

src/error.rs

+    fn description(&self) -> &str {
+        match *self {}
+    }
+}


newline missing

kvark · 2017-10-02T14:17:07Z

src/lib.rs

@@ -216,6 +219,9 @@ pub use world::{Component, CreateIter, CreateIterAtomic, EntitiesRes, Entity, En
 #[cfg(feature = "common")]
 pub mod common;

+#[cfg(feature = "serialize")]
+pub mod saveload;


why is it called saveload and not serialize/serial?

Because it is also do some tricky data conversion by functions save and load.
If you insist I'll rename.

kvark · 2017-10-02T14:17:54Z

src/saveload/de.rs

@@ -0,0 +1,146 @@
+
+use std::fmt::{self, Display, Formatter};


I'd prefer either just use std::fmt or no self in the list for consistency.

Why don't you like self here?

I just find it inconsistent that some of the types in the same namespace are referred directly while others - via fmt::Type. I believe it reads better through the code if all the imports from the same namespace are consistent. In case of fmt, I find fmt::Display and fmt::Formatter quite nice, but that's a matter of taste.

kvark · 2017-10-02T14:18:00Z

src/saveload/de.rs

+use std::fmt::{self, Display, Formatter};
+use std::marker::PhantomData;
+
+use serde::de::{self, Deserialize, DeserializeSeed, Deserializer, SeqAccess, Visitor};


nit, but same here

kvark · 2017-10-02T14:41:17Z

src/saveload/marker.rs

+    /// But usually implementer may ignore `update.id()` value
+    /// as deserialization algorithm ensures `id()`s match.
+    fn update(&mut self, update: Self) {
+        ::std::mem::drop(update);


nit: let's not use global imports

kvark · 2017-10-02T14:43:30Z

src/saveload/ser.rs

+        .map(|(e, m)| (e, *m))
+        .collect::<Vec<_>>();
+    loop {
+        if to_serialize.is_empty() {


any reason to not write it as while !to_serialize.is_empty() {...}

No reason now.

kvark · 2017-10-02T14:48:59Z

src/saveload/ser.rs

+            /// Create serializable structure from storages
+            #[allow(non_snake_case)]
+            pub fn new(entities: Entities<'a>,
+                       markers: ReadStorage<'a, X>


nit: please use the new formatting

This is not resolved, could you use the latest version of rustfmt-nightly and format this please?

kvark · 2017-10-02T14:49:14Z

src/saveload/ser.rs

+                WorldSerialize {
+                    entities: entities,
+                    storages: ($($a,)*),
+                    markers: markers,


nit: could be just markers, and entities,

kvark · 2017-10-02T14:52:02Z

src/saveload/ser.rs

+    for (entity, marker) in (&**entities, &*markers).join() {
+        serseq.serialize_element(&EntityData::<M, E, T> {
+            marker: *marker,
+            components: T::save(entity, storages, &ids).map_err(ser::Error::custom)?,


this looks a bit convoluted, ids gets an entity's marker while we are already iterating entities with markers (so we already have that marker)

If component contains some Entity it must use this closure to get marker of that entity.
Then Entity gets replaced with marker in SaveLoadComponent::Data returned from save

torkleyy · 2017-10-02T15:29:43Z

src/lib.rs

@@ -216,6 +219,9 @@ pub use world::{Component, CreateIter, CreateIterAtomic, EntitiesRes, Entity, En
 #[cfg(feature = "common")]
 pub mod common;

+#[cfg(feature = "serialize")]


This wasn't rebased correctly, it's called serde now.

torkleyy · 2017-10-02T15:34:13Z

src/saveload/marker.rs

@@ -0,0 +1,211 @@
+//! Provides with `Marker` and `MarkerAllocator` traits


Provides ~~with~~ Marker

torkleyy · 2017-10-02T15:45:40Z

src/saveload/mod.rs

@@ -0,0 +1,13 @@
+//! Save and load entites from various formats with serde


I would really appreciate if you could add a top-level overview here.

It would be better if someone with better writing skills do this

torkleyy · 2017-10-02T15:46:27Z

src/saveload/mod.rs

+
+use self::details::{Components, EntityData, SerializableComponent, Storages};
+
+pub mod marker;


Could we not make the hierarchy any deeper and just reexport from marker?

torkleyy · 2017-10-02T15:47:06Z

src/saveload/ser.rs

+
+use {Entities, FetchMut, Join, ReadStorage, WriteStorage};
+
+use super::{Components, EntityData, Storages};


These are all local imports, please move them together and use saveload:: instead of super::.

torkleyy · 2017-10-02T15:50:12Z

src/saveload/details.rs

+
+#[derive(Serialize, Deserialize)]
+#[serde(bound = "")]
+pub struct EntityData<M: Marker, E, T: Components<M::Identifier, E>> {


No need for the Marker bound

Either bound or additional type parameter to use instead of M::Identifier.
And bound them equal in all impls

rebased

kvark · 2017-10-02T17:06:56Z

@omni-viral thanks for addressing my notes!

Re:naming. How do you feel about "loadable" component/feature/trait? In my understanding, this is the make thing you are trying to achieve. Serialization/deserialization are just means to be able to load components from disk. Even saving is rather useless if you can't load it.

Also note:

saveload feature

This is not a cargo feature, so we could have a less ambiguous PR name.

WaDelma

I haven't looked at the actual implementation at all, but I gave a pass over the documentation.

WaDelma · 2017-10-02T22:08:53Z

src/saveload/de.rs

+    storages: &'a mut <T as Storages<'b>>::WriteStorages,
+    markers: &'a mut WriteStorage<'b, M>,
+    allocator: &'a mut FetchMut<'b, M::Allocator>,
+    pd: PhantomData<(M, E, T)>,


M shouldn't be needed in the PhantomData as it's already used in &'a mut WriteStorage<'b, M>.

Types went serious changes and now it is indeed unnecessary 😄

WaDelma · 2017-10-02T22:11:06Z

src/saveload/de.rs

+    storages: &'a mut <T as Storages<'b>>::WriteStorages,
+    markers: &'a mut WriteStorage<'b, M>,
+    allocator: &'a mut FetchMut<'b, M::Allocator>,
+    pd: PhantomData<(M, E, T)>,


WaDelma · 2017-10-02T22:13:36Z

src/saveload/details.rs

+
+    (@) => {};
+
+    (@ $ah:ident|$bh:ident $(,$a:ident|$b:ident)*) => {


Maybe add comment for explaining this.

WaDelma · 2017-10-02T22:14:38Z

src/saveload/marker.rs

+use serde::de::DeserializeOwned;
+
+
+/// This trait should be implemetened by component which is gonna be used as marker.


s/gonna/going to be/

s/implemetened by/implemented by a/

WaDelma · 2017-10-02T22:18:43Z

src/saveload/marker.rs

+/// This trait should be implemetened by component which is gonna be used as marker.
+/// This marker should be set to entity that should be serialized.
+/// If serialization strategy needs to set marker to some entity it should use
+/// new marker allocated for `Marker::Allocator`.


s/it should use new marker allocated for/then it should use newly allocated marker from/

WaDelma · 2017-10-02T22:38:46Z

src/saveload/ser.rs

+
+
+/// Serialize components from specified storages via `SerializableComponent::save`
+/// of all marked entities with provided serializer.


Same as before.

WaDelma · 2017-10-02T22:39:28Z

src/saveload/ser.rs

+/// closure passed in `ids` arguemnt returns `None` for unmarked `Entity`.
+/// In this case `SerializableComponent::save` may perform workaround (forget about `Entity`)
+/// or fail.
+/// So function doesn't recursively mark referenced entities.


s/So/So the/

WaDelma · 2017-10-02T22:40:20Z

src/saveload/ser.rs

+}
+
+/// This type implements `Serialize` so that it may be used in generic environment
+/// where `Serialize` implementer is expected.


s/implementer/implementation/

WaDelma · 2017-10-02T22:40:39Z

src/saveload/ser.rs

+/// This type implements `Serialize` so that it may be used in generic environment
+/// where `Serialize` implementer is expected.
+/// It may be constructed manually with `WorldSerialize::new`.
+/// Or fetched by `System` as `SystemData`.


I'm not sure here. It's not fetched from System as it isn't in System. System fetches SystemData from World.

WaDelma · 2017-10-02T22:41:33Z

src/saveload/ser.rs

+    T: Components<M::Identifier, E>,
+{
+    /// Remove all marked entities
+    /// Use it if you want to delete entities that was just serialized


s/Use it/Use this/
s/was/were/

torkleyy

This my last review, I will merge after you address my comments!

torkleyy · 2017-10-04T12:36:46Z

src/error.rs

+/// Used as a placeholder for associated error types if
+/// something cannot fail.
+#[derive(Debug)]
+pub enum NoError {}


I don't have a strong opinion here, @kvark do you insist on this request?

torkleyy · 2017-10-04T12:38:55Z

src/saveload/marker.rs

+///
+/// ## Example
+///
+/// ```rust,no_run


Again, why no_run? Please remove that.

torkleyy · 2017-10-04T12:40:52Z

src/saveload/ser.rs

+                match markers.get(entity).cloned() {
+                    Some(marker) => Some(marker.id()),
+                    None => {
+                        let marker = allocator.mark(entity, markers);


Would you like to rewrite this to use the Entry API in this PR or in a follow-up?

torkleyy · 2017-10-04T12:42:01Z

src/saveload/ser.rs

+            /// Create serializable structure from storages
+            #[allow(non_snake_case)]
+            pub fn new(entities: Entities<'a>,
+                       markers: ReadStorage<'a, X>


This is not resolved, could you use the latest version of rustfmt-nightly and format this please?

torkleyy

LGTM, thank you!

torkleyy · 2017-10-04T16:50:15Z

@omni-viral Thank you for this big PR!

bors r+

275: Add an ability to save and load entities. r=torkleyy a=omni-viral Add (de)serialization support for entities to specs. Main parts are: * `Marker` trait. `Marker` is used for several purposes: * Mark which entities should be serialized. * It contains ID to replace `Entity` stored in `Component`s during serialization process. * It may contain additional data to be used for tracking owner of the component. * `MarkerAllocator` trait for allocating `Marker`s, storing `Marker -> Entity` mapping and other stuff. * `SerializableComponent` trait that is auto implemented by components if they implements `Serialize`, `DeserializeOwned` and `Copy` (later is rather strict, maybe it should be replaced with `Clone` or removed somehow). Mainly `Component` implements `SerializableComponent` manually when it contains `Entity`. `Component` should replace `Entity` with provided `Marker` in `SerializableComponent::save` function. And replace `Marker` with `Entity` in `SerializableComponent::load` function. * `Components` trait is implemented for tuples of `SerializableComponent`s It allows to save all existing and mentioned in tuple components of one entity from `ReadStorage`s into serializable structure. And load it into `WriteStorage`s again. Helper types `WorldSerialize` and `WorldDeserialize` are good example how to use this facility.

bors · 2017-10-04T17:03:48Z

Build succeeded

continuous-integration/travis-ci/push

torkleyy requested review from kvark and csherratt September 28, 2017 13:43

torkleyy requested changes Sep 28, 2017

View reviewed changes

zakarumych mentioned this pull request Sep 29, 2017

Serializing/Deserializing the Entity struct #138

Closed

AnneKitsune approved these changes Oct 2, 2017

View reviewed changes

torkleyy reviewed Oct 2, 2017

View reviewed changes

kvark reviewed Oct 2, 2017

View reviewed changes

torkleyy reviewed Oct 2, 2017

View reviewed changes

(de)serialization feature

9d6baa0

rebased

zakarumych force-pushed the serde branch from 01d0ab3 to 9d6baa0 Compare October 2, 2017 16:19

zakarumych changed the title ~~saveload feature~~ Add an ability to save and load entities. Oct 2, 2017

Fix example

fd39bcf

WaDelma reviewed Oct 2, 2017

View reviewed changes

Fix spelling in docs and other styling stuff

18ad0e3

torkleyy reviewed Oct 4, 2017

View reviewed changes

Addes @torkleyy s comments

b1ad528

torkleyy approved these changes Oct 4, 2017

View reviewed changes

bors bot merged commit b1ad528 into amethyst:master Oct 4, 2017

zakarumych deleted the serde branch January 19, 2018 12:09



		/// `DeerializeSeed` implementation for `World`
		//#[derive(SystemData)]

		}


		impl<'a, M, E, T> SystemData<'a> for WorldSerialize<'a, M, E, T>

		@@ -0,0 +1,211 @@
		//! Provides with `Marker` and `MarkerAllocator` traits

		@@ -0,0 +1,13 @@
		//! Save and load entites from various formats with serde


		use self::details::{Components, EntityData, SerializableComponent, Storages};

		pub mod marker;


		use {Entities, FetchMut, Join, ReadStorage, WriteStorage};

		use super::{Components, EntityData, Storages};


		(@) => {};

		(@ $ah:ident\|$bh:ident $(,$a:ident\|$b:ident)*) => {

		use serde::de::DeserializeOwned;


		/// This trait should be implemetened by component which is gonna be used as marker.



		/// Serialize components from specified storages via `SerializableComponent::save`
		/// of all marked entities with provided serializer.

Add an ability to save and load entities. #275

Add an ability to save and load entities. #275

Conversation

zakarumych commented Sep 27, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

zakarumych Oct 2, 2017 • edited Loading

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

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

zakarumych Oct 2, 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

zakarumych Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

kvark commented Oct 2, 2017

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

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

zakarumych commented Sep 27, 2017 •

edited

Loading

zakarumych Sep 29, 2017 •

edited

Loading

zakarumych Oct 2, 2017 •

edited

Loading

zakarumych Oct 2, 2017 •

edited

Loading

zakarumych Oct 2, 2017 •

edited

Loading