WIP #104054 Symmetize visible edit bones if nothing has been selected #105385

Denys Hsu · 2023-03-02T20:58:29+01:00

Denys Hsu commented

2023-03-02 20:58:29 +01:00

Symmetrize does nothing when there are no bones selected (Silently exits). Expected behaviour would be that when nothing is selected, the armature gets symmetrized as if all bones are selected.

When all bones are selected, all visible, selectable bones are symmetrized. This makes sense from the user perspective and also should be expected when nothing has been selected.

Options to solve this problem:

Select all, if nothing has been selected ("reuse the deselection code").
Select bones depending on the selected direction if mirrowed bones are available.

Selecting all visible bones seemed like the naive solution to this problem as Symmetrize operates on selected bones and deselects mirrored bones if all bones have been selected.

Symmetrize does nothing when there are no bones selected (Silently exits). Expected behaviour would be that when nothing is selected, the armature gets symmetrized as if all bones are selected. When all bones are selected, all visible, selectable bones are symmetrized. This makes sense from the user perspective and also should be expected when nothing has been selected. Options to solve this problem: - Select all, if nothing has been selected ("reuse the deselection code"). - Select bones depending on the selected direction if mirrowed bones are available. Selecting all visible bones seemed like the naive solution to this problem as Symmetrize operates on selected bones and deselects mirrored bones if all bones have been selected.

symmetrize.mov

2.9 MiB

Denys Hsu added 1 commit 2023-03-02 20:58:43 +01:00

0b8d13ba19 FIX #104054 symmetize visible bones of armature if nothing has been selected

Sybren A. Stüvel requested changes 2023-03-03 14:28:47 +01:00

Sybren A. Stüvel left a comment

This doesn't just symmetrize the visible edit bones, it actually selects them, which is quite a different thing. Selection has to be treated carefully, as often it's the result of specific actions by the artist.

You may want to check Ingredients of a PR as well, to expand the PR description a bit. A think that's missing now is an analysis of the alternatives. For example, the code could also be adjusted to symmetrize all the bones when there is no selection, and not just only the visible ones. I'm not saying your choice is bad, quite the contrary, I think it's good to limit the effect to visible bones only. But having this motivated as an explicit choice will make it easier to understand later on.

This doesn't just symmetrize the visible edit bones, it actually selects them, which is quite a different thing. Selection has to be treated carefully, as often it's the result of specific actions by the artist. You may want to check [Ingredients of a PR](https://wiki.blender.org/wiki/Process/Contributing_Code#Ingredients_of_a_Pull_Request) as well, to expand the PR description a bit. A think that's missing now is an analysis of the alternatives. For example, the code could also be adjusted to symmetrize all the bones when there is no selection, and not just only the visible ones. I'm not saying your choice is bad, quite the contrary, I think it's good to limit the effect to visible bones only. But having this motivated as an explicit choice will make it easier to understand later on.

Denys Hsu changed title from ~~Ref #104054 Symmetize visible edit bones if nothing has been selected~~ to WIP #104054 Symmetize visible edit bones if nothing has been selected

2023-03-04 23:26:52 +01:00

Denys Hsu commented

2023-03-04 23:37:34 +01:00

This doesn't just symmetrize the visible edit bones, it actually selects them, which is quite a different thing. Selection has to be treated carefully, as often it's the result of specific actions by the artist.

You may want to check Ingredients of a PR as well, to expand the PR description a bit. A think that's missing now is an analysis of the alternatives. For example, the code could also be adjusted to symmetrize all the bones when there is no selection, and not just only the visible ones. I'm not saying your choice is bad, quite the contrary, I think it's good to limit the effect to visible bones only. But having this motivated as an explicit choice will make it easier to understand later on.

Thanks for the input. I'm not sure if selecting nothing is a specific action in this case, but I totally get that selections should be treated carefully. I updated the PR description decided to not select all via the context, if nothing has been selected.

This makes the actual change more readable - the operator still acts like the symmetrize when everything is selected. Kinda would like to remove some nesting swell but I wanted to keep the actual changes easy to read.

> This doesn't just symmetrize the visible edit bones, it actually selects them, which is quite a different thing. Selection has to be treated carefully, as often it's the result of specific actions by the artist. > > You may want to check [Ingredients of a PR](https://wiki.blender.org/wiki/Process/Contributing_Code#Ingredients_of_a_Pull_Request) as well, to expand the PR description a bit. A think that's missing now is an analysis of the alternatives. For example, the code could also be adjusted to symmetrize all the bones when there is no selection, and not just only the visible ones. I'm not saying your choice is bad, quite the contrary, I think it's good to limit the effect to visible bones only. But having this motivated as an explicit choice will make it easier to understand later on. Thanks for the input. I'm not sure if selecting nothing is a specific action in this case, but I totally get that selections should be treated carefully. I updated the PR description decided to not select all via the context, if nothing has been selected. This makes the actual change more readable - the operator still acts like the symmetrize when everything is selected. Kinda would like to remove some nesting swell but I wanted to keep the actual changes easy to read.

Denys Hsu added 1 commit 2023-03-06 14:16:43 +01:00

7284e058e9 select only relevant side to mirrow

Denys Hsu added 1 commit 2023-03-06 14:41:49 +01:00

c339aa19d1 do not select bones in center

Denys Hsu requested review from Sybren A. Stüvel 2023-03-06 14:48:48 +01:00

Sybren A. Stüvel requested changes 2023-03-07 17:32:25 +01:00

Sybren A. Stüvel left a comment

Kinda would like to remove some nesting swell but I wanted to keep the actual changes easy to read.

I completely agree. I've added two inline notes, but then I hit the more complex code and it's just super hard to follow what's going on.

I think it's better to first clean up the existing code. Some flipping of conditions and added continue statements will already help a lot in reducing the indentation levels. Also I feel the code should be split up into a few smaller functions, so that the overall purpose of each block becomes clearer.

Would you be up for that, in a diffent PR? After that, we can revisit this change, I'm sure it'll be a lot easier to implement once the code has been cleaned up.

> Kinda would like to remove some nesting swell but I wanted to keep the actual changes easy to read. I completely agree. I've added two inline notes, but then I hit the more complex code and it's just super hard to follow what's going on. I think it's better to first clean up the existing code. Some flipping of conditions and added `continue` statements will already help a lot in reducing the indentation levels. Also I feel the code should be split up into a few smaller functions, so that the overall purpose of each block becomes clearer. Would you be up for that, in a diffent PR? After that, we can revisit this change, I'm sure it'll be a lot easier to implement once the code has been cleaned up.

source/blender/editors/armature/armature_add.c

						
				@ -1102,2 +1102,3 @@

				  /* cancel if nothing selected */

				  /* check is any ebone is selected on init */

				  bool is_selected = true;

Sybren A. Stüvel commented

2023-03-07 17:22:50 +01:00

IMO is_selected doesn't quite cover the purpose. In other code I've used the name is_selection_relevant for this.

IMO `is_selected` doesn't quite cover the purpose. In other code I've used the name `is_selection_relevant` for this.

source/blender/editors/armature/armature_add.c

						
				@ -1104,2 +1104,3 @@

				  bool is_selected = true;

				  if (CTX_DATA_COUNT(C, selected_bones) == 0) {

				    return OPERATOR_CANCELLED;

				    is_selected = false;

Sybren A. Stüvel commented

2023-03-07 17:23:52 +01:00

The declaration and if can be reduced to:

const bool is_selected = CTX_DATA_COUNT(C, selected_bones) > 0;

This has the added advantage that now the variable can be const.

The declaration and `if` can be reduced to: ```c const bool is_selected = CTX_DATA_COUNT(C, selected_bones) > 0; ``` This has the added advantage that now the variable can be `const`.

Denys Hsu commented

2023-03-08 16:58:57 +01:00

Kinda would like to remove some nesting swell but I wanted to keep the actual changes easy to read.

I completely agree. I've added two inline notes, but then I hit the more complex code and it's just super hard to follow what's going on.

I think it's better to first clean up the existing code. Some flipping of conditions and added continue statements will already help a lot in reducing the indentation levels. Also I feel the code should be split up into a few smaller functions, so that the overall purpose of each block becomes clearer.

Would you be up for that, in a diffent PR? After that, we can revisit this change, I'm sure it'll be a lot easier to implement once the code has been cleaned up.

Sure, was thinking the same. I'll create another PR soon.

> > Kinda would like to remove some nesting swell but I wanted to keep the actual changes easy to read. > > I completely agree. I've added two inline notes, but then I hit the more complex code and it's just super hard to follow what's going on. > > I think it's better to first clean up the existing code. Some flipping of conditions and added `continue` statements will already help a lot in reducing the indentation levels. Also I feel the code should be split up into a few smaller functions, so that the overall purpose of each block becomes clearer. > > Would you be up for that, in a diffent PR? After that, we can revisit this change, I'm sure it'll be a lot easier to implement once the code has been cleaned up. Sure, was thinking the same. I'll create another PR soon.

Sybren A. Stüvel added the

Module

Animation & Rigging

label 2023-04-07 12:51:02 +02:00

Sybren A. Stüvel added this to the Animation & Rigging project 2023-04-07 12:51:07 +02:00

Denys Hsu referenced this pull request

2023-05-08 16:26:04 +02:00

Animation: Armature symmetrize ebones refactor #106487

Denys Hsu closed this pull request

2023-05-13 01:10:13 +02:00

Sybren A. Stüvel removed this from the Animation & Rigging project 2023-05-15 11:37:55 +02:00