Details

Reviewers

Sybren A. Stüvel (sybren)

Group Reviewers

Animation & Rigging

Commits

rB5e77ff79ccda: Add ability to get a selection list of bones

Summary

The task for this is: T85796

I've added the ability to get a selection list for both edit mode bones and pose mode bones.

To do this I had rework the selection logic for this a bit.
Before it only stored the names of objects. This might work will of objects, however as stated it might fail for linked objects (so multiple object can have the same name in some corner cases).

For bones this naming lookup is not good enough as you can select while having multiple armatures active (with the same bone names).
So I had to store the bone pointers instead.
However I guess the previous way of looking up things was because these pointers might be invalidated.
(I'm guessing it can break if you playback or skip frames while selecting)
So I'm not sure that my way is safe or if we need to introduce some safe guards so we can be sure that the pointers are still valid.

In addition to this, I'm quite sure @Sybren A. Stüvel (sybren) will not be happy with me just copy pasting old code and calling it a day as they might need some cleanup. ;)

Diff Detail

Event Timeline

Sebastian Parborg (zeddb) requested review of this revision.Mar 1 2021, 7:12 PM

Sebastian Parborg (zeddb) created this revision.

Sebastian Parborg (zeddb) edited the summary of this revision. (Show Details)

Sebastian Parborg (zeddb) mentioned this in T85796: Alt+Click to select overlapping bones.Mar 1 2021, 7:19 PM

Updated so it now also works in bone edit mode.

Sebastian Parborg (zeddb) edited the summary of this revision. (Show Details)Mar 4 2021, 5:25 PM

Sebastian Parborg (zeddb) edited the summary of this revision. (Show Details)Mar 23 2021, 6:03 PM

Sebastian Parborg (zeddb) added a reviewer: Animation & Rigging.

Sebastian Parborg (zeddb) added a project: Animation & Rigging.

Sebastian Parborg (zeddb) edited the summary of this revision. (Show Details)

Sebastian Parborg (zeddb) added a subscriber: Sybren A. Stüvel (sybren).

Sebastian Parborg (zeddb) added a reviewer: Sybren A. Stüvel (sybren).Mar 23 2021, 6:05 PM

The code works, so that's a good start :)

I feel that even copy-pasted code should adhere to the current style guide. There are quite a few C++-style comments in the C files, and also comments that are not proper English sentences.
Then there are also some anonymous TODO comments in there, which feel like they've been copied as well. Do you really think they're relevant to add as TODO here?

The bone_mouse_select_menu function has a cognitive complexity that is too high (didn't measure it, but the sheer number of nested conditionals is enough of an indication for me). Is there really no way to massage it into shape a bit more, and maybe extract some functions with functionality that can be used in both copies of the code?

There are a bunch of debug printf statements still in the code.

source/blender/editors/armature/pose_select.c
257–259	These can probably also be `const`
source/blender/editors/space_view3d/view3d_select.c
1632–1633	What does "prepending about" mean? What does `...` mean? Is it something that you're still going to look at?
1751–1755	Are these all modified in this function? If not → `const`
1761	That's clear from the code itself, no need to comment on it. If you feel that the code needs such an explanation, embed it in the `BLI_assert()` call so that it's also visible on the terminal when the assertion fails.
1764	This was probably copy-pasted as well, but how does this TODO relate to the current code? What does it even mean?
1772	I think nothing else happens after this `if`, so just flip the condition and use `continue`.
1810–1811	This means that this is using the wrong datastructure. Why not use a set instead of a list? Otherwise for sure this has to be a separate function.

This revision now requires changes to proceed.Mar 29 2021, 12:29 PM

Sebastian Parborg (zeddb) added inline comments.Mar 29 2021, 12:58 PM

source/blender/editors/space_view3d/view3d_select.c
1632–1633	Basically the code above always prepends the items in the list. However we actually want to order to be reversed. We don't have to reverse the list if we append instead of prepend. I'm guessing that it was done this way for speed reasons. However I don't think appending will be a bottle neck here so that is why I left the comment so we could discuss this. What do you think? If we use sets, then I guess this might not be an issue. (We do have ordered sets right?)

Sybren A. Stüvel (sybren) added inline comments.Mar 30 2021, 11:47 AM

source/blender/editors/space_view3d/view3d_select.c
1632–1633	With a singly-linked list, prepending is much faster than appending (O(1) vs. O(n)), so I guess that's where it came from. However, since we have doubly-linked lists, appending should be equally fast. C++ code can use `blender::VectorSet<Key>` as an ordered set, not sure about C code, though.

Updated with the latest feedback.

I modified bone_mouse_select_menu a bit. I hope it is better now.

I tried to make things share a bit more code, but in the end the code became just more messy as there was if bone select else object select galore, so I think having the logic separate like this is more clean and easier to parse.

source/blender/editors/space_view3d/view3d_select.c
1764	This is here as discussion starter. This means that we could try to detect which mode we are in and only list valid bones. Now we always get a list of all bones under the cursor. But for example if you are in deselect mode, we could only list the bones that can be deselected. However this is not how it works currently in object select mode. So it would be new functionality. Perhaps we can just skip this? What do you think?

In D10570#278256, @Sebastian Parborg (zeddb) wrote:

I tried to make things share a bit more code, but in the end the code became just more messy as there was if bone select else object select galore, so I think having the logic separate like this is more clean and easier to parse.

Yeah, the code really needs some overall cleanup to remove flag arguments and replace them with an API that actually supports the different needs in a nicely decoupled way. That's out of scope for this patch, though.

source/blender/editors/include/ED_armature.h
131–134	Don't use `const` for the declaration of pass-by-value parameters. It has no semantic value.
215–217	Don't use `const` for the declaration of pass-by-value parameters. It has no semantic value.
source/blender/editors/space_view3d/view3d_select.c
1690	Should be sentence. Same further below.
1764	Skip it, implement it separately. I think it's a nice feature (I also like that my shell completes `chmod -x <tab>` only with directory entries that are currently marked executable).
1817	`const`
1817	Typo in `dupblicate_bone`, also `is_duplicate_bone` is clearer as "duplicate bone" can also indicate the imperative "you there, duplicate this bone!".
2649	`const`?

This revision now requires changes to proceed.Apr 8 2021, 3:32 PM

Fixed the latest comments

Sybren A. Stüvel (sybren) accepted this revision.Apr 8 2021, 5:08 PM

This revision is now accepted and ready to land.Apr 8 2021, 5:08 PM

Closed by commit rB5e77ff79ccda: Add ability to get a selection list of bones (authored by Sebastian Parborg (zeddb)). · Explain WhyApr 8 2021, 5:26 PM

This revision was automatically updated to reflect the committed changes.

Sebastian Parborg (zeddb) added a commit: rB5e77ff79ccda: Add ability to get a selection list of bones.

炎臻 (PrettyFireNOI7) awarded a token.Apr 13 2021, 3:31 AM

Add ability to Alt+Click to get selection list of bones
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 35988

source/blender/editors/armature/armature_select.c

source/blender/editors/armature/pose_select.c

source/blender/editors/include/ED_armature.h

source/blender/editors/space_view3d/view3d_intern.h

source/blender/editors/space_view3d/view3d_ops.c

source/blender/editors/space_view3d/view3d_select.c

Add ability to Alt+Click to get selection list of bonesClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 35988

source/blender/editors/armature/armature_select.c

source/blender/editors/armature/pose_select.c

source/blender/editors/include/ED_armature.h

source/blender/editors/space_view3d/view3d_intern.h

source/blender/editors/space_view3d/view3d_ops.c

source/blender/editors/space_view3d/view3d_select.c

Add ability to Alt+Click to get selection list of bones
ClosedPublic
Actions

Revision Contents
Changeset List