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

Move AppTypeRegistry to bevy_ecs #8901

Merged
merged 2 commits into from
Jun 21, 2023

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Jun 20, 2023

Objective

A lot of the API on Reflect depends on a registry. When it comes to the ECS. We should use AppTypeRegistry in the general case.

This is however impossible in bevy_ecs, since AppTypeRegistry is defined in bevy_app.

Solution

  • Move AppTypeRegistry resource definition from bevy_app to bevy_ecs
  • Still add the resource in the App plugin, since bevy_ecs itself doesn't know of plugins

Note that bevy_ecs is a dependency of bevy_app, so nothing revolutionary happens.

Alternative

  • Define the API as a trait in bevy_app over bevy_ecs. (though this prevents us from using bevy_ecs internals)
  • Do not rely on AppTypeRegistry for the API in question, requring users to extract themselves the resource and pass it to the API methods.

Changelog

  • Moved AppTypeRegistry resource definition from bevy_app to bevy_ecs

Migration Guide

  • If you were not using a prelude::* to import AppTypeRegistry, you should update your imports:
- use bevy::app::AppTypeRegistry;
+ use bevy::ecs::reflect::AppTypeRegistry

This allows using it in `bevy_ecs`'s `reflect` module.
@nicopap nicopap added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types A-App Bevy apps and plugins labels Jun 20, 2023
@alice-i-cecile
Copy link
Member

TypeRegistryArc is a Arc<RwLock>, which means even mutable

This sentence seems incomplete :)

@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jun 20, 2023
Copy link
Member

@alice-i-cecile alice-i-cecile left a 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 :)

@nicopap
Copy link
Contributor Author

nicopap commented Jun 20, 2023

TypeRegistryArc is a Arc, which means even mutable

This sentence seems incomplete :)

Ooops. I realized it should implement DerefMut in the middle of making that sentence and went back to add the impl. But forgot to remove the sentence.

/// A [`Resource`] storing [`TypeRegistry`](bevy_reflect::TypeRegistry) for
/// type registrations relevant to a whole app.
#[derive(Resource, Clone, Default)]
pub struct AppTypeRegistry(pub TypeRegistryArc);
Copy link
Member

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.

Copy link
Contributor Author

@nicopap nicopap Jun 20, 2023

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.

Copy link
Member

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 {
Copy link
Member

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();
}

Copy link
Contributor Author

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.

Copy link
Member

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);
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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 :)

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 20, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 21, 2023
Merged via the queue into bevyengine:main with commit 0294bb1 Jun 21, 2023
@nicopap nicopap deleted the ecs-app-registry branch August 30, 2023 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-App Bevy apps and plugins A-ECS Entities, components, systems, and events A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants