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

Reflect now requires DynamicTypePath. Remove Reflect::get_type_path() #8764

Merged
merged 2 commits into from
Jun 6, 2023

Conversation

cart
Copy link
Member

@cart cart commented Jun 5, 2023

Followup to #7184

This makes Reflect: DynamicTypePath which allows us to remove Reflect::get_type_path, reducing unnecessary codegen and simplifying Reflect implementations.

@cart cart added C-Code-Quality A section of code that is hard to understand or change A-Reflection Runtime information about types labels Jun 5, 2023
@cart cart added this to the 0.11 milestone Jun 5, 2023
@cart
Copy link
Member Author

cart commented Jun 5, 2023

It would be good to hear from @soqb on this just to make sure this doesn't break any scenarios they had in mind.
(although I can't personally imagine what it would break as these seem functionally equivalent)

Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the addition of #6971, it may also be worth considering how dynamic types should behave. Should they always return their own data or the data they represent? This is the reason we have Reflect::get_represented_type_info return an Option<&'static TypeInfo>.

My current take is that we should always return the values for the dynamic type itself and not what it represents. We already have TypeInfo for that purpose (we may even consider storing this information on TypeInfo as well). Additionally, DynamicTypePath wouldn't even be able to handle that anyways since it's a trait.

But anyway, I think this makes sense and makes using these methods a little simpler.

@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 Jun 6, 2023
Copy link
Contributor

@soqb soqb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this certainly looks like a straightforward improvement and i agree with @MrGVSV that the implementation should be valid for the dynamic types themselves and not what they represent.

it's left me a little confused as to why we need Reflect::as_any but not Reflect::get_type_path, though.

edit: now wondering if we wouldn't just be able to impose a Reflect: TypePath bound. because it would violate object safety!

@cart cart added this pull request to the merge queue Jun 6, 2023
Merged via the queue into bevyengine:main with commit 8b9d88f Jun 6, 2023
20 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-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants