Page MenuHome

Custom Properties: Rewrite edit operator, improve UX
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Sep 8 2021, 10:18 PM.

Details

Summary

This patch changes the custom property edit operator to make editing different
types of properties more obvious, and to expose more of the data, made more
easily possible by the recent UI data refactor.

Previously the operator guessed the type you wanted based on what you wrote
in a text box. That was problematic though, you couldn't make a string property
with a value of 1234, and you had to know about the Python syntax for lists
in order to create an array property. It was also slow and error prone; it was
too easy to make a typo and end up with a string property instead.

Improvements compared to the old operator:

  • A type drop-down to choose between the property types.
  • Step and precision values are exposed.
  • Buttons that have the correct type based on the property.
  • String properties no longer display min, max, etc. buttons.
  • Generally works in more cases. The old operator tended to break.
  • Choose array length with a slider.
  • Easy to choose to use python evaluation to type a value when necessary.
  • Code is commented, split up, and much easier to understand (for me at least).
  • Code is explicit, works more "on purpose" than "by mistake".

The custom property's value is purposefully not exposed, since the Edit
operator is for changing the property's metadata now, rather than the value itself.
In the "Python" mode though, the value is still presented as a string.


In the process of working on this, I ended up having to make cleanup
changes to understand the code properly. I could split out some of these
if necessary, but the patch is really more of a rewrite anyway.

Diff Detail

Repository
rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Sep 8 2021, 10:18 PM
Hans Goudey (HooglyBoogly) created this revision.
Hans Goudey (HooglyBoogly) retitled this revision from Initial refactor of custom properties edit operator to Improve custom property edit operator.Sep 8 2021, 10:23 PM
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
  • Fix incorrect name

There will need to be separate properties for array/non-array defaults.

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

Getting an error here: TypeError: 'array_length' is an invalid keyword argument for IntProperty() ... causing registration to fail elsewhere.

  • Fix errors with property registration
  • Implement remaining features, fix remaining issues
Hans Goudey (HooglyBoogly) retitled this revision from Improve custom property edit operator to Custom Properties: Rewrite edit operator, improve UX.Sep 10 2021, 5:10 AM
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
Hans Goudey (HooglyBoogly) marked an inline comment as done.Sep 10 2021, 5:20 AM
Campbell Barton (campbellbarton) requested changes to this revision.Sep 10 2021, 6:08 AM

With this patch applied editing property groups converts them to strings.

For example, adding editing this property in the console:

C.object["test"] = {"key": 3}

In this case I think it would be better to either:

  • Disable editing these groups entirely.
  • Support editing them as Python literals which are then re-evaluated (what is happening currently).

Note that editing values as Python literals scales poorly and isn't very practical so I'm personally not fussed if support for this is removed,
although we should check with TD's/technical-users if this functionality will be missed.

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

Use of UNKNOWN here seems sloppy, since ID properties only support a limited number of types, we should be able to cover all possible cases.

If a truely unexpected type is encountered, this should assert.

1463

This looks incorrect, 0, "" or [] will return unknown here.

Whats intended by this check? if the property doesn't exist, it would have raised an exception, if it does exist it will be a known type that ID-properties support.

This revision now requires changes to proceed.Sep 10 2021, 6:08 AM
Hans Goudey (HooglyBoogly) marked 2 inline comments as done.Sep 11 2021, 11:10 PM

I also like the idea of not supporting editing values as Python literals, for a few reasons:

  • It seems like the proper place to edit them is the python console editor.
  • It would be the only type where the actual value was edited in the popup, rather than the metadata. (Group properties do not support UI data at all).
  • If there is a commonly needed Python literal property, it could become an actual property type with a dedicated widget for editing, rather than a hack.

But yeah, maybe it's used a lot and would be missed enough that it's worth making it work somehow.

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

Yeah, the word "UNKNOWN" was bad in this case. I changed it to "UNSUPPORTED" for now, which I think is much clearer.

1463

Good point, I removed it.

Hans Goudey (HooglyBoogly) marked 2 inline comments as done.
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
  • Adjust order of soft min and max in panel
  • Merge branch 'master' into refactor-custom-property-edit
  • Remove bad "None" check, rename "UNKNOWN" to "UNSUPPORTED"
Campbell Barton (campbellbarton) requested changes to this revision.EditedSep 13 2021, 12:51 PM

In general, am not so keen on automatic string conversion, at least it should be explicit, not something that could happen unintentionally / automatically.
Perhaps it's better to disable editing entirely, users can always delete and create new properties if they want.
Or have a button in the UI to convert the property into a string.
Otherwise users might accidentally change the type of properties used by add-ons for example.

Having said this, I wonder if the hassle of properly managing string conversion is worth it, compared to not supporting it - or using evaluated Python.

Run in the console, then edit the property:

  • C.object["prop"] = [].

    This converts the array into a string even if the UI is cancelled.
  • C.object["prop"] = [1].

    This raises an error: File "scripts/startup/bl_operators/wm.py", line 1491, in _convert_new_value_array
  • C.object["prop"] = [""].

    This shows the property editor with "String" type (with no hint it's not a string), pressing OK shows a warning: Property type does not support editing, converting to string

Another strange issue I ran into is the array size getting out of sync:

  • Add a property, set it to float-array with a length of 32.
  • In the python console run: C.object["prop"] = [1.0, 1.0, 1.0]
  • Now open the UI and the length still says 32.

Noticed vectors over 4 in length aren't editable, although their defaults can be edited.

This revision now requires changes to proceed.Sep 13 2021, 12:51 PM

That for the review. Generally I agree about the string conversion.
I changed the operator to have a "Python" mode, which allows editing python values directly as a string, using eval to store the property again.
This works with the conversions from other property types too, so a nice side effect is that you can convert freely between the types.

The other issues you mentioned should be fixed.

Noticed vectors over 4 in length aren't editable, although their defaults can be edited.

Yes, and I'd like to change that in the UI outside of this operator. We should be able to draw larger vectors.
For very long vectors, we could allow collapsing them in the UI somehow. But I do really like that the edit operator is just for the UI data, not the value itself. That fact also simplifies some logic.

  • Merge branch 'master' into refactor-custom-property-edit
  • Fix object['prop'] = [1] case
  • Load array_length operator property from IDProperty
  • Properly support a "Python" edit type
Campbell Barton (campbellbarton) requested changes to this revision.Sep 14 2021, 2:44 AM

Generally working well.

Noticed an issue where using an invalid Python type shows an exception and deletes the original property.

  • Create a new property.
  • Edit it, set the type to Python.
  • use the value set()
  • Press OK.
Error: Python: Traceback (most recent call last):
  File "/src/cmake_debug/bin/3.0/scripts/startup/bl_operators/wm.py", line 1680, in execute
    item[name] = new_value
TypeError: invalid id-property type set not supported

The property is removed.

Doing the same operation in the Python console doesn't remove the existing property, so the operator should be able to keep the current property if there is an error setting it.

Solving this should also handle similar cases, such as integer overflow which happens attempting to set values such as: [1<<128].

Since assigning Python values are more likely to fail, this should be handled gracefully (made into a report instead of raising a Python exception), as well as keeping the existing property unchanged.

This revision now requires changes to proceed.Sep 14 2021, 2:44 AM
  • Merge branch 'master' into refactor-custom-property-edit
  • Add error for assigning unsuppored custom propery type like set()

Good point! This version should behave better in those cases. It's actually an improvement to the code too.

Campbell Barton (campbellbarton) requested changes to this revision.Sep 16 2021, 4:36 AM
Campbell Barton (campbellbarton) added inline comments.
release/scripts/startup/bl_operators/wm.py
1410–1414

"Unsupported type" is awkward/misleading term.

Perhaps note this is a fallback for otherwise unsupported types.

1557

Think it would be better to raise an error if the string cant be evaluated instead of coercing into a string.

At the very least the error should be shown to the user so they can tell what was wrong.


It's probably simpler to inline this function since it becomes more involved if the caller needs to distinguish between an error result and a Python value.

1670–1673

As this might not be a type error (as mentioned before, integers might be too large). If the error is hidden it will be difficult for the user to troubleshoot.

This revision now requires changes to proceed.Sep 16 2021, 4:36 AM

Played around with the patch, this is something I've wanted to have for a long time, I'm really glad you're working on it! Here's some stuff I've run into:

  • Float properties seem to have wrong sub-types (Float Array subtypes). I would expect the options percentage, angle, factor, and whatever else you think is worth/possible to support :D.
  • Operator seems to remember min/max from a previous call to it, even if this time I'm editing a different property.
  • Min/Max doesn't seem to actually be applying to the properties.
  • Array properties with >5 elements can't be edited through the UI.*
  • Editing a datablock pointer created with Python will convert it to a string. (not possible in master either though)

*: I absolutely love the idea of separating the concepts of editing a property's definition versus editing its value. Would it be possible to have two edit operators for such cases? "Edit Settings" vs "Edit Value". This could also be useful for maintaining this clear split for "Python" type properties.

Feature request territorry:

  • Any chance for a string sub-type selector? :) I don't have a use case, but I tried DIRPATH and FILEPATH through the pyConsole and it just works!
  • I wish datablocks could be selected (C.object['text'] = bpy.data.texts['Text']), but I think that would mean doubling the size of the operator + a lot of pain.
  • I wish the custom properties list could be displayed with a UIList, but I assume that would be another major refactor with lots of implications so nvm. ;)
Hans Goudey (HooglyBoogly) marked 3 inline comments as done.EditedSep 22 2021, 4:33 AM

Thanks for the comments, sorry about the delay responding.

Float properties seem to have wrong sub-types (Float Array subtypes). I would expect the options percentage, angle, factor, and whatever else you think is worth/possible to support :D.
Any chance for a string sub-type selector? :) I don't have a use case, but I tried DIRPATH and FILEPATH through the pyConsole and it just works!

I think I'd like to postpone adding different subtypes for different property types to a separate patch. It shouldn't be pretty simple, but it just adds a bit more complexity than I'd like to add to this patch.

Operator seems to remember min/max from a previous call to it, even if this time I'm editing a different property.
Min/Max doesn't seem to actually be applying to the properties.

I can't reproduce these, do I have to do anything special?

Array properties with >5 elements can't be edited through the UI.*

It could make sense to add an "edit value" operator for those properties, though it seems like we could also display array properties with <10 elements or so in the UI directly.
Since this is not really new behavior (you can still edit them the same way as before with the "Python" mode), I'd prefer to approach that as a separate step.

Editing a datablock pointer created with Python will convert it to a string. (not possible in master either though)
I wish datablocks could be selected (C.object['text'] = bpy.data.texts['Text']), but I think that would mean doubling the size of the operator + a lot of pain.

We need a bit more internal handling for these custom property types before exposing them properly here IMO

  • Merge branch 'master' into refactor-custom-property-edit
  • Fix issues mentioned by Campbell and Demeter

Minor request inline otherwise LGTM.

release/scripts/startup/bl_operators/wm.py
1675–1676
This revision is now accepted and ready to land.Sep 22 2021, 7:06 AM

Float properties seem to have wrong sub-types (Float Array subtypes). I would expect the options percentage, angle, factor, and whatever else you think is worth/possible to support :D.

I think I'd like to postpone adding different subtypes for different property types to a separate patch. It shouldn't be pretty simple, but it just adds a bit more complexity than I'd like to add to this patch.

That's fair, but then I think in the meanwhile the Subtype drop-down shouldn't appear for single Float values, since afaict the available options don't make sense (Quaternion Rotation and Gamma Corrected Color as a single float)

Operator seems to remember min/max from a previous call to it, even if this time I'm editing a different property.
Min/Max doesn't seem to actually be applying to the properties.

I can't reproduce these, do I have to do anything special?

I can't either anymore!

That's fair, but then I think in the meanwhile the Subtype drop-down shouldn't appear for single Float values, since afaict the available options don't make sense (Quaternion Rotation and Gamma Corrected Color as a single float)

Ah right, should have mentioned that I did that in the last update.

It's still showing the wrong Float subtypes on my end, possible that I'm doing something wrong or my arcanist is failing:

Thanks for checking @Demeter Dzadik (Mets), turns out I'm just dumb and wrote == instead of !=!

Hans Goudey (HooglyBoogly) marked an inline comment as done.Sep 23 2021, 4:58 AM

I've added improving the UI of the custom properties panel and exposing more subtypes to my list of things to do relatively soon.