RNA-property tagging using tags defined by struct (+ operator properties tagged 'BASIC')
ClosedPublic

Authored by Julian Eisel (Severin) on Oct 16 2017, 7:25 PM.

Details

Summary

RNA-property tagging using tags defined by struct (+ operator properties tagged 'BASIC')

Adds support for defining a number of tags as part of struct definition, which its properties can set for custom behaviour. Further, it uses it to tag operator properties as BASIC (alternative to D2881).
Note that this also includes Python integration.

Defining & setting tags

  • To define tags for a struct (which its properties can use then), define the tags in an EnumPropertyItem array, and assign them to the struct using RNA_def_struct_property_tags(...). E.g.:
    1static EnumPropertyItem some_prop_tags[] = {
    2{TAG_VALUE, "TAG_ID", 0, "Tag UI Name", "Description for tag"},
    3{0, NULL, 0, NULL, NULL}
    4};
    5
    6StructRNA *srna = RNA_def_struct(brna, "SomeStruct", NULL);
    7RNA_def_struct_property_tags(srna, some_prop_tags);
  • To set tags for an RNA-property in C, use the new RNA_def_property_tags(...).
  • To set tags for an RNA-property in Python, use the newly added tags parameter. E.g.:
    1my_float = bpy.props.FloatProperty(name="Some Float", tags={'SOME_TAG', 'ANOTHER_TAG'})

How to test

  • For testing I tagged a bunch of operator properties as BASIC. Redo panel only draws these. (see D2881, "For Testing" section)
  • Python: The following script should spawn a popup containing only properties tagged as BASIC (note the tags parameters):
    1import bpy
    2
    3class DialogOperator(bpy.types.Operator):
    4bl_idname = "object.dialog_operator"
    5bl_label = "Property Tagging Example"
    6
    7my_float = bpy.props.FloatProperty(name="Some Floating Point", tags={'BASIC'})
    8my_string = bpy.props.StringProperty(name="String Value") # no 'BASIC' tag, so not visible
    9my_bool = bpy.props.BoolProperty(name="Toggle Option", tags={'BASIC'})
    10
    11def execute(self, context):
    12return {'FINISHED'}
    13
    14def invoke(self, context, event):
    15wm = context.window_manager
    16return wm.invoke_props_dialog(self)
    17
    18bpy.utils.register_class(DialogOperator)
    19bpy.ops.object.dialog_operator('INVOKE_DEFAULT')
  • Python console :)

Notes

  • Using BASIC tag is more of a proof-of-concept, we can decide to use something different instead.
  • We discussed auto-tagging of operator properties by setting a state (e.g. call WM_operatortype_props_advanced to auto-tag following properties as advanced). Shall we do this?
  • Need to update BPY documentation in bpy_props.c still.
  • Would be nice to have a place to explain the tagging system. Wiki maybe?

Diff Detail

Repository
rB Blender
Branch
temp-rna_prop_tagging
Build Status
Buildable 987
Build 987: arc lint + arc unit
Julian Eisel (Severin) edited the summary of this revision. (Show Details)Oct 16 2017, 7:26 PM
Julian Eisel (Severin) edited the summary of this revision. (Show Details)
Julian Eisel (Severin) edited the summary of this revision. (Show Details)Oct 16 2017, 7:29 PM
Julian Eisel (Severin) retitled this revision from RNA-property tagging w/ tags defined by struct (+ operator properties tagged 'BASIC') to RNA-property tagging using tags defined by struct (+ operator properties tagged 'BASIC').Oct 16 2017, 7:52 PM
Julian Eisel (Severin) edited the summary of this revision. (Show Details)
Julian Eisel (Severin) edited the summary of this revision. (Show Details)Oct 16 2017, 7:55 PM
Campbell Barton (campbellbarton) requested changes to this revision.Oct 18 2017, 7:18 AM

Generally fine, though I'd try avoid forcing looping over EnumPropertyItem - in many cases the flag can be used directly.

Also much prefer ADVANCED over a BASIC flag, for testing purposes its fine though.

source/blender/makesrna/RNA_access.h
821

picky - prefer RNA_property_tags_* as common prefix. eg: RNA_property_tags_test?

source/blender/makesrna/intern/rna_access.c
979

While not so much against this function, I think it would be good to have

RNA_property_tags_get() to avoid having to loop over each bit - eg: rna_Property_tags_get - while this isn't a performance critical code path,
Only being able to check for a single flag forces this elsewhere.

989

Agree, should be debug, maybe worth storing all used bits next to srna->prop_tag_defines for quick one liner checks.

source/blender/makesrna/intern/rna_define.c
1298–1306

Again, storing all used bits could make this a one liner.

source/blender/python/intern/bpy_props.c
1878

*picky*, better assign a variable for RNA_struct_property_tag_define result in a nested scope.

1885

Cast is no longer needed as of rBab7ebf2b10f67b002447fb0e2cb352c2c178e128

2035

Rather keep callbacks last, these are keyword-only arguments, see: "all args must be keywords" message, so adding new args between existing ones is fine.

Adding after options would be best, since they're both set's used for all property definitions.

This revision now requires changes to proceed.Oct 18 2017, 7:18 AM

Generally fine, though I'd try avoid forcing looping over EnumPropertyItem - in many cases the flag can be used directly.

Agree it's quite annoying, OTOH we wouldn't have any proper type-safety without it in C. A dev could (unintentionally) do RNA_def_property_tags(prop, PROP_HIDDEN), without explicit complaints. We could do a tag value sanity check in RNA_def_property_tags() for debug builds only. Stored tag values should always be valid then (at least in debug builds), so a RNA_property_tags_get() would be okay, even if it doesn't do any additional value sanity checks.

Also much prefer ADVANCED over a BASIC flag, for testing purposes its fine though.

Yeah might probably make more sense, although I'm wondering what the best way to do it is. Likely the vast majority of operator-properties would have to be tagged as ADVANCED which would be annoying. Possible solutions:

  • Tag operator-properties as ADVANCED by default and add RNA_def_property_clear_tags(). But how would we set an operator-property-only tag by default? Probably wrappers would be needed (e.g. WM_operatortype_prop_def_boolean()).
  • We add WM_operatortype_props_advanced() which would remove ADVANCED tag from all properties added so far (from D2881). Would need some (small & ugly) trickery to get it to work like expected when called multiple times. That is, all properties added after the first call should be ADVANCED.
  • Or we add something like WM_operatortype_props_advanced_begin() and WM_operatortype_props_advanced_end(). Former would store current count of properties in a static variable, _end would tag all properties added after this count to be ADVANCED.

In any case, do we agree that this in essence is the way to go, and D2881 should be closed?

  • Get rid of redundant, loop-based sanity checks and only check in debug builds (see my last comment)
  • Add an "Advanced" tag for operator properties, use it instead of "Basic" one
  • Add WM_operatortype_props_advanced_begin() and WM_operatortype_props_advanced_end() to avoid need to add tags everywhere manually
  • Tag all operator-properties as advanced by default.
Julian Eisel (Severin) marked 4 inline comments as done.Oct 21 2017, 3:40 AM
Julian Eisel (Severin) added inline comments.
source/blender/makesrna/RNA_access.h
821

Removed function (wasn't needed anymore).

Julian Eisel (Severin) marked an inline comment as done.
  • Avoid multiple calls to RNA_struct_property_tag_defines
  • Move tags argument before callback arguments of BPY property definition
Julian Eisel (Severin) marked 4 inline comments as done.Oct 21 2017, 4:24 AM

Just realized the Python integration doesn't work really. It only allows tagging operator-properties as ADVANCED, but we already set that tag to all operator-properties.
How should we handle this? By not tagging Python defined op-properties as advanced by default? Not tagging any op-properties by default? Not sure really...

  • Agree D2881 can be closed.
  • Would really prefer keep current operator properties as-is (not advanced by default).
Setting any tags by default is a bit odd. This can be handled as separate patch, messing with defaults gets in the way of reviewing the basics.
  • Re: RNA_def_property_tags(prop, PROP_HIDDEN), Well, this is C, you can use flags all over the place without complaints. Since custom tags are by definition unknown at compile time, I'm not sure its worth trying too much to prevent wrong args from C code.

    Am not against some form of safety tho: eg, use a common prefix for hard coded tag names, then use a macro by default for access:
#define RNA_some_tag_function(prop, id) \
        RNA_some_tag_function_flag(prop, RNA_TAG_PRESET_##id)

... or better, wrap in a macro and check against an enum type see, CHECK_TYPE_ANY

  • Would really prefer keep current operator properties as-is (not advanced by default).

Fine with that.
Reason why still lean towards having properties be advanced by default is so devs (Python & C) are kinda forced to think about which properties they want to be advanced and which not. If by default all properties are visible in the top-bar (=non-advanced), they will just leave it as is and not think about it further. But well... we can blame those devs then ;)

  • Re: RNA_def_property_tags(prop, PROP_HIDDEN), Well, this is C, you can use flags all over the place without complaints. Since custom tags are by definition unknown at compile time, I'm not sure its worth trying too much to prevent wrong args from C code. Am not against some form of safety tho: eg, use a common prefix for hard coded tag names, then use a macro by default for access:
#define RNA_some_tag_function(prop, id) \
        RNA_some_tag_function_flag(prop, RNA_TAG_PRESET_##id)

... or better, wrap in a macro and check against an enum type see, CHECK_TYPE_ANY

First solution would only allow passing a single tag (or we add ELEM like wrappers). Also, it wouldn't be so nice for searching, and tags names wouldn't have the usual pseudo namespace prefix (e.g. OP_PROP_TAG_), except if we use really long names.
Checked second one, and seems this wouldn't work either since typeof(SOME_ENUM_ITEM) results in unsigned int (or similar), not the actual enum type. Seems like there is no way to get this to work, otherwise it would be my preferable solution.

What about the approach in the current patch? It's just doing a single (iterative) check for valid tag values in debug builds.

... or better, wrap in a macro and check against an enum type see, CHECK_TYPE_ANY

First solution would only allow passing a single tag (or we add ELEM like wrappers). Also, it wouldn't be so nice for searching, and tags names wouldn't have the usual pseudo namespace prefix (e.g. OP_PROP_TAG_), except if we use really long names.
Checked second one, and seems this wouldn't work either since typeof(SOME_ENUM_ITEM) results in unsigned int (or similar), not the actual enum type. Seems like there is no way to get this to work, otherwise it would be my preferable solution.

What about the approach in the current patch? It's just doing a single (iterative) check for valid tag values in debug builds.

Think the second approach (using enum's) would be better. Why would OP_PROP_TAG_* need to be unsigned int type? We can define an enum and use that.

Iterating is OK as some debug check, but don't see the need to over-complicate flag usage for the common case.

Think the second approach (using enum's) would be better. Why would OP_PROP_TAG_* need to be unsigned int type? We can define an enum and use that.

What I mean is, for the compiler, a defined enum type is an unsigned int (or whatever integer type the compiler chooses). Checked online & tested myself, seems like there is no way to do a compile time check for an enum type. So there is effectively no difference between defining a constant using an enum or as #define, even at compile time. Both are replaced by a value literal wherever accessed (I assume), additional type information is lost.

With this you can see what I mean:

1diff --git a/source/blender/windowmanager/intern/wm_init_exit.c b/source/blender/windowmanager/intern/wm_init_exit.c
2index e73ec2b081a..283660fd141 100644
3--- a/source/blender/windowmanager/intern/wm_init_exit.c
4+++ b/source/blender/windowmanager/intern/wm_init_exit.c
5@@ -149,10 +149,21 @@ static void wm_undo_kill_callback(bContext *C)
6
7​ bool wm_start_with_console = false; /* used in creator.c */
8
9+typedef enum {
10+ SOME_ENUM_ITEM = 1,
11+} SomeEnum;
12+
13+#define SOME_CONSTANT 42
14+
15​ /* only called once, for startup */
16​ void WM_init(bContext *C, int argc, const char **argv)
17​ {
18-
19+ SomeEnum item = SOME_ENUM_ITEM;
20+
21+ printf("%s %s\n", SOME_ENUM_ITEM, SOME_CONSTANT);
22+ printf("%s\n", item);
23+ CHECK_TYPE(SOME_ENUM_ITEM, SomeEnum);
24+
25​ if (!G.background) {
26​ wm_ghost_init(C); /* note: it assigns C to ghost! */
27​ wm_init_cursor_data();
The printf's should cause some warnings, printing the types the compiler interprets the constants or variable as (integer types). The CHECK_TYPE prints a warning too: even though SOME_ENUM_ITEM is defined as enumerator of SomeEnum, the compiler treats it as integer type, not as SomeEnum.

Actually, the C standard requires this handling, see http://en.cppreference.com/w/c/language/enum.

While not nice, here is your example, modified to work by forcing a cast in the enum:

1diff --git a/source/blender/windowmanager/intern/wm_init_exit.c b/source/blender/windowmanager/intern/wm_init_exit.c
2index 8346535cd6a..e2ae4af9066 100644
3--- a/source/blender/windowmanager/intern/wm_init_exit.c
4+++ b/source/blender/windowmanager/intern/wm_init_exit.c
5@@ -150,10 +150,24 @@ static void wm_undo_kill_callback(bContext *C)
6
7​ bool wm_start_with_console = false; /* used in creator.c */
8
9+typedef enum SomeEnum {
10+ SOME_ENUM_ITEM = 1,
11+ SOME_ENUM_BLAH = 2,
12+} SomeEnum;
13+#define SOME_ENUM_ITEM ((SomeEnum)SOME_ENUM_ITEM)
14+#define SOME_ENUM_BLAH ((SomeEnum)SOME_ENUM_BLAH)
15+
16​ /* only called once, for startup */
17​ void WM_init(bContext *C, int argc, const char **argv)
18​ {
19-
20+ SomeEnum item = SOME_ENUM_ITEM;
21+
22+ CHECK_TYPE(item, SomeEnum); // OK
23+ CHECK_TYPE(SOME_ENUM_ITEM, SomeEnum); // OK
24+ CHECK_TYPE(SOME_ENUM_ITEM | SOME_ENUM_BLAH, SomeEnum); // OK
25+
26+ CHECK_TYPE(42, SomeEnum); // NOT OK (which is good in this case :D)
27+
28+ UNUSED_VARS(item);
29+
30+
31​ if (!G.background) {
32​ wm_ghost_init(C); /* note: it assigns C to ghost! */
33​ wm_init_cursor_data();

  • Hack to use CHECK_TYPE macro for tag enumerator type safety
  • Don't tag operator properties as advanced by default

@Campbell Barton (campbellbarton), this is now using the hack you suggested. Also operator properties aren't tagged by default anymore. Python usage should also work fine now.

source/blender/editors/interface/interface_layout.c
65 ↗(On Diff #9503)

Merge issue, please ignore.

Campbell Barton (campbellbarton) requested changes to this revision.EditedNov 2 2017, 4:37 AM

Generally LGTM, noticed one problem, otherwise think this can be committed to master. (maybe other devs want to sign off on it too?)

source/blender/python/intern/bpy_props.c
2040

tags verible must define the type O! & &PySet_Type, &pyopts.
This means we get a useful error early on when the wrong type is passed and is needed because pyrna_set_to_enum_bitfield doesn't check for a set type.

This revision now requires changes to proceed.Nov 2 2017, 4:37 AM

LGTM, besides @Campbell Barton (campbellbarton)’s remark (which is trivial to address).

source/blender/editors/interface/interface_templates.c
3731–3745

Think this could be done much simpler/nicer by defining our own check_prop… if we could find a way to pass original check_prop to new one…

Nothing mandatory/blocking, more like a nice TODO to make things cleaner.

source/blender/editors/interface/interface_templates.c
3731–3745

For the top-bar that shouldn't be an issue. What I did there was wrapping this function (uiTemplateOperatorPropertyButs) into one that also considers op-macros, adds the "More..." button, etc. So we can totally pass a custom check_prop to filter out advanced props.

Julian Eisel (Severin) updated this revision to Diff 9592.EditedNov 23 2017, 12:15 AM
  • Cleanup: Update comments
  • Fix issues parsing BPY defined props, add BPY documentation for tags

Will probably merge this tomorrow.

This revision is now accepted and ready to land.Nov 23 2017, 1:59 AM

Committed in separate commits: rB23d148ecaf92, rB60cbdb01527e, rB3cdd99767ef8, rB641d870b375a.
Note that this doesn't add a visible change to the user yet.

Thanks for the reviews!