Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PrefabData derive support for enums #1625

Merged
merged 16 commits into from May 26, 2019

Conversation

Projects
None yet
5 participants
@alec-deason
Copy link
Contributor

commented May 21, 2019

Description

PrefabData derive support for enums.

This is my first ever macro code in rust and I'm still learning about how the prefab system works so I would super, super appreciate it if folks could double check that I haven't missed cases in the code or the test.

This requires an update to the book which I'm still working on (though this already includes an updated example). I'll add that to the PR shortly but I was hoping to get some eyes on the code while I work on the docs.

Additions

You can now derive PrefabData for enums which is a thing I would like to do.

Removals

n/a

Modifications

n/a

PR Checklist

By placing an x in the boxes I certify that I have:

  • Ran cargo test --all locally if this modified any rs files.
  • Ran cargo +stable fmt --all locally if this modified any rs files.
  • Updated the content of the book if this PR would make the book outdated.
  • Added a changelog entry if this will impact users, or modified more than 5 lines of Rust that wasn't a doc comment.
  • Added unit tests for new APIs if any were added in this PR.
  • Acknowledged that by making this pull request I release this code under an MIT/Apache 2.0 dual licensing scheme.
@jojolepro
Copy link
Member

left a comment

Ooooh looks very interesting!

@jaynus

jaynus approved these changes May 22, 2019

@alec-deason

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

Ok. I think that's a reasonable update to the book for this stuff. I'm done changing things unless someone has additional comments.

@torkleyy
Copy link
Member

left a comment

Lovely, thank you @alec-deason!

player: Option<Named>,
weapon: Option<Weapon>,
```rust,edition2018,no_run,noplaypen
# extern crate amethyst;

This comment has been minimized.

Copy link
@torkleyy

torkleyy May 24, 2019

Member

I think we can get rid of all those extern crate statements.

This comment has been minimized.

Copy link
@alec-deason

alec-deason May 24, 2019

Author Contributor

Is there a good way to check syntax on these snippets? mdbook test doesn't seem to run them.

# extern crate serde;
# extern crate specs_derive;
#
# use amethyst::{

This comment has been minimized.

Copy link
@torkleyy

torkleyy May 24, 2019

Member

Wouldn't it make sense to show these imports (and maybe even the two structs below)?

This comment has been minimized.

Copy link
@alec-deason

alec-deason May 24, 2019

Author Contributor

Yeah, I was following the style of the existing writeup but I think you're right, showing more is useful. I'll open it up.

This comment has been minimized.

Copy link
@alec-deason

alec-deason May 24, 2019

Author Contributor

I included the structs but now that I look at this again I see that the imports are already visible in a block just above. I think that's cleaner that showing them twice so I'm going to leave it that way.

This comment has been minimized.

Copy link
@torkleyy

torkleyy May 24, 2019

Member

Ah, you're right, sorry. I've missed that.

Three {},
Four,
Five(Stuff<usize>, #[prefab(Component)] External),
}

This comment has been minimized.

Copy link
@torkleyy

torkleyy May 24, 2019

Member

Could you add some actual tests (functions tagged with #[test])? I think EnumPrefab covers the cases pretty well, we just need to check that it correctly instantiates the prefab.

This comment has been minimized.

Copy link
@alec-deason

alec-deason May 24, 2019

Author Contributor

Yeah, that's a good idea.

@alec-deason

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

As I start to use this in my project I realize that it falls down for a more complex, but common, case. If two variants both use the same resource then that leads to a double borrow so you have to go back to the superset struct with nullable fields model.

That's a bad experience obviously and makes this much less useful in practice. I think I see how to support the shared resource case but I'm going to need fiddle with it to make sure it works. I think I'd like to leave the scope of this PR as is and do the remaining work as a second PR unless folks think this isn't worth having without support for the shared resource case.

@torkleyy

This comment has been minimized.

Copy link
Member

commented May 24, 2019

If two variants both use the same resource then that leads to a double borrow so you have to go back to the superset struct with nullable fields model.

Ah, right, that was the reason it wasn't implemented in the first place! I'm not sure how useful enum support is on its own, with the current behaviour.

If we want to merge enum support with the current limitation, it would be great to check duplicated resource access in the derive macro; I'm not sure how easy this is, and I believe we can only do this for collisions caused by one struct (not being able to check nested prefabs).

cc @azriel91 Any opinion on this?

@torkleyy torkleyy requested a review from azriel91 May 24, 2019

@alec-deason

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

Assuming SystemData works the way I think it does then I can check for duplicates and emit a useful error even with arbitrarily deep nesting. So there's that at least.

If every SystemData in the tree is a tuple then I think I should be able to automatically construct everything that needs to get passed down while correctly handling duplicates. Since tuple SystemDatas are (I think) overwhelmingly the most common case then I believe that would be enough to be useful.

Both those things assume that my mental model of some stuff I don't really understand well is correct so I'll have to write code to know if I can actually do it. But I really want this feature so I'll poke at it today.

@alec-deason

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

No, wait. The derive macro only cares about the first level of the tree relative to itself. I think that's true... Makes it simpler.

@alec-deason

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

No, this is much harder. I'm not thinking clearly about what it means to be inside a macro. Sigh. Seems like it must be possible though...

@azriel91
Copy link
Member

left a comment

I haven't finished reviewing, but thank you for working through the whole flow (code, tests, docs) 🙇!

@@ -165,7 +165,7 @@ cargo run --example prefab_multi

### Multiple Entities, Different Components

The next level is to instantiate multiple entities, each with their own set of [`Component`]s. The current implementation of [`Prefab`] requires the `data` field to be the same type for *every* [`PrefabEntity`] in the list. This means we would be unable to declare something like the following snippet, because it uses a `Player` prefab data in one entity, and `Weapon` in another:
The next level is to instantiate multiple entities, each with their own set of [`Component`]s. The current implementation of [`Prefab`] requires the `data` field to be the same type for *every* [`PrefabEntity`] in the list. This means that to have different types of entity in the same prefab they must be variants of an enum. For instance, a prefab like this:

```rust,ignore
// Note: Invalid / erroneous example

This comment has been minimized.

Copy link
@azriel91

azriel91 May 25, 2019

Member

no longer invalid 😄

Sword,
}
/// All fields implement `PrefabData`, and are wrapped in `Option<_>`.

This comment has been minimized.

Copy link
@azriel91

azriel91 May 25, 2019

Member

This comment is somewhat inconsistent with the enum

This comment has been minimized.

Copy link
@alec-deason

alec-deason May 26, 2019

Author Contributor

Yeah, that's wrong.

@@ -29,101 +24,91 @@ If you intend to include a [`Component`] that has not yet got a corresponding [`
Error,
};
use serde::{Deserialize, Serialize};
use specs_derive::Component;
```

3. Define the aggregate prefab data type.

In these examples, `Named`, `Position`, and `Weapon` all derive [`PrefabData`].

This comment has been minimized.

Copy link
@azriel91

azriel91 May 25, 2019

Member

This line and the code snippet don't match up any more; I think the enum prefab data should come under here now

This comment has been minimized.

Copy link
@alec-deason

alec-deason May 26, 2019

Author Contributor

Hmm, actually I think I disagree. It shows an example of grouping multiple PrefabDatas into an aggregate first and then moves to the more complex example of using an enum to support heterogeneous types. That seems like the right flow to me.

This comment has been minimized.

Copy link
@azriel91

azriel91 May 26, 2019

Member

oh yes, read the whole thing (instead of just the snippet) and it makes sense

@azriel91
Copy link
Member

left a comment

Thank you very much for this, also the attention to detail (like _suppressing_warnings)

some comments on cleaning, but overall amazing work 💯

Show resolved Hide resolved amethyst_derive/src/prefab_data.rs
Show resolved Hide resolved amethyst_derive/src/prefab_data.rs
.enumerate()
.map(|(field_number, field)| match &field.ident {
Some(name) => {
if !have_component_attribute(&field.attrs[..]) {

This comment has been minimized.

Copy link
@azriel91

azriel91 May 25, 2019

Member

this negation appears in a number of places, so this tiny helper function would help my brain a bit better 😛

#[inline]
fn is_aggregate_prefab(attrs: &[Attribute]) -> bool {
    !have_component_attribute(attrs)
}

Maybe rename have_component_attribute to is_component_prefab

This comment has been minimized.

Copy link
@alec-deason

alec-deason May 25, 2019

Author Contributor

Legit.

Show resolved Hide resolved amethyst_derive/src/prefab_data.rs
@@ -132,48 +117,48 @@ If you intend to include a [`Component`] that has not yet got a corresponding [`
Finally, the [`#[serde(deny_unknown_fields)]`] ensures that deserialization produces an error if it encounters an unknown field. This will help expose mistakes in the prefab file, such as when there is a typo.

4. Now the type can be used in a prefab.

* *Superset* prefab data:
* *struct* prefab data:

This comment has been minimized.

Copy link
@azriel91

azriel91 May 25, 2019

Member

maybe backticks instead of italics now

@alec-deason

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2019

Ok I give up. I've churned through about six designs for how to solve the double borrow problem and all of them fall apart when I get into the details. I'm not sure you can do it without fundamentally changing shred. Or at least I really don't see the solution. I'll respond to the feedback on this PR (thank you everyone for your comments) and be done.

@azriel91

This comment has been minimized.

Copy link
Member

commented May 26, 2019

Sorry I missed replying to the earlier comment about the double borrow. I think the way @alec-deason implemented it is the cleanest way it can be done (without attempting to change an insurmountable amount of things -- shred, specs, prefab system).

@codecov

This comment has been minimized.

Copy link

commented May 26, 2019

Codecov Report

Merging #1625 into master will increase coverage by 1.03%.
The diff coverage is 95.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1625      +/-   ##
==========================================
+ Coverage   64.45%   65.48%   +1.03%     
==========================================
  Files          94      102       +8     
  Lines        6122     6696     +574     
==========================================
+ Hits         3946     4385     +439     
- Misses       2176     2311     +135
Impacted Files Coverage Δ
amethyst_input/src/button.rs 66.66% <ø> (ø) ⬆️
amethyst_input/src/bindings.rs 97.68% <100%> (+0.22%) ⬆️
amethyst_input/src/axis.rs 100% <100%> (ø) ⬆️
amethyst_input/src/system.rs 62.5% <100%> (+1.63%) ⬆️
...methyst_core/src/transform/components/transform.rs 98.42% <100%> (+0.22%) ⬆️
amethyst_input/src/input_handler.rs 87.14% <90.19%> (+5.55%) ⬆️
amethyst_network/src/connection.rs 92.85% <0%> (ø)
amethyst_audio/src/components/audio_emitter.rs 61.11% <0%> (ø)
amethyst_network/src/server/host.rs 100% <0%> (ø)
amethyst_network/src/test.rs 60.31% <0%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4800ac5...d3ab328. Read the comment docs.

@alec-deason

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

Ok, I believe I've responded to everyone's comments (thanks again for the reviews). The biggest change is a small new section in the book about the limitations of enum PrefabDatas and how to get around them. Basically it brings back the discussion of superset prefabs from the original version.

@torkleyy
Copy link
Member

left a comment

Thank you! Looks good to me!

@azriel91
Copy link
Member

left a comment

Sweet thank you! This is an exemplary PR

@azriel91

This comment has been minimized.

Copy link
Member

commented May 26, 2019

bors r=jaynus, torkleyy, azriel91

bors bot added a commit that referenced this pull request May 26, 2019

Merge #1625
1625: PrefabData derive support for enums r=jaynus,torkleyy,azriel91 a=alec-deason

## Description

PrefabData derive support for enums.

This is my first ever macro code in rust and I'm still learning about how the prefab system works so I would super, super appreciate it if folks could double check that I haven't missed cases in the code or the test.

This requires an update to the book which I'm still working on (though this already includes an updated example). I'll add that to the PR shortly but I was hoping to get some eyes on the code while I work on the docs.

## Additions

You can now derive PrefabData for enums which is a thing I would like to do.

## Removals

n/a

## Modifications

n/a

## PR Checklist

By placing an x in the boxes I certify that I have:

- [x] Ran `cargo test --all` locally if this modified any rs files.
- [x] Ran `cargo +stable fmt --all` locally if this modified any rs files.
- [x] Updated the content of the book if this PR would make the book outdated.
- [x] Added a changelog entry if this will impact users, or modified more than 5 lines of Rust that wasn't a doc comment.
- [x] Added unit tests for new APIs if any were added in this PR.
- [x] Acknowledged that by making this pull request I release this code under an MIT/Apache 2.0 dual licensing scheme.


Co-authored-by: Alec Deason <alec@tinycountry.com>
@bors

This comment has been minimized.

Copy link
Contributor

commented May 26, 2019

@bors bors bot merged commit d3ab328 into amethyst:master May 26, 2019

2 checks passed

bors Build succeeded
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.