-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
bevy_reflect: Replace "value" terminology with "opaque" #15240
base: main
Are you sure you want to change the base?
Conversation
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.
On board with the change. Several of the types touched here are missing trait reflection attributes, primarily for Debug. Your call if you want to change it here.
Done! |
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.
Calling a primitive opaque is a little surprising, but think this is in general an improvement relative to "value", as it can be argued a primitive has no interior representation. Just one note on minor doc issue.
c6e3cb9
to
fddbf45
Compare
fddbf45
to
8c69a80
Compare
totally in favour of the direction! i think opaque is a good name but i would also like to suggest |
Hmm, I like both. |
This is pretty good too. Like Alice said, this would help clear up the confusion with primitives. My only concern is in documentation like you mentioned. We might write "returns an atom type"/"takes an atom value", but readers might see "returns an atomic type"/"takes an atomic value", especially since "atomic" is the adjective form. For that reason, I think I lean a bit towards "opaque", but I'd love to hear if anyone else has any opinions on the matter! We can definitely go with "atom" if people prefer it. |
Objective
Currently, the term "value" in the context of reflection is a bit overloaded.
For one, it can be used synonymously with "data" or "variable". An example sentence would be "this function takes a reflected value".
However, it is also used to refer to reflected types which are
ReflectKind::Value
. These types are usually either primitives, opaque types, or types that don't fall into any otherReflectKind
(or perhaps could, but don't due to some limitation/difficulty). An example sentence would be "this function takes a reflected value type".This makes it difficult to write good documentation or other learning material without causing some amount of confusion to readers. Ideally, we'd be able to move away from the
ReflectKind::Value
usage and come up with a better term.Solution
This PR replaces the terminology of "value" with "opaque" across
bevy_reflect
. This includes in documentation, type names, variant names, and macros.The term "opaque" was chosen because that's essentially how the type is treated within the reflection API. In other words, its internal structure is hidden. All we can do is work with the type itself.
Primitives
While primitives are not technically opaque types, I think it's still clearer to refer to them as "opaque" rather than keep the confusing "value" terminology.
We could consider adding another concept for primitives (e.g.
ReflectKind::Primitive
), but I'm not sure that provides a lot of benefit right now. In most circumstances, they'll be treated just like an opaque type. They would also likely use the same macro (or two copies of the same macro but with different names).Testing
You can test locally by running:
Migration Guide
The reflection concept of "value type" has been replaced with a clearer "opaque type". The following renames have been made to account for this:
ReflectKind::Value
→ReflectKind::Opaque
ReflectRef::Value
→ReflectRef::Opaque
ReflectMut::Value
→ReflectMut::Opaque
ReflectOwned::Value
→ReflectOwned::Opaque
TypeInfo::Value
→TypeInfo::Opaque
ValueInfo
→OpaqueInfo
impl_reflect_value!
→impl_reflect_opaque!
impl_from_reflect_value!
→impl_from_reflect_opaque!
Additionally, declaring your own opaque types no longer uses
#[reflect_value]
. This attribute has been replaced by#[reflect(opaque)]
:Note that the order in which
#[reflect(opaque)]
appears does not matter.