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

tim-blackbird · 2022-08-04T23:33:03Z

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 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 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.

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.

alice-i-cecile · 2022-08-05T00:36:15Z

Thank you very much for picking this up :) Shouldn't be much left to do.

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

nicopap

LGTM, just a few tinny questions.

crates/bevy_app/src/app.rs

crates/bevy_core_pipeline/src/clear_color.rs

crates/bevy_ecs/src/component.rs

crates/bevy_ecs/src/schedule/executor_parallel.rs

crates/bevy_ecs/src/system/function_system.rs

crates/bevy_log/Cargo.toml

crates/bevy_pbr/src/material.rs

crates/bevy_render/src/render_asset.rs

crates/bevy_winit/src/web_resize.rs

cart

Looks good to me!

cart · 2022-08-08T20:54:57Z

crates/bevy_app/src/app.rs

+/// The [`Resource`] that stores the [`App`]'s [`TypeRegistry`](bevy_reflect::TypeRegistry).
+#[cfg(feature = "bevy_reflect")]
+#[derive(Resource, Clone, Deref, DerefMut, Default)]
+pub struct AppTypeRegistry(pub bevy_reflect::TypeRegistryArc);


Not an issue to be solved in this pr, but given that this is the only TypeRegistry instance 99.99% of Bevy users will interact with, I think it might be worth trying to sort out a non-confusing way to give it the TypeRegistry name directly. Not sure the best way to do that. Just something to think about.

crates/bevy_render/src/camera/camera.rs

cart · 2022-08-08T21:36:17Z

bors r+

@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>

bors · 2022-08-08T21:58:13Z

Pull request successfully merged into main.

Build succeeded:

@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 added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR labels Aug 5, 2022

alice-i-cecile mentioned this pull request Aug 5, 2022

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

Closed

alice-i-cecile and others added 14 commits August 5, 2022 11:12

Remove blanket impl of Resource

aa9f717

Add docs for Resource trait

1f67ac9

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

3fb7fc8

Add Resource trait to prelude

667bbcd

Weaken Local trait bound to Send + Sync + 'static

032fd34

Derive resource everywhere

e1d68ba

Fix SchedulingEvents test

536766e

Manually implement Resource for TypeRegistryArc

3b22fc1

More trivial derives

e825571

Revert safety comment on Local

db81a3d

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

AAAAAA

cebe61b

Add docs.

068a256

wasm

3bcfbb2

Add AppTypeRegistry Resource to bevy_app

c999eea

tim-blackbird force-pushed the derive-resource branch from 07c9760 to c999eea Compare August 5, 2022 09:17

tim-blackbird added 7 commits August 5, 2022 11:38

missing comma

779a0e0

Add AppTypeRegistry to prelude.

6cf8264

Update gampad doctest.

41b7361

docs

737a288

Address Nilirad's comments.

e1abe83

Remove Component derive from ClearColor.

255e8d5

more docs

cc47e7b

tim-blackbird marked this pull request as ready for review August 5, 2022 14:14

alice-i-cecile approved these changes Aug 6, 2022

View reviewed changes

nicopap approved these changes Aug 6, 2022

View reviewed changes

Address comments.

310e46c

tim-blackbird force-pushed the derive-resource branch from c520811 to 310e46c Compare August 6, 2022 17:18

Well, that explains that.

ef43c68

alice-i-cecile mentioned this pull request Aug 8, 2022

PartialEq and Eq based change detection #5618

Open

Make RenderAssets hashmap field private

952a29f

cart reviewed Aug 8, 2022

View reviewed changes

bors bot changed the title ~~Make Resource trait opt-in, requiring #[derive(Resource)] V2~~ [Merged by Bors] - Make Resource trait opt-in, requiring #[derive(Resource)] V2 Aug 8, 2022

bors bot closed this Aug 8, 2022

Nilirad mentioned this pull request Aug 10, 2022

Explicitely derive resources Nilirad/bevy_prototype_lyon#173

Merged

bayou-brogrammer mentioned this pull request Oct 14, 2022

Bevy 0.9 amethyst/bracket-lib#308

Open

4 tasks

tim-blackbird deleted the derive-resource branch October 21, 2022 15:10

rparrett added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Oct 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tim-blackbird commented Aug 4, 2022 •

edited

Loading

alice-i-cecile commented Aug 5, 2022

nicopap left a comment

cart left a comment

cart Aug 8, 2022

cart commented Aug 8, 2022

bors bot commented Aug 8, 2022

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

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

Conversation

tim-blackbird commented Aug 4, 2022 • edited Loading

Objective

Notes to reviewers

Changelog

Migration Guide

alice-i-cecile commented Aug 5, 2022

nicopap left a comment

Choose a reason for hiding this comment

cart left a comment

Choose a reason for hiding this comment

cart Aug 8, 2022

Choose a reason for hiding this comment

cart commented Aug 8, 2022

bors bot commented Aug 8, 2022

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

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

tim-blackbird commented Aug 4, 2022 •

edited

Loading