Transform2D::interpolate_with result produces unintended skew

[SlugFiller]

Godot version

v4.0.beta14.official [28a2463] (But also present in master)

System information

Irrelevant

Issue description

Transform2D::interpolate_with works by breaking two Transform2Ds into rotation, scale, and translation, interpolating the two, then rebuilding a new Transform2D from the interpolated values. However, the rebuild is wrong. This is the offending code:

	Transform2D res(v.angle(), p1.lerp(p2, p_c));
	res.scale_basis(s1.lerp(s2, p_c));

The transform is first created with a rotation, then scaled. However, the scale is done along the axes, not along the rotated vectors. If the scale is uniform, this doesn't matter. If it's non-uniform, this can actually introduce a skew into the result, which is pretty bad, considering the function actually discards skew.

The correct order should be backwards - First scale, then rotation. Alternately, replace the axis-aligned scale with a scale of the vectors themselves. Or add a function to build a matrix from rotation, scale, skew, and transform, using the formula

mat.x = scale.x*Vector2(cos(rotation), sin(rotation))
mat.y = scale.y*Vector2(cos(rotation+skew+PI*0.5), sin(rotation+skew+PI*0.5))
mat.origin = translation

The bigger issue is that it has an effect even if p_c is 1.0, making classes that use it internally like Skeleton2D introduce bugs in code that should be fairly safe.

A possible workaround is to add special cases for p_c==0.0 and p_c==1.0, as going through all the math, and introducing numerical instability and information loss in the middle, is pointless for those cases.

Steps to reproduce

			var source = Transform2D.IDENTITY
			source = source.scaled(Vector2(1, 2))
			source = source.rotated(0.25*PI)
			print(source)
			print(source.get_scale())
			print(source.get_rotation())
			print(source.get_origin())
			print(source.get_skew())
			var interpolated = Transform2D.IDENTITY.interpolate_with(source, 1.0)
			print(interpolated)
			print(interpolated.get_scale())
			print(interpolated.get_rotation())
			print(interpolated.get_origin())
			print(interpolated.get_skew())

The interpolation value is 1.0 and source.get_skew() returns 0, yet interpolated.get_skew() returns -0.64350110292435, even the the interpolation is supposed to erase skew, not create it.

Minimal reproduction project

N/A

[TokageItLab]

the interpolate_with() will discard skew because transformations including skew are not common in Transform2D,3D.

Actually, I thought that was a problem in 3D as well (see also #65131), but it is definitely a niche use case. It would be more make sense to construct the Basis ourself from the rotation and scale, and then interpolate the Basis.

[SlugFiller]

@TokageItLab Why did you tag this as enhancement? This is clearly a bug. Non-uniform scaling is not only a pretty common use case, it's a use case the function is supposed to support. Even if it wasn't, it should at least do something sensible, like force the scale to be uniform, not introduce a skew.

[TokageItLab]

Currently Transform2D::interpolate_with() is consistent with Transform3D::interpolate_with(), so changing it means that Transform3D must also be changed. This is never allowed.

Since Transform3D has Basis in it, interpolating that Basis is the solution. But if we were to do it the same way, Transform2D would need us to construct and interpolate the Basis (2x2 matrix).

In other words, this issue can be read as a "proposal" that adding of a method to make it easy to linearly interpolate Transform2D::Basis, so not a bug.

I know some methods for rotation are lacking in gdscript, see also #69202.

However, if Transform2D::interpolate_with() behaves differently compared to Transform3D::interpolate_with(), then it is a bug and you should submit a movie and MRP.

[SlugFiller]

@TokageItLab This has nothing to do with which methods are lacking in GDScript, because this function is used internally by Skeleton2D for its modification stack implementation.

I've made a test with Transform3D, and the bug is NOT present there:

var s = Transform3D.IDENTITY
s = s.scaled(Vector3(2,1,1))
s = s.rotated(Vector3.BACK, PI*0.25)
var t = Transform3D.IDENTITY.interpolate_with(s, 1.0)
print(s)
print(t)

The resulting two transformations are nearly identical (up to 5 decimal digits)

So, even according to your criteria of needing both Transform2D and Transform3D to behave the same, the bug needs to be fixed.

Incidentally, this adds another possible fix: Expand the Transform2D to Transform3D by adding Vector3.BACK as a 3rd vector, use Transform3D::interpolate_with, then reduce back to Transform2D by discarding the Z values.

As for MRP, the code sample I gave can be pasted basically anywhere for verification. Although, if absolutely necessary, I can create a project that uses Skeleton2D to demonstrate how this breaks horribly in practical conditions.

P.S. Why would the 2D and 3D need to match anyway? The entire skeletal system is different between the two, with 2D using bones and nodes and modifications as data, while 3D uses bones as data and modifications as nodes. If they were supposed to match, that ship has sailed.

[TokageItLab]

As for MRP, the code sample I gave can be pasted basically anywhere for verification. Although, if absolutely necessary, I can create a project that uses Skeleton2D to demonstrate how this breaks horribly in practical conditions.

Please send it and I can look into it.

The entire skeletal system is different between the two, with 2D using bones and nodes and modifications as data, while 3D uses bones as data and modifications as nodes. If they were supposed to match, that ship has sailed.

This is for performance reasons godotengine/godot-proposals#1767. Also, the ModificationStack was not implemented in the proper way, so the architectural design is wrong. ModificationStack3D has already been removed and it is scheduled for major changes again in 4.1, and ModificationStack2D will eventually be outdated (those ModificationStack will become independent Node, not DataStack in Skeleton in the future plan).

[SlugFiller]

@TokageItLab

This is for performance reasons

Which, for some reason, didn't apply to 2D, which just goes to show that 2D and 3D are different beasts.

and ModificationStack2D will eventually be outdated

Which may reduce one instance of the problem, depending on what it is replaced with. Specifically, depending on whether the internal use of Transform2D::interpolate_with() is removed from it or not.

Please send it and I can look into it.

reproduce_transform2d_bug.zip

Took a bit of effort to make it clearly visible, because modification stack runs on every frame, and there's no way (or at least, I couldn't find one) to reduce the frame rate in the editor. To see the issue, go to the skeleton, and enable the modification stack.

The stack is at strength 1.0, and contains only a two-bone IK, which should be deterministic. So, under normal circumstances, it should simply give the correct result on the first frame where it is activated. Instead, it actually goes through several states, several of which contain a skew, even though the modification is only supposed to apply a rotation.

Incidentally, I could have demonstrated this more easily using a custom modification that simply sets the transform, but I preferred making a no-script example, to show that this isn't due to using the system in some unintended way, or having an error in the script.

[TokageItLab]

Thanks, I'll look into it later.

Which, for some reason, didn't apply to 2D, which just goes to show that 2D and 3D are different beasts.

Node and MathClass are different. For example, methods with the same name in Vector2 and Vector3 should produce the same process. This also applies to Transform2D and Transform3D.

[SlugFiller]

Another possible fix is to replace the scale_basis with scaled_local. Although that seems redundant, given that Transform2D has an inline set_rotation_and_scale (and also set_rotation_scale_and_skew) which would be much more appropriate and efficient than doing step by step composition.

[kleonc]

Haven't read deeply (pretty much just the title), just saying there's #64418 which you might wanna review/test.

[SlugFiller]

@kleonc Haven't tested, but I took one look at the code and it looks correct. Especially the part where it constructs the matrix directly, rather than in steps. I believe that PR would indeed fix this bug (In addition to making skew transforms functional as well). It's 4 months old, though, and is unclear if/when it will be merged.

[SlugFiller]

@TokageItLab Why is this still tagged "enhancement"? Did you look at the MRP I made? While #64418 does fix this, it is unlikely to be added to 4.0, since it (technically) adds functionality, and 4.0 is in feature freeze. This means that, without a fix for this, 4.0 would be released with at least one broken/unusable feature (Skeleton 2D modification stack) (This bug might influence other features. but I did not do an exhaustive search).

Even if releasing 4.0 broken is "ok", at least give the issue the correct tag. It's a bug, not an enhancement. At the very least, it would be a "known bug" of 4.0. "Using 2D skeleton modifications on non-uniformly scaled bones causes unpredictable behavior".

[TokageItLab]

I looked at your MRP but it was unclear what you meant, so I stayed away for a while. I also had other priorities to work on, so I just started looking into this. So sorry.

Yeah, it definitely seems to be broken, I just animated Transfomr2D and it is skewed. This is not good. At least this does not happen with Transform3D, so I definitely think it is a bug.

Godot.2023.01.28.-.21.24.18.16.mp4

transform_lerp.zip

There is already a PR as #64418, but I will have to ask @reduz if that is the right way to do it. As I commented on that, we may need to find the right way to rotate 2DBasis.

---

In addition, it is described for consistency with Transform3D:

"Transform2D interpolation without skew causes skew"

This is a bug. It is clearly shown in the above video.

"Transform2D interpolation with skew changes skew value"

This is not a bug in one sense, but it is a bug in another sense: Transform3D::interpolate_with needs to be normalized in order to perform a slerp, and skew is discarded at this time.

So it is consistent with Transform3D if Transform2D skew is set to 0, but it is wrong that skew becomes skew with a different value.

However, it may not be a good thing in Transform2D for skew to be zero. In that case, it may be necessary to consider an algorithm that can perform a rotation including skew in Transform3D as well.

[SlugFiller]

The way #64418 fixes 2D can also be used for 3D:

1. It first breaks the the basis transform into two vectors. i.e. The transform of Vector2.RIGHT and the transform of Vector2.DOWN
2. It breaks each vector into length and angle
3. It makes a direct lerp of the length and angle of matching vectors.
4. It recombines the lerped vectors into a transformation.

Generalized for 3D, this would be:

1. Break the basis transform into vectors representing the transformation of Vector3.RIGHT, Vector3.UP, and Vector3.BACK
2. Break each vector into length and quaternion (the 3D equivalent of an angle)
3. Make a linear lerp of the length, and a spherical lerp of the quaternion for each of the vectors.
4. Recombine the lerped vectors back into a transformation.

P.S. But it might not be the correct solution. In 2D, if you do angular lerp between two pairs of lines that are at a 90 degree angle of each other, you get a pair of lines that are at a 90 degree angle, because they are basically rotated by the same amount. In 3D, this guarantee does not hold, because of the non-Euclidean nature of the sphere. The mathematically correct method might be to SVD decompose the basis transform, do spherical lerp on the rotations matrices, and linear lerp on the scale matrix.