Component Grouping #201

Aceeri · 2017-06-18T03:31:24Z

fixes #155

Restrutures into a workspace of specs and specs_derive.

torkleyy · 2017-06-18T06:29:21Z

specs/README.md

@@ -0,0 +1,71 @@
+# specs


It seems your rebase wasn't quite correct, this is the old README.

WaDelma

The call macro being limited to certain maximum recursion is unfortunate, but as we discussed there isn't really way to make it better. Otherwise looking good.

WaDelma · 2017-06-18T07:38:43Z

specs_derive/src/component_group.rs

+            #[allow(unused_mut)]
+            let mut list = #name::local_components();
+            #(
+                for component in #subgroup_ty::components() {


Can't this use extend instead of manual loop?

WaDelma · 2017-06-18T07:41:07Z

specs/src/group.rs

+pub trait SerializeGroup: ComponentGroup {
+    /// Serializes the group of components from the world.
+    fn serialize_group<S: Serializer>(world: &World, serializer: S) -> Result<S::Ok, S::Error>;
+    /// Helper method for serializing the world.


Do we need to expose these helpers in the trait or are they only called in trait implementation? If they are not needed anywhere else then these helpers are just implementation detail and should probably removed.

kvark · 2017-06-19T14:45:09Z

Can we have the restructurization PR separate from the new functionality please?

WaDelma · 2017-06-19T16:05:41Z

You have to use workspace to include the custom derive as it needs to be it's own crate. Dunno if splitting it to it's own PR would be possible.

Aceeri · 2017-06-21T02:18:44Z

I mean, I could have another PR for it if we want, but it would either just be a workspace of "specs" and "specs_derive" (without functionality).

torkleyy · 2017-06-21T14:02:15Z

@Aceeri Maybe you could structure it the same way @ebkalderon did in their PR?

Aceeri · 2017-06-21T23:29:49Z

@torkleyy I feel it makes more sense to use a workspace given this is exactly the usecase for it (multiple crates in one repo).

torkleyy · 2017-06-22T14:17:50Z

@Aceeri They also implemented it as a workspace, see https://github.com/slide-rs/specs/pull/192/files

torkleyy

The syntax in the example looks nice! IIRC it's a great improvement over the current serialize example.

torkleyy · 2017-07-06T07:37:42Z

specs_derive/README.md

@@ -0,0 +1,107 @@
+# Specs


I don't think we need a second readme here (or at least, don't copy the specs README and just show a small example of component groups).

torkleyy · 2017-07-06T07:39:22Z

src/world.rs

+
+#[cfg(feature="serialize")]
+/// Structure used to serialize a world using a component group.
+pub struct WorldSerializer<'a, G> {


Do we really need a struct for this?

Yeah, if I implemented Serialize or Deserialize on the World itself, then it would make a new copy of the world.

I was talking about just a function actually.

Well, it ties in better with serdes traits and such this way rather than just have some special methods for it.

I don't really mind either way since it would still be possible if it was a function instead of a struct.

Aceeri · 2017-07-07T22:33:54Z

Well uh, I have made a mistake so please ignore this for the next couple hours while I go get my backup.

Aceeri · 2017-07-08T18:29:12Z

@torkleyy For the WorldSerializer, I just realized why I originally had it. Some serde libraries don't expose Serializer in their interfaces which is annoying, but the only way to serialize with them is to pass in a type that implements Serialize.

torkleyy · 2017-07-08T18:37:58Z

src/world.rs

+    /// Creates a new world deserializer out of a world and a list of entities.
+    ///
+    /// The list of entities will be used to merge into the component storages.
+    pub fn new(world: &'a mut World, entities: &'a [Entity]) -> WorldDeserializer<'a, G> {


I wonder if it really makes sense to pass a slice of entities. Is there any case where you want to deserialize components and insert them for existing entities? I mean can you really ensure this works reliable? Which brings me to the question if it should be allowed for an entity to not have a component of the component group.

Is there any case where you want to deserialize components and insert them for existing entities?

I was mainly thinking stuff like prefabs here, so a prefab could be deserialized into an existing entity.

Which brings me to the question if it should be allowed for an entity to not have a component of the component group.

Well, if we disallowed that then every entity would have to have every component in that component group if we wanted to serialize the world properly.

I was mainly thinking stuff like prefabs here, so a prefab could be deserialized into an existing entity.

Exactly, so a new entity has to be created.

What if you want to put in multiple prefabs for a single entity though?

torkleyy · 2017-07-08T18:39:45Z

@Aceeri I see. It's up to you how you implement it then.

Aceeri · 2017-10-20T04:40:10Z

Closing this for now since I'm not sure if it is worth the complexity yet.

201: Add `ci.yml` github actions workflow. r=azriel91 a=azriel91 Switch CI to use github actions, as bors isn't getting past travis (always times out). Co-authored-by: Azriel Hoh <azriel91@gmail.com>

Aceeri force-pushed the group branch 2 times, most recently from 1173116 to 993ba09 Compare June 18, 2017 04:22

torkleyy reviewed Jun 18, 2017

View reviewed changes

specs/README.md Outdated

@@ -0,0 +1,71 @@

# specs

Copy link

Member

torkleyy Jun 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems your rebase wasn't quite correct, this is the old README.

WaDelma reviewed Jun 18, 2017

View reviewed changes

Aceeri force-pushed the group branch 7 times, most recently from 905620a to 6524127 Compare July 2, 2017 15:11

torkleyy reviewed Jul 6, 2017

View reviewed changes

Aceeri force-pushed the group branch 4 times, most recently from d0a6153 to 377d248 Compare July 7, 2017 21:41

Aceeri force-pushed the group branch 6 times, most recently from a730839 to 0376ecf Compare July 8, 2017 01:45

Component Group derive macro

b280fec

Aceeri force-pushed the group branch 16 times, most recently from 2a0277d to 3fd2365 Compare July 8, 2017 18:23

Aceeri force-pushed the group branch from 3fd2365 to 7df74b5 Compare July 8, 2017 18:34

torkleyy reviewed Jul 8, 2017

View reviewed changes

Aceeri force-pushed the group branch 2 times, most recently from 1137f3a to f96b221 Compare July 8, 2017 19:42

Aceeri mentioned this pull request Jul 8, 2017

Nightly regression on x86_64_unknown_linux_gnu rust-lang/rust#43126

Closed

Aceeri force-pushed the group branch from f96b221 to 07060bc Compare July 9, 2017 17:44

Aceeri force-pushed the group branch from 07060bc to e76d59b Compare July 19, 2017 21:36

Extend component list

7f95c6c

Aceeri force-pushed the group branch from e76d59b to 7f95c6c Compare July 23, 2017 20:23

Aceeri closed this Oct 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Component Grouping #201

Component Grouping #201

Aceeri commented Jun 18, 2017 •

edited

Loading

torkleyy Jun 18, 2017

WaDelma left a comment

WaDelma Jun 18, 2017

WaDelma Jun 18, 2017

kvark commented Jun 19, 2017

WaDelma commented Jun 19, 2017 •

edited

Loading

Aceeri commented Jun 21, 2017

torkleyy commented Jun 21, 2017

Aceeri commented Jun 21, 2017

torkleyy commented Jun 22, 2017

torkleyy left a comment

torkleyy Jul 6, 2017

torkleyy Jul 6, 2017

Aceeri Jul 6, 2017

torkleyy Jul 6, 2017

Aceeri Jul 6, 2017

Aceeri Jul 6, 2017

Aceeri commented Jul 7, 2017

Aceeri commented Jul 8, 2017

torkleyy Jul 8, 2017

Aceeri Jul 8, 2017

torkleyy Jul 8, 2017

Aceeri Jul 8, 2017

torkleyy commented Jul 8, 2017

Aceeri commented Oct 20, 2017

Component Grouping #201

Component Grouping #201

Conversation

Aceeri commented Jun 18, 2017 • edited Loading

Choose a reason for hiding this comment

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

kvark commented Jun 19, 2017

WaDelma commented Jun 19, 2017 • edited Loading

Aceeri commented Jun 21, 2017

torkleyy commented Jun 21, 2017

Aceeri commented Jun 21, 2017

torkleyy commented Jun 22, 2017

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

Aceeri commented Jul 7, 2017

Aceeri commented Jul 8, 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

torkleyy commented Jul 8, 2017

Aceeri commented Oct 20, 2017

Aceeri commented Jun 18, 2017 •

edited

Loading

WaDelma commented Jun 19, 2017 •

edited

Loading