-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
FromType
specialization
#6055
FromType
specialization
#6055
Conversation
The specific issue of |
fn from_type() -> Self; | ||
} | ||
|
||
pub trait SpecializedFromType<T>: FromType<T> { |
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.
Could you add docs explaining what this struct is and how to use it? Including documentation on the method as well.
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.
Done.
crates/bevy_reflect/src/type_data.rs
Outdated
fn specialized_from_type() -> Self; | ||
} | ||
|
||
pub struct FromTypeCollector<S, T: FromType<S>>( |
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.
Same here, some documentation since it's public.
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 moved this inside the macro's GetTypeRegistration
implementation.
crates/bevy_reflect/src/type_data.rs
Outdated
} | ||
} | ||
|
||
pub trait Collect<T> { |
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.
Also documentation here (and on method)
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.
Likewise
/// | ||
/// This is used by the `#[derive(Reflect)]` macro to generate an implementation | ||
/// of [`TypeData`] to pass to [`TypeRegistration::insert`]. | ||
pub trait FromType<T> { |
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 think it would also be worthwhile to reference the other struct somewhere in the documentation here, briefly indicating how they can be used together.
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.
are you thinking something like "to specialize an existing blanket implementation, see SpecializedFromType
"?
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.
Exactly 🙂
crates/bevy_reflect/src/lib.rs
Outdated
}; | ||
|
||
assert_eq!( | ||
*type_registry |
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.
Is the dereference here (and assertion below) necessary?
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.
Nope, removed it and made the right side a reference.
dcb0752
to
968cba8
Compare
if implemented, `SpecializedFromType<T>` will be used instead of `FromType<T>` in the context of the `GetTypeRegistration` macro implementation. see the new test in `crates/bevy_reflect/src/lib.rs` for a demonstration.
968cba8
to
c8f4173
Compare
Gone back and forward several times on this one but it's a bit over my head! Leaning "close" due to inactivity, but pinging @MrGVSV in case I've missed something important... seems like an interesting idea, but not sure how easy it would be to adopt two years later. |
@richchurcher I haven't been keeping up with Bevy development, to be honest with you, so I don't really know if the purpose of this feature has been satisfied in other ways, or what it would take to update it. I also do not think I will have time to work on it any time soon, though if someone else would like to adopt it I would be happy to answer questions or help them get oriented. If I recall correctly, my main motivation for this was that I wanted to be able to override the implementation of certain functions that existed in the Anyway, my input would be that if the type registry still works like it used to then a feature like this is probably still useful because it gives users a clean way to create custom behaviors for |
@oddfacade thanks! Sadly I'm nowhere near qualified to answer, but hopefully someone will 🙂 |
Sorry this got lost in my inbox. Personally, I'm not fully sold on incorporating specialization hacks into the repo unless there's been a consensus in the community that we should embrace these patterns. I think for now this could be solved by creating the type data manually (i.e. use There's also #13723 which could be used to allow type data to be registered with parameters. For the So I'll close this for now unless anyone disagrees 🙂 |
Nice one, ty! |
If implemented,
SpecializedFromType<T>::specialized_from_type
will be used instead ofFromType<T>::from_type
in the context of theGetTypeRegistration
macro implementation.Objective
When creating a type data struct, it is often desirable to implement
FromType
as broadly as possible, in order to maximize the number of types which can make use of it without introducing additional boilerplate. For example,ReflectComponent
(a type data struct which is used primarily to add, remove, and modify type-erased components in relation to aWorld
) implementsFromType<T>
for anyT
which isReflect
,FromWorld
, and aComponent
. However, there are occasions where knowledge of the specific type which is being operated on can produce a more effectiveReflectComponent
.Consider the
Handle
. It is required to beDefault
(and thusFromWorld
) because we want to be able to use it in bundles, and so we haveReflectComponent: FromType<Handle<T>>
. The insert method provided byReflectComponent
looks roughly like this (rewritten as a function for clarity)Notice that the
from_world
constructor cannot depend on the value ofreflected_component
. This is a sensible compromise in general, but in the handle's case it means that it is impossible to create strong handles from scene files. The behavior we really want in that situation is something more like this:(untested and from memory, so probably wrong, but hopefully you get the idea). As it stands, the only way to do that would be to change the blanket implementation over all components, which isn't particularly feasible. Thus: specialization.
Solution
The solution involves a fairly straightforward implementation of dtolnay specialization (aka autoref specialization), and is inspired by how the pyo3 crate solved a similar problem. In essence, we define a trivial struct called
FromTypeCollector<S, T>
. We then implement a traitCollect<T>
like so:Specializations are then created by implementing
Collect<T>
forFromTypeCollector<U, T>
(without the&
) whereU
is more specific thanS
. To make this less cumbersome, we introduce a traitSpecializedFromType
and implement the specialization:So by implementing
SpecializedFromType
you can... well... specializeFromType
.Changelog
Added
FromType
implementations by implementingSpecializedFromType