blender
/
blender
137
416
Fork 599
Code Issues 6.3k Pull Requests 774 Projects 19 Wiki Activity

Refactor: fcurve bounds functions #105177

Merged
Christoph Lendenfeld merged 16 commits from ChrisLend/blender:refactor_fcurve_bounds into main 2023-03-10 11:33:21 +01:00
Conversation 0 Commits 16 Files Changed 9 +20 -16
2 changed files with 20 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 40e149e06b - Show all commits

8
source/blender/blenkernel/intern/fcurve.c
View File

@ -670,7 +670,7 @@ static void calculate_bezt_bounds_y(BezTriple *bezt_array,
}
/* Right handle - only if applicable. */
dr.sybren marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-27 15:33:06 +01:00
Review
Copy Link

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.

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.
Christoph Lendenfeld commented 2023-03-02 13:03:14 +01:00
Review
Copy Link

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

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 :)
if (bezt.ipo == BEZT_IPO_BEZ) {
if (bezt->ipo == BEZT_IPO_BEZ) {
*r_min = min_ff(*r_min, bezt->vec[2][1]);
*r_max = max_ff(*r_max, bezt->vec[2][1]);
}
@ -710,8 +710,8 @@ static bool calculate_fpt_bounds(const FCurve *fcu, const float frame_range[2],
if (frame_range != NULL) {
/* Start index can be calculated because fpt has a key on every full frame. */
const int start_index = int(frame_range[0] - fcu->fpt[0].vec[0]);
const int end_index = start_index + int(frame_range[1] - frame_range[0]);
const int start_index = (int)(frame_range[0] - fcu->fpt[0].vec[0]);
const int end_index = start_index + (int)(frame_range[1] - frame_range[0]);
dr.sybren marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-27 15:39:28 +01:00
Review
Copy Link

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 the xmax. Per point you then only need to consider the y-coordinates, cutting the number of operations in half.

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 the `xmax`. Per point you then only need to consider the y-coordinates, cutting the number of operations in half.
Christoph Lendenfeld commented 2023-03-02 12:55:15 +01:00
Review
Copy Link

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.

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.
if (start_index > fcu->totvert - 1 || end_index < 0) {
/* Range is outside of keyframe samples. */
ChrisLend marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-27 15:41:49 +01:00
Review
Copy Link

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.

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.
@ -719,7 +719,7 @@ static bool calculate_fpt_bounds(const FCurve *fcu, const float frame_range[2],
}
/* Range might be partially covering keyframe samples. */
const int start_index_clamped = clamp_i(start_index, 0 fcu->totvert - 1);
const int start_index_clamped = clamp_i(start_index, 0, fcu->totvert - 1);
const int end_index_clamped = clamp_i(end_index, 0, fcu->totvert - 1);
r_bounds->xmin = fcu->fpt[start_index_clamped].vec[0];

28
source/blender/blenkernel/intern/fcurve_test.cc
View File

@ -367,19 +367,20 @@ TEST(BKE_fcurve, BKE_fcurve_calc_range)
/* All keys. */
success = BKE_fcurve_calc_range(fcu, &min, &max, false);
EXPECT_TRUE(success);
EXPECT_TRUE(success) << "A non-empty FCurve should have a range.";
ChrisLend marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-27 15:54:35 +01:00
Review
Copy Link

Avoid plain EXPECT_TRUE/EXPECT_FALSE. You can add an explanation with stream operators, like this:

EXPECT_TRUE(success) << "A non-empty FCurve should have a range";
Avoid plain `EXPECT_TRUE`/`EXPECT_FALSE`. You can add an explanation with stream operators, like this: ```cpp EXPECT_TRUE(success) << "A non-empty FCurve should have a range"; ```
EXPECT_FLOAT_EQ(fcu->bezt[0].vec[1][0], min);
EXPECT_FLOAT_EQ(fcu->bezt[4].vec[1][0], max);
/* Only selected. */
success = BKE_fcurve_calc_range(fcu, &min, &max, true);
EXPECT_FALSE(success);
EXPECT_FALSE(success)
<< "Using selected keyframes only should not find a range if nothing is selected.";
fcu->bezt[1].f2 |= SELECT;
fcu->bezt[3].f2 |= SELECT;
success = BKE_fcurve_calc_range(fcu, &min, &max, true);
EXPECT_TRUE(success);
EXPECT_TRUE(success) << "Range of selected keyframes should have been found.";
EXPECT_FLOAT_EQ(fcu->bezt[1].vec[1][0], min);
EXPECT_FLOAT_EQ(fcu->bezt[3].vec[1][0], max);
@ -389,7 +390,7 @@ TEST(BKE_fcurve, BKE_fcurve_calc_range)
fcurve_store_samples(fcu, NULL, sample_start, sample_end, fcurve_samplingcb_evalcurve);
success = BKE_fcurve_calc_range(fcu, &min, &max, true);
EXPECT_TRUE(success);
EXPECT_TRUE(success) << "FCurve samples should have a range.";
EXPECT_FLOAT_EQ(sample_start, min);
EXPECT_FLOAT_EQ(sample_end, max);
@ -422,7 +423,7 @@ TEST(BKE_fcurve, BKE_fcurve_calc_bounds)
/* All keys. */
success = BKE_fcurve_calc_bounds(
fcu, false /* sel only */, false /* include handles */, NULL /* frame range */, &bounds);
EXPECT_TRUE(success);
EXPECT_TRUE(success) << "A non-empty FCurve should have bounds.";
EXPECT_FLOAT_EQ(fcu->bezt[0].vec[1][0], bounds.xmin);
EXPECT_FLOAT_EQ(fcu->bezt[4].vec[1][0], bounds.xmax);
EXPECT_FLOAT_EQ(fcu->bezt[4].vec[1][1], bounds.ymin);
@ -431,14 +432,15 @@ TEST(BKE_fcurve, BKE_fcurve_calc_bounds)
/* Only selected. */
success = BKE_fcurve_calc_bounds(
fcu, true /* sel only */, false /* include handles */, NULL /* frame range */, &bounds);
EXPECT_FALSE(success);
EXPECT_FALSE(success)
<< "Using selected keyframes only should not find bounds if nothing is selected.";
fcu->bezt[1].f2 |= SELECT;
fcu->bezt[3].f2 |= SELECT;
success = BKE_fcurve_calc_bounds(
fcu, true /* sel only */, false /* include handles */, NULL /* frame range */, &bounds);
EXPECT_TRUE(success);
EXPECT_TRUE(success) << "Selected keys should have been found.";
EXPECT_FLOAT_EQ(fcu->bezt[1].vec[1][0], bounds.xmin);
EXPECT_FLOAT_EQ(fcu->bezt[3].vec[1][0], bounds.xmax);
EXPECT_FLOAT_EQ(fcu->bezt[1].vec[1][1], bounds.ymin);
@ -447,7 +449,7 @@ TEST(BKE_fcurve, BKE_fcurve_calc_bounds)
/* Including handles. */
success = BKE_fcurve_calc_bounds(
fcu, false /* sel only */, true /* include handles */, NULL /* frame range */, &bounds);
EXPECT_TRUE(success);
EXPECT_TRUE(success) << "A non-empty FCurve should have bounds including handles.";
EXPECT_FLOAT_EQ(fcu->bezt[0].vec[0][0], bounds.xmin);
EXPECT_FLOAT_EQ(fcu->bezt[4].vec[2][0], bounds.xmax);
EXPECT_FLOAT_EQ(fcu->bezt[4].vec[1][1], bounds.ymin);
@ -460,13 +462,13 @@ TEST(BKE_fcurve, BKE_fcurve_calc_bounds)
range[1] = 30;
success = BKE_fcurve_calc_bounds(
fcu, false /* sel only */, false /* include handles */, range /* frame range */, &bounds);
EXPECT_FALSE(success);
EXPECT_FALSE(success) << "A frame range outside the range of keyframes should not find bounds.";
range[0] = 0;
range[1] = 18.2f;
success = BKE_fcurve_calc_bounds(
fcu, false /* sel only */, false /* include handles */, range /* frame range */, &bounds);
EXPECT_TRUE(success);
EXPECT_TRUE(success) << "A frame range within the range of keyframes should find bounds.";
EXPECT_FLOAT_EQ(fcu->bezt[0].vec[1][0], bounds.xmin);
EXPECT_FLOAT_EQ(fcu->bezt[3].vec[1][0], bounds.xmax);
EXPECT_FLOAT_EQ(fcu->bezt[1].vec[1][1], bounds.ymin);
@ -475,7 +477,8 @@ TEST(BKE_fcurve, BKE_fcurve_calc_bounds)
/* Range and handles. */
success = BKE_fcurve_calc_bounds(
fcu, false /* sel only */, true /* include handles */, range /* frame range */, &bounds);
EXPECT_TRUE(success);
EXPECT_TRUE(success)
<< "A frame range within the range of keyframes should find bounds with handles.";
EXPECT_FLOAT_EQ(fcu->bezt[0].vec[0][0], bounds.xmin);
EXPECT_FLOAT_EQ(fcu->bezt[3].vec[2][0], bounds.xmax);
EXPECT_FLOAT_EQ(fcu->bezt[1].vec[1][1], bounds.ymin);
@ -486,7 +489,8 @@ TEST(BKE_fcurve, BKE_fcurve_calc_bounds)
range[1] = 18.2f;
success = BKE_fcurve_calc_bounds(
fcu, true /* sel only */, true /* include handles */, range /* frame range */, &bounds);
EXPECT_TRUE(success);
EXPECT_TRUE(success)
<< "A frame range within the range of keyframes should find bounds of selected keyframes.";
EXPECT_FLOAT_EQ(fcu->bezt[3].vec[0][0], bounds.xmin);
EXPECT_FLOAT_EQ(fcu->bezt[3].vec[2][0], bounds.xmax);
EXPECT_FLOAT_EQ(fcu->bezt[3].vec[2][1], bounds.ymin);