feat: Add World::get_reflect() and World::get_reflect_mut()

[futile]

Objective

Sometimes one wants to retrieve a &dyn Reflect for an entity's component, which so far required multiple, non-obvious steps and unsafe-code.
The docs for MutUntyped contain an example of the unsafe part.

Solution

This PR adds the two methods:

// immutable variant
World::get_reflect(&self, entity: Entity, type_id: TypeId) -> Result<&dyn Reflect, GetComponentReflectError>

// mutable variant
World::get_reflect_mut(&mut self, entity: Entity, type_id: TypeId) -> Result<Mut<'_, dyn Reflect>, GetComponentReflectError>

which take care of the necessary steps, check required invariants etc., and contain the unsafety so the caller doesn't have to deal with it.

Testing

• Did you test these changes? If so, how?
  • Added tests and a doc test, also (successfully) ran cargo run -p ci.
• Are there any parts that need more testing?
  • Could add tests for each individual error variant, but it's not required imo.
• How can other people (reviewers) test your changes? Is there anything specific they need to know?
  • Run cargo test --doc --package bevy_ecs --all-features -- world::World::get_reflect --show-output for the doctest
  • Run cargo test --package bevy_ecs --lib --all-features -- world::tests::reflect_tests --show-output for the unittests
• If relevant, what platforms did you test these changes on, and are there any important ones you can't test?
  • Don't think it's relevant, but tested on 64bit linux (only).

---

Showcase

Copy of the doctest example which gives a good overview of what this enables:

use bevy_ecs::prelude::*;
use bevy_reflect::Reflect;
use std::any::TypeId;

// define a `Component` and derive `Reflect` for it
#[derive(Component, Reflect)]
struct MyComponent;

// create a `World` for this example
let mut world = World::new();

// Note: This is usually handled by `App::register_type()`, but this example can not use `App`.
world.init_resource::<AppTypeRegistry>();
world.get_resource_mut::<AppTypeRegistry>().unwrap().write().register::<MyComponent>();

// spawn an entity with a `MyComponent`
let entity = world.spawn(MyComponent).id();

// retrieve a reflected reference to the entity's `MyComponent`
let comp_reflected: &dyn Reflect = world.get_reflect(entity, TypeId::of::<MyComponent>()).unwrap();

// make sure we got the expected type
assert!(comp_reflected.is::<MyComponent>());

Migration Guide

No breaking changes, but users can use the new methods if they did it manually before.

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[futile]

Open question: I think these might also make sense for EntityRef (and the mutable variant of that), and someone on Discord also mentioned it. Should I implement it, and if so, in this PR or a follow-up one?

[futile]

Question: Would it make sense to add something similar for Resources as well? Only thinking about it cause they can also be Reflect. But I don't really have an idea here, just wondering if then maybe the functions should not be called just get_reflect{_mut}(), but maybe get_component_reflect{_mut}()? But the other methods on World also don't use an infix for Components, just for Resources, so it would probably be get_resource_reflect{_mut}() then.

[futile]

Also, just realized that relying on AppTypeRegistry is maybe not ideal.. But a World doesn't have an own TypeRegistry I think, so that's what I used.

A possible alternative would be to extend the arguments of World::get_reflect{_mut} to also take a &TypeRegistry argument. This would not create a coupling between World and AppTypeRegistry, but would add some overhead for the caller. In practice, that might be ok though. It would also remove one of the error cases, which is "no AppTypeRegistry-Resource in the world".

[soqb]

generally very happy with this succinct and useful PR. it definitely could be merged as it is, i've just left a few comments/questions!

i reviewed this on my phone so the comment quality might not be great.

regardless, the bulk of the changes might be better off in their own file under the bevy_ecs::reflect module since the file there currently in is already exorbitantly long!

[soqb]

i would definitely move this assertion to above the unsafe block. it's a precondition for e unsafe code, which should be as minimal as possible.

[futile]

I actually thought of that and tried it out, but I didn't like it, because it felt like it "decouples" the assertion too much from the unsafe block. I.e., copying/moving the unsafe block should also copy/move the assertion with it, which automatically happens if the assertion is inside the unsafe block, but can easily be forgotten if it's outside.

That said, I can also just go with the common style in bevy, and if that is to keep assertions outside, I'll just change it.

[futile]

Since the assertion is actually required for soundness of the unsafe-block, I'd prefer to keep it inside, also for the reasons from my previous comment. Would that be ok?

[alice-i-cecile]

@futile ping me when you've addressed review comments (or if you're happy enough with the PR for now) and I'll give this the second review you need :)

[futile]

generally very happy with this succinct and useful PR. it definitely could be merged as it is, i've just left a few comments/questions!

i reviewed this on my phone so the comment quality might not be great.

regardless, the bulk of the changes might be better off in their own file under the bevy_ecs::reflect module since the file there currently in is already exorbitantly long!

Thank you for the review! :) You mean like in a trait, e.g., WorldReflectExt or similar, which "adds" the methods to World? I also thought of that, can definitely do that 👍 As long as it doesn't use unsafe/internal APIs of World I'd think that is fine, because otherwise it might easily be missed when changing some other code..

[soqb]

it's even simpler than using an extension trait. since it's in the same crate you can write an inherent impl (impl World {}) from a module like bevy_ecs::reflect::world_ext.

[MrGVSV]

Awesome! I think this will be a very useful API since people tend to ask how they can convert TypeId to a dyn Reflect component reference.

I agree with @soqb though, it will probably be cleaner to move these methods to a separate module.

[MrGVSV]

You should be able to link to the method like:

    /// [`App::register_type`]: crate::app::App::register_type

Note that I believe we tend to leave off the () when referencing functions/methods.

Same for the MissingReflectFromPtrTypeData variant.

[futile]

Removed the () (everywhere I could find).

So, about the linking for App::register_type.. I started out with something like crate::app::App::register_type, but that doesn't work:

warning: unresolved link to `crate::app::App::register_type`
  --> crates/bevy_ecs/src/world/error.rs:29:33
   |
29 |     /// [`App::register_type`]: crate::app::App::register_type
   |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no item named `app` in module `bevy_ecs`

I think the problem is that bevy_ecs does not depend on bevy_app (but the other way around), so rustdoc has no chance to find App. That's why I went with the manual file-path link.

That said, I just tried out crate::app::App::register_type again, because I really would like a better solution for this, but it doesn't work, with the error given above.

Is it possible to declare like a "doc-dependency" on bevy_app, without introducing a cycle? 😅 I think not, but I feel like it could work in theory, but probably not in rustdoc/cargo doc for now.

Edit: Found an upstream issue for this: rust-lang/rust#74481

[MrGVSV]

Oh that makes sense. In that case I might suggest linking to TypeRegistry::register directly or just not making App::register_type a link at all. I don't know I we ever link to crate dependents with a relative rustdoc path (I also don't know if that works for docs.rs docs).

@alice-i-cecile any thoughts here?

[futile]

I guess we can check what's gonna happen on docs.rs? Shouldn't break anything anyways.

[futile]

@MrGVSV thank you for your review as well! :)

I think I should have addressed all of it, and also moved the new methods + error type to a new module world::reflect (also thanks to @soqb). I didn't add world::reflect::world_ext, because world::reflect was already a new, empty module, and I thought it would be fine to keep the nesting level more shallow until it's required.

I left most of the comments I took care of with a "done"-comment or similar instead of closing them, because I feel like (only) the author of a comment can decide whether it's actually done :) Is there a common practice for this in the bevy repo that I should follow?

Oh yeah, open question from before: Is it fine to depend on AppTypeRegistry here, or should the functions instead take a &TypeRegistry parameter or similar? 🤔

[alice-i-cecile]

Is there a common practice for this in the bevy repo that I should follow?

Not particularly. For trivial / boring fixes (typos, code style), I tend to resolve it as an author or third-party reviewer. For particularly complex stuff, or where there's disagreement, I leave it open until the OP comes by. For particularly interesting threads, I leave it open even after it's fully resolved to help teach folks.

[futile]

Not particularly. For trivial / boring fixes (typos, code style), I tend to resolve it as an author or third-party reviewer. For particularly complex stuff, or where there's disagreement, I leave it open until the OP comes by. For particularly interesting threads, I leave it open even after it's fully resolved to help teach folks.

Ah ok, yeah, makes sense. In that case I went ahead and resolved a few more issues, where the solution should be pretty obvious & expected. Thanks! :)

[MrGVSV]

Just a couple cleanup suggestions. Everything else looks good!

[MrGVSV]

I didn't add world::reflect::world_ext, because world::reflect was already a new, empty module, and I thought it would be fine to keep the nesting level more shallow until it's required.

Makes sense to me!

Oh yeah, open question from before: Is it fine to depend on AppTypeRegistry here, or should the functions instead take a &TypeRegistry parameter or similar? 🤔

Let's stick to using AppTypeRegistry for now. It's cleaner and easier to use. If we need to add a parameter for the registry we can always do so in a later PR. And that could even be a non-breaking change if we did it like ReflectCommandExt.

[futile]

Just a couple cleanup suggestions. Everything else looks good!

Got it, fixed them. Nice catches, ty!

Let's stick to using AppTypeRegistry for now. It's cleaner and easier to use. If we need to add a parameter for the registry we can always do so in a later PR. And that could even be a non-breaking change if we did it like ReflectCommandExt.

Sounds good to me as well 👍

[alice-i-cecile]

Great docs, great error handling, useful bit of sugar. Nice work y'all :D