bevy icon indicating copy to clipboard operation
bevy copied to clipboard

bevy_reflect: Custom attributes

Open MrGVSV opened this issue 1 year ago • 21 comments

Objective

As work on the editor starts to ramp up, it might be nice to start allowing types to specify custom attributes. These can be used to provide certain functionality to fields, such as ranges or controlling how data is displayed.

A good example of this can be seen in bevy-inspector-egui with its InspectorOptions:

#[derive(Reflect, Default, InspectorOptions)]
#[reflect(InspectorOptions)]
struct Slider {
    #[inspector(min = 0.0, max = 1.0)]
    value: f32,
}

Normally, as demonstrated in the example above, these attributes are handled by a derive macro and stored in a corresponding TypeData struct (i.e. ReflectInspectorOptions).

Ideally, we would have a good way of defining this directly via reflection so that users don't need to create and manage a whole proc macro just to allow these sorts of attributes.

And note that this doesn't have to just be for inspectors and editors. It can be used for things done purely on the code side of things.

Solution

Create a new method for storing attributes on fields via the Reflect derive.

These custom attributes are stored in type info (e.g. NamedField, StructInfo, etc.).

#[derive(Reflect)]
struct Slider {
    #[reflect(@0.0..=1.0)]
    value: f64,
}

let TypeInfo::Struct(info) = Slider::type_info() else {
    panic!("expected struct info");
};

let field = info.field("value").unwrap();

let range = field.get_attribute::<RangeInclusive<f64>>().unwrap();
assert_eq!(*range, 0.0..=1.0);

TODO

  • [x] ~~Bikeshed syntax~~ Went with a type-based approach, prefixed by @ for ease of parsing and flexibility
  • [x] Add support for custom struct/tuple struct field attributes
  • [x] Add support for custom enum variant field attributes
  • [x] ~~Add support for custom enum variant attributes (maybe?)~~ ~~Will require a larger refactor. Can be saved for a future PR if we really want it.~~ Actually, we apparently still have support for variant attributes despite not using them, so it was pretty easy to add lol.
  • [x] Add support for custom container attributes
  • [x] Allow custom attributes to store any reflectable value (not just Lit)
  • [x] ~~Store attributes in registry~~ This PR used to store these in attributes in the registry, however, it has since switched over to storing them in type info
  • [x] Add example

Bikeshedding

[!note] This section was made for the old method of handling custom attributes, which stored them by name (i.e. some_attribute = 123). The PR has shifted away from that, to a more type-safe approach.

This section has been left for reference.

There are a number of ways we can syntactically handle custom attributes. Feel free to leave a comment on your preferred one! Ideally we want one that is clear, readable, and concise since these will potentially see a lot of use.

Below is a small, non-exhaustive list of them. Note that the skip_serializing reflection attribute is added to demonstrate how each case plays with existing reflection attributes.

List
1. @(name = value)

The @ was chosen to make them stand out from other attributes and because the "at" symbol is a subtle pneumonic for "attribute". Of course, other symbols could be used (e.g. $, #, etc.).

#[derive(Reflect)]
struct Slider {
    #[reflect(@(min = 0.0, max = 1.0), skip_serializing)]
    #[[reflect(@(bevy_editor::hint = "Range: 0.0 to 1.0"))]
    value: f32,
}
2. @name = value

This is my personal favorite.

#[derive(Reflect)]
struct Slider {
    #[reflect(@min = 0.0, @max = 1.0, skip_serializing)]
    #[[reflect(@bevy_editor::hint = "Range: 0.0 to 1.0")]
    value: f32,
}
3. custom_attr(name = value)

custom_attr can be anything. Other possibilities include with or tag.

#[derive(Reflect)]
struct Slider {
    #[reflect(custom_attr(min = 0.0, max = 1.0), skip_serializing)]
    #[[reflect(custom_attr(bevy_editor::hint = "Range: 0.0 to 1.0"))]
    value: f32,
}
4. reflect_attr(name = value)
#[derive(Reflect)]
struct Slider {
    #[reflect(skip_serializing)]
    #[reflect_attr(min = 0.0, max = 1.0)]
    #[[reflect_attr(bevy_editor::hint = "Range: 0.0 to 1.0")]
    value: f32,
}

Changelog

  • Added support for custom attributes on reflected types (i.e. #[reflect(@Foo::new("bar")])

MrGVSV avatar Feb 02 '24 01:02 MrGVSV

Any chance we could do something like this for some type safety / name clashing protection?

#[derive(Reflect)]
struct Slider {
    #[reflect(InspectRange { min = 0.0, max = 1.0 })]
    value: f32,
}

#[derive(Reflect)]
struct InspectRange {
  min: f32,
  max: f32,
}

let TypeInfo::Struct(info) = Slider::type_info() else {
    panic!("expected struct info");
};

let inspect_range = info.field("value").unwrap().get_attribute::<InspectRange>();

cart avatar Apr 30 '24 23:04 cart

The { syntax miiiight not be possible (don't remember attribute syntax constraints) , but im pretty sure this is:

#[reflect(InspectRange(min = 0.0, max = 1.0))]

cart avatar Apr 30 '24 23:04 cart

Any chance we could do something like this for some type safety / name clashing protection?

#[derive(Reflect)]
struct Slider {
    #[reflect(InspectRange { min = 0.0, max = 1.0 })]
    value: f32,
}

#[derive(Reflect)]
struct InspectRange {
  min: f32,
  max: f32,
}

let TypeInfo::Struct(info) = Slider::type_info() else {
    panic!("expected struct info");
};

let inspect_range = info.field("value").unwrap().get_attribute::<InspectRange>();

Ooh interesting. I do like this. Although, for ease of parsing, it might be helpful to still require the @ prefix.

One thing I like about the current #[reflect(@min = 0.0, @max = 1.0)] syntax, however, is actually the name clashing haha.

For example, if bevy_mod_editor_utils adds custom sliders for min/max fields, and bevy_mod_scene_stuff adds their own special support for min/max fields, then users might end up with something like:

#[derive(Reflect)]
struct MyStruct {
  #[reflect(EditorRange::new(1.0..=100.0))]
  #[reflect(SceneRange {min: 1.0, max: 100.0})]
  value: f32
}

Or maybe bevy_mod_editor_utils might think to add bevy_mod_scene_stuff as an optional dependency to just make use of SceneRange.

By "allowing a clash" we make it so that third-party crates can sort of rely on community standards without either requiring users to include their own special attribute or adding some external dependency just to share an attribute. And if they do need special-casing, they have the option of creating their own naming convention (such as prefixing the attribute name with a namespace).

That being said, I still think there's merit to having something like you proposed. It greatly improves type safety since the current system doesn't handle typos very well (or at all haha).

I wonder if it would make sense to do both? @foo = value registers by name and maybe @Foo::new registers by type? That way, we can sorta get the best of both worlds?

Any thoughts on this? Should we just settle on one over the other? Does allowing both make sense?

MrGVSV avatar May 02 '24 00:05 MrGVSV

For reference, @dreamertalin on Discord provided some of their own use cases for such custom attributes:

If it helps any, here's a list of some use cases for annotations that I have used before:

  • min/max
  • precision - number of decimal digits to display
  • increment - amount to increment / decrement
  • bitmask - indicates that the integer should be edited as a bitfield (bit names to be supplied somehow)
  • resource, instance - indicates that a string field is actually a key or path, and should be edited by browsing existing resources, not by typing
  • multiline - indicates that a string can contain newlines, and should be edited via a multiline text widget
  • mutable - whether a field should be considered immutable (means the whole field has to be replaced, can't edit parts)
  • script - indicates the string field is actually a script

MrGVSV avatar May 02 '24 00:05 MrGVSV

By "allowing a clash" we make it so that third-party crates can sort of rely on community standards without either requiring users to include their own special attribute or adding some external dependency just to share an attribute. And if they do need special-casing, they have the option of creating their own naming convention (such as prefixing the attribute name with a namespace).

Imo "allowing clash" should be done at the type level via an agreed upon api:

#[derive(Reflect)]
struct MyStruct {
  #[reflect(Range {min: 1.0, max: 100.0})]
  value: f32
}

Where Range would semantically mean "allowed value range". For common use cases like this, we could define "standard" types to encourage collaboration + overlap.

cart avatar May 07 '24 21:05 cart

I think the only real argument for "typeless" is ergonomics:

#[derive(Reflect)]
struct MyStruct {
  #[reflect(Range {min: 1.0, max: 100.0}, Precision(100))]
  value: f32
}

vs

#[derive(Reflect)]
struct MyStruct {
  #[reflect(@min: 1.0, @max: 100.0, @precision(100)]
  value: f32
}

However it isn't that different.

Also note that Rust Analyzer might be smart enough to provide F12 / inspection on the type names + fields if we implement the macro in a way that directly uses the parsed idents. This works for attributes at the type derive level. I haven't tried at the field level.

cart avatar May 07 '24 21:05 cart

Where Range would semantically mean "allowed value range". For common use cases like this, we could define "standard" types to encourage collaboration + overlap.

That's a good point. I suppose Bevy can define a set of common attributes to reduce the number of "type crates" and/or duplicates of the same type of attribute. And I didn't even think of the fact that users could rely on Range<T> from the std library.

Also note that Rust Analyzer might be smart enough to provide F12 / inspection on the type names + fields if we implement the macro in a way that directly uses the parsed idents. This works for attributes at the type derive level. I haven't tried at the field level.

Yeah iirc I'm literally copying the value tokens verbatim, so it should work. But I haven't tested that so I'm not 100% sure.

MrGVSV avatar May 08 '24 15:05 MrGVSV

Okay I think I'll move this over to the typed proposal unless someone has any other comments/opinions.

@cart I think we can make this work without the @ prefix for fields: if it's an unexpected identifier/path then parse it as a custom attribute. However, I don't think we can do the same for container attributes, since the "default" case is interpreted as registering type data.

So I think we should probably still require the @ prefix for both (at least for now, we can always deprecate it away in the future). That also protects us in the case where we want to change up parsing for field attributes in a way that might make the prefix-less version ambiguous.

So altogether, this is the new syntax I'm going to update this PR to:

#[derive(Reflect)]
struct Slider {
    #[reflect( @Range { min: 0.0, max: 1.0 }, skip_serializing )]
    #[reflect( @bevy_editor::Hint::new( "Range: 0.0 to 1.0" ) )]
    value: f32,
}

MrGVSV avatar May 08 '24 15:05 MrGVSV

Sounds good!

cart avatar May 08 '24 19:05 cart

Hm, one thing this new syntax doesn't protect against is duplicate attributes. I feel like that's a likely occurrence, so maybe we just ignore them? Maybe last attribute added just silently overwrites the previous?

Ideally, we'd at least be able to output a compile check (i.e. const _: () {/* some check */}), but neither TypeId::of nor std::any::type_name are const. So I'm not sure how we'd error on duplicates other than manually parsing the type. That wouldn't catch aliases and might get confused around methods/variants, but again this probably isn't that big a deal in the first place, so I'm thinking we just do nothing.

MrGVSV avatar May 08 '24 20:05 MrGVSV

Yeah doing nothing seems fine, especially for a first impl.

cart avatar May 09 '24 18:05 cart

I want to point out a trade-off here: many of the use cases for custom attributes can also be accomplished by wrapping the field data in a newtype struct and using that to drive the choice of editing ui. The downside of the latter approach is that now the code that uses the field has to do an extra deref. This isn't so bad if it's used rarely, but becomes quite painful when you have to wrap every f32.

Thus, this leads to a data design strategy in which uncommon or anomalous data types are customized by wrapping, but "popular" data types such as f32 and bool are customized via attributes. In which case, the vast majority of attributes are going to be ones that are common/shared. Also, these common attributes are likely to be editor-agnostic: min/max for example are attributes that describe the constraints on the data regardless of how that data is edited.

Also, there's another use case I wanted to point out that I didn't mention in the above list. Normally in a reflection-based property editor, each field is independent: that is, the property inspector decides what kind of field editing widget should be used based on the type and attributes of a given field. However, there are some cases where two fields are edited together: what's valid data for field A depends on the content of field B. In my old editor, I had attributes that would "point" to a different field within the same struct, or give other kinds of contextual hints as to where the editor should look to get the information needed to edit the field. Two examples of this:

  • For character dialogs, the drop-down menu that is used to edit the "next state" transition was populated based on the available dialog states.
  • For character color maps (shirt, pants, hair, skin, etc.) the set of valid keys depends on the character archetype.

viridia avatar May 09 '24 22:05 viridia

I want to point out a trade-off here: many of the use cases for custom attributes can also be accomplished by wrapping the field data in a newtype struct and using that to drive the choice of editing ui.

Yeah that's a valid way of doing it. But like you said, it can become cumbersome. Additionally, it does not handle compositional attributes very well.

For example, to add a Range and a Required attribute using newtypes, you'd have to probably create a dedicated type for that combination: RequiredRange. Whereas, with attributes, they can be composed however the user wants.

However, there are some cases where two fields are edited together: what's valid data for field A depends on the content of field B.

This would be cool to have. It may be something we look into in the future. I can see the attribute accepting a Attributable trait or something which takes an instance of the type and returns the computed attribute. It may also be that the editor itself will be responsible for handling validation between fields.

For this initial PR, though, I feel that may be out of scope.

MrGVSV avatar May 10 '24 00:05 MrGVSV

I can see the attribute accepting a Attributable trait or something which takes an instance of the type and returns the computed attribute. It may also be that the editor itself will be responsible for handling validation between fields.

It's actually not that complicated - all you really need is an attribute that stores the field name. Since you are in the middle of a reflection operation anyway, you can look up the other field by name as needed.

viridia avatar May 10 '24 00:05 viridia

I can see the attribute accepting a Attributable trait or something which takes an instance of the type and returns the computed attribute. It may also be that the editor itself will be responsible for handling validation between fields.

It's actually not that complicated - all you really need is an attribute that stores the field name. Since you are in the middle of a reflection operation anyway, you can look up the other field by name as needed.

Sorry, I meant having the attribute itself give you computed data based on another field. With this PR, you can already store a field name in an attribute and have your editor implementation handle the rest. But to make it work (mostly) automatically, we'd need a solution that takes a reference to Self.

But again that's probably better off being explored in a separate PR.

MrGVSV avatar May 10 '24 00:05 MrGVSV

The range (min/max) is by far the most common attribute in my previous projects, and it's also common in the sense that it's independent of how the field is being edited. I propose that we add a standard type, let's call it AllowedRange::<T> for this use case. I like this better than simply using a Range object because it has an implied meaning. Whether that has separate min/max fields or just wraps a Range I don't care.

viridia avatar May 10 '24 21:05 viridia

The range (min/max) is by far the most common attribute in my previous projects, and it's also common in the sense that it's independent of how the field is being edited. I propose that we add a standard type, let's call it AllowedRange::<T> for this use case. I like this better than simply using a Range object because it has an implied meaning. Whether that has separate min/max fields or just wraps a Range I don't care.

We can consider adding attribute types (maybe a custom range type, Required, Tooltip, etc.) in a future PR where discussion can be dedicated around them. This PR is more just the foundation for which we can build types like that.

And my assumption is that as editor work increasingly ramps up, we'll have more and more use cases and examples indicating what attributes could be added to Bevy proper.

Also, as a side note, those types should exist outside bevy_reflect. While still largely tied to Bevy, we do try to keep it somewhat agnostic.

MrGVSV avatar May 10 '24 21:05 MrGVSV

We can consider adding attribute types (maybe a custom range type, Required, Tooltip, etc.) in a future PR where discussion can be dedicated around them. This PR is more just the foundation for which we can build types like that.

Yeah I agree that we should postpone the "built in attribute types" conversation until after this pr.

cart avatar May 10 '24 21:05 cart

So is this not going to be in the 0.14 milestone? I could really use this.

viridia avatar May 19 '24 01:05 viridia

So is this not going to be in the 0.14 milestone? I could really use this.

It can. If there's a strong desire for this functionality in 0.14, we can probably go ahead and include it, assuming it gets another approval. I was hoping to wait for the 0.15 cycle as we will (hopefully) have a lot of reflection work going on and I wanted to ensure that all of it plays well together before entering production.

I don't have a strong opinion on holding this back or not, though, as I don't really foresee it being a problematic PR either way.

MrGVSV avatar May 19 '24 01:05 MrGVSV

@viridia if you're comfortable reviewing and approving this, please do so! That will give us the second approval required and I can merge it :)

(I can also review it myself; I just haven't gotten to it)

alice-i-cecile avatar May 19 '24 03:05 alice-i-cecile

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to https://github.com/bevyengine/bevy-website/issues/1320 if you'd like to help out.

alice-i-cecile avatar Jun 03 '24 20:06 alice-i-cecile