-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Missing registrations #11736
Missing registrations #11736
Conversation
@@ -57,7 +57,9 @@ fn register_rust_types(app: &mut App) { | |||
.register_type::<HashSet<String>>() | |||
.register_type::<Option<String>>() | |||
.register_type::<Option<bool>>() | |||
.register_type::<Option<f32>>() |
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.
Non-blocking: While we're here, we could also consider registering the ReflectSerialize
and ReflectDeserialize
for all of these Option
types.
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.
Also, it seems like the GetTypeRegistration
impl for Option<T>
doesn't register ReflectDefault
when it definitely could. If you want, you could add that as well. But definitely not necessary for the sake of this PR.
@@ -57,7 +57,9 @@ fn register_rust_types(app: &mut App) { | |||
.register_type::<HashSet<String>>() | |||
.register_type::<Option<String>>() | |||
.register_type::<Option<bool>>() | |||
.register_type::<Option<f32>>() |
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.
Non-blocking: Should we also register other Option
primitives? Like Option<usize>
, etc.?
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.
Curious if we should default to automatically registering the Option<T>
for all T: Reflect
that are already registered.
I wonder if this would be avoidable via #4154 so that we avoid needing to manually registering these individually.
Yeah I think it'd be fine to do this for |
Objective
During my exploratory work on the remote editor, I found a couple of types that were either not registered, or that were missing
ReflectDefault
.Solution
ReflectDefault
where applicableOption<f32>
registration tobevy_core
instead ofbevy_ui
, along with similar types.Changelog
FogSettings
,FogFalloff
,ParallaxMappingMethod
,OpaqueRendererMethod
structs for reflectionReflectDefault
trait forColorGrading
andCascadeShadowConfig
structs