Page MenuHome

Fix T68250: Camera keyframing (Walk/Fly) despite canceling movement
ClosedPublic

Authored by Philipp Oeser (lichtwerk) on Mon, Aug 5, 5:56 PM.

Diff Detail

Repository
rB Blender

Event Timeline

Restore end function locations

  • Minor formatting changes

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.

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.

Campbell Barton (campbellbarton) requested changes to this revision.Mon, Aug 5, 8:52 PM
This revision now requires changes to proceed.Mon, Aug 5, 8:52 PM
  • rename force_autokey --> is_confirm

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).

Philipp Oeser (lichtwerk) updated this revision to Diff 16921.EditedWed, Aug 7, 8:25 PM
  • better playback detection (only do once on init, avoid missing key on first frame)

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

  • correct comment: framechange --> playback
This revision is now accepted and ready to land.Wed, Aug 7, 8:44 PM