Use Hermite instead of Bezier for glTF spline interpolation

[Gurvan]

The cubic spline interpolation implementation in glTF animations wrongly uses Bezier splines instead of Hermite splines (gltf 2.0 spec, Hermite Spline).
This results in incorrect interpolation, see related issue.

Thankfully, there is an easy conversion from Hermite coefficients to Bezier coefficients that fixes the issue with minimal change.

• Bugsquad edit, closes: Spline interpolation for glTF wrongly uses Bezier instead of Hermite #93596

[fire]

We’ll look into this.

[TokageItLab]

Shouldn't it is fixed in SceneFormatImporterGLTFInterpolate T bezier() instead it?

[Gurvan]

Yes, it is the other option. I went for the minimal change in the code, but changing the bezier function to properly compute the interpolation the way it's explained in the glTF spec would probably be clearer.
In that case it would also be better to rename the function from bezier to hermite, and also rename the Quaternion one (that doesn't do Bezier nor Hermite interpolation by the way).

[fire]

Is the test case in #93596 ok to validate this or is there other cases?

[Gurvan]

It does test the spline interpolation for translation and scaling (rotation works differently) for arbitrary values and tangents.
A case that combine translation and scale could be made, but it wouldn't test anything more than the test in #93596.
The glTF samples repository has infact an InterpolationTest.glb, but all the tangents in the file are 0, so it can't be used as a actual test case.

Personally, I would be confident to use it as it is.

[fire]

I will try to do a review in a few days and then approve.

[TokageItLab]

I agree that rename the method to hermite().

However, in fact, these calculations are already defined in Godot's Math function which is completely the same, so all you need to do is to call it.

• catmull_rom() is same with Math::cubic_interpolate()
• hermite() is same with Math::smoothstep()

For now, if you change the code to call the Math function in those method, it should be approve-able.

BTW, to be consistent with the Math function, it would be also okay to revert the renaming of harmite() to smoothstep(), and rename catmull_rom() to cubic().

[TokageItLab]

Oh sorry, smoothstep() does not allow to set start and end tangents individually, so you probably need to define a function called hermite_interpolate() in Math class. Well, since that is an additional feature, we can follow up later. I think this PR is fine, although catmull_rom() could be renamed to cubic() (but might be odd since glTF definition calls the hermite spline as cubic...).

[Gurvan]

There are so many different types of cubic splines, I would say it's probably better to keep their names explicit when possible.
I would be happy to follow up with another PR for moving the interpolation functions into Math.

[akien-mga]

Looks good! Could you squash the commits? See PR workflow for instructions.

[akien-mga]

Thanks! And congrats for your first merged Godot contribution 🎉

[Gurvan]

Thanks to the Godot team for making the whole process feel great!

[TokageItLab]

@Gurvan If you have time, give it a try the PR to implement hermite_interpolate() in the Math class and call them in the glTF importer :) Although the merge will probably be 4.4.dev (after than 4.3.stable) at the earliest since it is enhancement.