Make `Resource` trait opt-in, requiring `#[derive(Resource)]` #5007

alice-i-cecile · 2022-06-13T21:57:19Z

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 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.
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 Add more granular features to bevy_ecs #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.

alice-i-cecile · 2022-06-14T00:44:56Z

@bevyengine/rendering-team the only remaining types to be updated are a bunch of reimported WGPU types / type aliased types (check CI for a list of failures). Would you like me to newtype those?

superdump · 2022-06-14T06:15:57Z

@bevyengine/rendering-team the only remaining types to be updated are a bunch of reimported WGPU types / type aliased types (check CI for a list of failures). Would you like me to newtype those?

Ohno. We have to newtype a bunch of stuff? Sadness. :/ I guess if we have to then sure.

DJMcNab

I can't remember what my first comments were.

But I don't think I found anything blocking.

DJMcNab · 2022-06-14T07:21:10Z

crates/bevy_ecs/examples/change_detection.rs

@@ -40,7 +40,7 @@ fn main() {
 }

 // This struct will be used as a Resource keeping track of the total amount of spawned entities


Since it has a derive Resource, this first sentence is superfluous

However I'm happy leaving this, for a doc consistency pass at some point in the future.

Yep; IMO this will be easier to do in a follow-up PR, where we can search our dev docs site for "resource" and make a call.

DJMcNab · 2022-06-14T07:23:05Z

crates/bevy_ecs/macros/src/lib.rs

@@ -480,6 +480,11 @@ pub(crate) fn bevy_ecs_path() -> syn::Path {
    BevyManifest::default().get_path("bevy_ecs")
 }

+#[proc_macro_derive(Resource)]
+pub fn derive_resource(input: TokenStream) -> TokenStream {
+    component::derive_resource(input)


Should this live in the component module?

I guess it's fine.

My argument here is that it should change in concert with the derive Component macro, so should live together.

I was thinking about renaming the module, but struggled to find a clear enough blanket name.

I think both components and resources can be viewed as storage.

DJMcNab · 2022-06-14T07:25:11Z

crates/bevy_ecs/src/reflect.rs

@@ -154,3 +156,6 @@ impl<C: Component + MapEntities> FromType<C> for ReflectMapEntities {
        }
    }
 }
+
+// We cannot implement this in bevy_reflect, or we would create a cyclic dependendency
+impl Resource for TypeRegistryArc {}


I'm not very happy with this.

I'd rather we created a wrapper resource.

Yep, I'm on board with this. This was the simple non-breaking solution so I wanted reviewer feedback.

DJMcNab · 2022-06-14T07:29:17Z

crates/bevy_ecs/src/system/system_param.rs

@@ -639,12 +647,12 @@ impl<'w, 's> SystemParamFetch<'w, 's> for WorldState {
 /// // .add_system(reset_to_system(my_config))
 /// # assert_is_system(reset_to_system(Config(10)));
 /// ```
-pub struct Local<'a, T: Resource>(&'a mut T);
+pub struct Local<'a, T: Send + Sync + 'static>(&'a mut T);


I don't think this Sync bound should be needed, although if removing it doesn't work that's also fine.

(I wonder if there's an abstraction to turn !Sync into Sync by enforcing access through a unique reference)

I agree here. Started with the trivial conversion though :)

(I wonder if there's an abstraction to turn !Sync into Sync by enforcing access through a unique reference)

There's sync_wrapper for exactly this purpose. A similar thing is proposed in the standard library.

crates/bevy_ecs/src/system/system_param.rs

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

Nilirad · 2022-06-15T09:55:45Z

crates/bevy_ecs/src/system/system_param.rs

+/// A type that can be inserted into a [`World`](crate::world::World) as a singleton.
+///
+/// Resources are commonly used to store global collections (like assets or events),
+/// or unique global information (such as the current level or state of the app).
+///
+/// You can access resource data in systems using the [`Res`] and [`ResMut`] system parameters.
+///
+/// Only one resource of each type can exist at any given time.
+/// Inserting a duplicate resource will overwrite the existing resource.


The second paragraph seems too specific to me: mentioning use cases and applications is something more suitable to the book imo. API docs are best suited to describe the item itself.

This description also does not illustrate how to insert a resource to the world (i.e. World::init_resource or World::insert_resource). I would avoid mentioning the App methods though, as they are not defined from the bevy_ecs perspective.

Nilirad · 2022-06-15T10:48:56Z

crates/bevy_ecs/src/system/system_param.rs

+/// A type that can be inserted into a [`World`](crate::world::World) as a singleton.
+///
+/// Resources are commonly used to store global collections (like assets or events),
+/// or unique global information (such as the current level or state of the app).
+///
+/// You can access resource data in systems using the [`Res`] and [`ResMut`] system parameters.
+///
+/// Only one resource of each type can exist at any given time.
+/// Inserting a duplicate resource will overwrite the existing resource.


Inserting a duplicate resource will overwrite the existing resource.

This is something that is up to the insert_resource method and not to the type that implements Resource. Also, if the concept of “inserting” is broadened to init_resource, that statement becomes incorrect.

tim-blackbird · 2022-08-04T19:00:23Z

I'm working on this :)

alice-i-cecile · 2022-08-05T04:49:58Z

Closing in favor of #5577.

@alice-i-cecile

*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 <alice.i.cecile@gmail.com> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: devil-ira <justthecooldude@gmail.com> Co-authored-by: Carter Anderson <mcanders1@gmail.com>

@alice-i-cecile

…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 <alice.i.cecile@gmail.com> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: devil-ira <justthecooldude@gmail.com> Co-authored-by: Carter Anderson <mcanders1@gmail.com>

@alice-i-cecile

…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 <alice.i.cecile@gmail.com> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: devil-ira <justthecooldude@gmail.com> Co-authored-by: Carter Anderson <mcanders1@gmail.com>

@alice-i-cecile

…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 <alice.i.cecile@gmail.com> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: devil-ira <justthecooldude@gmail.com> Co-authored-by: Carter Anderson <mcanders1@gmail.com>

alice-i-cecile marked this pull request as draft June 13, 2022 21:57

alice-i-cecile force-pushed the derive-resource branch from 690c1b5 to cc0b179 Compare June 13, 2022 22:04

alice-i-cecile added 5 commits June 13, 2022 18:38

Remove blanket impl of Resource

d1235fc

Add docs for Resource trait

8fa3677

Add derive macro, copying directly from derive(Component) macro

4f03af0

Add Resource trait to prelude

b168fe8

Weaken Local trait bound to Send + Sync + 'static

eb506fa

Derive resource everywhere

24d7c09

alice-i-cecile force-pushed the derive-resource branch from 1ec9b0c to 24d7c09 Compare June 14, 2022 00:01

alice-i-cecile added 3 commits June 13, 2022 20:13

Fix SchedulingEvents test

cbab3c1

Manually implement Resource for TypeRegistryArc

4f74c94

More trivial derives

b314e0a

alice-i-cecile added this to the Bevy 0.8 milestone Jun 14, 2022

DJMcNab approved these changes Jun 14, 2022

View reviewed changes

Revert safety comment on Local

c232410

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

Nilirad suggested changes Jun 15, 2022

View reviewed changes

alice-i-cecile removed this from the Bevy 0.8 milestone Jun 26, 2022

alice-i-cecile mentioned this pull request Jul 11, 2022

white outlines around sprite #5266

Closed

alice-i-cecile mentioned this pull request Jul 19, 2022

Add set_if_neq method to DetectChanges trait #5373

Closed

tim-blackbird mentioned this pull request Aug 4, 2022

[Merged by Bors] - Make Resource trait opt-in, requiring #[derive(Resource)] V2 #5577

Closed

alice-i-cecile closed this Aug 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `Resource` trait opt-in, requiring `#[derive(Resource)]` #5007

Make `Resource` trait opt-in, requiring `#[derive(Resource)]` #5007

alice-i-cecile commented Jun 13, 2022 •

edited

Loading

alice-i-cecile commented Jun 14, 2022 •

edited

Loading

superdump commented Jun 14, 2022

DJMcNab left a comment

DJMcNab Jun 14, 2022

alice-i-cecile Jun 14, 2022

DJMcNab Jun 14, 2022

alice-i-cecile Jun 14, 2022

Nilirad Jun 15, 2022

DJMcNab Jun 14, 2022

alice-i-cecile Jun 14, 2022

DJMcNab Jun 14, 2022

alice-i-cecile Jun 14, 2022

infmagic2047 Jun 15, 2022

Nilirad Jun 15, 2022 •

edited

Loading

Nilirad Jun 15, 2022

tim-blackbird commented Aug 4, 2022

alice-i-cecile commented Aug 5, 2022

		@@ -40,7 +40,7 @@ fn main() {
		}

		// This struct will be used as a Resource keeping track of the total amount of spawned entities

Make Resource trait opt-in, requiring #[derive(Resource)] #5007

Make Resource trait opt-in, requiring #[derive(Resource)] #5007

Conversation

alice-i-cecile commented Jun 13, 2022 • edited Loading

Objective

Notes to reviewers

Changelog

Migration Guide

alice-i-cecile commented Jun 14, 2022 • edited Loading

superdump commented Jun 14, 2022

DJMcNab 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

Nilirad Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tim-blackbird commented Aug 4, 2022

alice-i-cecile commented Aug 5, 2022

Make `Resource` trait opt-in, requiring `#[derive(Resource)]` #5007

Make `Resource` trait opt-in, requiring `#[derive(Resource)]` #5007

alice-i-cecile commented Jun 13, 2022 •

edited

Loading

alice-i-cecile commented Jun 14, 2022 •

edited

Loading

Nilirad Jun 15, 2022 •

edited

Loading