Fix for "off-by-1" bug when sampling "baked" curve data towards the end of the curve.

[tektrip-biggles]

I noticed that when using Curve::sample() & Curve::sample_baked(), I was getting different results for the same curve, when sampling an offset close to 1.

By setting a simple, rising curve's bake_resolution to a low value (e.g. 10) and plotting the sample values on a line, the error can be seen quite clearly.

Using Curve::sample(), the curve appears as expected:

Using Curve::sample_baked() for the same curve results in a strange "kink" followed by a horizontal, flat section towards the end (top right) of the curve:

This was tracked down to a small math error in Curve::bake(). Note the original function below from curve.cpp

void Curve::bake() {
	_baked_cache.clear();

	_baked_cache.resize(_bake_resolution);

	for (int i = 1; i < _bake_resolution - 1; ++i) {
		real_t x = i / static_cast<real_t>(_bake_resolution);
		real_t y = sample(x);
		_baked_cache.write[i] = y;
	}

	if (_points.size() != 0) {
		_baked_cache.write[0] = _points[0].position.y;
		_baked_cache.write[_baked_cache.size() - 1] = _points[_points.size() - 1].position.y;
	}

	_baked_cache_dirty = false;
}

Since the first and last baked points will be set to be equal to the first and last points of the curve itself, the loop part is essentially used to "cut" the curve into $n-1$ equally sized intervals with $n-2$ values baked between each interval (resulting in $n$ baked points in total once the first & last points are also added). To illustrate the situation more concretely, the error occurs because if we are to create, say, 10 "baked points", that will actually result in only $9$ sections or "intervals" (and $8$ "middle" baked points) so the length of each section should be $\frac{1}{9}$ instead of $\frac{1}{10}$.

In other words, this line was assuming the intervals between each baked point would be $\frac{1}{n}$:

real_t x = i / static_cast<real_t>(_bake_resolution);

But if we have $n-1$ intervals, the length of each should in fact be $\frac{1}{n-1}$, so the line just needs to be changed to:

real_t x = i / static_cast<real_t>(_bake_resolution - 1);

This will ensure that each of the "middle" baked points is in fact equally spaced between the "start" and "end" baked points.

Similarly, a corresponding change needs to be made to Curve::sample_baked(), changing:

real_t fi = p_offset * _baked_cache.size();

to:

real_t fi = p_offset * (_baked_cache.size() - 1);

Making the 2 small changes in this pull request results in Curve::sample_baked() returning the correct and expected values

Fixes #76623

[adamscott]

Hi @tektrip-biggles!

Can you create an issue about the baked curve issue? It's preferred for PRs to have linked issues. So we can know which issues have been created and which ones are fixed.

It will help to see if your PR fixes the issue too, as a MRP (a minimal reproduction project) is asked on creation.

Thank you for your first PR, by the way!

[anvilfolk]

I personally feel like for a bug as small as this one (2 line changes and a pretty believable explanation), maybe creating an associated issue might not be necessary :)

Also, notice that the tests are failing, which is to be expected if the values were generated using the previous (potentially buggy) implementation. Would you be able to check those tests that are failing and use them to show that your implementation is correct?

The PR would need to change those tests to the proper numbers, if it turns out that the tests are indeed "buggy" :)

[kleonc]

Checked the code, LGTM. I've added some comments for potential additional changes, they should't affect the behavior though (so they're not necessary).

As you can see tests fail after the changes, they need to be updated (tests/scene/test_curve.h). Would make sense to add test case baked-sampling at (bake_resolution - 0.5) / bake_resolution which shouldn't produce the same result as sampling at 1.0. Such test case would fail before/pass after this PR.

[kleonc]

The code below could be simplified to:

int i = Math::floor(fi);
if (i < 0) {
	return _baked_cache[0];
} else if (i + 1 < _baked_cache.size()) {
	real_t t = fi - i;
	return Math::lerp(_baked_cache[i], _baked_cache[i + 1], t);
} else {
	return _baked_cache[_baked_cache.size() - 1];
}

But it should work correctly as is.

[kleonc]

These _baked_cache.write[i] (this one and the following ones) could be replaced by obtaining the pointer once before the loop like: real_t *baked_cache_ptr = _baked_cache.ptrw(); and doing _baked_cache_ptr[i], less potential overhead.

[tektrip-biggles]

Hey, have created a MRP for others to toggle between sample_baked & sample as well as changing the bake_interval and the number of samples taken to draw a simple $y = x^2$ curve. I can file a bug report as well if needs be once I find the place to do that as well :-)

Will see about fixing those tests as well...

[tektrip-biggles]

Ok, in terms of the tests, I've added the "sawtooth" ones that were being used to test into my MRP, along with some controls to make it easier to measure specific sample values using both sample & sample_baked:

MRP_Curve_Sample_Testing.mov

(Note that the above is using the "fixed" code)

It's interesting that with a bake resolution of 11, the new code results in getting the exact same sample/output values from the test offset/input values (0.1 & 0.7 etc) for the sawtooth curves but this is more or less just a coincidence. Due to the very nature of baking curves, we shouldn't expect to get entirely consistent results from every bake resolution. Sometimes it will be more accurate & other times it will be less accurate.

As a result I'm not sure if the current tests for Curve::sample_baked are really super helpful tbh. If anything, I'd advise removing them until better ones can be written since they may give both "false positives" and "false negatives" just depending on what the bake resolution is... I'm a little inexperienced when it comes to writing unit tests though, so perhaps someone else would be better placed to write a more appropriate set of tests?

As an aside, from the MRP tool & video above it's also quite easy to see why this issue was not spotted earlier: the "test curve" used already has a "flat" section where the offset approaches 1!

[tektrip-biggles]

Have filed a bug report as well (#76623 )

[adamscott]

Have filed a bug report as well (#76623 )

I edited your PR description to include "Fixes #76623", as it's a magic word that informs Github to link the issue and auto completing the issue when the PR is merged.

[anvilfolk]

Just a reminder about this one! I'd be suuuper excited to see this bug fixed and to get a new contributor to Godot!

I feel like the path to get this merged is to:

1. Update current test values to the bug-fixed values
2. Add a new test case that catches this particular issue
3. Get it mergedddd!

@adamscott this one feels like more of a bugfix than an enhancement, and can likely be cherry-picked for 4.0 too?

[anvilfolk]

Haha, smart fix for the test 🤣 It's actually a pretty good illustration of how things were wrong, right?

Could we get CI going for this one @akien-mga, then maybe consider merging?

[anvilfolk]

Oh, also, please squash your commits into a single one :) It's part of the workflow! More info here :)

[tektrip-biggles]

Heya, @anvilfolk thanks for reminding me to get back to this! Apologies for the delay, a whole bunch of old project stuff popped up over the past few weeks.

Didn't actually realise that each commit to my branch would instantly show up here! Will rebase / squash my changes. That's just a case of running git rebase -i upstream/master, right?

Decided to fix some of the test messages that looked like they'd been copy/pasted without updating the mentioned offsets to match the values being tested.

Also added a new test case following @kleonc 's suggestion & have just confirmed that this fails before/passes after the change I made previously. I wanted to keep my changes to the bare minimum though so didn't implement his other suggestions

[tektrip-biggles]

Commits squashed as requested!

[anvilfolk]

Yay! Looks like a single tiny change needed and it's ready to rumble! Congrats on what will be your very first contribution! :D

[anvilfolk]

Now you gotta squash commits again 🤣

[akien-mga]

Thanks! And congrats for your first merged Godot contribution 🎉

[anvilfolk]

Yesssss, welcome to the club @tektrip-biggles! Big congratulations!!! :) If you're looking to hangout and contribute some more, don't forget there's the chat for godot contributors here :)

[tektrip-biggles]

Awesome! My first ever contribution to a major open source project! Thanks for all the support @anvilfolk & general help getting this merged, guys :-)