Indicate Parent Inverse Matrix State in UI #113364

Open
Andrej wants to merge 21 commits from Andrej730/blender:clear_inverse_poll into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Contributor

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.

image

  • For object's parent I've added new Parent Inverse Transform section in object's transform properties - it appears if object has a parent. It has a "Clear Parent Inverse Transform" button to explicitly clear it.

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

image

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 ](https://devtalk.blender.org/t/reevaluating-worldspace-and-localspace-treatment-need-some-dev-triage/9665) leading to some notes from [Animation & Rigging Module Meeting](https://devtalk.blender.org/t/2021-11-25-animation-rigging-module-meeting/21536) 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. ![image](/attachments/af20904e-9a83-4e5e-9165-cbfa84910e94) - For object's parent I've added new Parent Inverse Transform section in object's transform properties - it appears if object has a parent. It has a "Clear Parent Inverse Transform" button to explicitly clear it. 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). ![image](/attachments/37853195-7b0f-4a45-939d-8795a7eabedb)
Andrej added 4 commits 2023-10-06 21:11:32 +02:00
3421c0bc3d Parent Clear poll to check parent inverse in set
Since they are multiple uses for parent clear operator, It checks parent inverse only when it's called from Relations section from Object properties. It's passing object as context attribute `object_check_parent_inverse` so that poll check will occur only in this case and won't interrupt other workflows.
Andrej requested review from Sybren A. Stüvel 2023-10-06 21:12:22 +02:00

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

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.

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.

I did it in this PR by checking it in poll for object.parent_clear(type='CLEAR_INVERSE'), constraint.childof_clear_inverse, constraint.objectsolver_clear_inverse operators when they are called from UI so it won't interrupt other workflows:

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.

> 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 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. > Was following the [discussion on devtalk ](https://devtalk.blender.org/t/reevaluating-worldspace-and-localspace-treatment-need-some-dev-triage/9665) leading to some notes from [Animation & Rigging Module Meeting](https://devtalk.blender.org/t/2021-11-25-animation-rigging-module-meeting/21536) that concluded that it could be less confusing if there was some way to indicate whether parent inverse matrix is set or not. > > I did it in this PR by checking it in poll for `object.parent_clear(type='CLEAR_INVERSE')`, `constraint.childof_clear_inverse`, `constraint.objectsolver_clear_inverse` operators when they are called from UI so it won't interrupt other workflows: 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.
Sybren A. Stüvel added the
Module
Animation & Rigging
label 2023-10-09 12:34:59 +02:00
Author
Contributor

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 over col.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".

> 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 over `col.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".

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?

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.

> > 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? 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.
Andrej added 1 commit 2023-10-20 17:51:20 +02:00
Author
Contributor

After giving it more thought I've updated the indicator to just use UILayout.enabled instead of this a bit obscure UILayour.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.

image

For constraints "Clear Inverse" operators we're already passing constraint from the UI so it makes sense to just utilize it in the poll methods - that way we won't need to add anything new to UI and users get clear description what disabled Clear Inverse means.

image

After giving it more thought I've updated the indicator to just use `UILayout.enabled` instead of this a bit obscure `UILayour.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. ![image](/attachments/07b50537-b266-428f-9f26-89b8a80ee4b8) For constraints "Clear Inverse" operators we're already passing `constraint` from the UI so it makes sense to just utilize it in the `poll` methods - that way we won't need to add anything new to UI and users get clear description what disabled `Clear Inverse` means. ![image](/attachments/844f24c5-a14b-4333-aa84-6a63f8cbd33f)

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.

Regardless of whatever other visualisation we may use, I think this is a very good idea.

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.

image

The image is broken, could you upload it again?

For constraints "Clear Inverse" operators we're already passing constraint from the UI so it makes sense to just utilize it in the poll methods - that way we won't need to add anything new to UI and users get clear description what disabled Clear Inverse means.

image

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.

> 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. Regardless of whatever other visualisation we may use, I think this is a very good idea. > 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. > > ![image](/attachments/07b50537-b266-428f-9f26-89b8a80ee4b8) The image is broken, could you upload it again? > For constraints "Clear Inverse" operators we're already passing `constraint` from the UI so it makes sense to just utilize it in the `poll` methods - that way we won't need to add anything new to UI and users get clear description what disabled `Clear Inverse` means. > > ![image](/attachments/844f24c5-a14b-4333-aa84-6a63f8cbd33f) 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.
Andrej added 1 commit 2023-10-23 16:29:59 +02:00
Author
Contributor

The image is broken, could you upload it again?

Screenshot 2023-10-23 192901.png

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

Screenshot 2023-10-23 192721.png

> The image is broken, could you upload it again? ![Screenshot 2023-10-23 192901.png](/attachments/5422e7c4-5c8c-44ad-9d09-d19eec8ec092) > 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 ![Screenshot 2023-10-23 192721.png](/attachments/b4b03f60-9539-4c79-b281-c1a9301b1fc9)
Author
Contributor

@dr.sybren Hi! Anything to add or looks good?

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

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

@dr.sybren sure, updated the PR description.

@dr.sybren sure, updated the PR description.
Member

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.

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

  • Move the button, from behind the 'Parent' property to a line of its own.
  • It can then get a proper label, left of the button, that shows it's about the Parent Inverse matrix, and the button can be labeled as well.

Option 2: new sub-panel of the Transform panel, underneath Delta Transform

  • Add a sub-panel "Parent Inverse Matrix"
  • Decompose the matrix into location / rotation / scale components, and show these as read-only fields.
  • Show a warning when the matrix has shear.
  • Add a button to clear the matrix.

Option 2 is a bit more work, but also the more useful-for-artists one of the two.


To reply to @Mets :

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.

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.

This would change the object's local transforms, and it would fail if those are driven, which I think is still fine.

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

This was discussed in [last week's module meeting](https://devtalk.blender.org/t/2023-12-07-animation-rigging-module-meeting/32422). 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** - Move the button, from behind the 'Parent' property to a line of its own. - It can then get a proper label, left of the button, that shows it's about the Parent Inverse matrix, and the button can be labeled as well. **Option 2: new sub-panel of the Transform panel, underneath Delta Transform** - Add a sub-panel "Parent Inverse Matrix" - Decompose the matrix into location / rotation / scale components, and show these as read-only fields. - Show a warning when the matrix has shear. - Add a button to clear the matrix. Option 2 is a bit more work, but also the more useful-for-artists one of the two. -------------- To reply to @Mets : > 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. 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. > This would change the object's local transforms, and it would fail if those are driven, which I think is still fine. 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 :)
Member

That's a good idea. For a different PR, though

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.

> That's a good idea. For a different PR, though 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.

I like your option 2, exposing the parent inverse matrix as read-only decomposed loc/rot/scale values.

\o/

But do we want the same for constraints? I'm not against it, but it does admittedly add a bit of UI clutter.

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

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.

That might need another design pass as well then...

> I like your option 2, exposing the parent inverse matrix as read-only decomposed loc/rot/scale values. \o/ > But do we want the same for constraints? I'm not against it, but it does admittedly add a bit of UI clutter. 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). > 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. That might need another design pass as well then...
Andrej added 2 commits 2024-01-29 18:25:20 +01:00
Author
Contributor

image
@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 attributes location, rotation and scale attributes that would be read only and add this attribute parent_inverse_transform for all bpy.types.Object. But not sure how reasonable that would be to add new props for this, from Blender POV.
image

![image](/attachments/f1374657-4a54-4263-96e1-dae55bb096b2) @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 attributes `location`, `rotation` and `scale` attributes that would be read only and add this attribute `parent_inverse_transform` for all `bpy.types.Object`. But not sure how reasonable that would be to add new props for this, from Blender POV. ![image](/attachments/02b24934-1092-4227-be7d-fd56ed66ada4)
Member

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.

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.

Nice work @Andrej730 ! I agree with @Mets that having a bit more code here to show as actual Blender properties would be nice indeed.
Andrej added 1 commit 2024-02-06 19:40:42 +01:00
Sybren A. Stüvel requested changes 2024-02-08 11:27:57 +01:00
Sybren A. Stüvel left a comment
Member

The properties are now shown as editable, and you can even edit them. However, any changes are immediately reverted.

image

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?

The properties are now shown as editable, and you can even edit them. However, any changes are immediately reverted. ![image](/attachments/252ed7a6-9220-4def-8d2b-381837fafc0b) 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](https://docs.blender.org/api/current/bpy.types.UILayout.html#bpy.types.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?
Member

AFAIK the matrices are handled in the order "parent inverse", "transform", "delta transform",

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.

> AFAIK the matrices are handled in the order "parent inverse", "transform", "delta transform", 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.
Andrej added 2 commits 2024-02-08 16:20:15 +01:00
Author
Contributor

could you try deactivating the UI layout these properties are drawn in?

I think using something like row.enabled = False would be the most straightforward solution but the problem is that after you cannot copy values anymore 🙁
image

Maybe also look at the UILayout.emboss property to make them look less "for editing" and more "for display".

emboss = None pretty much conveys that those properties are read-only and personally would work for me. Though it could look a bit as a broken UI since it kind of looks just like bunch of labels at first and then on hover it seems like usual props but except there's something off (maybe it's because haven't seen this anywhere else in Blender UI).

For the rotation mode, I think this should implicitly use the object's rotation mode.

I wanted to avoid creating more fields for quaternion and axis+angle as it's unlikely that it will be used. And some small benefit of having separate rotation mode is that you can set it to the reverse of the parent's rotation mode to make sense what rotation parent matrix is trying to undo. Otherwise displaying rotation probably will be useless most of the times or would require toggling back and forth if we connect it to object's rotation mode.

image

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.

Done some experimenting too and it seems that delta transform is applied first the object's transform, so it makes sense to keep them close.

import numpy as np
import bpy
from mathutils import Matrix
from math import pi

l = Matrix.Rotation(pi/4, 4, 'Z')
d = Matrix.Rotation(pi/4, 4, 'Y')
p = Matrix.Rotation(pi/4, 4, 'X')

obj = bpy.context.object
obj.rotation_euler = l.to_euler()
obj.delta_rotation_euler = d.to_euler()
obj.matrix_parent_inverse = p

bpy.context.view_layer.update()
print(np.allclose(obj.matrix_world, p @ (d @ l), atol=1e-3)) # True
print(np.allclose(obj.matrix_world, d @ (p @ l), atol=1e-3)) # False
> could you try deactivating the UI layout these properties are drawn in? I think using something like `row.enabled = False` would be the most straightforward solution but the problem is that after you cannot copy values anymore 🙁 ![image](/attachments/7326f120-97c0-4501-8b65-bf6c57cbc505) > Maybe also look at the UILayout.emboss property to make them look less "for editing" and more "for display". `emboss = None` pretty much conveys that those properties are read-only and personally would work for me. Though it could look a bit as a broken UI since it kind of looks just like bunch of labels at first and then on hover it seems like usual props but except there's something off (maybe it's because haven't seen this anywhere else in Blender UI). <video src="/attachments/2532f192-2b1b-4700-9a7e-cf88a2982ef1" title="blender_8GfpHLbPJh.mp4" controls></video> > For the rotation mode, I think this should implicitly use the object's rotation mode. I wanted to avoid creating more fields for quaternion and axis+angle as it's unlikely that it will be used. And some small benefit of having separate rotation mode is that you can set it to the reverse of the parent's rotation mode to make sense what rotation parent matrix is trying to undo. Otherwise displaying rotation probably will be useless most of the times or would require toggling back and forth if we connect it to object's rotation mode. ![image](/attachments/7e0fad3c-eb1e-438a-b88b-d65d49e32836) > 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. Done some experimenting too and it seems that delta transform is applied first the object's transform, so it makes sense to keep them close. ```python import numpy as np import bpy from mathutils import Matrix from math import pi l = Matrix.Rotation(pi/4, 4, 'Z') d = Matrix.Rotation(pi/4, 4, 'Y') p = Matrix.Rotation(pi/4, 4, 'X') obj = bpy.context.object obj.rotation_euler = l.to_euler() obj.delta_rotation_euler = d.to_euler() obj.matrix_parent_inverse = p bpy.context.view_layer.update() print(np.allclose(obj.matrix_world, p @ (d @ l), atol=1e-3)) # True print(np.allclose(obj.matrix_world, d @ (p @ l), atol=1e-3)) # False ``` >

could you try deactivating the UI layout these properties are drawn in?

I think using something like row.enabled = False would be the most straightforward solution but the problem is that after you cannot copy values anymore 🙁

There's two options you can use, one is row.active and the other is row.enabled. I know, it's not that clearly described what these do. AFAIK they both have the same darkening effect, where row.active = False will still allow interaction with the properties.

Maybe also look at the UILayout.emboss property to make them look less "for editing" and more "for display".

emboss = None pretty much conveys that those properties are read-only and personally would work for me. Though it could look a bit as a broken UI since it kind of looks just like bunch of labels at first and then on hover it seems like usual props but except there's something off (maybe it's because haven't seen this anywhere else in Blender UI).

I think PULLDOWN_MENU also looks quite good. But IMO these should be checked with row.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.

For the rotation mode, I think this should implicitly use the object's rotation mode.

I wanted to avoid creating more fields for quaternion and axis+angle as it's unlikely that it will be used.

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.

And some small benefit of having separate rotation mode is that you can set it to the reverse of the parent's rotation mode to make sense what rotation parent matrix is trying to undo. Otherwise displaying rotation probably will be useless most of the times or would require toggling back and forth if we connect it to object's rotation mode.

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

  • Object
  • Parent (if needed at all)
  • Reverse of Parent

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.

> > could you try deactivating the UI layout these properties are drawn in? > > I think using something like `row.enabled = False` would be the most straightforward solution but the problem is that after you cannot copy values anymore 🙁 There's two options you can use, one is `row.active` and the other is `row.enabled`. I know, it's not that clearly described what these do. AFAIK they both have the same darkening effect, where `row.active = False` will still allow interaction with the properties. > > Maybe also look at the UILayout.emboss property to make them look less "for editing" and more "for display". > > `emboss = None` pretty much conveys that those properties are read-only and personally would work for me. Though it could look a bit as a broken UI since it kind of looks just like bunch of labels at first and then on hover it seems like usual props but except there's something off (maybe it's because haven't seen this anywhere else in Blender UI). > > <video src="/attachments/2532f192-2b1b-4700-9a7e-cf88a2982ef1" title="blender_8GfpHLbPJh.mp4" controls></video> I think `PULLDOWN_MENU` also looks quite good. But IMO these should be checked with `row.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. > > For the rotation mode, I think this should implicitly use the object's rotation mode. > > I wanted to avoid creating more fields for quaternion and axis+angle as it's unlikely that it will be used. 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. > And some small benefit of having separate rotation mode is that you can set it to the reverse of the parent's rotation mode to make sense what rotation parent matrix is trying to undo. Otherwise displaying rotation probably will be useless most of the times or would require toggling back and forth if we connect it to object's rotation mode. 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 - Object - Parent (if needed at all) - Reverse of Parent 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.
Andrej added 3 commits 2024-02-10 10:18:05 +01:00
ea90c56b1b more ui tweaks
- removed locks that can be confused with toggles
- set ui as not active to indicate that it's read-only
- moved rotation mode dropdown above rotation values as changing rotation affect the values
Andrej added 1 commit 2024-02-10 10:21:57 +01:00
Author
Contributor

row.active = False looks very good and allows copying values, it seems to look fine even without a bit confusing emboss:

This is different from the object properties, where changing the mode changes the meaning of the components, but not their values.

oh, makes total sense to indicate that values depend on the rotation mode

`row.active = False` looks very good and allows copying values, it seems to look fine even without a bit confusing `emboss`: <video src="/attachments/64469354-3597-4a92-8068-aaf768f68f96" title="blender_F8e64SwB62.mp4" controls></video> > This is different from the object properties, where changing the mode changes the meaning of the components, but not their values. 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.

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](https://projects.blender.org/blender/blender/pulls/113364#issuecomment-1118887), but also blocks any interaction, and values cannot be copy-pasted. - `layout.active = False` [shows the UI dimmed](https://projects.blender.org/blender/blender/pulls/113364#issuecomment-1120454), 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.
Member

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.

`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.
Julian Eisel requested changes 2024-02-12 12:57:21 +01:00
@ -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))
Member

Always use braces for if-statements, this can avoid critical bugs: https://developer.blender.org/docs/handbook/guidelines/c_cpp/#always-use-braces

Always use braces for if-statements, this can avoid critical bugs: https://developer.blender.org/docs/handbook/guidelines/c_cpp/#always-use-braces
Andrej730 marked this conversation as resolved
@ -1020,1 +1020,4 @@
static bool childof_clear_inverse_poll(bContext *C)
{
if (!edit_constraint_liboverride_allowed_poll(C))
Member

Braces.

Braces.
Andrej730 marked this conversation as resolved
@ -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)
Member

Braces.

Braces.
Andrej730 marked this conversation as resolved
@ -1278,1 +1301,4 @@
static bool objectsolver_clear_inverse_poll(bContext *C)
{
if (!edit_constraint_poll(C))
Member

Braces.

Braces.
Andrej730 marked this conversation as resolved
@ -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)
Member

Braces.

Braces.
Andrej730 marked this conversation as resolved
Andrej added 1 commit 2024-02-12 19:16:03 +01:00
Andrej added 1 commit 2024-02-12 19:33:59 +01:00
Author
Contributor

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.

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.

> `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. 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. <video src="/attachments/0f4800da-a730-4f92-9a11-121400801935" title="blender_femFzXdDwX.mp4" controls></video>

Thanks @JulianEisel . I agree that this is the best solution.

Thanks @JulianEisel . I agree that this is the best solution.
Author
Contributor

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 and ObjectSolverConstraint in rna_constraint.cc.

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` and `ObjectSolverConstraint` in `rna_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.

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.
Andrej added 1 commit 2024-02-22 14:56:22 +01:00
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
9de8aaba3e
removed icon from "clear parent inverse transform" button
Author
Contributor

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.

Removed the icon.

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.

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.

> 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. Removed the icon. > 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. 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.

@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.
Author
Contributor

@dr.sybren updated the PR description.

@dr.sybren updated the PR description.
Sybren A. Stüvel added this to the 4.2 LTS milestone 2024-03-01 14:36:04 +01:00

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR113364) when ready.
Sybren A. Stüvel requested changes 2024-03-01 15:09:16 +01:00
Sybren A. Stüvel left a comment
Member

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

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

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.
Andrej730 marked this conversation as resolved
@ -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'.

Prefix the function names with an `_` so that they're marked as 'private'.
Andrej730 marked this conversation as resolved
@ -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:

def _get_location(self):
    return self.id_data.matrix_parent_inverse.to_location()
Instead of decomposing the matrix three times, I think it's clearer to use the dedicated `Matrix.to_…()` functions. So for example: ``` def _get_location(self): return self.id_data.matrix_parent_inverse.to_location() ```
Andrej730 marked this conversation as resolved
@ -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.

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.
Andrej730 marked this conversation as resolved
@ -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. The set_read_only_prop can be removed.

The options=set() parameter is unnecessary.

Use `set=None` to create a read-only property. The `set_read_only_prop` can be removed. The `options=set()` parameter is unnecessary.
Author
Contributor

with set=None adding .enabled = False to UI elements turn out also to be unnecessary.

with `set=None` adding `.enabled = False` to UI elements turn out also to be unnecessary.
Andrej730 marked this conversation as resolved
@ -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.

Add a comment that explains why this property is added.
Andrej730 marked this conversation as resolved
@ -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?

If a constraint could not be found, why is it ok to run the operator? In other words, shouldn't this return `false`?
Author
Contributor

I've left it returning true since in childof_set_inverse_exec it's also possible to provide constraint without setting constraint in context and didn't wanted to interrupt that workflow if someone would be using it.

ac8dcb9ac7/source/blender/editors/object/object_constraint.cc (L942)

I've left it returning `true` since in `childof_set_inverse_exec` it's also possible to provide constraint without setting `constraint` in context and didn't wanted to interrupt that workflow if someone would be using it. https://projects.blender.org/blender/blender/src/commit/ac8dcb9ac763644f421483d2915dc538b478e7e4/source/blender/editors/object/object_constraint.cc#L942

Coming back to this later, I think it'll be good to explain this in a comment.

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?

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?
Andrej730 marked this conversation as resolved
@ -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.

Same as above, I think this should `return false`.
dr.sybren marked this conversation as resolved
@ -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?

Same as above, why is it relevant whether there is a camera set or not?
Andrej730 marked this conversation as resolved
Andrej added 3 commits 2024-03-02 15:48:04 +01:00
7da7ba7f6c refactor parent inverse matrix property group
1) renamed, added docstring
2) marked private methods as private with underscore, now they use `Matrix.to_xxx` instead of matrix decomposition.
3) Fixed comments
b0c974e292 make props read-only by setting setter to None
1) no need for `options=set()` to make it not animatable
2) no need for `row.enabled = False` to make it appear disabled
ac8dcb9ac7 allow clearing inverse matrix without parent object
clearing matrix could be useful in that case, so setting new object won't get have an influenced by old inverse
Sybren A. Stüvel requested changes 2024-05-02 11:30:35 +02:00
Sybren A. Stüvel left a comment
Member

Things are shaping up nicely!

There's one thing that I'd like a 2nd opinion on, the rest are just minor details.

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:

Use the 3x3 matrix, as shear in the 4x4 homogeneous matrix is expected due to the translation component.

This is not so much about "not caring", it is actually incorrect to use the 4x4 matrix here. Something like this would be better: > Use the 3x3 matrix, as shear in the 4x4 homogeneous matrix is expected due to the translation component.
@ -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.

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.

"""Expose decomposed matrix as read-only properties.

... more explanation here ...
"""
Adhere to [PEP 257](https://peps.python.org/pep-0257/) for docstrings. ```python """Expose decomposed matrix as read-only properties. ... more explanation here ... """ ```
@ -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:

@property
def matrix(self) -> Matrix:
    return self.id_data.matrix_parent_inverse

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.

Since this is a simple getter, better to make it a read-only property: ```python @property def matrix(self) -> Matrix: return self.id_data.matrix_parent_inverse ``` 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?

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

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

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.

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.
Campbell Barton reviewed 2024-05-02 13:42:19 +02:00
@ -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?

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?

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?

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.

> 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? 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`.
This pull request has changes conflicting with the target branch.
  • scripts/startup/bl_ui/__init__.py
  • source/blender/blenlib/intern/math_matrix.c

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u clear_inverse_poll:Andrej730-clear_inverse_poll
git checkout Andrej730-clear_inverse_poll
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser Project (Legacy)
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
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
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
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
EEVEE & Viewport
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
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
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
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#113364
No description provided.