Physics interpolation seems to break Sprite2D

[AlexOtsuka]

Tested versions

Reproducible in both 4.3 betas and current master.

System information

Godot v4.3.dev (4a01602) - Windows 10.0.19045 - Vulkan (Forward+) - dedicated NVIDIA GeForce GTX 1070 (NVIDIA; 31.0.15.3623) - Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz (8 Threads)

Issue description

It seems like a number of scenarios break Sprite2D at runtime when enabling physics interpolation. skew seems to be ignored entirely, rotation and scale work by themselves, but not when used together with a non-proportional scale value.

Steps to reproduce

In the screenshots below,

• Column 1 is untouched, default Sprite2D properties.
• Column 2 is using skew only.
• Column 3 is using rotation only.
• Column 4 is using rotation and a non-proportional scale value.

Besides that, it's a fresh empty project without a single line of code (MRP included below).

Setup inside the editor (looks as expected):

At runtime, physics interpolation OFF (looks as expected):

At runtime, physics interpolation ON (broken):

Minimal reproduction project (MRP)

SpriteBugMRP.zip

Enable physics interpolation on/off in Project Settings > Physics > Common (Advanced Settings). Note that you might have to run the project, close it, and run it again every time you change the setting for it to take effect.

[akien-mga]

CC @rburing @lawnjelly

[cosparks]

I checked out the mrp and this issue seems to be caused by that fact that transform interpolation isn't implemented correctly. The following unit test fails (values are copied from the transform of row 2, column 2 in the mrp):

TEST_CASE("[TransformInterpolator] Skewed Transform -- prev == cur") {
	Transform2D skewed = Transform2D(Vector2(1.f, 0.f), Vector2(-0.766044676f,0.642787337f), Vector2(0.f, 0.f));
    Transform2D result = Transform2D();
	TransformInterpolator::interpolate_transform_2d(skewed, skewed, result, 0.f);
    CHECK(result == skewed);
}

ERROR: CHECK( result == skewed ) is NOT correct!
values: CHECK( [X: (1, 0), Y: (0, 1), O: (0, 0)] == [X: (1, 0), Y: (-0.766045, 0.642787), O: (0, 0)] )

I know the test shouldn't be checking for equality because of floating point error, but clearly the second column of the result transform is incorrect.

[cosparks]

Interpolating the transform's skew value appears to have fixed the results in the second column:

edit: It looks like the issues were skew interpolation and then a problem with how the interpolated transform was being constructed. Fortunately Transform2D has the following constructor, so we don't need to think about the order in which we are scaling / rotating / etc..

Transform2D(real_t p_rot, const Size2 &p_scale, real_t p_skew, const Vector2 &p_pos)

The results now all look correct in the branch that I'm working on. I'm going to add some more unit tests and spend some time trying to understand what exactly was causing the problem in the original code.

[lawnjelly]

Ah sorry I missed this, can reproduce in 3.x.

Great issue, the order of operations is likely the wrong way around in the interpolator (a while since I did this, but I suspect copy pasted from elsewhere in the engine). It may not have been reported before in 3.x because we don't have a skew and it is rarely used.

Will see if I can fix this up before 3.6 RC 1. 👍

EDIT:
Yes, can confirm, this was copy pasted from Transform2D::interpolate_with() which has been there for 6 years in 3.x, and likely has the same problem. 😁

I'll likely copy paste the fixed Transform2D versions from 4.x to 3.x and use these (we don't have the constructor with skew in 3.x), this should make it easier for @rburing to fix in 4.x.

@rburing looks like you can fix in 4.x with this:

void TransformInterpolator::interpolate_transform_2d(const Transform2D &p_prev, const Transform2D &p_curr, Transform2D &r_result, real_t p_fraction) {
	// Special case for physics interpolation, if flipping, don't interpolate basis.
	// If the determinant polarity changes, the handedness of the coordinate system changes.
	if (_sign(p_prev.determinant()) != _sign(p_curr.determinant())) {
		r_result.elements[0] = p_curr.elements[0];
		r_result.elements[1] = p_curr.elements[1];
		r_result.set_origin(Vector2::linear_interpolate(p_prev.get_origin(), p_curr.get_origin(), p_fraction));
		return;
	}

	r_result = p_prev.interpolate_with(p_curr, p_fraction);
}

(I put interpolate_with() in the header in 3.x to ensure it gets inlined, this may not be necessary in 4.x, check assembly to be sure.)

[cosparks]

If Transform2D::interpolate_with exists then this should definitely use it. No sense having the exact same logic in two places.

I'd be happy to put up the PR for this