Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use Hermite instead of Bezier for glTF spline interpolation #93597

Merged
merged 1 commit into from
Jun 29, 2024
Merged

Use Hermite instead of Bezier for glTF spline interpolation #93597

merged 1 commit into from
Jun 29, 2024

Conversation

Gurvan
Copy link
Contributor

@Gurvan Gurvan commented Jun 25, 2024 •
edited by AThousandShips
Loading

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.

@Gurvan Gurvan requested a review from a team as a code owner June 25, 2024 15:38
@Gurvan Gurvan mentioned this pull request Jun 25, 2024
Closed
@AThousandShips AThousandShips changed the title Fix glTF spline interpolation by using Hermite interpolation instead of Bezier Use Hermite interpolation instead of Bezier glTF spline interpolation Jun 25, 2024
@AThousandShips AThousandShips changed the title Use Hermite interpolation instead of Bezier glTF spline interpolation Use Hermite instead of Bezier for glTF spline interpolation Jun 25, 2024
@AThousandShips AThousandShips added this to the 4.3 milestone Jun 25, 2024
@fire
Copy link
Member

fire commented Jun 25, 2024

We’ll look into this.

@fire fire requested a review from a team June 25, 2024 21:53
TokageItLab
TokageItLab reviewed Jun 26, 2024
View reviewed changes
Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Gurvan
Copy link
Contributor Author

Gurvan commented Jun 26, 2024

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

@Gurvan Gurvan requested review from a team as code owners June 26, 2024 08:40
@AThousandShips

This comment was marked as resolved.

Sign in to view
@AThousandShips AThousandShips removed request for a team June 26, 2024 08:42
@Gurvan Gurvan force-pushed the fix/gltf-spline-interpolation branch from ba61f00 to 640dfac Compare June 26, 2024 08:45
@fire
Copy link
Member

fire commented Jun 26, 2024

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

@Gurvan
Copy link
Contributor Author

Gurvan commented Jun 27, 2024

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 fire requested a review from a team June 27, 2024 14:37
@fire
Copy link
Member

fire commented Jun 27, 2024 •
edited
Loading

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

@fire fire requested a review from TokageItLab June 28, 2024 17:13
TokageItLab
TokageItLab requested changes Jun 28, 2024
View reviewed changes
Copy link
Member

@TokageItLab TokageItLab left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
TokageItLab approved these changes Jun 28, 2024
View reviewed changes
Copy link
Member

@TokageItLab TokageItLab left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

Gurvan commented Jun 28, 2024

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
Copy link
Member

akien-mga commented Jun 29, 2024 •
edited
Loading

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

@Gurvan Gurvan force-pushed the fix/gltf-spline-interpolation branch from 640dfac to e7f34aa Compare June 29, 2024 12:37
@akien-mga akien-mga merged commit 3c9949e into godotengine:master Jun 29, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@Gurvan
Copy link
Contributor Author

Gurvan commented Jun 29, 2024

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

@TokageItLab
Copy link
Member

TokageItLab commented Jun 29, 2024 •
edited
Loading

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fire fire fire approved these changes

@TokageItLab TokageItLab TokageItLab approved these changes

Assignees
No one assigned
Labels
bug topic:import topic:3d
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

Spline interpolation for glTF wrongly uses Bezier instead of Hermite
5 participants
@Gurvan @fire @AThousandShips @akien-mga @TokageItLab