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.

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

GLTFExporter: CubicSpline interpolation support #13377

Merged
merged 5 commits into from
Feb 21, 2018

Conversation

takahirox
Copy link
Collaborator

This PR adds CubicSpline interpolation support to GLTFExporter.

CubicSpline interpolation isn't in Three.js core yet, so exporting as CUBICSPLINE only when a custom interpolation GLTFCubicSplineInterpolant declared in GLTFLoader is used.

As GLTFCubicSplineInterpolant is a custom interpolation track.getInterpolation() doesn't return a valid value, it returns Three.js global interpolation type like THREE.InterpolateLinear but there's no global type for GLTFCubicSplineInterpolant.

Then I named GLTFCubicSplineInterpolant factory function and check that name to detect if interpolation is CubicSpline instead of calling track.getInterpolation().

// Detecting glTF cubic spline interpolant by checking factory method name because
// GLTFCubicSplineInterpolant is a custom interpolant and doesn't return
// valid value from .getInterpolation().
if ( track.createInterpolant.name === 'InterpolantFactoryMethodGLTFCubicSpline' ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

.name will disappear in some minified builds, I think... :/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahhh, right. Hmmmmmm

@donmccurdy
Copy link
Collaborator

donmccurdy commented Feb 20, 2018

Related: I imagine we could support exporting three.js's cubic spline interpolant by filling in some inferred tangents? (does not need to be in this PR, though)

@takahirox
Copy link
Collaborator Author

I haven't seen CubicInterpolant (InterpolateSmooth) in the detail yet but probably we could. Yeah let's do that in another PR.

@takahirox
Copy link
Collaborator Author

takahirox commented Feb 21, 2018

Related: I'm thinking to add an option to GLTFLoader which loads CUBICSPLINE as CubicInterpolant. Pros, it should always work well with other Three.js stuffs, ex: (de)serialization, because it's in Three.js core. (Custom interpolant sometimes wouldn't work for a certain purpose.) Cons, less precision. Getting (GLTF)CubicSplineInterpolant into core would be another option.

@donmccurdy
Copy link
Collaborator

I wonder if we could add support for in/out tangents in THREE.CubicInterpolant without breaking backwards compatibility? 🤔

@takahirox
Copy link
Collaborator Author

takahirox commented Feb 21, 2018

Values layout and itemSize valueSize will be differ, so hard to do that without breaking compatibility maybe?
Adding an option to turn on/off in/out tangents could solve this issue but adding an new intepolant type would be easier.

@takahirox
Copy link
Collaborator Author

Updated. Added isInterpolantFactoryMethodGLTFCubicSpline property to the factory method. Workaround indeed.

@donmccurdy
Copy link
Collaborator

Adding an option to turn on/off in/out tangents could solve this issue but adding an new intepolant type would be easier.

Yeah, it just seems weird to have two different cubic spline implementations, especially when one is a superset of the other. We could deprecate but not remove the version without tangents, I suppose, or automatically create tangents when they aren't provided? Would want to verify that interpolation is as fast or faster when tangents are used.

Meanwhile, this change looks fine to me.

@takahirox
Copy link
Collaborator Author

takahirox commented Feb 21, 2018

or automatically create tangents when they aren't provided

Ah, probably we could detect old version by checking valueSize(values.length / items.length) and generate tangents maybe?

Another concern is increased memory consumption of sample values, 3x larger, if we create tangents at initialization.

@mrdoob mrdoob added this to the r91 milestone Feb 21, 2018
@mrdoob mrdoob merged commit 23b2546 into mrdoob:dev Feb 21, 2018
@mrdoob
Copy link
Owner

mrdoob commented Feb 21, 2018

Thanks!

@takahirox takahirox deleted the GLTFExporterCubicSpline branch February 24, 2018 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants