Physics interpolation seems to break Sprite2D

[AlexOtsuka]

Physics interpolation seems to break Sprite2D

[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.

[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.

[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.)

[akien-mga]

CC @rburing @lawnjelly

[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