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

[Merged by Bors] - Implement and require #[derive(Component)] on all component structs #2254

Closed
wants to merge 30 commits into from

Conversation

Frizi
Copy link
Contributor

@Frizi Frizi commented May 25, 2021

This implements the most minimal variant of #1843 - a derive for marker trait. This is a prerequisite to more complicated features like statically defined storage type or opt-out component reflection.

In order to make component struct's purpose explicit and avoid misuse, it must be annotated with #[derive(Component)] (manual impl is discouraged for compatibility). Right now this is just a marker trait, but in the future it might be expanded. Making this change early allows us to make further changes later without breaking backward compatibility for derive macro users.

This already prevents a lot of issues, like using bundles in insert calls. Primitive types are no longer valid components as well. This can be easily worked around by adding newtype wrappers and deriving Component for them.

One funny example of prevented bad code (from our own tests) is when an newtype struct or enum variant is used. Previously, it was possible to write insert(Newtype) instead of insert(Newtype(value)). That code compiled, because function pointers (in this case newtype struct constructor) implement Send + Sync + 'static, so we allowed them to be used as components. This is no longer the case and such invalid code will trigger a compile error.

@NathanSWard NathanSWard added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events labels May 25, 2021
@NathanSWard
Copy link
Contributor

#1843 is tagged with needs-rfc. Are we planning on not going down the rfc route for this?

@@ -252,7 +252,7 @@ impl AppBuilder {
/// adding [State::get_driver] to additional stages you need it in.
pub fn add_state<T>(&mut self, initial: T) -> &mut Self
where
T: Component + Debug + Clone + Eq + Hash,
T: StateData,
Copy link
Member

Choose a reason for hiding this comment

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

I like this change a lot. Dramatically clearer and safer.

@@ -292,7 +292,7 @@ impl AppBuilder {
/// and inserting a `Events::<T>::update_system` system into `CoreStage::First`.
pub fn add_event<T>(&mut self) -> &mut Self
where
T: Component,
T: Resource,
Copy link
Member

Choose a reason for hiding this comment

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

Yay separating out these bounds! I'll need to swap this to T: Component + Resource for #1626, but that shouldn't be any issue.

@@ -146,20 +143,20 @@ impl<'a> LoadContext<'a> {

/// The result of loading an asset of type `T`
#[derive(Debug)]
pub struct AssetResult<T: Component> {
pub struct AssetResult<T> {
Copy link
Member

Choose a reason for hiding this comment

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

This surprises me a bit: why were we able to remove this component bound / why did it exist in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure exactly. I was surprised about that bound as well, as it didn't make much sense to me. I think it bound should be more like T: Asset, but clearly that bound wasn't used either. Removing it seems to be just fine, so I went with that.

@@ -13,7 +14,7 @@ use std::{
};

/// A collection of labels
#[derive(Default, Reflect)]
#[derive(Component, Default, Reflect)]
#[reflect(Component)]
Copy link
Member

Choose a reason for hiding this comment

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

Is there any chance we can wrap this into the derive, probably in a follow-up PR? That would make working with reflection much less frustrating.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I believe this is one of the first things we might add. But I intentionally kept it as is to limit the scope of changes here. Details around automatic component reflection might need further discussion.

@Frizi
Copy link
Contributor Author

Frizi commented May 25, 2021

#1843 is tagged with needs-rfc. Are we planning on not going down the rfc route for this?

I believe there was a green light for this minimal implementation. Though it might have ben said only on discord. I'm explicilty avoiding adding any more features than necessary here, as those might indeed need further discussion and possible RFC. This PR doesn't close that linked issue.

Comment on lines 32 to 69
// For our own convinience, let's implement Component for primitives in tests.
// It will eventually be removed, once tests are not using them anymore.
// Consider those impls deprecated.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we create an issue for this (if/when) this PR gets merged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, it's a good follow-up once those changes are accepted. Unless we decide to get rid of that in this PR anyway :)

@@ -13,7 +14,7 @@ use thiserror::Error;
/// A component is data associated with an [`Entity`](crate::entity::Entity). Each entity can have
/// multiple different types of components, but only one of them per type.
///
/// Any type that is `Send + Sync + 'static` automatically implements `Component`.
/// Any type that is `Send + Sync + 'static` can implement `Component` using `#[derive(Component)]`.
Copy link
Member

Choose a reason for hiding this comment

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

I would like a comment here describing how to work around the orphan rules for new Rust users.

@alice-i-cecile
Copy link
Member

Another follow-up issue that should be created for discussion: require #[derive(Resource).

@cart
Copy link
Member

cart commented May 30, 2021

I believe there was a green light for this minimal implementation. Though it might have ben said only on discord. I'm explicilty avoiding adding any more features than necessary here, as those might indeed need further discussion and possible RFC. This PR doesn't close that linked issue.

Hmm I'm not remembering that (but my memory isn't perfect 😄). I am pretty sold on this, but my stance is still this: #1843 (comment).

Before merging I'd like an RFC with a clear plan for the future (I'm most interested in what the Component trait will look like and the derive macro interface), and some killer feature implemented to make this more palatable for users.

As it stands from a user perspective, this pr currently just (1) adds boilerplate and (2) solves a problem that, while common, probably isn't on most people's radar.

I think implementing static storage should be that killer feature, because it:

  1. Ideally makes normal for loop Query iteration faster (people respond very favorably to benchmark results). I think showing "optimization graphs" in our release notes is a big part of bevy's positive reception. I think a graph is the "spoon full of sugar" that will make the boilerplate "medicine" go down.
  2. Forces us to prove that one of our (or at least, my) biggest reasons for merging (better for loop perf) is grounded in reality
  3. Hopefully makes storage configuration simpler
  4. Might optimize other things because we no longer need to store intermediate storage info or do runtime storage type checks.

@Frizi
Copy link
Contributor Author

Frizi commented Jun 2, 2021

Before merging I'd like an RFC with a clear plan for the future (I'm most interested in what the Component trait will look like and the derive macro interface), and some killer feature implemented to make this more palatable for users.

Okay, I'm fine with that. The reason I wanted the minimal version in the first place is due to current efforts to bring relations into ECS. Implementing type-level storages will require deeper changes that would severely conflict with that branch. So, we have a choice to make here.

I can ignore that conflict and start the implementation anyway. This would allow us to measure performance gains quickly, but later will add nontrivial merging overhead. Alternatively, this can just wait until relations are merged in.

@alice-i-cecile
Copy link
Member

I would prefer to do this before relations; it may open up new architectural options for us to help resolve some of the outstanding concerns.

@Frizi Frizi force-pushed the derive-component branch from fbfd8be to 7d2348c Compare June 2, 2021 12:30
@cart
Copy link
Member

cart commented Jun 3, 2021

I can ignore that conflict and start the implementation anyway. This would allow us to measure performance gains quickly, but later will add nontrivial merging overhead. Alternatively, this can just wait until relations are merged in.

This is a tough call. I think this will be less controversial and easier to review/merge than relations. But no matter what we do somebody is gonna need to deal with a->b or b->a merge conflicts (likely either me, you, or @BoxyUwU).

I personally think moving each project forward independently and merging them when ready is the right call. Merge conflicts aren't fun, but I don't think they should be a show stopper here.

@Frizi Frizi force-pushed the derive-component branch from 7d2348c to c529535 Compare June 9, 2021 15:14
@Frizi Frizi force-pushed the derive-component branch from be1bc42 to 40711df Compare June 9, 2021 16:39
@Frizi
Copy link
Contributor Author

Frizi commented Jun 9, 2021

I've added very basic implementation for static storages. Now derive understands #[storage(sparse)] attribute (the syntax will probably change). That static storage data is used to transform all fn is_dense into associated const DENSE: bool. That way its evaluation is forced to be compile-time. That means iterators don't even store is_dense field, as the const is used directly instead. This allows the unused code path to be trivially eliminated.

Some benchmark numbers:

group                                     runtime is_dense (aafa863)  const IS_DENSE (40711df)
-----                                     ----------                 ------------
add_remove_component/bevy_sparse_set      1.04  1407.9±196.77µs      1.00  1348.9±84.43µs
add_remove_component/bevy_table           1.00  1699.0±57.21µs       1.00  1699.8±167.69µs
add_remove_component_big/bevy_sparse_set  1.00  1545.9±281.94µs      1.04  1611.1±193.76µs
add_remove_component_big/bevy_table       1.02      4.0±0.16ms       1.00      3.9±0.09ms
fragmented_iter/bevy                      1.14    450.5±9.97ns       1.00   394.6±31.22ns
fragmented_iter/bevy_foreach              1.00   310.0±20.17ns       1.02   315.9±24.39ns
get_component/bevy                        1.00  1495.5±106.58µs      1.00  1499.3±208.11µs
get_component/bevy_system                 1.00  1194.1±48.76µs       1.01  1211.0±85.75µs
heavy_compute/bevy                        1.06  1140.0±174.05µs      1.00  1070.5±22.27µs
schedule/bevy                             1.02     60.0±3.09µs       1.00     58.8±2.87µs
simple_insert/bevy                        1.03   647.8±56.01µs       1.00   627.0±10.97µs
simple_insert/bevy_unbatched              1.05  1602.2±160.03µs      1.00  1529.1±52.90µs
simple_iter/bevy                          1.28     21.5±0.74µs       1.00     16.9±0.77µs
simple_iter/bevy_foreach                  1.00     12.5±0.32µs       1.01     12.6±0.58µs
simple_iter/bevy_sparse                   1.00     63.4±7.15µs       1.00     63.6±3.73µs
simple_iter/bevy_sparse_foreach           1.18    61.2±10.73µs       1.00     51.8±2.07µs
sparse_fragmented_iter/bevy               1.13     15.1±0.28ns       1.00     13.4±0.48ns
sparse_fragmented_iter/bevy_foreach       1.13     12.8±0.42ns       1.00     11.3±1.42ns

This implementation is actually fairly straightforward and doesn't introduce any hard to resolve conflicts. In the future, we could do more in-depth refactoring to simplify the iterator implementations and deduplicate some code.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review labels Oct 2, 2021
@cart
Copy link
Member

cart commented Oct 3, 2021

bors r+

bors bot pushed a commit that referenced this pull request Oct 3, 2021
…#2254)

This implements the most minimal variant of #1843 - a derive for marker trait. This is a prerequisite to more complicated features like statically defined storage type or opt-out component reflection.

In order to make component struct's purpose explicit and avoid misuse, it must be annotated with `#[derive(Component)]` (manual impl is discouraged for compatibility). Right now this is just a marker trait, but in the future it might be expanded. Making this change early allows us to make further changes later without breaking backward compatibility for derive macro users.

This already prevents a lot of issues, like using bundles in `insert` calls. Primitive types are no longer valid components as well. This can be easily worked around by adding newtype wrappers and deriving `Component` for them.

One funny example of prevented bad code (from our own tests) is when an newtype struct or enum variant is used. Previously, it was possible to write `insert(Newtype)` instead of `insert(Newtype(value))`. That code compiled, because function pointers (in this case newtype struct constructor) implement `Send + Sync + 'static`, so we allowed them to be used as components. This is no longer the case and such invalid code will trigger a compile error.


Co-authored-by: = <=>
Co-authored-by: TheRawMeatball <[email protected]>
Co-authored-by: Carter Anderson <[email protected]>
@bors bors bot changed the title Implement and require #[derive(Component)] on all component structs [Merged by Bors] - Implement and require #[derive(Component)] on all component structs Oct 3, 2021
@bors bors bot closed this Oct 3, 2021
bors bot pushed a commit that referenced this pull request Oct 18, 2021
# Objective

- Bevy has several `compile_fail` test
- #2254 added `#[derive(Component)]`
- Those tests now fail for a different reason.
- This was not caught as these test still "successfully" failed to compile.

## Solution

- Add `#[derive(Component)]` to the doctest
- Also changed their cfg attribute from `doc` to `doctest`, so that these tests don't appear when running `cargo doc` with `--document-private-items`
bors bot pushed a commit that referenced this pull request Aug 8, 2022
*This PR description is an edited copy of #5007, written by @alice-i-cecile.*
# Objective
Follow-up to #2254. The `Resource` trait currently has a blanket implementation for all types that meet its bounds.

While ergonomic, this results in several drawbacks:

* it is possible to make confusing, silent mistakes such as inserting a function pointer (Foo) rather than a value (Foo::Bar) as a resource
* it is challenging to discover if a type is intended to be used as a resource
* we cannot later add customization options (see the [RFC](https://github.com/bevyengine/rfcs/blob/main/rfcs/27-derive-component.md) for the equivalent choice for Component).
* dependencies can use the same Rust type as a resource in invisibly conflicting ways
* raw Rust types used as resources cannot preserve privacy appropriately, as anyone able to access that type can read and write to internal values
* we cannot capture a definitive list of possible resources to display to users in an editor
## Notes to reviewers
 * Review this commit-by-commit; there's effectively no back-tracking and there's a lot of churn in some of these commits.
   *ira: My commits are not as well organized :')*
 * I've relaxed the bound on Local to Send + Sync + 'static: I don't think these concerns apply there, so this can keep things simple. Storing e.g. a u32 in a Local is fine, because there's a variable name attached explaining what it does.
 * I think this is a bad place for the Resource trait to live, but I've left it in place to make reviewing easier. IMO that's best tackled with #4981.

## Changelog
`Resource` is no longer automatically implemented for all matching types. Instead, use the new `#[derive(Resource)]` macro.

## Migration Guide
Add `#[derive(Resource)]` to all types you are using as a resource.

If you are using a third party type as a resource, wrap it in a tuple struct to bypass orphan rules. Consider deriving `Deref` and `DerefMut` to improve ergonomics.

`ClearColor` no longer implements `Component`. Using `ClearColor` as a component in 0.8 did nothing.
Use the `ClearColorConfig` in the `Camera3d` and `Camera2d` components instead.


Co-authored-by: Alice <[email protected]>
Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: devil-ira <[email protected]>
Co-authored-by: Carter Anderson <[email protected]>
maccesch pushed a commit to Synphonyte/bevy that referenced this pull request Sep 28, 2022
…yengine#5577)

*This PR description is an edited copy of bevyengine#5007, written by @alice-i-cecile.*
# Objective
Follow-up to bevyengine#2254. The `Resource` trait currently has a blanket implementation for all types that meet its bounds.

While ergonomic, this results in several drawbacks:

* it is possible to make confusing, silent mistakes such as inserting a function pointer (Foo) rather than a value (Foo::Bar) as a resource
* it is challenging to discover if a type is intended to be used as a resource
* we cannot later add customization options (see the [RFC](https://github.com/bevyengine/rfcs/blob/main/rfcs/27-derive-component.md) for the equivalent choice for Component).
* dependencies can use the same Rust type as a resource in invisibly conflicting ways
* raw Rust types used as resources cannot preserve privacy appropriately, as anyone able to access that type can read and write to internal values
* we cannot capture a definitive list of possible resources to display to users in an editor
## Notes to reviewers
 * Review this commit-by-commit; there's effectively no back-tracking and there's a lot of churn in some of these commits.
   *ira: My commits are not as well organized :')*
 * I've relaxed the bound on Local to Send + Sync + 'static: I don't think these concerns apply there, so this can keep things simple. Storing e.g. a u32 in a Local is fine, because there's a variable name attached explaining what it does.
 * I think this is a bad place for the Resource trait to live, but I've left it in place to make reviewing easier. IMO that's best tackled with bevyengine#4981.

## Changelog
`Resource` is no longer automatically implemented for all matching types. Instead, use the new `#[derive(Resource)]` macro.

## Migration Guide
Add `#[derive(Resource)]` to all types you are using as a resource.

If you are using a third party type as a resource, wrap it in a tuple struct to bypass orphan rules. Consider deriving `Deref` and `DerefMut` to improve ergonomics.

`ClearColor` no longer implements `Component`. Using `ClearColor` as a component in 0.8 did nothing.
Use the `ClearColorConfig` in the `Camera3d` and `Camera2d` components instead.


Co-authored-by: Alice <[email protected]>
Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: devil-ira <[email protected]>
Co-authored-by: Carter Anderson <[email protected]>
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
…yengine#5577)

*This PR description is an edited copy of bevyengine#5007, written by @alice-i-cecile.*
# Objective
Follow-up to bevyengine#2254. The `Resource` trait currently has a blanket implementation for all types that meet its bounds.

While ergonomic, this results in several drawbacks:

* it is possible to make confusing, silent mistakes such as inserting a function pointer (Foo) rather than a value (Foo::Bar) as a resource
* it is challenging to discover if a type is intended to be used as a resource
* we cannot later add customization options (see the [RFC](https://github.com/bevyengine/rfcs/blob/main/rfcs/27-derive-component.md) for the equivalent choice for Component).
* dependencies can use the same Rust type as a resource in invisibly conflicting ways
* raw Rust types used as resources cannot preserve privacy appropriately, as anyone able to access that type can read and write to internal values
* we cannot capture a definitive list of possible resources to display to users in an editor
## Notes to reviewers
 * Review this commit-by-commit; there's effectively no back-tracking and there's a lot of churn in some of these commits.
   *ira: My commits are not as well organized :')*
 * I've relaxed the bound on Local to Send + Sync + 'static: I don't think these concerns apply there, so this can keep things simple. Storing e.g. a u32 in a Local is fine, because there's a variable name attached explaining what it does.
 * I think this is a bad place for the Resource trait to live, but I've left it in place to make reviewing easier. IMO that's best tackled with bevyengine#4981.

## Changelog
`Resource` is no longer automatically implemented for all matching types. Instead, use the new `#[derive(Resource)]` macro.

## Migration Guide
Add `#[derive(Resource)]` to all types you are using as a resource.

If you are using a third party type as a resource, wrap it in a tuple struct to bypass orphan rules. Consider deriving `Deref` and `DerefMut` to improve ergonomics.

`ClearColor` no longer implements `Component`. Using `ClearColor` as a component in 0.8 did nothing.
Use the `ClearColorConfig` in the `Camera3d` and `Camera2d` components instead.


Co-authored-by: Alice <[email protected]>
Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: devil-ira <[email protected]>
Co-authored-by: Carter Anderson <[email protected]>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…yengine#5577)

*This PR description is an edited copy of bevyengine#5007, written by @alice-i-cecile.*
# Objective
Follow-up to bevyengine#2254. The `Resource` trait currently has a blanket implementation for all types that meet its bounds.

While ergonomic, this results in several drawbacks:

* it is possible to make confusing, silent mistakes such as inserting a function pointer (Foo) rather than a value (Foo::Bar) as a resource
* it is challenging to discover if a type is intended to be used as a resource
* we cannot later add customization options (see the [RFC](https://github.com/bevyengine/rfcs/blob/main/rfcs/27-derive-component.md) for the equivalent choice for Component).
* dependencies can use the same Rust type as a resource in invisibly conflicting ways
* raw Rust types used as resources cannot preserve privacy appropriately, as anyone able to access that type can read and write to internal values
* we cannot capture a definitive list of possible resources to display to users in an editor
## Notes to reviewers
 * Review this commit-by-commit; there's effectively no back-tracking and there's a lot of churn in some of these commits.
   *ira: My commits are not as well organized :')*
 * I've relaxed the bound on Local to Send + Sync + 'static: I don't think these concerns apply there, so this can keep things simple. Storing e.g. a u32 in a Local is fine, because there's a variable name attached explaining what it does.
 * I think this is a bad place for the Resource trait to live, but I've left it in place to make reviewing easier. IMO that's best tackled with bevyengine#4981.

## Changelog
`Resource` is no longer automatically implemented for all matching types. Instead, use the new `#[derive(Resource)]` macro.

## Migration Guide
Add `#[derive(Resource)]` to all types you are using as a resource.

If you are using a third party type as a resource, wrap it in a tuple struct to bypass orphan rules. Consider deriving `Deref` and `DerefMut` to improve ergonomics.

`ClearColor` no longer implements `Component`. Using `ClearColor` as a component in 0.8 did nothing.
Use the `ClearColorConfig` in the `Camera3d` and `Camera2d` components instead.


Co-authored-by: Alice <[email protected]>
Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: devil-ira <[email protected]>
Co-authored-by: Carter Anderson <[email protected]>
Subserial pushed a commit to Subserial/bevy_winit_hook that referenced this pull request Jan 24, 2024
*This PR description is an edited copy of #5007, written by @alice-i-cecile.*
# Objective
Follow-up to bevyengine/bevy#2254. The `Resource` trait currently has a blanket implementation for all types that meet its bounds.

While ergonomic, this results in several drawbacks:

* it is possible to make confusing, silent mistakes such as inserting a function pointer (Foo) rather than a value (Foo::Bar) as a resource
* it is challenging to discover if a type is intended to be used as a resource
* we cannot later add customization options (see the [RFC](https://github.com/bevyengine/rfcs/blob/main/rfcs/27-derive-component.md) for the equivalent choice for Component).
* dependencies can use the same Rust type as a resource in invisibly conflicting ways
* raw Rust types used as resources cannot preserve privacy appropriately, as anyone able to access that type can read and write to internal values
* we cannot capture a definitive list of possible resources to display to users in an editor
## Notes to reviewers
 * Review this commit-by-commit; there's effectively no back-tracking and there's a lot of churn in some of these commits.
   *ira: My commits are not as well organized :')*
 * I've relaxed the bound on Local to Send + Sync + 'static: I don't think these concerns apply there, so this can keep things simple. Storing e.g. a u32 in a Local is fine, because there's a variable name attached explaining what it does.
 * I think this is a bad place for the Resource trait to live, but I've left it in place to make reviewing easier. IMO that's best tackled with bevyengine/bevy#4981.

## Changelog
`Resource` is no longer automatically implemented for all matching types. Instead, use the new `#[derive(Resource)]` macro.

## Migration Guide
Add `#[derive(Resource)]` to all types you are using as a resource.

If you are using a third party type as a resource, wrap it in a tuple struct to bypass orphan rules. Consider deriving `Deref` and `DerefMut` to improve ergonomics.

`ClearColor` no longer implements `Component`. Using `ClearColor` as a component in 0.8 did nothing.
Use the `ClearColorConfig` in the `Camera3d` and `Camera2d` components instead.


Co-authored-by: Alice <[email protected]>
Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: devil-ira <[email protected]>
Co-authored-by: Carter Anderson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants