Get Component's dyn Trait by ComponentId

[ycysdf]

Objective

• Get Component's dyn Trait by ComponentId

Solution

• Add TraitTypeData trait. This trait associates the trait object corresponding to TypeData

Changelog

• Add TypeDataMapper trait. It map a type to a TraitTypeData. This trait is implemented automatically by #[reflect_trait]. It map dyn MyTrait maps to ReflectMyTrait.

Migration Guide

• Methods get ,get_mut,get_boxed move onto the trait, Need to import the use bevy: : reflect: : TraitTypeData ;

[alice-i-cecile]

I like having these methods stored separately, but I don't really see the point of the trait extension method rather than just another impl block.

[ycysdf]

Well, I referred to ReflectCommandExt, and these methods depend on AppTypeRegistry, so I also defined a WorldExt, which I'm not sure is better

[alice-i-cecile]

This should be a get_resource, with a ? operator again. No need to introduce panics.

[MrGVSV]

I'm not a huge fan of creating a new trait, but I understand why it might be useful.

However, if we decide to add this, I feel like the name should reflect the specific kind of type data we're dealing with here, which are reflected traits. (The other kind would be custom type data, which may represent traits but do not convert a dyn Reflect into a dyn Trait.)

Perhaps, we rename this to TraitTypeData? Or maybe even just TraitObject?

[nicopap]

Ping @JoJoJet because they made bevy-trait-query and they probably know a lot of useful things about this.

[nicopap]

I'd prefer defer on JoJoJet for whether this should be added or not.

[nicopap]

This breaks when the TypeData doesn't have an associated trait. For example in https://lib.rs/crates/cuicui_reflect_query, Queryable is not a trait. It would stop compiling, and I don't think it's an isolated use-case

[ycysdf]

TypeData can choose not to implement this trait.

[MrGVSV]

This is why I think renaming this trait would help clarify its intent

[alice-i-cecile]

I cleaned up all the clearly resolved conversations for you :) @ycysdf, feel free to do that yourself in the future on your Bevy PRs as you submit fixes: it really helps reviewers understand what's outstanding.

[ycysdf]

@alice-i-cecile thank you

[JoJoJet]

I am mostly unfamiliar with the bevy's reflection system, since I used my own registry in order to implement bevy-trait-query, which let me optimize single-item trait queries to be nearly as fast as concrete type queries. One benefit of the approach used in this PR is that is plays more nicely with reflection, since trait impls will be registered along with the type -- compare this to bevy-trait-query, where each trait impl currently needs to be registered separately, even when the type itself has already been registered. However, I don't know what the performance implications of doing it this way are -- since this PR does not attempt to implement queries, I take it that performance is not a priority here.

One syntactic concern I have is the use of auto-generated reflect types for traits. If I have

#[reflect_trait]
trait MyTrait {}

I would get a trait object by calling

world.get_dyn_mut_by_id::<ReflectMyTrait>(entity, component_id);

I believe it would be more intuitive if this instead looked like

world.get_dyn_mut_by_id::<dyn MyTrait>(entity, component_id);

Would this change be doable?

[ycysdf]

I am mostly unfamiliar with the bevy's reflection system, since I used my own registry in order to implement bevy-trait-query, which let me optimize single-item trait queries to be nearly as fast as concrete type queries. One benefit of the approach used in this PR is that is plays more nicely with reflection, since trait impls will be registered along with the type -- compare this to bevy-trait-query, where each trait impl currently needs to be registered separately, even when the type itself has already been registered. However, I don't know what the performance implications of doing it this way are -- since this PR does not attempt to implement queries, I take it that performance is not a priority here.

One syntactic concern I have is the use of auto-generated reflect types for traits. If I have

#[reflect_trait]
trait MyTrait {}

I would get a trait object by calling

world.get_dyn_mut_by_id::<ReflectMyTrait>(entity, component_id);

I believe it would be more intuitive if this instead looked like

world.get_dyn_mut_by_id::<dyn MyTrait>(entity, component_id);

Would this change be doable?

Good Idea ! I tried to implement it, but the solution needed to add a new Trait 'TraitTypeDataRelevance'

[ycysdf]

Now can get the dyn Trait from both

world.get_dyn_mut_by_id::<ReflectMyTrait>(entity, component_id);

and

world.get_dyn_mut_by_id::<dyn MyTrait>(entity, component_id);

[ycysdf]

@JoJoJet
I'm a bit of a newbie to Bevy

I think the trait query of 'bevy-trait query' is very good

[MrGVSV]

This probably needs more documentation considering manual implementors may want to make use of it.

[MrGVSV]

Bikeshed: I think a name like AsoociatedTypeData, TypeDataAssoc, TypeDataMapper, etc. would be more indicative of how this is meant to be used.

Additionally, I think we can leave out the Trait part of the name since this doesn't necessarily need to be used for dyn Trait. For example, a user could associate a Player struct type with a ReflectPlayerSettings or something.

[ycysdf]

I rename to TyepDataMapper and update TypeDataMapper::TypeData bound to TypeData. I feel better about it.

[MrGVSV]

Is it possible to add the ?Sized bound as a supertrait of TraitTypeDataRelevance? Or does that not work/isn't desirable?

[JoJoJet]

Doesn't work unfortunately. The implicit Sized bound comes from the type parameter T, not from the trait. All traits can have ?Sized implementors, unless you explicitly add a Sized bound on the trait definition.