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

bevy_reflect: Nested TypeInfo getters #13321

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented May 10, 2024

Objective

Right now, TypeInfo can be accessed directly from a type using either Typed::type_info or Reflect::get_represented_type_info.

However, once that TypeInfo is accessed, any nested types must be accessed via the TypeRegistry.

#[derive(Reflect)]
struct Foo {
  bar: usize
}

let registry = TypeRegistry::default();

let TypeInfo::Struct(type_info) = Foo::type_info() else {
  panic!("expected struct info");
};

let field = type_info.field("bar").unwrap();

let field_info = registry.get_type_info(field.type_id()).unwrap();
assert!(field_info.is::<usize>());;

Solution

Enable nested types within a TypeInfo to be retrieved directly.

#[derive(Reflect)]
struct Foo {
  bar: usize
}

let TypeInfo::Struct(type_info) = Foo::type_info() else {
  panic!("expected struct info");
};

let field = type_info.field("bar").unwrap();

let field_info = field.type_info().unwrap();
assert!(field_info.is::<usize>());;

The particular implementation was chosen for two reasons.

Firstly, we can't just store TypeInfo inside another TypeInfo directly. This is because some types are recursive and would result in a deadlock when trying to create the TypeInfo (i.e. it has to create the TypeInfo before it can use it, but it also needs the TypeInfo 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 the OnceLock.

Secondly, a new wrapper trait had to be introduced: MaybeTyped. Like RegisterForReflection, this trait is #[doc(hidden)] and only exists so that we can properly handle dynamic type fields without requiring them to implement Typed. We don't want dynamic types to implement Typed due to the fact that it would make the return type Option<&'static TypeInfo> for all types even though only the dynamic types ever need to return None (see #6971 for details).

Users should never have to interact with this trait as it has a blanket impl for all Typed types. And Typed is automatically implemented when deriving Reflect (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 the Option 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:

cargo test --package bevy_reflect

Changelog

Public Changes

  • Added ArrayInfo::item_info method
  • Added NamedField::type_info method
  • Added UnnamedField::type_info method
  • Added ListInfo::item_info method
  • Added MapInfo::key_info method
  • Added MapInfo::value_info method
  • All active fields now have a Typed bound (remember that this is automatically satisfied for all types that derive Reflect)

Internal Changes

  • Added MaybeTyped trait

Migration 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 implement Typed if they weren't already.

Additionally, custom dynamic types will need to implement the new hidden MaybeTyped trait.

@MrGVSV MrGVSV added C-Enhancement A new feature A-Reflection Runtime information about types D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels May 10, 2024
Copy link
Contributor

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

@MrGVSV MrGVSV force-pushed the mrgvsv/reflect/nested-type-info branch from 11e0a42 to f1e5a5a Compare May 22, 2024 21:10
@MrGVSV MrGVSV added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label May 22, 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-Enhancement A new feature D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

None yet

2 participants