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

Add Reflect derive to Events and contained types #13149

Merged
merged 3 commits into from
May 1, 2024
Merged

Conversation

alice-i-cecile
Copy link
Member

Objective

The Events containerr should be reflectable, in order to make dev tools that examine its state more useful.

Fixes #13148.

Solution

  • Add a Reflect derive to Events, gated behind the bevy_reflect feature
  • Add Reflect to the contained types to make everything compile.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Apr 30, 2024
Copy link
Contributor

@ItsDoot ItsDoot left a comment

Choose a reason for hiding this comment

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

Looks good for the most part.

@@ -34,6 +36,7 @@ pub trait Event: Send + Sync + 'static {}
/// sent to the point it was processed. `EventId`s increase montonically by send order.
///
/// [`World`]: crate::world::World
#[cfg_attr(feature = "bevy_reflect", derive(Reflect))]
pub struct EventId<E: Event> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a ReflectValue instead? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. It's definitely value-like. I can make that change if others think it's useful.

Suggested change
pub struct EventId<E: Event> {
#[cfg_attr(feature = "bevy_reflect", reflect_value]
pub struct EventId<E: Event> {

Copy link
Member

@MrGVSV MrGVSV May 1, 2024

Choose a reason for hiding this comment

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

I think it depends. Do we ever want to access the id? If so, then we should probably leave it as is.

Although, I believe you will need to ignore the PhantomData.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, you'll want to access the ID. I'll ignore the PhantomData.

@ramon-bernardo
Copy link

ramon-bernardo commented Apr 30, 2024

@alice-i-cecile thanks, its works!

use bevy::prelude::*;

#[derive(Component, Reflect, Default)]
struct Position;

#[derive(Event, Reflect, Default)]
struct MoveEvent;

#[derive(Resource, Reflect, Default)]
struct Houses;

fn main() {
    App::new()
        // Position
        .register_type::<Position>()
        // MoveEvent
        .add_event::<MoveEvent>()
        .register_type::<MoveEvent>()
        // Houses
        .init_resource::<Houses>()
        .register_type::<Houses>()
        // Systems
        .add_systems(Startup, startup)
        .run();
}

fn startup(registry: Res<AppTypeRegistry>) {
    let registry = registry.read();
    for i in registry.iter() {
        println!("{:?}", i);
    }
}

output:

TypeRegistration { type_info: Struct(StructInfo { type_path: TypePathVtable { type_path: "bevy::Houses", short_type_path: "Houses", type_ident: Some("Houses"), crate_name: Some("bevy"), module_path: Some("bevy") }, type_id: TypeId { t: 262497429019092159181105962761250037597 }, fields: [], field_names: [], field_indices: {} }) }
TypeRegistration { type_info: Struct(StructInfo { type_path: TypePathVtable { type_path: "bevy::Position", short_type_path: "Position", type_ident: Some("Position"), crate_name: Some("bevy"), module_path: Some("bevy") }, type_id: TypeId { t: 79962707724654762771215367196927044087 }, fields: [], field_names: [], field_indices: {} }) }
TypeRegistration { type_info: Struct(StructInfo { type_path: TypePathVtable { type_path: "bevy::MoveEvent", short_type_path: "MoveEvent", type_ident: Some("MoveEvent"), crate_name: Some("bevy"), module_path: Some("bevy") }, type_id: TypeId { t: 175823575746073673475871982974340081787 }, fields: [], field_names: [], field_indices: {} }) }

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 good! I think the one thing we might want to consider adding is a method on App to more easily register these events and their resources into the type registry.

@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 May 1, 2024
@alice-i-cecile
Copy link
Member Author

Looks good! I think the one thing we might want to consider adding is a method on App to more easily register these events and their resources into the type registry.

Agreed. Ideally we'd register the type during add_event, but without specialization that would require a Reflect (or equivalent) bound on Event. Ultimately this feels like a question for #13111.

@alice-i-cecile alice-i-cecile requested a review from MrGVSV May 1, 2024 15:40
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 1, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 1, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 1, 2024
Merged via the queue into main with commit b3ed0dd May 1, 2024
27 checks passed
@alice-i-cecile alice-i-cecile deleted the reflect-events branch May 1, 2024 21:49
Comment on lines +179 to 182
#[cfg_attr(feature = "bevy_reflect", derive(Reflect))]
pub struct Events<E: Event> {
/// Holds the oldest still active events.
/// Note that `a.start_event_count + a.len()` should always be equal to `events_b.start_event_count`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it intended to expose the events_a/events_b split of the Events resource and the internal EventSequence type?

Copy link
Member Author

Choose a reason for hiding this comment

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

This seemed like the least intrusive solution at the time. This doesn't seem like a particularly dangerous implementation detail to provide backdoor access to, while ensuring that inspectors still work easily with events.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events A-Reflection Runtime information about types C-Usability A simple quality-of-life change that makes Bevy easier to use 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.

Implement Reflect for Events type
5 participants