-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
ReflectComponent Commands
#5541
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
ReflectComponent Commands
#5541
Conversation
implement `insert_command` for `ReflectComponent`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks fine to me. The one thing I'm hesitant with is that this removes the ability for components to initialize from the world, right? I haven't played around too much with scenes, but is this something that's used often? How much are we losing by removing that ability?
|
I haven't looked at this implementation yet, and I'm not sure if this was considered, but I've used two different strategies that worked for inserting dynamic components using commands. Using
|
|
Probably superseded by #8895 should this be closed? |
|
Sounds good to me! I forgot this even existed. |
(Note: Primary changes are in bevy_ecs/src/reflect.rs, almost every other file just has
FromReflectderives added)Objective
Currently,
ReflectComponent(andReflectResource) contain a series of functions that require a&mut Worldin order to use them. This severely affects the parallelism of using these reflection tools and limits user's interest in them. It also leads to more convoluted code when having to request a lot of resources for a system that could otherwise take advantage of theSystemParamsystem. This PR is an attempt to improve this API by adding functions toReflectComponentandReflectResourcethat takeCommandsinstead of&mut World.Solution
The most significant change that needs to happen to support this API update is that instead of implementing
FromType<C>forReflectComponent/ResourceforC: Component + Reflect + FromWorld, we implement forC: Component + Reflect + FromReflect. In some ways this might be considered more permissive, but in others more restrictive. Some types may be constructible withFromWorldbut notFromReflect, and vice versa. I believe that overall this change is more permissive than not, especially as types that cannot be constructed from a reflected value are more likely to behave unintuitively in reflection-heavy systems. A more impactful issue with this is that now many types will need#[derive(FromReflect)]added in order to work withReflectComponent/Resource, which may be addressed in a different PR (opt-outFromReflect?).Changelog
Added:
insert_commandfunction toReflectComponentFromReflectderives to roughly 50-100 bevy types which used#[reflect(Component)]Changed:
FromWorldbound onReflectComponent'sFromTypeimpl toFromReflectMigration Guide
#[reflect(Component)]or#[reflect(Resource)]does not have#[derive(FromReflect)], it needs that added. A small amount of complex types may not be capable of this, and will need to be changed.