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

Automatically register types for Reflect #3936

Open
alice-i-cecile opened this issue Feb 13, 2022 · 9 comments
Open

Automatically register types for Reflect #3936

alice-i-cecile opened this issue Feb 13, 2022 · 9 comments
Labels
A-ECS Entities, components, systems, and events A-Reflection Runtime information about types A-Scenes Serialized ECS data stored on the disk C-Enhancement A new feature C-Usability A simple quality-of-life change that makes Bevy easier to use

Comments

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Feb 13, 2022

What problem does this solve or what need does it fill?

As seen in the examples, each type that we wish to reflect must be manually registered.

This is a serious frustration when using type reflection, and severely limits the ability to implement richer features using it (like trait queries), as we have no guarantee that resources / components are reflectable.

What solution would you like?

  1. Expand the functionality of the Reflect trait to fully support all reasonable data types. Add reflection for enum types #1347 is particularly critical.
  2. Make Resource opt-in, using a derive macro.
  3. Change the bounds on the various non-send resource tools to Resource, and make Res<T>: Resource + Send + Sync.
  4. Remove the blanket impl on Resource and force users to use #[derive(Resource)].
  5. Whenever a schedule is initialized (see Order independence of App construction #1255 for related thoughts on how we need a distinct initialization step), register the relevant traits (such as Reflect) all of the types used in it to the App that contains it. See the prior art on trait queries for an idea of how this might be done.

Open questions

We may need to force an explicit Event trait / derive that handles reflection as well in order to ensure that Res<Events<T>> is reflectable.

What alternative(s) have you considered?

Reducing this boilerplate is quite essential: currently, using scenes is very tedious due to this error-prone boilerplate.

However, there are several reasonable changes to consider:

  1. Make Reflect a required subtrait of Component and Resource . This does not play nice when external types are embedded in components and resources.
  2. Do not automatically implement Reflect in the Component / Resource derive. This is technically clearer, but introduces advanced concepts right away and adds to boilerplate.
  3. Fully remove the existing manual registration API. This will probably still be needed for dynamic component types.
  4. Register the component / resource types the first time that a system that needs it is run. This doesn't work well with e.g. trait queries as we need a complete list at the start, and forces users to reason about whether or not all of the required types are registered. It also spreads work out in unpredictable ways, rather than increasing boot-up costs.

Additional context

This is an old idea, and certainly not entirely my own. It was previously discussed in #1843 / #2254 / bevyengine/rfcs#27.

This strategy is also useful for including other information in the appropriate traits, such as the size of assets / components / resources (seen in https://github.com/BGR360/bevy_datasize).

The same technology used above for trait registration would be very useful for #1515.

@alice-i-cecile alice-i-cecile added C-Enhancement A new feature C-Usability A simple quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types A-Scenes Serialized ECS data stored on the disk A-ECS Entities, components, systems, and events labels Feb 13, 2022
@TheRawMeatball
Copy link
Member

I'd be strongly opposed to requiring Reflect on Component and Resource personally. I'd prefer to leave it opt-in, with hooks added in a different way.

@bjorn3
Copy link
Contributor

bjorn3 commented Feb 13, 2022

Whenever a schedule is initialized (see #1255 for related thoughts on how we need a distinct initialization step), register all of the types used in it to the App that contains it.

That means you would now only sometimes need to manually register it, which I think is less clear. In addition because it is only rarely needed, it may be very confusing for the cases where it is necessary.

@cart
Copy link
Member

cart commented Feb 13, 2022

Just calling out that if we had specialization, it could be both optional and implicit:

pub trait RegisterReflect {
  fn register_reflect(registry: &mut Registry);
}

impl<T> RegisterReflect for T {
    default fn register_reflect(registry: &mut Registry) {
        // Do nothing
    }
}

#[derive(Reflect, Component)]
struct Foo {
}

// in derive(Reflect), generate this RegisterReflect impl for type 
impl RegisterReflect for Foo {
    fn register_reflect(registry: &mut Registry)  {
      registry.register::<Foo>()
    }
}

// Component traits could then do something with the reflect impl
trait Component: RegisterReflect {
  fn register_reflection(world: &mut World) {
    let mut registry = world.get_resource_mut::<Registry>();
    Self::register_reflect(&mut registry);
  }
}

@cart
Copy link
Member

cart commented Feb 13, 2022

I do think its worth entertaining the idea of marrying Component + Reflect. But I am also very wary of doing so. The pros have already been stated, so here are some cons:

  • Conflated functionality. Reflection would be fully abstracted out, making users think Component == reflect functionality.
  • Confusing: all fields in a Component struct would need a Reflect impl. Most Component fields probably shouldn't have a Component derive, so they will need manual Reflect impls. Reflect therefore isn't fully abstracted and will still be a part of common use cases. Not only that, but these manual reflect impls will still need to be manually registered because they dont have Component impls / aren't being used directly in a Bevy ECS context.
  • Potentially increased compile times due to all components needing Reflect. That being said, given that in practice most components will want a Reflect impl, this might be a bit of a non-issue.

trait queries

Not (yet) convinced that this is a pattern we should support. Very non-rustey (getting all instances of a trait isn't a rust pattern) and footgun-ey (because it requires that the user manually ties the trait info to the reflected type in the derive. forgetting to do so for every relevant component type breaks the integrity of the query). Worth considering because it would be powerful, but I wouldn't use that as justification for merging Component + Reflect until that conversation has come to a conclusion.

@alice-i-cecile
Copy link
Member Author

Ambee and Cart have convinced me that tying Component + Reflect so tightly is probably a mistake. In particular, I think that the conflation is pretty rough, and the pain around type reflection for external (but wrapped) types is rather high. If we can get it to work from a technical perspective, we should try to avoid doing so.

@pcone
Copy link
Contributor

pcone commented Sep 10, 2023

In case it's useful to anyone in the meantime I've written a macro (with the help of discord) that makes this a bit less arduous:

macro_rules! define_components {
    (@out_pass $plugin_name:ident
        $(
            $(#[$outer:meta])*
            $vis:vis struct $name:ident$structdef:tt$(;)?
        )*
    ) => {
            fn build(&self, app: &mut bevy::prelude::App) {
                app$(.register_type::<$name>())*;
            }
    };

    (Type registration plugin: $plugin_name:ident $($tokens:tt)*) => {
        $($tokens)*

        pub struct $plugin_name;

        impl Plugin for $plugin_name {
            define_components!(
                @out_pass $plugin_name $($tokens)*
            );
        }
    };
}

Used as follows:

define_components! {
    Type registration plugin: MyPluginName
    
    #[derive(Component, Default, Reflect)]
    #[reflect(Component)
    pub struct Position(pub IVec2);
    
    #[derive(Component, Default, Reflect)]
    #[reflect(Component)
    pub struct SomeComplexStruct {
        with: String,
        fields: u16,
    }
}

And then just app.add_plugins(MyPluginName) when setting up your app.

You need to add impl blocks outside of the macro, but someone with better macro skills might be able to modify the macro to allow those inside too.

@james7132
Copy link
Member

james7132 commented Mar 8, 2024

#12332 provides a (hacky) way to do this without forcing a subtrait, conditionally injecting code via the Component derive macro based on a Relfect derive macro attribute. It comes with some major caveats. Roughly similar to the way that cart's specialization proposal above works.

#4154 also helped by removing the need for dependent types to be registered as well, so just the top level types need to be registered.

Alternatively, have we entertained the idea of using inventory or something similar to handle automatic registration on non-WASM platforms? This wouldn't help plugin authors, but it would provide significant UX improvements to end users of the engine that aren't targeting the web.

@james7132
Copy link
Member

dtolnay/inventory#71 does seem to suggest there is a path forward for using inventory in wasm builds too.

@thebluefish
Copy link
Contributor

thebluefish commented Mar 8, 2024

Alternatively, have we entertained the idea of using inventory or something similar to handle automatic registration on non-WASM platforms? This wouldn't help plugin authors, but it would provide significant UX improvements to end users of the engine that aren't targeting the web.

I have attempted something like this based on linkme (but I suspect the same principle should work with inventory), which would be a viable approach for bevy if we specifically target non-WASM. While writing that crate, we discussed this potential approach for bevy somewhere in #reflection-dev and ultimately it was a no-go without WASM support.

I would be interested in forking this concept to a bevy-specific auto-registration PoC, that hopefully can support WASM once inventory does.

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 A-Scenes Serialized ECS data stored on the disk C-Enhancement A new feature C-Usability A simple quality-of-life change that makes Bevy easier to use
Projects
Status: Open
Development

No branches or pull requests

7 participants