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 auto registration #15030

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

Conversation

eugineerd
Copy link
Contributor

@eugineerd eugineerd commented Sep 3, 2024

Objective

This PR adds automatic registration of non-generic types annotated with #[derive(Reflect)].
Manually registering all reflect types can be error-prone, failing to reflect during runtime if some type is not registered. #5781 fixed this for nested types, improving ergonomics of using reflection dramatically, but top-level types still need to be registered manually. This is especially painful when prototyping and using inspector to change values of components.

// from #5781
#[derive(Reflect)]
struct Foo(Bar);

#[derive(Reflect)]
struct Bar(Baz);

#[derive(Reflect)]
struct Baz(usize);

fn main() {
  // ...
  app.register_type::<Foo>() // <- can still forget to add this
  // ...
}

Solution

Automatically register all types that derive Reflect. This works for all non-generic types, allowing users to just add #[derive(Reflect)] to any compatible type and not think about where to register it.

fn main() {
  // No need to manually call .register_type::<Foo>()
  App::new()
    .add_plugins(DefaultPlugins)
    .add_systems(Startup, setup)
    .run();
}

#[derive(Reflect)]
pub struct Foo {
  a: usize,
}

fn setup(type_registry: Res<AppTypeRegistry>) {
  let type_registry = type_registry.read();
  assert!(type_registry.contains(TypeId::of::<Foo>()));
}

Automatic registration can be opted-out of by adding #[reflect(no_auto_register)] reflect attribute to a type:

#[derive(Reflect)]
#[reflect(no_auto_register)]
pub struct Foo {
  a: usize,
}

Registration normally happens at app creation time, but TypeRegistry itself has a new register_derived_types method to register all collected types at any appropriate time:

#[derive(Reflect)]
struct Foo {
  name: Option<String>,
  value: i32
}

let mut type_registry = TypeRegistry::empty();
type_registry.register_derived_types();

assert!(type_registry.contains(TypeId::of::<Foo>()));

Testing

  • Tested by running reflect example and removing explicit type registration, both on native and on wasm32-unknown-unknown.
  • Added a doctest to register_derived_types

Impact on startup time is negligible: <40ms on debug wasm build and <25ms on debug native build for 1614 registered types .

Wasm binary size comparison. All sizes are in KB as reported by du on the modified reflection example.

auto register, KiB no auto register, KiB abs diff, KiB % diff
wasm-release && wasm-bindgen 27000 24224 2776 11.46%
wasm-opt -Oz 16376 15340 1036 6.75%
wasm-opt -Os 17728 16424 1304 7.94%
wasm-opt -Oz && gzip -c 5620 5364 256 4.77%

Considerations

While automatic type registration improves ergonomics quite a bit for projects that make heavy use of it, there are also some problems:

  • Generic types can't be automatically registered. However, as long as top-level reflect types don't need generics, recursive registration can take care of the rest.
  • Wasm bundle size increase is not insignificant. This is mostly due to registering various engine types that weren't registered before. Size overhead of automatic registration of types instead of manual is relatively small, as can be see from this (somewhat outdated but still relevant) table (see last 2 rows):
1000 simple structs with reflect wasm size comparison, no engine types
auto register, KiB no auto register, KiB abs diff, KiB % diff
wasm-release && wasm-bindgen 27944 21644 6300 22.55%
wasm-release && wasm-bindgen +register_type 28084 27764 320 1.14%
wasm-opt -Oz 15612 13928 1684 10.79%
wasm-opt -Oz +register_type 15676 15580 96 0.61%
wasm-release && wasm-bindgen (manual vs auto) 27944 27764 180 0.64%
wasm-opt -Oz (manual vs auto) 15612 15580 32 0.2%

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types X-Controversial There is active debate or serious implications around merging this PR M-Needs-Release-Note Work that should be called out in the blog due to impact labels Sep 3, 2024
@alice-i-cecile alice-i-cecile requested a review from cart September 3, 2024 15:47
@alice-i-cecile alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Sep 3, 2024
@alice-i-cecile
Copy link
Member

Wasm bundle size increase is not insignificant

Is this compared to the equivalent manual registration? Can / should we feature flag this for authors who particularly care?

Copy link
Contributor

github-actions bot commented Sep 3, 2024

You added a new feature but didn't update the readme. Please run cargo run -p build-templated-pages -- update features to update it, and commit the file change.

@eugineerd
Copy link
Contributor Author

Is this compared to the equivalent manual registration

As far as I understand it, wasm-init works by exporting a bunch of symbols to final wasm and calling them from outside when wasm_init() is called for the first time. I think most of the size increase is due to these exports, but I'm not sure. In the table I just built the modified registration example and compared the .wasm file sizes using du. As the number of registered types from type_registry.iter() differs (445 without, 614 with), it is not the same as manually registering types.

@eugineerd
Copy link
Contributor Author

eugineerd commented Sep 4, 2024

Managed to reduce wasm size considerably by removing closures from automatic type registration:

All sizes are in KB as reported by du on reflection example.

wasm-release && wasm-bindgen wasm-opt -Oz wasm-opt -Os wasm-opt -Oz && gzip -c
+reflect_auto_register 22560 14332 15352 5044
default 21716 13968 14896 4944
diff +3.7% +2.5% +3% +2%

I'm not very familiar with all the internal reflection machinery, maybe this can be optimized even further?

Update: Disregard that, I misunderstood how reflection works and missed registering type data.

@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 31, 2024
@BenjaminBrienen BenjaminBrienen added S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Dec 1, 2024
Copy link
Contributor

@BenjaminBrienen BenjaminBrienen left a comment

Choose a reason for hiding this comment

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

I like this! I like that you updated examples, but can you also add some unit tests?

@eugineerd
Copy link
Contributor Author

eugineerd commented Dec 1, 2024

Added a basic test for no_auto_register, there is also a doctest that should cover most of auto register functionality.

but can you also add some unit tests?

What other tests do you have in mind? I'm not really sure what else should be tested here, so If you have any ideas I'd love to hear them.

Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

I'll do a more thorough review tonight, but at first glance I believe this is compatible with my no_std work for non-wasm platforms. On wasm, I will need to replace that RwLock with something else (probably spin::RwLock). However, no action needs to be taken here from a no_std perspective, I'll update my PR if this gets merged first, or provide a change-request in the reverse.

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.

Just a few comments. Overall looks really clean!

crates/bevy_app/src/app.rs Show resolved Hide resolved
crates/bevy_ecs/src/reflect/mod.rs Outdated Show resolved Hide resolved
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.

Looks great! I think this is a good compromise between fully automatic registration and fully manual registration. It's completely opt-in via a feature flag and even opt-in on a per-registry basis.

I do think we should recommend libraries—including Bevy—manually register their types whenever possible to make it easier for downstream users to opt-out of this behavior.

But overall, this is going to make things a lot cleaner and easier for our users!

crates/bevy_reflect/derive/src/container_attributes.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/derive/src/impls/common.rs Outdated Show resolved Hide resolved
@MrGVSV MrGVSV added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 1, 2024
eugineerd and others added 2 commits December 1, 2024 22:57
Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
@alice-i-cecile alice-i-cecile added S-Needs-SME Decision or review from an SME is required and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Dec 2, 2024
Copy link
Contributor

github-actions bot commented Dec 8, 2024

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

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 targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Release-Note Work that should be called out in the blog due to impact S-Needs-SME Decision or review from an SME is required X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants