Details

Reviewers

Commits

rB8e3879ab5276: Cleanup: Rename & refactor several F-curve functions

Summary

This change was initially spurred by the fact that delete_fcurve_keys was improperly named, but this was a good opportunity to fix the location and naming of a few of these functions.

The functions formerly known as delete_fcurve_key, delete_fcurve_keys, and clear_fcurve_keys have been moved out of ED_keyframes_edit.h and have been moved to BKE_fcurve.h and have been renamed according to hierarchical naming rules.

Below is a table of the naming changes.

From	To
`delete_fcurve_key(fcu, index, do_recalc)`	`BKE_fcurve_delete_key(fcu, index)`
`delete_fcurve_keys(fcu)`	`BKE_fcurve_delete_keys_selected(fcu)`
`clear_fcurve_keys(fcu)`	`BKE_fcurve_delete_keys_all(fcu)`
`calchandles_fcurve()`	`BKE_fcurve_handles_recalc()`
`calchandles_fcurve_ex()`	`BKE_fcurve_handles_recalc_ex()`

The function formerly known as delete_fcurve_key no longer takes a do_fast parameter, which determined whether or not to call calchandles_fcurve. Now, the responsibility is on the caller to run the new BKE_fcurve_handles_recalc function if they have want to recalculate the handles.

In addition, there is now a new static private function called fcurve_bezt_free which sets the key count to zero and frees the key array. This function is now used in couple of instances of functionally equivalent code. Note that BKE_fcurve_delete_keys_all is just a wrapper around fcurve_bezt_free.

Diff Detail

Repository: rB Blender

Event Timeline

Colin Basnett (cmbasnett) requested review of this revision.Jun 23 2022, 10:40 PM

Colin Basnett (cmbasnett) created this revision.

Harbormaster completed remote builds in B22663: Diff 52846.Jun 23 2022, 10:40 PM

Colin Basnett (cmbasnett) retitled this revision from Renamed `delete_fcurve_keys` to `delete_selected_fcurve_keys`. to Renamed delete_fcurve_keys to delete_selected_fcurve_keys.Jun 23 2022, 10:40 PM

Colin Basnett (cmbasnett) added a reviewer: Sybren A. Stüvel (sybren).

Colin Basnett (cmbasnett) added a project: Animation & Rigging.

Good find, I agree the naming is confusing.

I feel that the original code is not in the right place. It would make sense to have such functions (delete_fcurve_key, delete_fcurve_keys, clear_fcurve_keys) in blenkernel instead of editors.

Moving these to BKE would also give the opportunity to name them in a more general-to-specific hierarchical way. That way alphabetical sorting & auto-completion will group related functions together:

From	To
`delete_fcurve_key(fcu, index, do_recalc)`	`BKE_fcurve_delete_key(fcu, index)`	Remove the `do_recalc` boolean parameter. The caller can just do the call to `calchandles_fcurve()` if necessary. All but one call to the `delete_fcurve_key()` have a hard-coded value for that parameter anyway.
`delete_fcurve_keys(fcu)`	`BKE_fcurve_delete_keys_selected(fcu)`
`clear_fcurve_keys(fcu)`	`BKE_fcurve_delete_keys_all(fcu)`
`calchandles_fcurve()`	`BKE_fcurve_handles_recalc()`	This function is already contained in BKE (`BKE_fcurve.h`), so this change is a tiny bit out of scope here. IMO it is necessary though to pull the call to this function out of `BKE_fcurve_delete_key()`, for consistency in naming & local understandability of what's happening.

If you want to go for even more clarity in naming, you could consider adding an internal static function fcurve_bezt_free(fcu) that contains the functionality of the current clear_fcurve_keys(fcu), and have both BKE_fcurve_delete_keys_selected(fcu) and BKE_fcurve_delete_keys_all(fcu) call that one (respectively conditionally and unconditionally).

This revision now requires changes to proceed.Jun 24 2022, 11:04 AM

Renamed and re-factored F-curve key manipulation functions.

Harbormaster completed remote builds in B22812: Diff 53141.Jul 1 2022, 3:54 AM

Colin Basnett (cmbasnett) retitled this revision from Renamed delete_fcurve_keys to delete_selected_fcurve_keys to Renamed and refactored several F-curve key manipulation functions.Jul 1 2022, 4:12 AM

Colin Basnett (cmbasnett) edited the summary of this revision. (Show Details)

Colin Basnett (cmbasnett) mentioned this in D15283: Add clear method to FCurveKeyframePoints.Jul 1 2022, 4:32 AM

Thanks for the big overhaul, this is a really nice improvement.

One thing though: the calls to delete_fcurve_key(…, 1); have now been replaced with only a call to BKE_fcurve_delete_key(…);. These should be followed by a call to BKE_fcurve_handles_recalc();. If these are not necessary for some reason, I would still ask to put them in here. That way the refactor won't change any of the functionality, making it easier to grasp what's going on.

If there are places where things can be optimised by removing BKE_fcurve_handles_recalc(); calls, that's for another patch.

source/blender/editors/animation/keyframing.c
1285–1290	These two calls originally passed `do_recalc=true`, which means here the calls to `BKE_fcurve_handles_recalc(fcu);` is missing.
1688–1689	missing call to `BKE_fcurve_handles_recalc(fcu);`
2715–2716	missing call to `BKE_fcurve_handles_recalc(fcu);`
source/blender/editors/armature/pose_lib.c
619–620	missing call to `BKE_fcurve_handles_recalc(fcu);`
source/blender/python/intern/bpy_rna_anim.c
490–491	missing call to `BKE_fcurve_handles_recalc(fcu);`

This revision now requires changes to proceed.Jul 1 2022, 10:44 AM

Added missing BKE_fcurve_handles_recalc calls

Harbormaster completed remote builds in B22922: Diff 53475.Jul 10 2022, 1:46 AM

Colin Basnett (cmbasnett) marked 4 inline comments as done.Jul 10 2022, 1:47 AM

Almost there, just one more subtle change that was introduced & should be addressed.

source/blender/editors/animation/keyframing.c
1294–1295	This is a subtle semantic change. There are four possible values for `insert_mode`. Before, handles were only recalculated for two of those, and the new code recalculates handles for all four of them. Just put the `BKE_fcurve_handles_recalc(fcu);` calls directly underneath the `BKE_fcurve_delete_key()` calls.

This revision now requires changes to proceed.Jul 12 2022, 11:13 AM

No longer calling BKE_fcurve_handles_recalc for insert_modes where it does not apply

Harbormaster completed remote builds in B22968: Diff 53575.Jul 13 2022, 11:40 AM

Colin Basnett (cmbasnett) marked an inline comment as done.Jul 13 2022, 11:41 AM

🎉

This revision is now accepted and ready to land.Jul 14 2022, 10:23 AM

Closed by commit rB8e3879ab5276: Cleanup: Rename & refactor several F-curve functions (authored by Colin Basnett (cmbasnett), committed by Sybren A. Stüvel (sybren)). · Explain WhyJul 14 2022, 10:27 AM

This revision was automatically updated to reflect the committed changes.

Sybren A. Stüvel (sybren) added a commit: rB8e3879ab5276: Cleanup: Rename & refactor several F-curve functions.

Renamed and refactored several F-curve key manipulation functions
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 53618

source/blender/blenkernel/BKE_fcurve.h

source/blender/blenkernel/intern/action_mirror.c

source/blender/blenkernel/intern/fcurve.c

source/blender/blenkernel/intern/fmodifier.c

source/blender/editors/animation/anim_deps.c

source/blender/editors/animation/drivers.c

source/blender/editors/animation/fmodifier_ui.c

source/blender/editors/animation/keyframes_edit.c

source/blender/editors/animation/keyframes_general.c

source/blender/editors/animation/keyframing.c

source/blender/editors/armature/pose_lib.c

source/blender/editors/gpencil/gpencil_convert.c

source/blender/editors/include/ED_keyframes_edit.h

source/blender/editors/space_action/action_edit.c

source/blender/editors/space_graph/graph_buttons.c

source/blender/editors/space_graph/graph_edit.c

source/blender/editors/space_nla/nla_edit.c

source/blender/editors/transform/transform_convert.c

source/blender/editors/transform/transform_convert_graph.c

source/blender/io/collada/AnimationImporter.cpp

source/blender/io/collada/BCAnimationCurve.cpp

source/blender/makesrna/intern/rna_fcurve.c

source/blender/python/intern/bpy_rna_anim.c

Renamed and refactored several F-curve key manipulation functionsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 53618

source/blender/blenkernel/BKE_fcurve.h

source/blender/blenkernel/intern/action_mirror.c

source/blender/blenkernel/intern/fcurve.c

source/blender/blenkernel/intern/fmodifier.c

source/blender/editors/animation/anim_deps.c

source/blender/editors/animation/drivers.c

source/blender/editors/animation/fmodifier_ui.c

source/blender/editors/animation/keyframes_edit.c

source/blender/editors/animation/keyframes_general.c

source/blender/editors/animation/keyframing.c

source/blender/editors/armature/pose_lib.c

source/blender/editors/gpencil/gpencil_convert.c

source/blender/editors/include/ED_keyframes_edit.h

source/blender/editors/space_action/action_edit.c

source/blender/editors/space_graph/graph_buttons.c

source/blender/editors/space_graph/graph_edit.c

source/blender/editors/space_nla/nla_edit.c

source/blender/editors/transform/transform_convert.c

source/blender/editors/transform/transform_convert_graph.c

source/blender/io/collada/AnimationImporter.cpp

source/blender/io/collada/BCAnimationCurve.cpp

source/blender/makesrna/intern/rna_fcurve.c

source/blender/python/intern/bpy_rna_anim.c

Renamed and refactored several F-curve key manipulation functions
ClosedPublic
Actions

Revision Contents
Changeset List