Page MenuHome

Refactor IDProperty UI data storage
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Dec 1 2020, 2:53 PM.

Details

Summary

The Change

The storage of IDProperty UI data (min, max, default value, etc) is quite
complicated. For every property, retrieving a single one of these values
involves three string lookups. First for the "_RNA_UI" group property,
then another for a group with the property's name, then for the data
value name. Not only is this inefficient, it's hard to reason about,
unintuitive, and not at all self-explanatory.

This commit replaces that system with a UI data struct directly in the
IDProperty. If it's not used, the only cost is of a NULL pointer. Beyond storing
the description, name, and RNA subtype, derived structs are used to store
type specific UI data like min and max.

BeforeAfter

Note that this means that addons using (abusing) the _RNA_UI custom
property will have to be changed. I made the few changes required to the
addon directory in D9919 (which I will update again if this patch is reviewed).

The New API

Before

# Find the special "UI data" IDProperty group and create a new subgroup for a property.
prop = rna_idprop_ui_prop_get(idproperties_owner, "prop_name", create=True)
# Use more IDProperties to set some UI data values.
prop["min"] = 1.0

After

# Retrieve the UI data manager for the property.
ui_data = idproperties_owner.id_properties_ui("prop_name")
# Update some UI data values.
ui_data.update(min=1.0)

In addition to update, there are now other helper function for accessing the UI data:

  • as_dict: Returns a dictionary of a property's UI data.
  • clear: Removes a property's UI data.
  • update_from: Copy UI data between properties, even if they have different owners

Motivations

I made a post about this on devtalk. The original impetus for these changes came
from implementing the IDProperty creation for the nodes modifier. Doing that I got
frustrated enough that I was motivated to make a system that made more sense in my free time.
This should also make it easier to add new IDProperty types in the future, since their UI
data is clearly separated (boolean properties for example).

Diff Detail

Repository
rB Blender
Branch
refactor-idprop-ui-data (branched from master)
Build Status
Buildable 11760
Build 11760: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • Merge branch 'master' into refactor-idprop-ui-data
  • Cleanup: Use BLI_assert_unreachable
  • Refactor new IDProperty UI data API (the changes described above)
  • Small cleanup
Campbell Barton (campbellbarton) requested changes to this revision.Aug 2 2021, 6:48 AM
Campbell Barton (campbellbarton) added inline comments.
source/blender/python/generic/idprop_py_ui_api.cc
1 ↗(On Diff #40206)

I'd rather keep source/blender/python/ C-only unless there is a strong reason to use C++.

64–71 ↗(On Diff #40206)

This can be simplified using BPy_EnumProperty_Parse, see: rB9764d90fda685492c8fcb3bb55a8949749d461f2.

This also gives a useful error message, listing the incorrect argument and possible correct values.

This revision now requires changes to proceed.Aug 2 2021, 6:48 AM
source/blender/python/generic/idprop_py_ui_api.cc
1 ↗(On Diff #40206)

Personally I think the ability to use references, etc. is worth it, as well as allowing for further improvements in the future like interfacing with other areas of Blender that use types like Span which I see as a huge improvement.
However in the interest of just getting this finished I changed this new file to C.

64–71 ↗(On Diff #40206)

I had to do this at the top of the file: #include "../intern/bpy_rna.h"
If that's not okay I can revert this change.

  • Switch to C for the new file
  • Use pyrna_enum_value_parse_string

LGTM, noted minor issues but I don't think another iteration from me is needed.


Some picky warnings applying the diff:

D9697.diff:204: trailing whitespace.
    # Update the UI settings
D9697.diff:207: trailing whitespace.
        subtype=subtype,
D9697.diff:208: trailing whitespace.
        min=min,
D9697.diff:209: trailing whitespace.
        max=max,
D9697.diff:210: trailing whitespace.
        soft_min=soft_min,
warning: 5 lines add whitespace errors.
source/blender/python/intern/bpy_rna.c
4232

*picky* I normally put each "\n" on it's own line so this reads roughtly as it would as plain-text.

source/blender/python/generic/idprop_py_ui_api.cc
64–71 ↗(On Diff #40206)

This if fine, although we could move this to a more generic define that doesn't pull in the whole Python RNA API.

source/blender/python/generic/py_capi_utils.h
21–23 ↗(On Diff #40206)

This is no longer needed.

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
Sybren A. Stüvel (sybren) requested changes to this revision.Aug 5 2021, 12:52 PM

Creating a custom property (via the Object Properties → Custom Properties panel) now fails:

Python: Traceback (most recent call last):
  File "blender-git/build_linux/bin/3.0/scripts/startup/bl_operators/wm.py", line 1658, in execute
    rna_idprop_ui_create(item, prop, default=1.0)
  File "blender-git/build_linux/bin/3.0/scripts/modules/rna_prop_ui.py", line 120, in rna_idprop_ui_create
    ui_data.update(
TypeError: expected a string enum, not NoneType

About the do_versions_idproperty_ui_data() function: it might be a good idea to refactor that function, and separate the "loop over all datablocks with IDProperties" from "convert the UI stuff". That loop might be valuable for other things as well. For me this is a "nice to have", and doesn't stand in the way of acceptance of this patch.

release/scripts/startup/bl_operators/wm.py
1306

This should really be refactored into something better; a sequence of if-statements is something to avoid (source). Not for this patch, though, but a good target for a cleanup pass.

1307

Using type(x) == y is generally the wrong way to compare types in Python. Unless there is a good reason to reject subclasses of the tested-for type (which then should be documented) this should be using isinstance(default_eval, str) instead.

Same goes for the other type comparisons.

1320–1322

This looks like it should be its own function, something like:

T = TypeVar('T')  # Can be anything but str.
def value_if_evaluatable(value: T) -> Optional[T]:
    if isinstance(value, str):
        # A string means the value could not be evaluated.
        return None
    return value

This can then be used in the places where the same logic is used.

1386–1387

Why only for these types?

1388

Why only for these types?

source/blender/blenkernel/intern/idprop.c
292–306

I think these conditions can be written in terms of the return value of IDP_ui_data_type().

source/blender/blenloader/intern/versioning_300.c
243–245 ↗(On Diff #40220)

Nice to have this list here.

source/blender/makesrna/RNA_access.h
1010

No need to use const in the declaration, as it has no semantic meaning (the int is passed by value, so the function cannot alter its value anyway).

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

Doesn't that mean it's a good idea to put this code into a separate function?

source/blender/modifiers/intern/MOD_nodes.cc
430–450

None of these curly braces are actually necessary.

431–520

These case statements don't need those curly braces.

source/blender/python/generic/idprop_py_ui_api.c
137 ↗(On Diff #40220)

Flip the condition and just do return true;, unindenting the rest of the code. If that's not possible for some reason, I'd advice to move the body of this conditional into its own function, so that it has a name that explains what it's for, and it can do if (!PySequence_Check(default_value)) { return true; } and unindent the code by yet another level.

246–247 ↗(On Diff #40220)

Same comment as above.

366–397 ↗(On Diff #40220)

Parentheses for the case statements aren't necessary.

This revision now requires changes to proceed.Aug 5 2021, 12:52 PM

Noticed an issue where it's possible to for the function to partially succeed before returning an error, replied to some points inline.

release/scripts/startup/bl_operators/wm.py
1320–1322

Changes to type comparison would be better handled in a separate patch so it's not just limited to lines this patch touches.

source/blender/makesrna/RNA_access.h
1010

There can be a benefit in the function it's self not being able to modify the argument (similar to const local variables).

source/blender/modifiers/intern/MOD_nodes.cc
430–450

If this is something to avoid it should probably be handled at the level of code style proposal, there are many instances of this in the code base. The code-style page even uses braces in it's examples.

source/blender/python/generic/idprop_py_ui_api.c
137 ↗(On Diff #40220)

I'd rather keep the successful return last, otherwise it complicates adding other kind of arguments later on, as well as cleanup functions which are fairly common with C/Python API.

138–168 ↗(On Diff #40220)

With the error returns here, it's possible to half-initialize data.

In general this should be avoided, it means a try/except could run the command, changing min/max but not defaults - for eg.

It might be simplest to operate on a copy of ui_data, writing it back at the very end, before returning true.

Hans Goudey (HooglyBoogly) marked 13 inline comments as done.
  • Use pyrna_enum_value_from_id to parse enum items (fix bug creating custom property)
  • Use IDP_ui_data_type instead of more specific checks
  • Add function to fill default array from doubles, simplify a bit
  • Remove curly braces
  • Remove const from int argument in header
  • Split setting defaults to a separate function for int and float
  • Rename IDP_free_ui_data -> IDP_ui_data_free
  • Write to a temporary local copy of the UI data
  • Free unused result in failure cases
  • Add key arg to id_properties_ui doc
  • Remove more curly braces
  • Put "\n" on newline
Campbell Barton (campbellbarton) requested changes to this revision.Aug 6 2021, 2:56 AM
Campbell Barton (campbellbarton) added inline comments.
source/blender/python/generic/idprop_py_ui_api.c
147–153 ↗(On Diff #40380)

IDP_ui_data_free_contents as a way to cleanup seems incorrect, as it will free the default array when that will be shared with the original.

There could be a wrapper cleanup function that takes both the original ui_data and it's copy - that NULL's any duplicate pointers, then call IDP_ui_data_free_contents on the copy.

This revision now requires changes to proceed.Aug 6 2021, 2:56 AM
source/blender/makesrna/RNA_access.h
1010

Yes, but that is only effective when you declare the parameter as const in the definition. As I said, in the declaration it has no meaning, and should thus be removed to avoid confusion.

source/blender/python/generic/idprop_py_ui_api.c
137 ↗(On Diff #40220)

I think it's a very bad idea to avoid clearer code now. Reduction of cognitive complexity is important. If there is a desire to do cleanup after the code runs, it might be an indication that it should be split up into smaller functions (init, work, cleanup).

What you propose is keeping code more complex, just for the hypothetial case that the code might be expanded in the future; this usually is a bad way to design code, unless those expansions are already known & to be taken into account.

source/blender/python/generic/idprop_py_ui_api.c
137 ↗(On Diff #40220)

What you propose is keeping code more complex, just for the hypothetial case that the code might be expanded in the future; this usually is a bad way to design code.

It's quite reasonable to write code so adding new arguments doesn't require existing shuffling code blocks about.

To avoid half initialized data it's likely some cleanup/finalizing functionality will need to run before the function exists.

As you've suggested the code was split into a function so this should settle both our concerns.

Sybren A. Stüvel (sybren) added inline comments.
release/scripts/startup/bl_operators/wm.py
1320–1322

👍

source/blender/blenkernel/BKE_idprop.h
188–190

👍

source/blender/python/generic/idprop_py_ui_api.c
137 ↗(On Diff #40220)

👌 thanks!

Good point Campbell, I missed that. I experimented a bit with different options to
only free the changed pointers and I came up with something I think is quite nice actually.

  • Cleanup: Rename variables
  • Don't apply any changes on failure, refactor with IDP_ui_data_free_unique_contents

LGTM.

Getting trailing spaces warnings when applying, suggest to configure editing Python files to strip trailing space on save, or run ./source/tools/utils_maintenance/trailing_space_clean.py

tests/python/bl_pyapi_idprop.py
255 ↗(On Diff #40418)

*picky* end comments with full-stops.

Hans Goudey (HooglyBoogly) marked 11 inline comments as done.Aug 9 2021, 10:13 PM

I realized I had all of these un-submitted replies below, oops! Anyway, I've resolved the the final comments and fixed the white-space errors in the python code.


Creating a custom property (via the Object Properties → Custom Properties panel) now fails:

Sorry about that, it was caused by the last change I did which I guess I didn't test properly.

About the do_versions_idproperty_ui_data() function: it might be a good idea to refactor that function, and separate the "loop over all datablocks with IDProperties" from "convert the UI stuff". That loop might be valuable for other things as well. For me this is a "nice to have", and doesn't stand in the way of acceptance of this patch.

My worry about that is that it will become stale quite quickly as new structs with IDProperties are added. So maybe best to do that refactor if it's ever needed, and not have rotting code in the meantime.

release/scripts/startup/bl_operators/wm.py
1306

I think refactoring this to use separate classes would just increase the complexity, and anyway, it would require a larger refactor of the surrounding code which I don't want to do in this patch.

1307

In this case I'm just rearranging the code that was already here: prop_type == float, etc.

I get your point, but I'd rather keep this patch focused on its goal, not refactoring/improving type comparisons in this operator.

1320–1322

Agreed. Currently it's pretty easy to understand the change looking at this diff. With the change suggested above the change becomes less obvious.

1386–1387

Because these are the property types that support having a default.

I started adding a comment here, but decided not to, since I was just restating the logic of the code itself.

Generally I think this operator could be refactored, there could be some UX improvements like selecting the property type from a dropdown.
But I'm trying to work within the logic already here, so I'd really like to avoid larger changes for now.

1388

Those are the types that support these UI data values.

source/blender/blenkernel/intern/idprop.c
292–306

Good point, thanks!

source/blender/makesrna/RNA_access.h
1010

Yeah, but Sybren's point is that the declaration doesn't need to have const by-value arguments, only the definition.

Personally I disagree (I'd much rather have the declaration and definition exactly the same), but it's in the style guide apparently, so I will change this.

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

Yes, good point.

source/blender/modifiers/intern/MOD_nodes.cc
431–520

I'd rather have them be consistent with the other cases that do need the braces.

source/blender/python/generic/idprop_py_ui_api.c
137 ↗(On Diff #40220)

I'll split it to a separate function, but I don't want to return early in the second way you suggested because I don't think it's more readable-- the two cases are around the same size, and it's only one if statement in a function-- 2 levels of indentation.

Hans Goudey (HooglyBoogly) marked 2 inline comments as done.
  • Merge branch 'master' into refactor-idprop-ui-data
  • Remove trailing whitespace
  • End comments with period
source/blender/makesrna/RNA_access.h
1010

I got into the habit of matching const otherwise MSVC throws a lot of warnings, if that changed recently, we could consider removing const values in headers.

Campbell Barton (campbellbarton) added inline comments.
source/blender/makesrna/RNA_access.h
1010

According to @Ray Molenkamp (LazyDodo) this still warns with MSVC 2017, which is still supported. But it's no longer an issue in MSVC 2019.

2019 is our preferred compiler, 2017 I try to keep running, but if that throws out some extra warns, i'm not going to lose sleep over it

Don't really have time to review this, and two or more blender devs have already done the work here, so I'll just trust them and original author.

This revision is now accepted and ready to land.Aug 12 2021, 5:27 PM
Hans Goudey (HooglyBoogly) marked an inline comment as done.Aug 13 2021, 4:19 PM

I'm going to be away next week, so I'll commit this when I get back so I'm around if any issues come up. In the meantime I'll update the patch to update the addons repository.

This revision was automatically updated to reflect the committed changes.