Refactor: fcurve bounds functions #105177
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#105177
Loading…
Reference in New Issue
No description provided.
Delete Branch "ChrisLend/blender:refactor_fcurve_bounds"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Refactor the following functions and cover with tests
BKE_fcurve_calc_bounds
- used to get therctf
bounding box of an fcurveBKE_fcurve_calc_range
- used to get only the x-range, potentially faster when not needing y extentsget_fcurve_end_keyframes
has been replaced withget_bounding_bezt_indices
- dealing with indices allows to iterate over that range laterBKE_fcurve_calc_bounds
rctf
instead of float pointersBKE_fcurve_calc_range
do_min_length
it was always false, and this function shouldn't concern itself with clamping.Calling code can do that if the return bool is false
BKE_fcurve_calc_bounds
Nice work, thanks!
Just a few inline comments.
@ -368,3 +368,3 @@
/**
* Calculate the extents of F-Curve's keyframes.
* Calculate the x range of the given F-Curve's data.
*/
Document what the return value means.
@ -370,3 +370,2 @@
*/
bool BKE_fcurve_calc_range(
const struct FCurve *fcu, float *min, float *max, bool do_sel_only, bool do_min_length);
bool BKE_fcurve_calc_range(const struct FCurve *fcu, float *min, float *max, bool do_sel_only);
Return parameters should be prefixed with
r_
. I think this is an excellent opportunity to tackle that too.And it's weird, French is not my favourite language, I'm not even that good at it. But still when I read
do_sel_only
I'm thinking "only salt?". No strong opinion here, but at the risk of doing even more in the same patch, I wouldn't mind if that parameter were renamed touse_selected_keys_only
. I'll leave the final decision on whether to include that change or not to you though.@ -640,2 +632,2 @@
const bool use_range = range != NULL;
*r_min = bezt_array[index_range[0]].vec[1][0];
*r_max = bezt_array[index_range[0]].vec[1][0];
I think this should be using
index_range[1]
.@ -663,0 +635,4 @@
if (include_handles) {
/* Need to check all handles because they might extend beyond their neighboring keys. */
for (int i = index_range[0]; i <= index_range[1]; i++) {
BezTriple bezt = bezt_array[i];
bezt
can beconst
, same in the function below.Better to not make a copy at all here.
BezTriple
is a rather large struct. In C the best option isconst BezTriple *bezt = &bezt_array[i];
The compiler probably optimizes that but it's a good way to show intentions too.I've made it a
Beztriple *
now@ -663,0 +640,4 @@
*r_max = max_fff(*r_max, bezt.vec[1][0], bezt.vec[2][0]);
}
}
else {
If I'm right about the initial assignment to
*r_max
usingindex_range[1]
, this entireelse
block can be removed.@ -717,0 +669,4 @@
if (include_handles) {
/* Left handle - only if applicable.
* NOTE: for the very first keyframe,
* the left handle actually has no bearings on anything. */
This is a tricky one. If the idea is to zoom the FCurve so that all the keys and handles are on screen, it doesn't matter that the left handle has no meaning; an animator may still want to interact with it in order to edit the animation. And with, for example, "Aligned" handle type, manipulating the left handle will move the right one, and thus still has an effect.
I was hesitant to change that because I wasn't quite sure of the purpose
I removed that logic completely now. IMO this function shouldn't concern itself with the visibility/functionality of handles.
Controversial though :)
@ -734,3 +697,1 @@
}
if (ymax) {
*ymax = ymaxv;
if (fcu->bezt) {
I think this nesting of conditionals is too deep. Better to split up the function into one for Bezier curves and one for baked points.
@ -746,0 +711,4 @@
if (frame_range != NULL) {
bool found_indices = false;
for (int i = 0; i < fcu->totvert; i++) {
if (fcu->fpt[i].vec[0] < frame_range[0] || fcu->fpt[i].vec[0] > frame_range[1]) {
Since there are no handles, you can use the ordering of the points and do a binary search for the start of the range. You can then immediately handle the
xmin
, and when the last point in the range is found, update thexmax
. Per point you then only need to consider the y-coordinates, cutting the number of operations in half.instead of doing a binary search i calculate the index based on the assumption that a baked fcurve has a sample on every full frame.
@ -746,0 +714,4 @@
if (fcu->fpt[i].vec[0] < frame_range[0] || fcu->fpt[i].vec[0] > frame_range[1]) {
continue;
}
if (!found_indices) {
Initialize
r_bounds
to±∞
(so all minimum values to∞
and all maximum values to-∞
). Those are valid IEEE 754 floating point values, and work well with the min/max functions. That'll remove a boolean, a condition, and half of the lines of code.@ -746,0 +728,4 @@
r_bounds->ymax = max_ff(r_bounds->ymax, fcu->fpt[i].vec[1]);
}
}
return found_indices;
Here you can return
r_bounds->xmin <= r_bounds->xmax
.@ -747,2 +733,2 @@
if (xmax) {
*xmax = use_range ? range[1] : 1.0f;
else {
/* X range can be directly calculated from end verts. */
A different structure of the function could work well here:
totvert - 1
, or the result of a binary search)can you clarify that, is that in regards to finding the start index when using a frame range?
I was thinking about code like this:
This uses
INFINITY
instead ofFLT_MAX
so that valid and invalid values can always be distinguished. And it separates "determine the start/endices" from "finding the bounds".I'm not 100% about the name of
start_index_f
, mayberange_start_index
would be better.Update: using
BLI_rctf_is_valid()
instead of writing all of that out.thanks got it
and changed to that code
@ -349,0 +367,4 @@
/* All keys. */
success = BKE_fcurve_calc_range(fcu, &min, &max, false);
EXPECT_TRUE(success);
Avoid plain
EXPECT_TRUE
/EXPECT_FALSE
. You can add an explanation with stream operators, like this:Just one thing that Clang-Tidy will trip over, rest LGTM! You can resolve this before merging the PR.
@ -774,3 +749,2 @@
if (bezt_first) {
BLI_assert(bezt_last != NULL);
else if (fcu->fpt) {
No
else
afterreturn
;clang-tidy
will complain about this.