Page MenuHome

UI: Changes to graph editor selection and transform
ClosedPublic

Authored by Julian Eisel (Severin) on Nov 12 2019, 5:20 PM.
Tags
None
Tokens
"Like" token, awarded by threedslider."100" token, awarded by Tetone."Love" token, awarded by jonathanl."Love" token, awarded by xrg.

Details

Summary

When introducing "drag-all-selected" support all over Blender, we figured this wouldn't work well with the Graph Editor selection/transform behavior.
Hence, William and I have been working on the following changes, although we used this chance to improve the behavior in general too.
For more info see T70634.

  • Handles now always move with the key, regardless if they are selected or not.
  • Selecting the key doesn't select the handles anymore, their selection is separate.
  • Multiple keys and handles can now be dragged.
  • Dragging a handle moves all selected handles on the same side.
  • Tweak-dragging any handle can never affect any keyframe location, only handles.
  • G/R/S should behave as before.
  • Changing the handle type with a key selected always applies the change to both handles.
  • Box selection with Ctrl+Drag now allows deselecting handles (used to act on entire triple only).
  • Box selection Include Handles option now only acts on visible handles, wasn't the case with Only Selected Keyframes Handles enabled.
  • Box selection Include Handles is now enabled by default in all bundled keymaps.

If needed I can give more detailed rationales for each change, but keeping this short.

The changes have been tested for some days by the animators here in the Blender Animation Studio. Some changes are based on their feedback.

Diff Detail

Repository
rB Blender

Event Timeline

Julian Eisel (Severin) edited the summary of this revision. (Show Details)Nov 12 2019, 5:28 PM

Getting this in will allow us to close a high priority UI module task: T57918: Tweaks & Fixes for Improved Left Click Select Support (Parent task).
So of course I'd appreciate a swift review ;)

  • Handles now always move with the key, regardless if they are selected or not.
  • Selecting the key doesn't select the handles anymore, their selection is separate.
  • Dragging a handle moves all selected handles on the same side.
  • Changing the handle type with a key selected always applies the change to both handles.

This seems well though out. I wish similar tweaks could be applied to the behavior of Bezier Curve objects in the 3D View

Sybren A. Stüvel (sybren) requested changes to this revision.EditedNov 13 2019, 2:28 PM

In this review I've only looked at the code. Commenting on the usability I leave to @Luciano Muñoz Sessarego (looch) and @Hjalti Hjálmarsson (hjalti)

There is this flag wait_to_deselect_others that's passed around a lot. This seems to be modelling some idea, but the idea itself is still a bit mystifying to me. There is also no explanation at its RNA definition (in WM_operator_properties_generic_select()); there the only 'documentation' is the string "Wait to Deselect Others", which is of course useless.

source/blender/blenkernel/BKE_fcurve.h
331–332

Why is handle_sel_flag an int and not an enum? If it really, really, really isn't possible to have an enum here, at least add a comment that explains what it does and what valid values are.

332

Add a comment that explains what the function does. I know wasn't documented before either, but if you know what it does now, let's put that knowledge into the source.

Same question as above about sel_flag not being an enum.

source/blender/blenkernel/intern/curve.c
3605–3606

Why the line break?

4128

Nice, doesn't mention at all what the function does.

4132–4133

Point to where those other values can be found. Right now the only thing I can find is the horrible #define SELECT 1 in DNA_object_types.h.

source/blender/editors/include/ED_keyframes_edit.h
144–147

This comment doesn't mean anything to me. Hiding handles seems like something that's an intentional action, and isn't always done when iterating keyframes. The same goes for getting the visibility state, that's also not something that's generic to "iteration".

source/blender/editors/space_graph/graph_select.c
729

What's the reason behind the change in default value here?

1434

I really don't like ret_val variables. They force the reader to read through all the conditionals and code branches to figure out what will be returned. Currently there are some flows that return ret_val, but not all of them do, so it's not even a correctly named variable.

If it really can't be helped, I would prefer a boolean is_running_modal that is checked at the return statement to determine the return value.

1447

Do an explicit (nvi != NULL) here for clarity.

1451

no need to check nvi, because if it's NULL then already_selected will be false anyway.

1499–1500

what does "the handle that applied" mean?

1583

select_mode should be of type eEditKeyframes_Select instead of short. Let's properly use enums wherever possible.

1584

The meaning of wait_to_deselect_others and the interaction between select_mode and wait_to_deselect_others are undocumented.

1589

Same comment as above about ret_val.

1608–1616

This is quite a spaghetto.

source/blender/editors/transform/transform.h
635–636

Not specific to this patch, but the word 'tweak' is used a lot, also in tooltips (so presented to users). However, it's not defined in the glossary. The concept "tweak event" is not defined anywhere.

source/blender/editors/transform/transform_convert.h
38–47 ↗(On Diff #19574)

Why is this necessary? Don't we always want to have one kind of behaviour in Blender?

source/blender/editors/transform/transform_convert_graph.c
160–161

When talking about curves, I would keep the term "vertices" for actual vertices of the curve and not invent a different meaning to a mathematically well-defined concept. In this comment the term seems to receive another meaning, namely as a group term for a control point and its handles.

186–195

I would split this up differently:

#ifdef USE_HANDLES_AS_CHILD
  if (!key) {
    left = right = false;
  }
#endif

*r_left_handle = left;
*r_right_handle = right;

For me this is easier to understand. You could also replace the if (!key) { ... } with left ||= key; right ||= key; if you like the logical operators more.

You almost entirely separate the logic from the assignment to *r_xxx, and I think it's worth making that separation complete.

199

Much thank you on renaming 1/2/3 to left/key/right :)

This revision now requires changes to proceed.Nov 13 2019, 2:28 PM

Commented with some rationales and context. All in all this is spaghetti code, so it's hard to convey this in code.

Thanks for reviewing @Sybren A. Stüvel (sybren), and thanks for kicking me to leave the campground cleaner. Working in the areas I work in, I fight legacy spaghetti code everyday. At some point you become sick of it and start compromising some things, so some occasional butt kicking is healthy :)

@Duarte Farrajota Ramos (duarteframos) curve should get the same treatment indeed, but I have important projects to get to first, so I'll leave this up for grabs for now.

source/blender/blenkernel/BKE_fcurve.h
331–332

Because of a widely known language design flaw of C, for which we don't have a nice workaround. See my comment in the related devtalk thread.

332

I only have a rough understanding of what the function does. All I had to do was making the precedence of handle sides controllable by a caller, the remaining logic should be valid still.
I can add a few words on what the (badly named...) function does though.

source/blender/blenkernel/intern/curve.c
3605–3606

Because clang-format :)

4128

It's worse even, the name doesn't fit what it's actually doing either.

source/blender/editors/include/ED_keyframes_edit.h
144–147

The comment is poorly worded indeed. The handles weren't intentionally hided, but by default only triples with a selected control-point/handle are visible.
We have to skip those in some cases.

source/blender/editors/space_graph/graph_select.c
729

From all feedback that I got, the old default wasn't at all what animators wanted. The reason it was using the "bad" default, is that the "good" default was broken in a way:
With ViewOnly Selected Keyframes Handles enabled, box selecting with include_handles would act on handles that were not visible.
William, Hjalti and I agreed on fixing this and changing the default.

See rB70d86d55c393.

1434

I should mention that the logic here is very similar to the logic we use in multiple select operators, wherever we've introduced "drag-all-selected" now. I'm personally not too happy with the naming either, I stuck with what was already there though.
I also think it's useful to have naming and usage similar to the other operators, so that it's easy to catch the similarities.
Nevertheless I could do some local improvements here.

1451

If already_selected is changed in some way, so that the NULL-check is removed, this would break with possibly hard to find consequences. Note how the variable is only there for readability, it's not used anywhere outside of the if. So the first NULL-check is only there to prevent NULL-pointer-dereference, while the one in the if matters for the actual logic.

In such cases I personally prefer the redundancy this introduces.

1499–1500

Good question, I copied and updated the comment for my changes, but this could be made more clear.

1584

It is rather difficult to describe this well.
Basically it is logic that's designed to get a very specific behavior to work with our event-handling system. Bastien was kinda forced to introduce it, but he wasn't really happy about it either, see discussion in T63994. Brecht helped improve things quite a bit, but it's still complicated.

I'm not sure how to convey the logic here, maybe I'll just link to T63994 in cases where this is used.

1608–1616

See my comment above regarding this trickery.

source/blender/editors/transform/transform.h
635–636

We use this term to describe click+drag events, in the code and in the UI (see keymap editor). So it is the technically correct term here, even though I agree it's overloaded a bit.

source/blender/editors/transform/transform_convert.h
38–47 ↗(On Diff #19574)

Yeah, again things are a bit hard to explain...

There are some lower level functions taking a use_handles parameter, some of which are shared with other code. We call them from multiple places here in the transform code, so it makes sense to have a constant in some header file to toggle the behavior.

The reason I ended up with this is because I think it helps understanding how things work together. You can just search for IS_USE_HANDLE or disable the define to see how it affects the behavior and how the parts using it are connected. It wasn't easy to find this out myself so I prefer helping others to get there faster.

I could explain this in the comment. And maybe it's not worth keeping the alternative behavior.

source/blender/editors/transform/transform_convert_graph.c
160–161

AFAIK and FWIW, "vertices" is the term we use for this elsewhere, but in this case I can explicitly name handles and keys/control-points.

source/blender/editors/space_graph/graph_select.c
1584

T63994 has a huge discussion attached to it, and no summary in the task description. I wouldn't rely on it when trying to explain the current implementation.

1608–1616

I'm just commenting on local code readability here. It's conditionally changing a variable that's used only in a conditional in the next statement. This makes the code unnecessarily complex.

This code:

if (select_mode != SELECT_REPLACE) {
  wait_to_deselect_others = false;
}
if (wait_to_deselect_others && (nvi->bezt->f2 & SELECT)) ...

is, as long as I'm not missing anything, equivalent to:

if (select_mode == SELECT_REPLACE && wait_to_deselect_others && (nvi->bezt->f2 & SELECT)) ...

Once you have this, you can see that both the if and the else if require select_mode == SELECT_REPLACE, and thus you can move that to an outer if-statement:

if (select_mode == SELECT_REPLACE) {
  if (wait_to_deselect_others && (nvi->bezt->f2 & SELECT)) {
    ret_val = OPERATOR_RUNNING_MODAL;
  }
  else {
    select_mode = SELECT_ADD;
    deselect_graph_keys(ac, 0, SELECT_SUBTRACT, false);
  }
}
source/blender/editors/transform/transform.h
635–636

It's only technically correct if it has been properly defined in a technical manner. AFAICS this is not the case.

Click + drag, is that mouse down, mouse up, mouse down, move? If it's just mouse down + move, what is the difference between "tweak" and "drag"?

source/blender/editors/transform/transform_convert.h
38–47 ↗(On Diff #19574)

Yeah, again things are a bit hard to explain...

To me this is unacceptable. If something is hard to explain it MUST be explained. If something is easy to explain, ok, then you might consider not adding a comment. But if it's hard, what reason is there to leave out any explanation?

There are some lower level functions taking a use_handles parameter, some of which are shared with other code. We call them from multiple places here in the transform code, so it makes sense to have a constant in some header file to toggle the behavior.

This doesn't make sense to me. The fact that code is used in different places doesn't imply to me that we want to be able to switch behaviour by editing some preprocessor directive.

And maybe it's not worth keeping the alternative behavior.

I think simpler code is easier to explain. As you said, the UI code is already complex, and when things can be changed around, without any clear explanation why, it only makes understanding things harder.

William Reynish (billreynish) accepted this revision.EditedNov 14 2019, 1:31 PM

Tested. This is a massive improvement for grabbing items in the Graph Editor.

  • Multi-dragging keyframes works as expected
  • The previous bug in master (has been in Blender forever it seems) that made the handles go haywire when transforming only keyframes is fixed here
  • Multi-dragging handles works as expected, correctly transforming all handles on the same side.
  • Selecting handles works as expected, only allowing you to select handles that are actually visible.

This is a superb improvement to the Graph Editor.

Julian Eisel (Severin) marked 21 inline comments as done.EditedFri, Nov 15, 5:08 PM

Alright, I think all comments should be addressed now. I actually found and fixed some smaller bugs.

Put quite some effort into trying to improve related legacy code and add better documentation.
For the whole wait_to_deselect_others related stuff, I added a section to our WIP interface guidelines explaining the desired behavior: https://wiki.blender.org/wiki/Human_Interface_Guidelines/Selection#Select-tweaking. I referenced that in code and explained the technical implications there. We wanted to have such guidelines on selection anyway :)

source/blender/editors/transform/transform.h
635–636
source/blender/editors/transform/transform_convert.h
38–47 ↗(On Diff #19574)

Checking over this again, I notice that the changes to this aren't needed actually. I had a wrong understanding of what option this flag belongs to.

Reverted related changes.

Julian Eisel (Severin) marked an inline comment as done.
  • Cleanup: Comments, use enum types or aliases in parameter lists
  • Fix incorrect selection flag usage
  • Cleanup: Clarify iterator visibility flag
  • Minor cleanup: Clarify logic
  • Fix incorrect use of use_handle option
  • Fix incorrect return value, cleanup return values
  • Cleanup: Detailed comments on selection helpers & simplify logic
source/blender/editors/transform/transform.h
635–636

It defines neither "click" nor "tweak".

Julian Eisel (Severin) added inline comments.
source/blender/editors/transform/transform.h
635–636

True, it just explains how click/drag and click/tweak can be used differently.
But this is a documentation issue, since it is literally the term we use in the UI to describe this specific event type:


Tagging @Aaron Carlisle (Blendify) here :)

source/blender/editors/transform/transform.h
635–636

It's a documentation issue for sure. I would suggest to limit ourselves to the English meaning of words, or to properly define such terms in the glossary.

source/blender/windowmanager/intern/wm_operator_props.c
407

Probably easier to read if you use - for itemisation rather than *.

Also, what is a 'select-mouse'?

Apart from that, great to see more documentation of this behaviour, it clarifies quite a bit.

411–412

What's the difference between 'tweak-move' and 'tweak'? If 'tweak-move' is 'click+drag', is 'tweak' the same as 'click'?

Julian Eisel (Severin) marked 6 inline comments as done.
  • Cleanup: Slight adjustments to comment
source/blender/editors/transform/transform.h
635–636

Okay, I've set up a Glossary in the HIG now, where we can give more precise technical definitions.
Also added a definition of Tweak and Drag (as I understand it ;) https://wiki.blender.org/wiki/Human_Interface_Guidelines/Glossary#Actions

We should probably improve terminology but I'll have to leave that up to others for now.
Creating better definitions and applying them more consequently is something we'll have to do progressively. And it will have to become a habit to look definitions up in the Glossary (or add definitions). Could be a great improvement to our process.

source/blender/windowmanager/intern/wm_operator_props.c
407

It's RMB for right click select, LMB for left click select :)
But I guess it's clear with out the select- prefix, so got rid of it.

411–412

Indeed, I thought it was easier to understand that way, but in fact it might be just as well misleading. Adjusted it slightly.

Thanks a lot for the review @Sybren A. Stüvel (sybren).

I would like to get the usability side officially signed off by an animation module owner/member. Hjalti tested the changes for a while now, we've discussed them quite a bit and I know that he is fine with them.
I've spoken to other animators about the changes and so far everybody was fine with them too.

So given this positive feedback, approval by William for the UI team, and approval by Sybren on the technical level, I'm gonna move this forward by pushing to master now.
Should there be critical bugs for anyone's workflow, I'm around over the weekend to fix them.

This revision was not accepted when it landed; it landed in state Needs Review.Fri, Nov 22, 4:57 PM
This revision was automatically updated to reflect the committed changes.