Autokeyframe deselect
Needs ReviewPublic

Authored by cris (InsigMathK) on Apr 21 2017, 5:28 PM.

Details

Summary

This patch creates a new checkbox in the graph editor at View > Autodeselect Keyframes that when checked will automatically deselect keyframes on an fcurve when switching between new objects. This functionality works for selecting new objects in both the viewport and the outliner.

Diff Detail

Repository
rB Blender
Branch
autokeyframe_deselect
Build Status
Buildable 707
Build 707: arc lint + arc unit
source/blender/editors/space_view3d/view3d_select.c
2345

Is this a duplicate of the code above? Maybe it should be a separate function if it is.

source/blender/blenkernel/intern/context.c
644 ↗(On Diff #8681)

Since the size of copyC->wm.area->type->name is known (it's BKE_ST_MAXNAME), I'd habitually use STREQLEN() instead of strcmp() == 0. Same for the strcmp()s below.

  • Fixed the problems stefan noticed, mainly the use of strcmp, replaced with STREQNLEN and some braces issues.
Joshua Leung (aligorith) requested changes to this revision.May 14 2017, 6:16 PM

In its current form, I do not think this patch is fit for inclusion. My main objections are:

  1. It's not a good idea to be temporarily changing the context to do things that are not supported in the current context. Yes, it is the hacky way that many many scripts do it, but they do it due to limitations in the PyAPI. We should not however allow such tricks in the core code. If what you want to do isn't supported in the current context, you should delegate the task to be handled later by the correct context at a more convenient time when it is in scope instead (i.e. by sending suitable notifer flags)
  2. In terms of the animation tools, it's incorrect to do it this way - What you're doing here is you're creating a temporary context that will use "default" filtering options. These can however be different to what the user actually sees in any animation editors they have open.

Thinking about this more, you may be able to shoehorn this functionality into the selection-state syncing functionality (see editors/animation/anim_deps.c), which already takes care of this sort of "if objects/bones get selected, a/b/c FCurves/Groups need to get updated too in response to that"

Also, I think the dopesheet (and potentially the nla) could also benefit from having options like this. It's always a good idea when adding settings like this to try to keep all three editors consistent in terms of what functionality is provided/not provided.

source/blender/editors/space_outliner/outliner_select.c
1062

Instead of manually looping here (and risking running into a F-Curve full of FSamples, when fcu->bezt = NULL and fcu->fpt != NULL), it'ld be better to just use: ANIM_fcurve_keyframes_loop(ked, fcu, key_ob_cb, key_edit_cb, fcu_update_cb)

For example, you could just do:

KeyframeEditFunc key_ok = ANIM_editkeyframes_ok(BEZT_OK_SELECTED); 
   // XXX: This first line could just be omitted (i.e. key_ok == NULL) and everything would work just as well. 
 
 KeyframeEditFunc sel_cb = ANIM_editkeyframes_select(SELECT_SUBTRACT);
 ANIM_fcurve_keyframes_loop(NULL, fcu, key_ok, sel_cb, NULL);
1064

Use the BEZT_ISSEL_ANY macro instead for this sort of test.

This revision now requires changes to proceed.May 14 2017, 6:16 PM
cris (InsigMathK) marked 2 inline comments as done.

Removed the context switcher function
This is not quite what was suggested to do to fix this issue, but I got rid of the context function and just rely on iterating through windows till I get the proper 'Graph'
window. It still relies on changing context, but instead creates a copy of a context right away and frees it after it is done, the old context never actually changes.