Details

Reviewers

Campbell Barton (campbellbarton)
Dalai Felinto (dfelinto)
Brecht Van Lommel (brecht)
Bastien Montagne (mont29)

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.:

P540 StructRNA property-tag definition example

1	static EnumPropertyItem some_prop_tags[] = {
2	{TAG_VALUE, "TAG_ID", 0, "Tag UI Name", "Description for tag"},
3	{0, NULL, 0, NULL, NULL}
4	};
5
6	StructRNA *srna = RNA_def_struct(brna, "SomeStruct", NULL);
7	RNA_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.:
P541 Set property-tag Python example
1 my_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):

P542 Property-tags Python test script

1	import bpy
2
3	class DialogOperator(bpy.types.Operator):
4	bl_idname = "object.dialog_operator"
5	bl_label = "Property Tagging Example"
6
7	my_float = bpy.props.FloatProperty(name="Some Floating Point", tags={'BASIC'})
8	my_string = bpy.props.StringProperty(name="String Value") # no 'BASIC' tag, so not visible
9	my_bool = bpy.props.BoolProperty(name="Toggle Option", tags={'BASIC'})
10
11	def execute(self, context):
12	return {'FINISHED'}
13
14	def invoke(self, context, event):
15	wm = context.window_manager
16	return wm.invoke_props_dialog(self)
17
18	bpy.utils.register_class(DialogOperator)
19	bpy.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

Event Timeline

Julian Eisel (Severin) created this revision.Oct 16 2017, 7:25 PM

Harbormaster completed remote builds in B887: Diff 9407.Oct 16 2017, 7:25 PM

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) mentioned this in D2881: Support Marking Operator Properties as Basic or Advanced.Oct 16 2017, 7:40 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) added a reviewer: Bastien Montagne (mont29).

Julian Eisel (Severin) edited the summary of this revision. (Show Details)Oct 16 2017, 7:55 PM

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

Campbell Barton (campbellbarton) awarded a token.Oct 20 2017, 6:13 AM

In D2883#67793, @Campbell Barton (campbellbarton) wrote:

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.

Harbormaster completed remote builds in B915: Diff 9444.Oct 21 2017, 3:37 AM

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).

Avoid multiple calls to RNA_struct_property_tag_defines
Move tags argument before callback arguments of BPY property definition

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

Julian Eisel (Severin) mentioned this in rB3ebfd533cad5: Top-bar operator settings: Add simple "More..." button to show all settings.Oct 21 2017, 3:37 PM

In D2883#68045, @Campbell Barton (campbellbarton) wrote:

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.

Julian Eisel (Severin) mentioned this in D2758: Global Top-Bar - Initial Implementation.Oct 23 2017, 3:18 AM

In D2883#68073, @Julian Eisel (Severin) wrote:

In D2883#68045, @Campbell Barton (campbellbarton) wrote:

... 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.

In D2883#68085, @Campbell Barton (campbellbarton) wrote:

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:

P544 Failing enum type checks

1	diff --git a/source/blender/windowmanager/intern/wm_init_exit.c b/source/blender/windowmanager/intern/wm_init_exit.c
2	index 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:

P546 Modified P544 Failing enum type checks

1	diff --git a/source/blender/windowmanager/intern/wm_init_exit.c b/source/blender/windowmanager/intern/wm_init_exit.c
2	index 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.

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.

Julian Eisel (Severin) added inline comments.Nov 20 2017, 9:51 PM

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.

Cleanup: Update comments
Fix issues parsing BPY defined props, add BPY documentation for tags

Will probably merge this tomorrow.

Campbell Barton (campbellbarton) accepted this revision.Nov 23 2017, 1:59 AM

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!

Julian Eisel (Severin) closed this revision.Nov 23 2017, 2:31 PM

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

Details

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

Defining & setting tags

How to test

Notes

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 9592

source/blender/editors/interface/interface_templates.c

source/blender/editors/mesh/editmesh_loopcut.c

source/blender/editors/mesh/editmesh_tools.c

source/blender/editors/transform/transform_ops.c

source/blender/makesrna/RNA_access.h

source/blender/makesrna/RNA_define.h

source/blender/makesrna/RNA_enum_types.h

source/blender/makesrna/intern/makesrna.c

source/blender/makesrna/intern/rna_access.c

source/blender/makesrna/intern/rna_define.c

source/blender/makesrna/intern/rna_internal_types.h

source/blender/makesrna/intern/rna_rna.c

source/blender/makesrna/intern/rna_wm.c

source/blender/python/intern/bpy_props.c

source/blender/windowmanager/WM_api.h

source/blender/windowmanager/WM_types.h

source/blender/windowmanager/intern/wm_operators.c

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

Details

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

Defining & setting tags

How to test

Notes

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 9592

source/blender/editors/interface/interface_templates.c

source/blender/editors/mesh/editmesh_loopcut.c

source/blender/editors/mesh/editmesh_tools.c

source/blender/editors/transform/transform_ops.c

source/blender/makesrna/RNA_access.h

source/blender/makesrna/RNA_define.h

source/blender/makesrna/RNA_enum_types.h

source/blender/makesrna/intern/makesrna.c

source/blender/makesrna/intern/rna_access.c

source/blender/makesrna/intern/rna_define.c

source/blender/makesrna/intern/rna_internal_types.h

source/blender/makesrna/intern/rna_rna.c

source/blender/makesrna/intern/rna_wm.c

source/blender/python/intern/bpy_props.c

source/blender/windowmanager/WM_api.h

source/blender/windowmanager/WM_types.h

source/blender/windowmanager/intern/wm_operators.c

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

Revision Contents
Changeset List