Add `ReflectFromWorld` and replace the `FromWorld` requirement on `ReflectComponent` and `ReflectBundle` with `FromReflect` #9623

SkiFire13 · 2023-08-29T12:43:23Z

Objective

FromType<T> for ReflectComponent and ReflectBundle currently require T: FromWorld for two reasons:
- they include a from_world method;
- they create dummy Ts using FromWorld and then apply a &dyn Reflect to it to simulate FromReflect.
However FromWorld/Default may be difficult/weird/impractical to implement, while FromReflect is easier and also more natural for the job.
See also https://discord.com/channels/691052431525675048/1146022009554337792

Solution

Split from_world from ReflectComponent and ReflectBundle into its own ReflectFromWorld struct.
Replace the requirement on FromWorld in ReflectComponent and ReflectBundle with FromReflect

Changelog

ReflectComponent and ReflectBundle no longer offer a from_world method.
ReflectComponent and ReflectBundle's FromType<T> implementation no longer requires T: FromWorld, but now requires FromReflect.
ReflectComponent::insert, ReflectComponent::apply_or_insert and ReflectComponent::copy now take an extra &TypeRegistry parameter.
There is now a new ReflectFromWorld struct.

Migration Guide

Existing uses of ReflectComponent::from_world and ReflectBundle::from_world will have to be changed to ReflectFromWorld::from_world.
Users of #[reflect(Component)] and #[reflect(Bundle)] will need to also implement/derive FromReflect.
Users of #[reflect(Component)] and #[reflect(Bundle)] may now want to also add FromWorld to the list of reflected traits in case their FromReflect implementation may fail.
Users of ReflectComponent will now need to pass a &TypeRegistry to its insert, apply_or_insert and copy methods.

SkiFire13 · 2023-08-29T14:03:01Z

Mhm that didn't happen when I ran cargo run -p ci on my Windows machine...

Anyway, look like the error occurs because a Component wants to be only partially deserialized (using #[reflect(skip_serializing)]) and then expects FromWorld to be used first when being deserialized. IMO this is not exactly straightforward to use, though look a valid usecase.

In the end the error is caused by bevy_scene calling ReflectComponent::apply_or_insert with a "partial" &dyn Reflect, while this PR expects a "complete" one, at least for types that don't implement Default.

One way to fix could be to fall back to using FromWorld (through ReflectFromWorld) if FromReflect fails. It adds yet another layer, but should have the wanted semantics. Ideally though apply_or_insert would only receive "full" &dyn Reflect and the additional initialization required could be performed by a different dedicated mechanism.

alice-i-cecile · 2023-08-29T15:25:46Z

crates/bevy_ecs/src/reflect/from_world.rs

+
+use crate::world::{FromWorld, World};
+
+/// A struct used to operate on reflected [`FromWorld`] of a type.


This line isn't clear to me. It feels like we're missing a noun after FromWorld.

I blindly copied the documentation for ReflectComponent and ReflectBundle. I guess their documentation should also be updated?

Yes please :)

Also FromWorld already feel kinda a noun, so I'm not so sure what to add after it. Any idea?

alice-i-cecile · 2023-08-29T15:26:52Z

I like not requiring Default (implicitly) on everything :) The general direction of this change makes a lot of sense to me now that we have FromReflect.

crates/bevy_ecs/src/reflect/component.rs

SkiFire13

I have rebased and fixed the merge conflicts. I'll leave here an auto-review to highlight some changes where some feedback would be welcome.

SkiFire13 · 2023-10-22T09:09:00Z

crates/bevy_ecs/src/reflect/component.rs

    /// Function pointer implementing [`ReflectComponent::insert()`].
-    pub insert: fn(&mut EntityWorldMut, &dyn Reflect),
+    pub insert: fn(&mut EntityWorldMut, &dyn Reflect, &TypeRegistry),


I have added a &TypeRegistry parameter to ReflectComponent's functions, mimicking the ones on ReflectBundle. Now the TypeRegistry in the World is no longer implicitly accessed.

SkiFire13 · 2023-10-22T09:10:52Z

crates/bevy_ecs/src/reflect/component.rs

+            // FIXME: once we have unique reflect, use `TypePath`.
+            std::any::type_name::<T>(),


Here I kinda copied what it's done in ReflectBundle. The alternative would be requiring TypePath in the bounds, but I guess that's probably better done in another PR.

SkiFire13 · 2023-10-22T09:16:22Z

crates/bevy_ecs/src/reflect/bundle.rs

+                if let Some(reflect_component) =
+                    registry.get_type_data::<ReflectComponent>(TypeId::of::<B>())
+                {
+                    reflect_component.apply(entity, reflected_bundle);
+                } else {


While reworking insert_field I realized it would make sense to check if the Bundle being inserted is a single Component, in which case it shouldn't insert each field as if it was itself a bundle/component (which would be wrong). I guess usually ReflectComponent should be checked before ReflectBundle, but if someone made a mistake we should catch it. This won't work though if someone only registers ReflectBundle and not ReflectComponent. It's also a technically a change in behaviour (though I would argue any case where a change is observable was likely behaving wrongly before).

SkiFire13 · 2023-12-14T20:02:07Z

Rebased and fixed merge conflicts.

… message

…raits

crates/bevy_ecs/src/reflect/from_world.rs

alice-i-cecile · 2024-01-18T20:46:19Z

crates/bevy_ecs/src/reflect/from_world.rs

@@ -0,0 +1,84 @@
+//! Definitions for [`FromWorld`] reflection.


This could use a sentence or two of docs about why this is useful.

Note that this is a private module. I tried to expand the documentation a bit anyway, though I'm not fully convinced. I also expanded a bit the documentation of the bundle and component modules to keep the same "structure".

Related: I just noticed that the component module has some kinda extensive documentation, but unfortunately it is not visible to users due to the module being private.

alice-i-cecile

I have nits about the documentation, but this change ultimately makes sense to me and seems both correct and necessary.

soqb

i strongly agree with the broad strokes of this change and the implementation is solid too.

i wonder if it would be possible in a followup to automatically register ReflectFromWorld when we write #[reflect(Default)].

SkiFire13 · 2024-01-19T08:27:51Z

i wonder if it would be possible in a followup to automatically register ReflectFromWorld when we write #[reflect(Default)].

It should be possible, though I wonder whether there is a more general way to do this that covers any Trait that has a blanket implementation impl<T: OtherTrait> Trait for T

soqb · 2024-01-20T08:39:05Z

It should be possible, though I wonder whether there is a more general way to do this that covers any Trait that has a blanket implementation impl<T: OtherTrait> Trait for T

probably not without linkme/inventory-like shenanigans, which has no wasm support.

edit: unless we made this part of the definition of #[reflect_trait], e.g. #[reflect_trait(impl FromWorld)] on Default trait.

…ource` for `State<S>` (#12136) # Objective - In #9623 I forgot to change the `FromWorld` requirement for `ReflectResource`, fix that; - Fix #12129 ## Solution - Use the same approach as in #9623 to try using `FromReflect` and falling back to the `ReflectFromWorld` contained in the `TypeRegistry` provided - Just reflect `Resource` on `State<S>` since now that's possible without introducing new bounds. --- ## Changelog - `ReflectResource`'s `FromType<T>` implementation no longer requires `T: FromWorld`, but instead now requires `FromReflect`. - `ReflectResource::insert`, `ReflectResource::apply_or_insert` and `ReflectResource::copy` now take an extra `&TypeRegistry` parameter. ## Migration Guide - Users of `#[reflect(Resource)]` will need to also implement/derive `FromReflect` (should already be the default). - Users of `#[reflect(Resource)]` may now want to also add `FromWorld` to the list of reflected traits in case their `FromReflect` implementation may fail. - Users of `ReflectResource` will now need to pass a `&TypeRegistry` to its `insert`, `apply_or_insert` and `copy` methods.

alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Aug 29, 2023

alice-i-cecile reviewed Aug 29, 2023

View reviewed changes

crates/bevy_ecs/src/reflect/component.rs Outdated Show resolved Hide resolved

nicopap reviewed Aug 29, 2023

View reviewed changes

crates/bevy_ecs/src/reflect/component.rs Outdated Show resolved Hide resolved

SkiFire13 mentioned this pull request Sep 3, 2023

Merge _Reflect and _WithRegistry commands #9680

Open

SkiFire13 force-pushed the fromworld-reflectcomponent branch from 12b486f to a1bed41 Compare October 22, 2023 09:05

SkiFire13 commented Oct 22, 2023

View reviewed changes

SkiFire13 force-pushed the fromworld-reflectcomponent branch from 8438cf3 to 43b8f6e Compare December 14, 2023 19:57

SkiFire13 requested review from nicopap and alice-i-cecile December 14, 2023 20:02

SkiFire13 added 11 commits January 16, 2024 21:25

Add ReflectFromWorld

0480568

Replace FromWorld requirement on ReflectBundle with FromReflect

face9a4

Replace FromWorld requirement on ReflectComponent with FromReflect

2f2f303

Fallback to FromWorld

5c11765

Avoid using FromReflect for the last step of from_reflect_or_world

eb11264

Add documentation to from_reflect_or_world

e88aaca

Forgot one use of from_reflect_or_world

125034c

ReflectBundle: delegate to ReflectComponent if possible and fix error…

9905af4

… message

ReflectComponent: add a TypeRegistry parameter for reflecting other t…

95579d0

…raits

Fix doc lint

29164e6

Some wording improvement

bd1dc46

SkiFire13 force-pushed the fromworld-reflectcomponent branch from 43b8f6e to bd1dc46 Compare January 16, 2024 20:28

alice-i-cecile reviewed Jan 18, 2024

View reviewed changes

crates/bevy_ecs/src/reflect/from_world.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Jan 18, 2024

View reviewed changes

alice-i-cecile approved these changes Jan 18, 2024

View reviewed changes

Improve documentation

eca893b

alice-i-cecile requested review from MrGVSV and soqb January 18, 2024 22:32

soqb approved these changes Jan 19, 2024

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 19, 2024

alice-i-cecile added this pull request to the merge queue Jan 19, 2024

Merged via the queue into bevyengine:main with commit eff96e2 Jan 19, 2024
23 checks passed

SkiFire13 deleted the fromworld-reflectcomponent branch January 19, 2024 18:53

SkiFire13 mentioned this pull request Feb 26, 2024

Replace FromWorld requirement on ReflectResource and reflect Resource for State<S> #12136

Merged

MrGVSV mentioned this pull request May 23, 2024

replace FromWorld with FromReflect in ReflectComponent, ReflectResource #6060

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ReflectFromWorld` and replace the `FromWorld` requirement on `ReflectComponent` and `ReflectBundle` with `FromReflect` #9623

Add `ReflectFromWorld` and replace the `FromWorld` requirement on `ReflectComponent` and `ReflectBundle` with `FromReflect` #9623

SkiFire13 commented Aug 29, 2023 •

edited

Loading

SkiFire13 commented Aug 29, 2023

alice-i-cecile Aug 29, 2023

SkiFire13 Aug 29, 2023

alice-i-cecile Aug 29, 2023

SkiFire13 Aug 29, 2023

alice-i-cecile commented Aug 29, 2023

SkiFire13 left a comment

SkiFire13 Oct 22, 2023

SkiFire13 Oct 22, 2023

SkiFire13 Oct 22, 2023

SkiFire13 commented Dec 14, 2023

alice-i-cecile Jan 18, 2024

SkiFire13 Jan 18, 2024

alice-i-cecile left a comment

soqb left a comment •

edited

Loading

SkiFire13 commented Jan 19, 2024

soqb commented Jan 20, 2024 •

edited

Loading


		use crate::world::{FromWorld, World};

		/// A struct used to operate on reflected [`FromWorld`] of a type.

		// FIXME: once we have unique reflect, use `TypePath`.
		std::any::type_name::<T>(),

		@@ -0,0 +1,84 @@
		//! Definitions for [`FromWorld`] reflection.

Add ReflectFromWorld and replace the FromWorld requirement on ReflectComponent and ReflectBundle with FromReflect #9623

Add ReflectFromWorld and replace the FromWorld requirement on ReflectComponent and ReflectBundle with FromReflect #9623

Conversation

SkiFire13 commented Aug 29, 2023 • edited Loading

Objective

Solution

Changelog

Migration Guide

SkiFire13 commented Aug 29, 2023

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

alice-i-cecile commented Aug 29, 2023

SkiFire13 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

SkiFire13 commented Dec 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

soqb left a comment • edited Loading

Choose a reason for hiding this comment

SkiFire13 commented Jan 19, 2024

soqb commented Jan 20, 2024 • edited Loading

Add `ReflectFromWorld` and replace the `FromWorld` requirement on `ReflectComponent` and `ReflectBundle` with `FromReflect` #9623

Add `ReflectFromWorld` and replace the `FromWorld` requirement on `ReflectComponent` and `ReflectBundle` with `FromReflect` #9623

SkiFire13 commented Aug 29, 2023 •

edited

Loading

soqb left a comment •

edited

Loading

soqb commented Jan 20, 2024 •

edited

Loading