-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Move AppTypeRegistry to bevy_ecs #8901
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
Conversation
This allows using it in `bevy_ecs`'s `reflect` module.
This sentence seems incomplete :) |
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.
I don't love the name still, but this is a very simple migration :)
Ooops. I realized it should implement |
| /// A [`Resource`] storing [`TypeRegistry`](bevy_reflect::TypeRegistry) for | ||
| /// type registrations relevant to a whole app. | ||
| #[derive(Resource, Clone, Default)] | ||
| pub struct AppTypeRegistry(pub TypeRegistryArc); |
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.
We could consider calling this WorldTypeRegistry since we're moving it to ECS and it should exist in the world. But we can also hold off on that for a future PR.
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.
I don't want to include it in this PR. Changing the name is way more breaking than changing the crate it's defined in, as people will usually import it through prelude::*. Basically most users wouldn't need to change a line with this. While renaming it would require a bit more work. I think it's the kind of change that deserves a bit more scrutiny, even if I'm for it.
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.
Agreed!
| #[derive(Resource, Clone, Default)] | ||
| pub struct AppTypeRegistry(pub TypeRegistryArc); | ||
|
|
||
| impl Deref for AppTypeRegistry { |
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.
QQ: These won't mess with cloning right? Does this still work?
fn system(registry: Res<AppTypeRegistry>) {
let registry2: AppTypeRegistry = registry.into_inner().clone();
}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.
It was implementing Deref and DerefMut through the bevy_derive::Deref{,Mut} macro before. So it should reproduce the same behavior. The macro basically writes this code.
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.
Ah I missed that part. Awesome, sounds good!
| /// The [`Resource`] that stores the [`App`]'s [`TypeRegistry`](bevy_reflect::TypeRegistry). | ||
| #[cfg(feature = "bevy_reflect")] | ||
| #[derive(Resource, Clone, bevy_derive::Deref, bevy_derive::DerefMut, Default)] | ||
| pub struct AppTypeRegistry(pub bevy_reflect::TypeRegistryArc); |
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.
Should we default a world to include this resource (under bevy_reflect feature)? Or is it still the responsibility of the user/bevy_app to initialize it?
I'm guessing the latter, but just want to clarify.
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.
That's an interesting proposition. I like the idea. Though I'm curious of the implications with regard to Scenes for example.
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.
We should avoid automatically inserting resources whenever possible :)
Objective
AppTypeRegistryon API defined inbevy_ecs(implement insert and remove reflected entity commands #8895 (comment))A lot of the API on
Reflectdepends on a registry. When it comes to the ECS. We should useAppTypeRegistryin the general case.This is however impossible in
bevy_ecs, sinceAppTypeRegistryis defined inbevy_app.Solution
AppTypeRegistryresource definition frombevy_apptobevy_ecsAppplugin, since bevy_ecs itself doesn't know of pluginsNote that
bevy_ecsis a dependency ofbevy_app, so nothing revolutionary happens.Alternative
bevy_appoverbevy_ecs. (though this prevents us from using bevy_ecs internals)AppTypeRegistryfor the API in question, requring users to extract themselves the resource and pass it to the API methods.Changelog
AppTypeRegistryresource definition frombevy_apptobevy_ecsMigration Guide
prelude::*to importAppTypeRegistry, you should update your imports: