-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
bevy_reflect: Nested TypeInfo
getters
#13321
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
Merged
alice-i-cecile
merged 4 commits into
bevyengine:main
from
MrGVSV:mrgvsv/reflect/nested-type-info
Jul 15, 2024
Merged
bevy_reflect: Nested TypeInfo
getters
#13321
alice-i-cecile
merged 4 commits into
bevyengine:main
from
MrGVSV:mrgvsv/reflect/nested-type-info
Jul 15, 2024
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mweatherley
approved these changes
May 20, 2024
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.
This seems like a good compromise to me, and the code seems well documented and tested.
11e0a42
to
f1e5a5a
Compare
f1e5a5a
to
b588f43
Compare
alice-i-cecile
approved these changes
Jul 14, 2024
@MrGVSV CI failed on merge; not sure why. I've updated the branch now, maybe you can figure it out from the logs? |
6dfbbb7
to
31f8a05
Compare
This was referenced Oct 29, 2024
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-Feature
A new feature, making something new possible
D-Modest
A "normal" level of difficulty; suitable for simple features or challenging fixes
S-Ready-For-Final-Review
This PR has been approved by the community. It's ready for a maintainer to consider merging it
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Right now,
TypeInfo
can be accessed directly from a type using eitherTyped::type_info
orReflect::get_represented_type_info
.However, once that
TypeInfo
is accessed, any nested types must be accessed via theTypeRegistry
.Solution
Enable nested types within a
TypeInfo
to be retrieved directly.The particular implementation was chosen for two reasons.
Firstly, we can't just store
TypeInfo
inside anotherTypeInfo
directly. This is because some types are recursive and would result in a deadlock when trying to create theTypeInfo
(i.e. it has to create theTypeInfo
before it can use it, but it also needs theTypeInfo
before it can create it). Therefore, we must instead store the function so it can be retrieved lazily.I had considered also using a
OnceLock
or something to lazily cache the info, but I figured we can look into optimizations later. The API should remain the same with or without theOnceLock
.Secondly, a new wrapper trait had to be introduced:
MaybeTyped
. LikeRegisterForReflection
, this trait is#[doc(hidden)]
and only exists so that we can properly handle dynamic type fields without requiring them to implementTyped
. We don't want dynamic types to implementTyped
due to the fact that it would make the return typeOption<&'static TypeInfo>
for all types even though only the dynamic types ever need to returnNone
(see #6971 for details).Users should never have to interact with this trait as it has a blanket impl for all
Typed
types. AndTyped
is automatically implemented when derivingReflect
(as it is required).The one downside is we do need to return
Option<&'static TypeInfo>
from all these new methods so that we can handle the dynamic cases. If we didn't have to, we'd be able to get rid of theOption
entirely. But I think that's an okay tradeoff for this one part of the API, and keeps the other APIs intact.Testing
This PR contains tests to verify everything works as expected. You can test locally by running:
Changelog
Public Changes
ArrayInfo::item_info
methodNamedField::type_info
methodUnnamedField::type_info
methodListInfo::item_info
methodMapInfo::key_info
methodMapInfo::value_info
methodTyped
bound (remember that this is automatically satisfied for all types that deriveReflect
)Internal Changes
MaybeTyped
traitMigration Guide
All active fields for reflected types (including lists, maps, tuples, etc.), must implement
Typed
. For the majority of users this won't have any visible impact.However, users implementing
Reflect
manually may need to update their types to implementTyped
if they weren't already.Additionally, custom dynamic types will need to implement the new hidden
MaybeTyped
trait.