Why is the keyframe comparison needed? Isn't it enough to know if the user cancelled or not?

When auto-keyframe is enabled and animation is playing, cancel doesn't remove all the keyframes added.

In D5418#122904, @Campbell Barton (campbellbarton) wrote:

Why is the keyframe comparison needed? Isn't it enough to know if the user cancelled or not?

When auto-keyframe is enabled and animation is playing, cancel doesn't remove all the keyframes added.

thx for checking on this @Campbell Barton (campbellbarton).
Not 100% sure if I understand correctly, but

• from modal we only want a keyframe if the the playback is running [thus no force but frame comparison will enable keyframing]
• we dont want a keyframe when canceled [no playback running, no force, frame comparision then prevent keyframing...]
• we do want a keyframe when fly/walk are confirmed [no playback running, then force keyframing]

Not sure if there are more elegant ways to detect playback?

and yes, this is not about removing keyframes on cancel [with running playback -- which would be cool but quite a hassle?]

Okay, the confusing part of this patch is it reads as if force_autokey would make auto-key happen, when it just lets keys be inserted if the option is already enabled.

Another confusion is there might be animation playback without auto-key being enabled so I was looking to see if this case might cause problems.

I think the logic of the patch is fine, just the variable name is misleading. it could be flipped, eg:

static void walkMoveCamera(bContext *C,
                           WalkInfo *walk,
                           const bool do_rotate,
                           const bool do_translate,
                           const bool is_cancel)
{
  Scene *scene = walk->scene;
  /* Allow auto-key even when canceling because previous frames can have keys recorded, 
   * canceling auto-keys from previous frames isn't supported. */
  bool use_autokey = !is_cancel || (walk->frame_last != CFRA);
  walk->frame_last = CFRA;

  ED_view3d_cameracontrol_update(
      walk->v3d_camera_control, use_autokey, C, do_rotate, do_translate);
}

The way frame_last is used is a bit indirect, really we want to know is have we inserted a keyframe yet, instead there could be a variable is_first_autokey or has_autokey and check this instead.

In D5418#122918, @Campbell Barton (campbellbarton) wrote:

Okay, the confusing part of this patch is it reads as if force_autokey would make auto-key happen, when it just lets keys be inserted if the option is already enabled.

Agree the naming was bad. How about is_confirm?

Another confusion is there might be animation playback without auto-key being enabled so I was looking to see if this case might cause problems.

Dont see any problems here

I think the logic of the patch is fine, just the variable name is misleading. it could be flipped, eg:

static void walkMoveCamera(bContext *C,
                           WalkInfo *walk,
                           const bool do_rotate,
                           const bool do_translate,
                           const bool is_cancel)
{
  Scene *scene = walk->scene;
  /* Allow auto-key even when canceling because previous frames can have keys recorded, 
   * canceling auto-keys from previous frames isn't supported. */
  bool use_autokey = !is_cancel || (walk->frame_last != CFRA);
  walk->frame_last = CFRA;

  ED_view3d_cameracontrol_update(
      walk->v3d_camera_control, use_autokey, C, do_rotate, do_translate);
}

---

The way frame_last is used is a bit indirect, really we want to know is have we inserted a keyframe yet, instead there could be a variable is_first_autokey or has_autokey and check this instead.

I dont get the drive to make this relate to cancel? [if we only check for canceling, then the keyframe has already landed?]
Also dont see how we could get around checking framechange? You say we really want to know if we inserted a keyframe already, but where would that lead us? You start the operator, you get to walkMoveCamera right away, you check you havent inserted a keyframe yet, OK, how do you check if you want one now? Sorry, might be blind, but would is_first_autokey or has_autokey help? We are already screwed if we insert a single keyframe (if the user decides to cancel later)
Hope I'm not missing the obvious... thanx for having a look again @Campbell Barton (campbellbarton)

Any reason to keep the frame comparison? It seems indirect/error prone (a file can play looping over the same frame for example).

In D5418#123328, @Campbell Barton (campbellbarton) wrote:

Any reason to keep the frame comparison? It seems indirect/error prone (a file can play looping over the same frame for example).

also talked to @Campbell Barton (campbellbarton) in #blendercoders: playback detection is needed, yes