Gpencil Layers integration in main dopesheet - Selection issue

Archive Blender Development
,
1

In the scope of issue T97477, GPlayers shall be displayed as channels in the main DopeSheet and Timeline.
I’m trying to fix keyframe selection in this context so that summary selection takes into account channels that are GPLayers.

The problem

I want to adapt the following functions, with they gplayer specific selection function :
[1] act_keys_mselect_single | ED_gpencil_layer_select_frame (action_select.c:1536)
[2] act_keys_mselect_channel_only | ED_gpencil_layer_select_frames (action_select.c:1645)
[3] box_select_elem | ED_gpencil_layer_frames_select_region (action_select.c:380)
[4] region_select_elem | ED_gpencil_layer_frames_select_box(action_select.c:627)

The code of all these function share a similar pattern :
action_select.c:[1536|1645|380|627]

 [..]
  /* select the nominated keyframe on the given frame */
  if (ale->type == ANIMTYPE_GPLAYER) {
    <ED_gpencil_specific_function>
    ale->update |= ANIM_UPDATE_DEPS;
  }
  else if (ale->type == ANIMTYPE_MASKLAYER) {
    <ED_mask_specific_function>
  }
  else {
    if (ELEM(ac->datatype, ANIMCONT_GPENCIL, ANIMCONT_MASK) && (ale->type == ANIMTYPE_SUMMARY) &&
        (ale->datatype == ALE_ALL)) {
      ListBase anim_data = {NULL, NULL};
      int filter;

      filter = (ANIMFILTER_DATA_VISIBLE | ANIMFILTER_LIST_VISIBLE /*| ANIMFILTER_CURVESONLY */ |
                ANIMFILTER_NODUPLIS);
      ANIM_animdata_filter(ac, &anim_data, filter, ac->data, ac->datatype);

      for (ale = anim_data.first; ale; ale = ale->next) {
        if (ale->type == ANIMTYPE_GPLAYER) {
    	  <ED_mask_specific_function>
          ale->update |= ANIM_UPDATE_DEPS;
        }
        else if (ale->type == ANIMTYPE_MASKLAYER) {
    	  <ED_mask_specific_function>
        }
      }

      ANIM_animdata_update(ac, &anim_data);
      ANIM_animdata_freelist(&anim_data);
    }
    else {
      ANIM_animchannel_keyframes_loop(&ked, ac->ads, ale, ok_cb, select_cb, NULL);
    }
  }

This code takes into account selection of single gplayer channels, single masklayer channels, or summaries in the scope of GPencil DopeSheet, but not gplayers channels in summaries of the main dopesheet.

and
keyframes_edit.c:362

short ANIM_animchannel_keyframes_loop(KeyframeEditData *ked,
                                      bDopeSheet *ads,
                                      bAnimListElem *ale,
                                      KeyframeEditFunc key_ok,
                                      KeyframeEditFunc key_cb,
                                      FcuEditFunc fcu_cb)
{
  /* sanity checks */
  if (ale == NULL) {
    return 0;
  }

  /* method to use depends on the type of keyframe data */
  switch (ale->datatype) {
    /* direct keyframe data (these loops are exposed) */
    case ALE_FCURVE: /* F-Curve */
      return ANIM_fcurve_keyframes_loop(ked, ale->key_data, key_ok, key_cb, fcu_cb);

    /* indirect 'summaries' (these are not exposed directly)
     * NOTE: must keep this code in sync with the drawing code and also the filtering code!
     */
    case ALE_GROUP: /* action group */
      return agrp_keyframes_loop(ked, (bActionGroup *)ale->data, key_ok, key_cb, fcu_cb);
    case ALE_ACT: /* action */
      return act_keyframes_loop(ked, (bAction *)ale->key_data, key_ok, key_cb, fcu_cb);

    case ALE_OB: /* object */
      return ob_keyframes_loop(ked, ads, (Object *)ale->key_data, key_ok, key_cb, fcu_cb);
    case ALE_SCE: /* scene */
      return scene_keyframes_loop(ked, ads, (Scene *)ale->data, key_ok, key_cb, fcu_cb);
    case ALE_ALL: /* 'all' (DopeSheet summary) */
      return summary_keyframes_loop(ked, (bAnimContext *)ale->data, key_ok, key_cb, fcu_cb);
  }

  return 0;
}

keyframes_edit.c:289

/* This function is used to loop over the keyframe data in a DopeSheet summary */
static short summary_keyframes_loop(KeyframeEditData *ked,
                                    bAnimContext *ac,
                                    KeyframeEditFunc key_ok,
                                    KeyframeEditFunc key_cb,
                                    FcuEditFunc fcu_cb)
{
 [..]
  /* loop through each F-Curve, working on the keyframes until the first curve aborts */
  for (ale = anim_data.first; ale; ale = ale->next) {
    switch (ale->datatype) {
      case ALE_MASKLAY:
      case ALE_GPFRAME:
        break;

      case ALE_FCURVE:
      default: {
          [..]
          ret_code = ANIM_fcurve_keyframes_loop(ked, ale->data, key_ok, key_cb, fcu_cb);
          [..]
    }

    if (ret_code) {
      break;
    }
  }

  ANIM_animdata_freelist(&anim_data);

  return ret_code;
}

Possible solutions

To account for gplayers in dopesheet + gain some readability, my idea is to get to something like this :

action_select.c:[1536|1645|380|627]

 [..]
  ANIM_animchannel_keyframes_loop(&ked, ac->ads, ale, ok_cb, select_cb, NULL);

(use same loop for all channel and dopesheet types)
and
keyframes_edit.c:362

short ANIM_animchannel_keyframes_loop(KeyframeEditData *ked,
                                      bDopeSheet *ads,
                                      bAnimListElem *ale,
                                      KeyframeEditFunc key_ok,
                                      KeyframeEditFunc key_cb,
                                      FcuEditFunc fcu_cb)
{
  [..]
  switch (ale->datatype) {
    /* direct keyframe data (these loops are exposed) */
    case ALE_FCURVE: /* F-Curve */
      return ANIM_fcurve_keyframes_loop(ked, ale->key_data, key_ok, key_cb, fcu_cb);

    case ALE_MASKLAY:
      return ANIM_masklay_keyframes_loop(ked, ale->key_data, key_ok, key_cb, fcu_cb);

    case ALE_GPFRAME:
      return ANIM_gplay_keyframes_loop(ked, ale->key_data, key_ok, key_cb, fcu_cb);

    /* indirect 'summaries' (these are not exposed directly)
     * NOTE: must keep this code in sync with the drawing code and also the filtering code!
     */
    case ALE_GROUP: /* action group */
      return agrp_keyframes_loop(ked, (bActionGroup *)ale->data, key_ok, key_cb, fcu_cb);
    case ALE_ACT: /* action */
      return act_keyframes_loop(ked, (bAction *)ale->key_data, key_ok, key_cb, fcu_cb);

    case ALE_OB: /* object */
      return ob_keyframes_loop(ked, ads, (Object *)ale->key_data, key_ok, key_cb, fcu_cb);
    case ALE_SCE: /* scene */
      return scene_keyframes_loop(ked, ads, (Scene *)ale->data, key_ok, key_cb, fcu_cb);
    case ALE_ALL: /* 'all' (DopeSheet summary) */
      return summary_keyframes_loop(ked, (bAnimContext *)ale->data, key_ok, key_cb, fcu_cb);
  }

  return 0;
}

keyframes_edit.c:289

static short summary_keyframes_loop(KeyframeEditData *ked,
                                    bAnimContext *ac,
                                    KeyframeEditFunc key_ok,
                                    KeyframeEditFunc key_cb,
                                    FcuEditFunc fcu_cb)
{
 [..]

  /* loop through each F-Curve, working on the keyframes until the first curve aborts */
  for (ale = anim_data.first; ale; ale = ale->next) {
    switch (ale->datatype) {
      case ALE_MASKLAY:
      	ret_code = ANIM_masklay_keyframes_loop(ked, ale->key_data, key_ok, key_cb, fcu_cb);
      	break;

      case ALE_GPFRAME:
      	ret_code = ANIM_gplay_keyframes_loop(ked, ale->key_data, key_ok, key_cb, fcu_cb);
        break;

      case ALE_FCURVE:
      default: {
         [..]
          ret_code = ANIM_fcurve_keyframes_loop(ked, ale->data, key_ok, key_cb, fcu_cb);
         [..]
        break;
      }
    }

    if (ret_code) {
      break;
    }
  }

  ANIM_animdata_freelist(&anim_data);

  return ret_code;
}

This would imply that ANIM_masklay_keyframes_loop and ANIM_gplay_keyframes_loop implement all selection functions listed above, based on the KeyframeEditData and the callbacks passed in parameters.
For example it could be something like :

OPTION 1

bool ANIM_gplay_keyframes_loop(bGPDlayer *gpl,
                                    KeyframeEditData *ked,
                                    bool (*gpf_ok)(bGPDframe *, KeyframeEditData *),
                                    bool (*gpf_cb)(bGPDframe *, KeyframeEditData *))
{
  /* error checker */
  if (gpl == NULL) {
    return false;
  }

  /* do loop */
  LISTBASE_FOREACH (bGPDframe *, gpf, &gpl->frames) {
    /* filter frames */
    if (gpf_ok(gpf, ked)) {
      gpf_cb(gpf, ked);
    }
  }

  /* nothing to return */
  return false;
}

where the callbacks would be :

gpf_ok for [1] : return gpf->framenum == ked->f1
gpf_cb for [1] : gpencil_frame_select(gpf, select_mode);
gpf_ok for [2] : return true
gpf_cb for [2] : gpencil_frame_select(gpf, select_mode);
gpf_ok for [3] :

    float pt[2] = {0};

    pt[0] = gpf->framenum;
    pt[1] = ked->channel_y;

    return ( (tool == BEZT_OK_CHANNEL_LASSO) && (keyframe_region_lasso_test(ked->data, pt))
    	  || (tool == BEZT_OK_CHANNEL_CIRCLE) && (keyframe_region_circle_test(ked->data,pt))

gpf_cb for [3] : gpencil_frame_select(gpf, select_mode);
gpf_ok for [4] : return IN_RANGE(gpf->framenum, ked->f1, ked->f2)
gpf_cb for [4] : gpencil_frame_select(gpf, select_mode);

Given the actual callbacks, the loop function could also be something like this :

OPTION 2

bool ANIM_gplay_keyframes_loop(bGPDlayer *gpl,
                                    KeyframeEditData *ked,
                                    eEditKeyframes_Select select_mode,
                                    eEditKeyframes_Validate tool)
{
  /* error checker */
  if (gpl == NULL) {
    return false;
  }

  /* do loop */
  LISTBASE_FOREACH (bGPDframe *, gpf, &gpl->frames) {

    float pt[2] = {0};
    pt[0] = gpf->framenum;
    pt[1] = ked->channel_y;

    bool gpf_ok = ((tool == BEZT_OK_FRAME) && (gpf->framenum == ked->f1)) ;
    gpf_ok = gpf_ok || ((tool == BEZT_OK_FRAMERANGE) && (IN_RANGE(gpf->framenum, ked->f1, ked->f2)));
    gpf_ok = gpf_ok || ((tool == BEZT_OK_CHANNEL_LASSO) && (keyframe_region_lasso_test(ked->data, pt)) ;
    gpf_ok = gpf_ok || ((tool == BEZT_OK_CHANNEL_CIRCLE) && (keyframe_region_circle_test(ked->data,pt));

    /* filter frames */
    if (gpf_ok){
      gpencil_frame_select(gpf, select_mode);
    }
  }

  /* nothing to return */
  return false;
}

The boolean computation could also be donne with a switch statement. Either way, may not be very good for performances.
Maybe a good compromise would be :
OPTION 3

bool ANIM_gplay_keyframes_loop(bGPDlayer *gpl,
                                    KeyframeEditData *ked,
                                    eEditKeyframes_Select select_mode,
                                    bool (*gpf_ok)(bGPDframe *, KeyframeEditData *))
{
  /* error checker */
  if (gpl == NULL) {
    return false;
  }

  /* do loop */
  LISTBASE_FOREACH (bGPDframe *, gpf, &gpl->frames) {
    /* filter frames */
    if (gpf_ok(gpf, ked)){
      gpencil_frame_select(gpf, select_mode);
    }
  }

  /* nothing to return */
  return false;
}

I am also open to hear your opinion on
OPTION 4
Do something completely different

My questions

First, what do you think about this issue, what would be the best option ?

Thing is, to implement this properly, I’d have to make code changes in both Gpencil and Animation parts :

  • the two last parameters (select_mode and (tool or gpf_ok)) have to be added to both summary_keyframes_loop and ANIM_animchannel_keyframes_loop
  • all gpf_ok functions need to be implemented, so they match the type bool (*gpf_ok)(bGPDframe *, KeyframeEditData *). This consists more or less in re-writing ED_gpencil_layer_select_frame, ED_gpencil_layer_select_frames, ED_gpencil_layer_frames_select_region, ED_gpencil_layer_frames_select_box but with different input parameters.

Would that be ok ?
FIY, so far, I’ve implemented option 2 because it was the easiest and just to make sure it worked, and it seems ok.

1 Like
2

One thing to note:
as it stands, *_keyframes_loop can be used for executing arbitrary callbacks, not just selection.
I personally used custom callbacks in D14811 since I like working with functional programming (callbacks instead of writing my own for-loops), but I understand this is not what this function “should be” used for, so reducing its scope while avoiding case-by-case boilerplate sounds more beneficial.

Whether a good solution here is having two functions (one specialized in selection, working for everything animation-related, and a second one that’s more flexible but needs more manual work for the different kinds of animation summaries etc.) or another solution, I’m not sure. But it might be worth thinking about.

Either way, if the decision is to stick with something like option 2 (changing the callback to a select mode enum), then make sure to update the function name as well to make this change clearer. Since *_keyframes_loop does not mention selection in any way.

3

Ok, I see. Actually, gpencil layers would need a different additional callback anyways, because the KeyframeEditFunc type is defined as short (*KeyframeEditFunc)(KeyframeEditData *ked, struct BezTriple *bezt); , and gpencil layer callbacks would need a GPFrame input parameter (instead of BezTriple).
So, in all 3 options, the input parameters (select mode, tool, gpl_ok, gpl_cb) need to be added to the function while maintaining the rest of the input parameters untouched. Something like this :

short ANIM_animchannel_keyframes_loop(KeyframeEditData *ked,
                                      bDopeSheet *ads,
                                      bAnimListElem *ale,
                                      KeyframeEditFunc key_ok,
                                      KeyframeEditFunc key_cb,
                                      FcuEditFunc fcu_cb,  
                                      eEditKeyframes_Select select_mode,
                                     bool (*gpf_ok)(bGPDframe *, KeyframeEditData *))
)

So I guess that wont be in conflict with your patch (expect you’d have to put dumb parameters for the gplayer ones…).

But I am wondering whether it makes sense or not ?
As you said, if ANIM_animchannel_keyframes_loop is meant to be used to loop in f-curves channel with arbitrary callbacks, not just selection, then maybe this is not the good way to approach the problem.

4

Well, I found a very easy solution that was under my noise the whole time, which is replacing each selection function (action_select.c:[1536|1645|380|627]) by :

[..]
  /* select the nominated keyframe on the given frame */
  if (ale->type == ANIMTYPE_GPLAYER) {
    <ED_gpencil_specific_function>
    ale->update |= ANIM_UPDATE_DEPS;
  }
  else if (ale->type == ANIMTYPE_MASKLAYER) {
    <ED_mask_specific_function>
  }
  else {
    if ((ale->type == ANIMTYPE_SUMMARY) &&  (ale->datatype == ALE_ALL)) {
      ListBase anim_data = {NULL, NULL};
      int filter;

      filter = (ANIMFILTER_DATA_VISIBLE | ANIMFILTER_LIST_VISIBLE /*| ANIMFILTER_CURVESONLY */ |
                ANIMFILTER_NODUPLIS);
      ANIM_animdata_filter(ac, &anim_data, filter, ac->data, ac->datatype);

      for (ale = anim_data.first; ale; ale = ale->next) {
        if (ale->type == ANIMTYPE_GPLAYER) {
    	  <ED_mask_specific_function>
          ale->update |= ANIM_UPDATE_DEPS;
        }
        else if (ale->type == ANIMTYPE_MASKLAYER) {
    	  <ED_mask_specific_function>
        }
      }

      ANIM_animdata_update(ac, &anim_data);
      ANIM_animdata_freelist(&anim_data);
    }
    if(!ELEM(ANIMCONT_GPENCIL, ANIMCONT_MASK)){
      ANIM_animchannel_keyframes_loop(&ked, ac->ads, ale, ok_cb, select_cb, NULL);
    }
  }

I still think this solution is not ideal and the readibility could be much improved by integrating the gplayers and gpmasks in the ANIM_animchannel_keyframes_loop (grease pencil have keyframes too ^^).