Page MenuHome

Extract keyframe segment calculation
ClosedPublic

Authored by Christoph Lendenfeld (ChrisLend) on Oct 27 2020, 4:43 PM.

Details

Summary

This patch extracts the search for keyframe segments (consecutive selection of keys).
It will be reused by future graph editor operators.

This patch was simplified from a patch that also changed the way a warning is displayed for incompatible keys.

Diff Detail

Repository
rB Blender

Event Timeline

Christoph Lendenfeld (ChrisLend) requested review of this revision.Oct 27 2020, 4:43 PM
Christoph Lendenfeld (ChrisLend) created this revision.
Christoph Lendenfeld (ChrisLend) retitled this revision from Extract calculation of keyframe segments to Refactor keyframe segment calculation.Oct 27 2020, 4:48 PM
Christoph Lendenfeld (ChrisLend) edited the summary of this revision. (Show Details)
Wayde Moss (GuiltyGhost) added inline comments.
source/blender/editors/animation/keyframes_general.c
338

It looks like only decimate() makes use of the tag. I think the unset should be placed in prepare_for_decimate(). Otherwise, it looks like the keyframe isn't completely prepared?

486–487

Unused return value due to refactor.

source/blender/editors/space_graph/graph_slider_ops.c
379 ↗(On Diff #31099)

Function name can be improved since nothing is actually validated. The functions seems to only warn the animator about skipped keys? Ignore me if I'm wrong.

source/blender/editors/animation/keyframes_general.c
439

Use FCurve* instead of bAnimListElem* since ale is immediately casted to FCurve* then no longer used.

Christoph Lendenfeld (ChrisLend) edited the summary of this revision. (Show Details)
  • simplify patch to be only about extracting a function to calculate keyframe segments
Christoph Lendenfeld (ChrisLend) marked 3 inline comments as done.Sep 4 2021, 6:38 PM
Christoph Lendenfeld (ChrisLend) added inline comments.
source/blender/editors/animation/keyframes_general.c
338

I think it can be moved somwhere else, but in this patch I'd like to keep it with the moved code

439

good point. Will be done in separate cleanup patch

486–487

doesn't apply anymore to to patch simplification.
Noted for follow up patch though

Christoph Lendenfeld (ChrisLend) retitled this revision from Refactor keyframe segment calculation to Extract keyframe segment calculation.Sep 14 2021, 8:52 PM
Sybren A. Stüvel (sybren) requested changes to this revision.Oct 18 2021, 1:06 PM

It's a nice cleanup, thanks!

I've added a few minor notes. The thing that is bigger IMO is the fcu->bezt[i].f2 &= ~SELECT; line. This means that the keyframe selection by the user is modified, which is a change in behaviour. Selection is something that Blender should treat carefully, as it can take an artist's effort and care to select things, and the deselection could happen off-screen and go unnoticed.

From what I understand of the code, the (change in) selection is used as a way to communicate between two bits of the code (the "analysis" bit and the "do the work" bit). Is there another way to get this communication that doesn't rely on changing the selection? Maybe the solution lies in a simultanous refactor of prepare_for_decimate() -- that function also does two things (again it's an "analysis" bit of whether decimation is possible, and the "do the work" bit to actually do the preparation). If those would be split up, the "analysis" bit could be called from both places, and the find_fcurve_segment() function could be turned into a find_fcurve_decimatable_segment() function.

source/blender/editors/animation/keyframes_general.c
323

The documentation & function name could be a bit clearer. How's this?

/** Find the largest segment of consecutive selected curve points, starting from \a start_index.
 * \param r_segment_start_idx returns the start index of the segment.
 * \param r_segment_len returns the number of curve points in the segment. 
 * \return whether such a segment was found or not.*/
static bool find_fcurve_selected_segment(...)
336

selected can be declared as const bool here. That way its scope is clearer, as well as the fact that it's no longer changed once set.

338

Based on the function name & documentation, it should just find fcurve segments, and not also do some other task. If that other task cannot be avoided, it must be documented as part of the responsibilities of this function.

348–349

Nice to have such explanation in the code, makes it super easy to follow.

354–359

I'm not a fan of if (condition) return true; else return false; constructs, as those basically are the same as return condition;. In this case I do like the comments, so I'm not 100% convinced the code simplification makes things clearer. What do you think of this @Christoph Lendenfeld (ChrisLend) ?

/* If the last curve point was in the segment, `r_segment_len` and `r_segment_start_idx`
 * are already updated. */
return in_segment;
454–456

I think this changes some of the semantics. In the original code, the selection was maintained, and can_decimate_all_selected = false then makes sense. In the new code, the keyframe is deselected when it cannot be decimated. Not only does this change the user's keyframe selection, it also makes the "can decimate all selected" concept a bit weird -- after all, once this code is done running, everything that is still selected can still be decimated.

This revision now requires changes to proceed.Oct 18 2021, 1:06 PM
Christoph Lendenfeld (ChrisLend) marked 3 inline comments as done.
  • adress comments
  • don't deselect keys anymore
  • instead add a new flag to the bezt BEZT_FLAG_IGNORE_TAG to mark keys that cannot be worked on

@Sybren A. Stüvel (sybren) let me know what you think of this solution. Is adding this new flag a valid option?
I can't use the BEZT_FLAG_TEMP_TAG because that is used by the decimate algorithm

Christoph Lendenfeld (ChrisLend) marked 4 inline comments as done.Nov 7 2021, 6:39 PM
Christoph Lendenfeld (ChrisLend) marked an inline comment as done.

I think no longer using the selection, and instead use an explicit flag for this, is a good approach.

I'm not entirely sure about the name, though, as BEZT_FLAG_IGNORE_TAG could be interpreted as "ignore BEZT_FLAG_TEMP_TAG". I must say I'm not entirely happy with the name BEZT_FLAG_TEMP_TAG either; usually a "flag" is something that's permanent data that's saved with the blend file, and "tag" is temporary runtime-only. Having a thing named with both makes things confusing. Not something that you can do anything about in this patch, I'm just painting a slightly bigger picture of what goes through my mind when considering a better name. I guess the conflict comes from having a "tag" stored in a "flags" field, and that's just what we'll have to deal with for now. So yes, keep BEZT_FLAG_IGNORE_TAG, but add a comment at its declaration where you explain that certain operations can ignore keyframe points with this flag. That way it's at least clear it's not some tag that gets ignored, but the bezt itself.

source/blender/editors/animation/keyframes_general.c
337

can be const bool

452–453

deselect → ignore

  • Added a comment to BEZT_FLAG_IGNORE_TAG
  • changed point_is_ignored to const
  • Tweaked comment

I also added a line that removes BEZT_FLAG_IGNORE_TAG after decimation. I think it's a good idea to clean that up.

This revision is now accepted and ready to land.Nov 23 2021, 5:33 PM