Custom derive for Component trait #192

ebkalderon · 2017-06-12T21:40:11Z

This PR introduces an optional specs_derive crate containing a custom derive macro for defining new components in a less verbose way. The ergonomics loosely resemble the derivative crate.

Before

#[derive(Debug)]
struct Pos(f32, f32, f32);

impl Component for Pos {
    type Storage = VecStorage<Pos>;
}

After

#[derive(Component, Debug)]
struct Pos(f32, f32, f32);

The macro will store components in VecStorages by default. To specify a different storage type, you may use the #[component] attribute.

#[derive(Component, Debug)]
#[component(HashMapStorage)]
struct Pos(f32, f32, f32);

I also included a revised basic.rs example called basic_derive.rs to demonstrate its usage, but this can probably be omitted from the PR if people ultimately prefer implementing Component the explicit way.

EDIT: Use DenseVecStorage by default.

torkleyy

Great to see you hear, @ebkalderon!

Looks great, but I would use another storage for the default. Options would be BTreeStorage and DenseVecStorage. The latter one was a bit faster when I benchmarked it IIRC.

torkleyy · 2017-06-13T06:59:48Z

specs_derive/src/lib.rs

+use quote::Tokens;
+
+#[proc_macro_derive(Component, attributes(component))]
+pub fn component(input: TokenStream) -> TokenStream {


Would be nice to add a small doc comment here.

Okay, will do.

torkleyy · 2017-06-13T07:01:45Z

specs_derive/src/lib.rs

+            _ => None,
+        })
+        .cloned()
+        .unwrap_or(Ident::new("::specs::VecStorage"));


I'm not sure if VecStorage should be the default. Maybe DenseVecStorage would be better here?

If that's indeed the case, I'll switch it to use DenseVecStorage.

torkleyy · 2017-06-13T07:02:42Z

specs_derive/src/lib.rs

+
+fn impl_component(ast: &MacroInput) -> Tokens {
+    let name = &ast.ident;
+    let (impl_generics, ty_generics, where_clause) = ast.generics.split_for_impl();


Didn't even know that syn has this method. Nice!

It's very nice! I learned about it from the Procedural Macros chapter of the Rust book.

torkleyy · 2017-06-13T07:03:11Z

src/join.rs

@@ -1,7 +1,7 @@
 use std;
 use std::cell::UnsafeCell;

-use hibitset::{BitSetAnd, BitIter, BitSetLike, BitProducer};
+use hibitset::{BitIter, BitProducer, BitSetAnd, BitSetLike};


Whoops, forgot to reorder this one.

torkleyy

Btw, I really like the derive for components, so maybe we could update the README with it?

ebkalderon · 2017-06-13T14:16:42Z

@torkleyy Sure, I would be happy to update the README! But if we're using #[derive(Component)] by default, should I remove the basic_derive.rs example and update the existing examples to use the macro?

torkleyy · 2017-06-13T14:19:30Z

@ebkalderon

should I remove the basic_derive.rs example and update the existing examples to use the macro?

Yes, I think we should do that. And we could just show both ways of implementing Component in the basic example to explain what it desugars to and then use the #[derive(Component)] everywhere we want.

ebkalderon · 2017-06-13T14:37:34Z

@torkleyy Another question: would you like me to re-export these macros under the top-level specs crate, or would you prefer users explicitly specify #[macro_use] extern crate specs_derive in their projects?

EDIT: If so, under that same logic, couldn't the shred_derive macros also be re-exported under specs too?

torkleyy · 2017-06-13T14:39:04Z

@ebkalderon Is it possible to reexport custom-derive macros?

ebkalderon · 2017-06-13T14:44:30Z

@torkleyy I'm pretty sure you can. I can quickly check on my end to verify, once I'm done revising the PR.

EDIT: Yes we can! 🎉 Would you like me to do that for both specs_derive and shred_derive?

ebkalderon · 2017-06-13T15:41:44Z

@torkleyy I successfully re-exported specs_derive and compiled the examples with only the specs crate, but I can't do the same with shred_derive since #[derive(SystemData)] has a line which expands to ::shred::SystemData. Oh well, at least specs_derive re-exports fine.

ebkalderon · 2017-06-13T16:42:25Z

Finally done! I have updated the README, book, examples, and and as many doc comments as I could find. Let me know if there are any nits to take care of.

torkleyy

Great, thanks for addressing all of my comments. Only one nit left.

torkleyy · 2017-06-13T16:36:16Z

README.md

@@ -33,20 +33,19 @@ Unlike most other ECS libraries out there, it provides
 ## Example

 ```rust
+#[macro_use]
+extern crate specs;


Let's not have any extern crates or imports in the README, it's irrelevant for the API usage.

I understand, I'll fix that.

torkleyy · 2017-06-13T16:46:59Z

specs_derive/src/lib.rs

+/// #[derive(Component, Debug)]
+/// #[component(HashMapStorage)]
+/// struct Pos(f32, f32, f32);
+/// ```


That's great, thanks 👍

torkleyy

LGTM, let's let @kvark glance over it before merging.

ebkalderon · 2017-06-13T17:18:39Z

examples/full.rs


 // -- Components --
 // A component exists for 0..n
 // entities.

-#[derive(Clone, Debug)]
+// `VecStorage` is meant to be used for components that are in almost every
+// entity tend to be highly contested.


Whoops, forgot the word "and" here.

kvark · 2017-06-14T01:57:28Z

I'm sorry but I don't find this useful enough. It's saving just a few lines of user code, at the cost of more lines in our code (specs_derive) but, more importantly, another layer of indirection to think about. I'd rather want people to implement Component and understand the storage types, which is arguably easier than applying magic attributes.

ebkalderon · 2017-06-14T02:13:26Z

@kvark Regardless, people will need to understand the storage types in order to use the attribute properly; it's just presented more concisely from the user's side. But still, you raise some good points. In the end, it's your call! 😀

malleusinferni · 2017-06-14T02:53:19Z

I'm using specs in a codebase with nearly 50 different component types (and climbing). I currently implement Component via a macro, just to cut down on the number of places where I can make a mistake. While I can't speak for other users, I would personally prefer to use #[derive] for this purpose. If this change is rejected, I'll roll my own implementation.

kvark · 2017-06-14T03:21:59Z

@malleusinferni thanks for your input! What's wrong with the macro you are using now?
Also, do @csherratt, @Aceeri and @WaDelma have strong opinions?

csherratt · 2017-06-14T03:59:09Z

My first instinct was exactly what @kvark said. It saves a few lines per component, and comes at a cost of some indirection which can confuse new users. That being said, I'm not strong in my position.

I think we could probably strike a balance, if @malleusinferni finds it useful. There are bound to be other users who want to use the more terse form. Since this must be published as a secondary crate anyhow, maybe we can show the old form in the examples. We can include a quick passage in the book/readme so that people can find this crate.

torkleyy · 2017-06-14T04:25:08Z

Please don't forget: while it's great to be beginner-friendly (which we are; we got a book, nice examples and a Gitter channel to ask for help), the people dealing with these components and the Specs API will do that for many, many hours (like @malleusinferni). The basic example explains what is implemented and AFAIK also tells which storage is used.

Using the right storage for the right situation isn't any different: now, beginners would just copy the example code, then they'd copy #[derive(Component)] #[component(VecStorage)]. I don't think that it would be bad for beginners.

ebkalderon · 2017-06-14T05:25:45Z

Just FYI, for all participants, specs_derive doesn't need to be published to crates.io given that it's a hard dependency and the macro is re-exported from within the specs crate.

Also, if you haven't noticed, the crate itself is pretty darn small (just one source file, with only two functions inside). Nearly all of the changes included in this PR are updating docs, examples, doc comments, the relevant book chapters, and the README. Both the book and the basic example clearly explain what the desugared API boils down to, and I've done my best to document all that I could.

Personally, I think that the reduced user-side boilerplate and the lessening of possible copy-paste errors is worth the effort, but those are just my two cents.

Aceeri · 2017-06-14T06:14:07Z

Im impartial about this, but I have one concern: Does this support wrapper storages like FlaggedStorage?

torkleyy · 2017-06-14T06:45:59Z

@Aceeri Maybe we could use DenseVecStorage as a default there?

WaDelma · 2017-06-14T09:08:58Z

Doesn't crates.io demand that all dependencies are in crates.io?

I would like to have derive as an option as it basically doesn't remove any relevant information and just reduces boilerplate.

It's shame that it cannot have syntax like this though: #[derive(Component(VecStorage))].

WaDelma

Also with custom derives, compiletest would be useful :P

WaDelma · 2017-06-14T09:10:09Z

specs_derive/src/lib.rs

+    let name = &ast.ident;
+    let (impl_generics, ty_generics, where_clause) = ast.generics.split_for_impl();
+
+    let storage = ast.attrs.first()


Doesn't this fail if there is more that one attribute and the component attribute isn't first?

Whoops, you're right! I forgot to change that while I was refactoring.

kvark · 2017-06-14T17:17:10Z

So, let me recap:
Positive: @ebkalderon, @WaDelma (?)
Negative: @kvark
Balanced (needs partial code revert): @csherratt , @torkleyy
Abstained: @Aceeri

Edit: I'm fine with a separate crate being published that provides the derives. I don't find it particularly useful, given that a user can just throw a 4 line macro to do the same thing in an even shorter form, but I'm fine with it if it's not propagating to specs and just builds on top of it.

torkleyy · 2017-06-14T17:22:34Z

Proposing compromise:

Merge this crate and leave it in-tree
Point to it in README, full example and book; code examples should implement it explicitly, only the book contains a single and short example of how to use #[derive(Component)]

torkleyy · 2017-06-14T19:18:07Z

Okay, I think we found a consensus here (I proposed, @kvark upvoted and @csherratt suggested pretty much the same earlier). @ebkalderon Would it be okay for you to move on like this?

ebkalderon · 2017-06-14T21:02:27Z

Yes! That sounds reasonable enough.

EDIT: Unfortunately, I don't have time to work on it tonight, but expect the PR to be revised sometime tomorrow when I'm off work.

ebkalderon · 2017-07-21T18:05:19Z

Okay, I've finally had some spare time to rebase and update this PR to address the concerns raised. Please feel free to review!

torkleyy

Nice to see progress here!

Could we use specs-derive instead of specs_derive like I did it with shred and shred-derive?

torkleyy · 2017-07-21T18:31:30Z

benches/parallel.rs

-use specs::{Component, DenseVecStorage, DispatcherBuilder, Entities, Entity, Fetch,
-            HashMapStorage, Join, NullStorage, ReadStorage, RunningTime, System, VecStorage,
-            World, WriteStorage};
+use specs::{DenseVecStorage, DispatcherBuilder, Entities, Entity, Fetch, HashMapStorage, Join,


We still need that export.

Good catch, thank you.

torkleyy · 2017-07-21T18:31:48Z

benches/world.rs

@@ -3,11 +3,12 @@
 extern crate cgmath;
 extern crate rand;
 extern crate rayon;
+#[macro_use]


Not necessary anymore

Thanks, I missed that.

torkleyy · 2017-07-21T18:32:02Z

benches/world.rs

 extern crate specs;

 extern crate test;

-use specs::{Component, HashMapStorage, Join, ParJoin, VecStorage, World};
+use specs::{HashMapStorage, Join, ParJoin, VecStorage, World};


Missing import

Sorry, forgot to add it back in after rebasing.

torkleyy · 2017-07-21T18:32:15Z

book/src/02_hello_world.md

@@ -20,7 +21,7 @@ extern crate specs;
 Let's start by creating some data:

 ```rust,ignore
-use specs::{Component, VecStorage};
+use specs::VecStorage;


Missing import

Thank you, I'll fix that.

torkleyy · 2017-07-21T18:33:00Z

book/src/02_hello_world.md

-(see [the storages chapter][sc] for more details), so we can associate some data
-with an entity. Before doing that, we need to create a world, because this is
-where the storage for all the components is located.
+These will be our two component types. Optionally, `specs` includes a convenient


I'm not sure. I think we agreed we wouldn't want #[derive] shown in the book - except in one dedicated chapter.

Do we need a dedicated chapter for this macro, though? It is mentioned here once as an optional thing, and then never used again in the book. Still, I can create a new chapter if you prefer.

No, we don't need to. If somebody wants it, it's not hard to do so.

Sounds good.

torkleyy · 2017-07-21T18:33:39Z

book/src/02_hello_world.md

@@ -137,6 +157,7 @@ hello_world.run_now(&world.res);
 Here the complete example of this chapter:

 ```rust,ignore
+<<<<<<< HEAD


I think this was left when rebasing.

Whoops, you're right. I'll go remove that.

torkleyy · 2017-07-21T18:34:00Z

book/src/03_dispatcher.md

@@ -102,6 +102,7 @@ dispatcher.dispatch(&mut world.res);
 Here the code for this chapter:

 ```rust,ignore
+<<<<<<< HEAD


Sorry, I messed up on that one.

torkleyy · 2017-07-21T18:35:26Z

src/lib.rs

@@ -167,17 +177,22 @@
 //!
 //!

+#[allow(unused_imports)]
+#[macro_use]
+extern crate specs_derive;


I don't think we need to include it here.

I was hoping to allow specs_derive to re-export the procedural macro to the user, meaning they don't need to add extern crate specs_derive; to their project. If you prefer that users add it explicitly as a separate crate to their projects, I can remove it.

Not sure. cc @kvark

Just discussed with @kvark, we decided not to reexport it here so it doesn't get included if it's unnecessary for the user.

Okay, I'll take it out.

torkleyy · 2017-07-21T18:36:14Z

src/lib.rs

 pub use storage::{BTreeStorage, CheckStorage, DenseVecStorage, DistinctStorage, FlaggedStorage,
                  HashMapStorage, InsertResult, NullStorage, ReadStorage, Storage,
                  UnprotectedStorage, VecStorage, WriteStorage};
 pub use world::{Component, CreateIter, CreateIterAtomic, EntitiesRes, Entity, EntityBuilder,
                Generation, LazyUpdate, World};

+#[cfg(not(target_os = "emscripten"))]
+pub use shred::{AsyncDispatcher};


Why move the re-export?

Sorry, I must have made a mistake while copy-pasting sections around during the rebase.

torkleyy · 2017-07-21T21:05:56Z

book/src/02_hello_world.md

@@ -12,6 +12,7 @@ specs = "0.9"
 And add this to your crate root (`main.rs` or `lib.rs`):

 ```rust,ignore
+#[macro_use]


No need for #[macro_use].

Thanks, removed.

torkleyy

Great PR, thank you so much!

torkleyy · 2017-07-21T21:15:13Z

Hold on, please add an entry to the changelog.

torkleyy · 2017-07-21T21:15:56Z

src/lib.rs

@@ -191,6 +202,7 @@ pub use shred::{Dispatcher, DispatcherBuilder, Fetch, FetchId, FetchIdMut,
 #[cfg(not(target_os = "emscripten"))]
 pub use shred::{AsyncDispatcher};

+pub use specs_derive::*;


This line causes the build to fail.

ebkalderon · 2017-07-22T00:40:21Z

@torkleyy Done! It should build correctly now, and I've updated the changelog.

torkleyy

Perfect, thank you so much!

torkleyy · 2017-07-22T07:17:15Z

bors r+

192: Custom derive for Component trait r=torkleyy This PR introduces an optional `specs_derive` crate containing a custom derive macro for defining new components in a less verbose way. The ergonomics loosely resemble the [`derivative`](https://github.com/mcarton/rust-derivative) crate. ### Before ```rust #[derive(Debug)] struct Pos(f32, f32, f32); impl Component for Pos { type Storage = VecStorage<Pos>; } ``` ### After ```rust #[derive(Component, Debug)] struct Pos(f32, f32, f32); ``` The macro will store components in `VecStorage`s by default. To specify a different storage type, you may use the `#[component]` attribute. ```rust #[derive(Component, Debug)] #[component(HashMapStorage)] struct Pos(f32, f32, f32); ``` I also included a revised `basic.rs` example called `basic_derive.rs` to demonstrate its usage, but this can probably be omitted from the PR if people ultimately prefer implementing `Component` the explicit way. **EDIT**: Use `DenseVecStorage` by default.

bors · 2017-07-22T07:22:13Z

Build succeeded

continuous-integration/travis-ci/push

193: Maintenance/192/ongoing code maintenance r=azriel91 a=azriel91 Closes amethyst#192. Co-authored-by: Azriel Hoh <[email protected]>

torkleyy added the ready label Jun 13, 2017

torkleyy self-requested a review June 13, 2017 03:42

torkleyy reviewed Jun 13, 2017

View reviewed changes

torkleyy added in progress and removed ready labels Jun 13, 2017

torkleyy added ready and removed in progress labels Jun 13, 2017

torkleyy approved these changes Jun 13, 2017

View reviewed changes

torkleyy requested a review from kvark June 13, 2017 17:07

torkleyy modified the milestone: 0.9 Jun 13, 2017

ebkalderon commented Jun 13, 2017

View reviewed changes

WaDelma reviewed Jun 14, 2017

View reviewed changes

torkleyy added in progress and removed ready labels Jun 16, 2017

ebkalderon added 2 commits July 21, 2017 13:56

Create specs_derive crate, rustfmt codebase, ignore *.bk files

6ce59f3

Address suggestions and concerns, reference #[derive] only in book

fb8d541

torkleyy reviewed Jul 21, 2017

View reviewed changes

ebkalderon added 2 commits July 21, 2017 16:57

Fix missing and moved imports, remove rebasing leftovers

9587538

Rename specs_derive crate to specs-derive

2912cdd

torkleyy reviewed Jul 21, 2017

View reviewed changes

ebkalderon added 2 commits July 21, 2017 17:08

Make specs-derive optional

2017538

Remove extra macro_use directive

1578392

torkleyy approved these changes Jul 21, 2017

View reviewed changes

torkleyy added ready and removed in progress labels Jul 21, 2017

torkleyy reviewed Jul 21, 2017

View reviewed changes

Update changelog, remove leftover re-export line

086f698

torkleyy approved these changes Jul 22, 2017

View reviewed changes

bors bot merged commit 086f698 into amethyst:master Jul 22, 2017

xMAC94x pushed a commit to xMAC94x/specs that referenced this pull request Mar 10, 2021

Merge amethyst#193

2a3e147

193: Maintenance/192/ongoing code maintenance r=azriel91 a=azriel91 Closes amethyst#192. Co-authored-by: Azriel Hoh <[email protected]>

Custom derive for Component trait #192

Custom derive for Component trait #192

Conversation

ebkalderon commented Jun 12, 2017 • edited Loading

Before

After

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

torkleyy Jun 13, 2017 • edited Loading

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

ebkalderon commented Jun 13, 2017

torkleyy commented Jun 13, 2017

ebkalderon commented Jun 13, 2017 • edited Loading

torkleyy commented Jun 13, 2017

ebkalderon commented Jun 13, 2017 • edited Loading

ebkalderon commented Jun 13, 2017

ebkalderon commented Jun 13, 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

torkleyy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvark commented Jun 14, 2017

ebkalderon commented Jun 14, 2017

malleusinferni commented Jun 14, 2017

kvark commented Jun 14, 2017

csherratt commented Jun 14, 2017

torkleyy commented Jun 14, 2017 • edited Loading

ebkalderon commented Jun 14, 2017

Aceeri commented Jun 14, 2017

torkleyy commented Jun 14, 2017

WaDelma commented Jun 14, 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

kvark commented Jun 14, 2017 • edited Loading

torkleyy commented Jun 14, 2017

torkleyy commented Jun 14, 2017

ebkalderon commented Jun 14, 2017 • edited Loading

ebkalderon commented Jul 21, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebkalderon Jul 21, 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

ebkalderon Jul 21, 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

ebkalderon commented Jun 12, 2017 •

edited

Loading

torkleyy Jun 13, 2017 •

edited

Loading

ebkalderon commented Jun 13, 2017 •

edited

Loading

ebkalderon commented Jun 13, 2017 •

edited

Loading

torkleyy commented Jun 14, 2017 •

edited

Loading

kvark commented Jun 14, 2017 •

edited

Loading

ebkalderon commented Jun 14, 2017 •

edited

Loading

ebkalderon Jul 21, 2017 •

edited

Loading

ebkalderon Jul 21, 2017 •

edited

Loading