Indicate Parent Inverse Matrix State in UI #113364
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
7 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#113364
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "Andrej730/blender:clear_inverse_poll"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
There is a problem that parent inverse matrix is not exposed in any way in UI and it's not possible to tell whether it's set or not without looking up
obj.matrix_parent_inverse
in Python API.Was following the discussion on devtalk leading to some notes from Animation & Rigging Module Meeting that concluded that it could be less confusing if there was some way to indicate whether parent inverse matrix is set or not.
In this PR I've added the indication for both constraints and object's inverse matrices.
For constraints operators (
constraint.childof_clear_inverse
,constraint.objectsolver_clear_inverse
) I've changed poll methods to check if parent is set and inverse matrix is present.Buttons with these operators in constraints UI will appear disabled and give a poll message (e.g No inverse correction is set, so there is nothing to clear) to indicate that there is a inverse matrix to clear.
Inverse matrix poll check works only if there is
context.constraint
so it won't interrupt other workflows.Panel breakdowns parent inverse matrix to location/rotation/scale components and warns if matrix has a shear (which means it cannot be completely represented using location/rotation/scale). Properties displayed as Blender custom properties to make them more readable and it's also possible to copy them with Ctrl+C/Ctrl+V.
All properties are read-only besides rotation mode - changing rotation mode recalculates displayed rotation (useful to represent rotation in more readable way).
Absolutely, and I'm glad you're looking into this.
I've edited your PR description so that the screenshot is embedded, making it easier to see.
I'm not sure that this is the right way to go. As you've seen, the poll function is quite limited, as it doesn't have access to the operator properties. Expanding the
context
for this is dubious IMO.I think an explicit indicator in the transform properties panel would be better. That way operators can be as they are, and the info is still visible somewhere.
Any ideas how it would work?
Initially I had an idea that there could be some icon appearing in Transform next to
parent
property, if parent inverse matrix is set. But the problem with that approach is that it seemed not too descriptive from user's point of view - there are no tooltips when you hover overcol.label(text="", icon="icon")
and user wouldn't know what the indicator means. This is why I chosen operator's poll approach - there is a tooltip describing when inverse is not set, so it will be more intuitive.Another option is add some label that explicitly says "Parent Inverse is set / is not set".
This may sound harsh, but that's up to you to spend time on. Basically: you picked this topic to work on, so it's now yours to work on and come up with a nice design, that focuses on comfort & clarity for Blender users.
After giving it more thought I've updated the indicator to just use
UILayout.enabled
instead of this a bit obscureUILayour.context_pointer_set
-way.So, now the operator just appears disabled if there is no parent / no parent inverse matrix set which should pretty straightforward from user's POV: Clear Parent Inverse is disabled => No parent inverse to clear out. It should also make sense since the indicator is explicitly connected to the operator that's clearing out the correction.
Another way I've considered is to add a label that says "Parent inverse correction is set" if matrix is not identity. However, this approach is taking an entire row and after awhile just gets in the way. Using enabled/disabled operator is less visually intrusive and you can see from the glance wheither correction is set. Adding an icon without operator as an indicator could pose an issue as it might lack a tooltip, and users wouldn't know what indicator is connected to.
For constraints "Clear Inverse" operators we're already passing
constraint
from the UI so it makes sense to just utilize it in thepoll
methods - that way we won't need to add anything new to UI and users get clear description what disabledClear Inverse
means.Regardless of whatever other visualisation we may use, I think this is a very good idea.
The image is broken, could you upload it again?
That looks good! Maybe add "so there is nothing to clear" to the end of the sentence in red. That way it clarifies that this is not an error situation, but rather a "this button prevents itself from being useless" situation.
Sure, changed the poll messages to be more clear
@dr.sybren Hi! Anything to add or looks good?
Could you update the PR description so that it gives a complete view of what's implemented in the PR? That way people can read the description only and understand it, without having to read through all the comments.
@dr.sybren sure, updated the PR description.
I recently had to work with a complex asset with lots of object parenting, and I want to propose the "Clear Inverse Matrix" button to also Keep Transforms, as I think this would be most commonly desired. This would change the object's local transforms, and it would fail if those are driven, which I think is still fine.
I know the current Alt+P menu doesn't have an option to "Clear Inverse Matrix and Keep Transforms", which I find a bit odd, donno what else to say, to me it's a missing feature.
This was discussed in last week's module meeting. The conclusions were that it's a nice PR and a good step forward. In order to land it, a GUI change is needed. Instead of having just a button next to the Parent property, there are two alternatives to go for. It's up to you @Andrej730 how far you want to take this, either choice would be fine for this PR:
Option 1: on a line by itself
Option 2: new sub-panel of the Transform panel, underneath Delta Transform
Option 2 is a bit more work, but also the more useful-for-artists one of the two.
To reply to @Mets :
That's a good idea. For a different PR, though, as that would be a new feature, rather than UI clarification for the current design.
In general it wouldn't be able to keep the transforms exactly the same, as the parent inverse matrix (PIM) can remove the parent's sheer. That transform can't be decomposed into the loc/rot/scale properties of the child. Blender can still just try to do its best, and the feature will still be useful. A note about this limitation should be included in the documentation for that feature, though. Of course a new PIM can be constructed that contains the "remainder" of the old matrix, such that the child's final transform doesn't change at all, but that would mean that after pressing the button to clear the PIM actually might not.
This to me makes it even more clear that this should be a separate design task that first nails this design, before starting implementation. It would fit well with Option 2 above :)
Fair enough. :)
I like your option 2, exposing the parent inverse matrix as read-only decomposed loc/rot/scale values.
But do we want the same for constraints? I'm not against it, but it does admittedly add a bit of UI clutter.
Also, I believe Hook modifiers also store an inverse matrix, which is updated whenever a new target is set, and it doesn't have Set/Clear buttons like the Child Of constraint has. There may be other modifiers too, but I don't think so.
\o/
I think the modifiers & constraints should get a design task (and thus subsequent PR) on their own. So far the discussions in the module meetings have been about the Parent Inverse matrix. IMO it's the "worst offender", as even the existence of this matrix is not acknowledged by the UI (apart from the Ctrl/Alt+P menus, but those are only temporarily visible).
That might need another design pass as well then...
@dr.sybren I've created a panel for the parent inverse transform, can you please take a look? Then I'd add similar tabs to the constraints UI. It is heavily inspired by transform helper addon 😁
There is an option to make it look better by using native Blender props - like creating PropertyGroup
parent_inverse_transform
with attributeslocation
,rotation
andscale
attributes that would be read only and add this attributeparent_inverse_transform
for allbpy.types.Object
. But not sure how reasonable that would be to add new props for this, from Blender POV.Having it as properties might be a bit more code but it has several benefits, like being able to easily copy values to clipboard, and seeing the properties with the approprieate units (degree symbol, "m" to indicate meters or whatever other unit is set in the scene settings).
I wish Blender had better UX for read-only properties. Right now you either have to grey them out, or user is able to type in a number, only to have it not go through when they press enter. Ideally, we would have something in between, where you can't click it, but it's not grayed out. But that's not a burden for this patch.
Nice work @Andrej730 !
I agree with @Mets that having a bit more code here to show as actual Blender properties would be nice indeed.
The properties are now shown as editable, and you can even edit them. However, any changes are immediately reverted.
I don't think this is a good idea, as the properties pretend to be something they're not. The lock icon behind the property doesn't convey this to me, but rather confuses until you actually try to edit things and fail. Again, these icons are used in a different way in other areas of Blender's UI (there they are toggles, and not just indicators/warnings), so we shouldn't use them like this. @Andrej730 could you try deactivating the UI layout these properties are drawn in? I think that might be enough to get this to work nicer. Maybe also look at the UILayout.emboss property to make them look less "for editing" and more "for display".
For the rotation mode, I think this should implicitly use the object's rotation mode.
As for the order of the sub-panels, I'm not sure if there is any "correct" order. AFAIK the matrices are handled in the order "parent inverse", "transform", "delta transform", and to clarify their relationship it would be good to also show them in that order. Doing that would move the regular transform into a sub-panel too, which I don't think is good. Or it would make it "subpanel, main content, another subpanel", which I don't think Blender even supports. And it would still look weird.
We could try and swap the Delta Transform and Parent Inverse sub-panels. @Mets @ChrisLend @nathanvegdahl do you have any opinions on this?
From a bit of testing, it looks like "transform" and "delta transform" are not combined with a simple matrix multiplication. For example, neither of their rotations affect the translation axes of either, whereas that would be the case with a full matrix multiplication. Similarly, you can't achieve shear, which you would be able to with matrix multiplication.
As far as I can tell, they're being combined separately for translation, rotation, and scale.
Given that that's the case, I'd say we can consider "transform" and "delta transform" as intertwined in some sense. So keeping them next to each other is probably best. Which then implies putting the parent inverse either first or last. And I think last makes more sense.
There's two options you can use, one is
row.active
and the other isrow.enabled
. I know, it's not that clearly described what these do. AFAIK they both have the same darkening effect, whererow.active = False
will still allow interaction with the properties.I think
PULLDOWN_MENU
also looks quite good. But IMO these should be checked withrow.active = False
to see how they behave then. Once we have something we like, we should also check with the UI module, I'm sure they'll have ideas as well.Then maybe just use the object rotation mode when it's euler, and switch to XYZ euler when the object uses quaternion or axis/angle? I think it's ok to always display the rotation component of a matrix as euler.
Hmmm that's a good point. Still, IMO the rotation mode should be shown above the rotation components, as the mode determines what values are shown & what they mean. This is different from the object properties, where changing the mode changes the meaning of the components, but not their values.
Also maybe we should then change these options altogether, and have options
That would make it clearer why there is a choice here anyway. But then again, it would limit to those options, instead of exposing all of them.
About the ordering of the panels: I actually agree with @nathanvegdahl here, and so the current ordering (transform, delta, parent inverse) is fine to keep.
ad8cb537af
5ab63e76df9c5abddc77
527f7c9cb1row.active = False
looks very good and allows copying values, it seems to look fine even without a bit confusingemboss
:oh, makes total sense to indicate that values depend on the rotation mode
Thanks for giving that a try @Andrej730
@pablovazquez @JulianEisel We need your UI superpowers. TL;DR: we want to show some derived values (location/rotation/scale components of a matrix) and ideally have those copy-pastable, but without users actually able to modify those values.
layout.enabled = False
removes the ability to modify the values, but also blocks any interaction, and values cannot be copy-pasted.layout.active = False
shows the UI dimmed, but still allows modification.layout.emboss
can be used to futher make the UI look more 'read-only-ish', but because the values can still be modified, it's still kind of confusing.So far
layout.active = False
seems to be the best one, but I'm still not quite happy with it as it also dims everything, making it hard to read. To me it also suggests that this feature may become active once you find the toggle switch, but that switch doesn't exist.layout.enabled = False
still allows copying values, we just don't expose that in the context menu (we never do). So I'd prefer using that since it avoids the "illusion" of being modifiable.@ -1832,0 +1833,4 @@
{
for (int row = 0; row < 4; row++) {
for (int col = 0; col < 4; col++) {
if (m[row][col] != ((row == col) ? 1.0f : 0.0f))
Always use braces for if-statements, this can avoid critical bugs: https://developer.blender.org/docs/handbook/guidelines/c_cpp/#always-use-braces
@ -1020,1 +1020,4 @@
static bool childof_clear_inverse_poll(bContext *C)
{
if (!edit_constraint_liboverride_allowed_poll(C))
Braces.
@ -1021,0 +1025,4 @@
PointerRNA ptr = CTX_data_pointer_get_type(C, "constraint", &RNA_Constraint);
bConstraint *con = static_cast<bConstraint *>(ptr.data);
if (con == nullptr)
Braces.
@ -1278,1 +1301,4 @@
static bool objectsolver_clear_inverse_poll(bContext *C)
{
if (!edit_constraint_poll(C))
Braces.
@ -1279,0 +1306,4 @@
PointerRNA ptr = CTX_data_pointer_get_type(C, "constraint", &RNA_Constraint);
bConstraint *con = static_cast<bConstraint *>(ptr.data);
if (con == nullptr)
Braces.
That's a bit unintuitive since you can't select the text and there is no context menu option about copying the prop value but that option looks best so far, since
layout.active
is indeed confusingly indicating that prop is editable.Thanks @JulianEisel . I agree that this is the best solution.
So, this UI will work? Then I'll start working on adding similar subpanels for constraints. In case of constraints I guess it won't be possible to simply create properties from UI python scripts (adding custom properties from python is available only for IDs, Bone and PoseBone) and will require adding them to
ChildOfConstraint
andObjectSolverConstraint
inrna_constraint.cc
.I think the "clear" button shouldn't have an icon. The one that's used now is also used in other places as a "back" icon rather than a "clear"/"erase" icon, so we certainly shouldn't use this one.
Apart from that I think the UI is good. I'd propose to keep this PR limited to the object's parent inverse matrix. Once it's landed, people can actually use it and no doubt will have more feedback. Once the feature is polished, then I think it's a good time to expand it into other areas of Blender. Especially those that have been made in C++, as that's harder to work with and thus iterating on different ideas is going to be a bit more cumbersome.
Removed the icon.
Sure, let's keep it for object transform only - I think users indeed will notice similarities and will ask for the similar panel in constraints or other UI changes if needed. For constraints atleast now there will be an indication whether inverse is set or not which was the most confusing part from my experience.
@Andrej730 could you edit the PR description so that it's up to date with the current state of the PR, including screenshots? That'll help when landing in
main
and also when writing the release notes.@dr.sybren updated the PR description.
@blender-bot package
Package build started. Download here when ready.
👍 the PR seems to work well, at least for the object parent inverse matrix. I haven't gotten around to test the constraints yet. I did find some things in the code to adjust, though.
@ -411,10 +454,54 @@ class OBJECT_PT_custom_props(ObjectButtonsPanel, PropertyPanel, Panel):
_property_type = bpy.types.Object
class TransformAttributes(PropertyGroup):
Add a docstring that explains what this class is and what it's for.
Maybe
ReadOnlyMatrixDecomposition
is a better name? At least it's less genereric, and conveys the fact that it's read-only.@ -414,0 +458,4 @@
def set_read_only_prop(self, value):
pass
def get_matrix_components(self) -> Tuple[Vector, Quaternion, Vector]:
Prefix the function names with an
_
so that they're marked as 'private'.@ -414,0 +462,4 @@
return self.id_data.matrix_parent_inverse.decompose()
def get_location(self):
return self.get_matrix_components()[0]
Instead of decomposing the matrix three times, I think it's clearer to use the dedicated
Matrix.to_…()
functions. So for example:@ -414,0 +470,4 @@
def get_scale(self):
return self.get_matrix_components()[2]
# we implement only euler rotation as it should cover most of the cases
I feel that "it should cover most of the cases" is not a very good argument here. For one, it doesn't even explain what "the cases" are.
I think what you mean is that people generally feel Euler angles are easier to understand when looking at the numbers, and that the presentation is thus limited to those. I think that's fine, so please update the comment for this.
Also comments should be full sentences, so start with a capital letter and end with a period.
@ -414,0 +481,4 @@
)
location: FloatVectorProperty(
name="Location", get=get_location, set=set_read_only_prop, options=set(), subtype="TRANSLATION", precision=5
Use
set=None
to create a read-only property. Theset_read_only_prop
can be removed.The
options=set()
parameter is unnecessary.with
set=None
adding.enabled = False
to UI elements turn out also to be unnecessary.@ -429,2 +517,4 @@
)
def register_props():
bpy.types.Object.parent_inverse_transform = PointerProperty(type=TransformAttributes)
Add a comment that explains why this property is added.
@ -1021,0 +1027,4 @@
PointerRNA ptr = CTX_data_pointer_get_type(C, "constraint", &RNA_Constraint);
bConstraint *con = static_cast<bConstraint *>(ptr.data);
if (con == nullptr) {
return true;
If a constraint could not be found, why is it ok to run the operator? In other words, shouldn't this return
false
?I've left it returning
true
since inchildof_set_inverse_exec
it's also possible to provide constraint without settingconstraint
in context and didn't wanted to interrupt that workflow if someone would be using it.Coming back to this later, I think it'll be good to explain this in a comment.
@ -1021,0 +1032,4 @@
bChildOfConstraint *data = (bChildOfConstraint *)con->data;
if (data->tar == nullptr) {
CTX_wm_operator_poll_msg_set(C, "No target object set, so there is nothing to clear");
Why is it relevant whether there is a target object set or not? Wouldn't it make sense to clear the matrix anyway, just so that later an object can be set without the constraint being influenced by an old matrix value?
@ -1279,0 +1310,4 @@
PointerRNA ptr = CTX_data_pointer_get_type(C, "constraint", &RNA_Constraint);
bConstraint *con = static_cast<bConstraint *>(ptr.data);
if (con == nullptr) {
return true;
Same as above, I think this should
return false
.@ -1279,0 +1315,4 @@
bObjectSolverConstraint *data = (bObjectSolverConstraint *)con->data;
if (data->camera == nullptr) {
CTX_wm_operator_poll_msg_set(C, "No camera set, so there is nothing to clear");
Same as above, why is it relevant whether there is a camera set or not?
Things are shaping up nicely!
There's one thing that I'd like a 2nd opinion on, the rest are just minor details.
@ -110,0 +127,4 @@
inverse_matrix = ob.matrix_parent_inverse
inverse_props = ob.parent_inverse_transform
# we use 3x3 matrix because we don't care about translation-shear
This is not so much about "not caring", it is actually incorrect to use the 4x4 matrix here. Something like this would be better:
@ -110,0 +136,4 @@
layout.prop(inverse_props, "rotation_euler", text="Rotation")
layout.prop(inverse_props, "scale")
op = layout.operator("object.parent_clear", text="Clear Parent Inverse Transform")
Usually the properties are stored in a variable named
props
.@ -414,0 +447,4 @@
class ReadOnlyMatrixDecomposition(PropertyGroup):
"""An utility property group for bpy.types.Object
for accessing read-only parent inverse matrix decomposition properties,
such as location, rotation and scale."""
Adhere to PEP 257 for docstrings.
@ -414,0 +448,4 @@
"""An utility property group for bpy.types.Object
for accessing read-only parent inverse matrix decomposition properties,
such as location, rotation and scale."""
def _get_matrix(self) -> Matrix:
Since this is a simple getter, better to make it a read-only property:
The other functions are used as 'getter callback' functions, so they have to stay as they are. By making this one a
@property
it also clarifies that it's different than the other_get_...()
functions.@ -430,1 +508,4 @@
def register_props():
# Property is used to present parent inverse matrix in UI as it's decomposed properties.
bpy.types.Object.parent_inverse_transform = PointerProperty(type=ReadOnlyMatrixDecomposition)
@ideasman42 I'd value your opinion on the approach taken here. To make the Blender GUI framework work for us, I think this is the right approach. However, I'm not too fond of having the
Object.parent_inverse_transform
property available to any other script. What do you think, would it be enough to prefix the property with an_
? Or do you think there's a different approach?Agree
_
prefix seems better than making it a public RNA property, although as there is no intention of animating, accessing this from Python ... etc, we could use a UI template and avoid RNA altogether. Noted in reply.@ -1832,0 +1833,4 @@
{
for (int row = 0; row < 4; row++) {
for (int col = 0; col < 4; col++) {
if (m[row][col] != ((row == col) ? 1.0f : 0.0f)) {
No need for
()
around(row == col)
.Implementing this via RNA seems a bit heavy with the definition, callbacks ... etc, I think it's worth considering adding a UI-template that can show these read-only properties for any transformation matrix. Was this considered?
Then it can be used for any matrix property in a single line, any scripts can use it to display matrix information.
The way this is implemented only works for one kind of matrix.
@ -414,0 +479,4 @@
get=_get_rotation_euler,
set=None,
subtype="EULER",
precision=5,
Is it really needed for every object to store its own
rotation_mode
? Adding custom-properties to objects, just for a display feature. Would it be acceptable to have this as a run-time only property?That sounds like a good approach. I didn't consider it, mostly because I'm so unfamilar with the C/C++ UI code (and, to be frank, a bit scared of it).
@Andrej730 would you be up for this? The relevant code can be found in
source/blender/editors/interface/templates
.Checkout
From your project repository, check out a new branch and test the changes.