Skip to content
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

Merged
merged 6 commits into from Feb 6, 2024
Merged

Conversation

coreh
Copy link
Contributor

@coreh coreh commented Feb 6, 2024

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

  • Added registration and ReflectDefault where applicable
  • (Drive by fix) Moved Option<f32> registration to bevy_core instead of bevy_ui, along with similar types.

Changelog

  • Fixed: Registered FogSettings, FogFalloff, ParallaxMappingMethod, OpaqueRendererMethod structs for reflection
  • Fixed: Registered ReflectDefault trait for ColorGrading and CascadeShadowConfig structs

@MrGVSV MrGVSV added C-Usability A simple quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Feb 6, 2024
@@ -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>>()
Copy link
Member

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.

Copy link
Member

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>>()
Copy link
Member

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

Copy link
Member

@james7132 james7132 left a 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.

@MrGVSV
Copy link
Member

MrGVSV commented Feb 6, 2024

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 Option considering how ubiquitous it is. Recursive registration should definitely help though.

@alice-i-cecile 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 Feb 6, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 6, 2024
Merged via the queue into bevyengine:main with commit e169b2b Feb 6, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Usability A simple quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

None yet

4 participants