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

GLTFLoader: Implement basic CUBICSPLINE support #12885

Merged
merged 1 commit into from
Dec 16, 2017

Conversation

donmccurdy
Copy link
Collaborator

Fixes #11761.

Also removes CATMULLROMSPLINE code, which has been removed from the glTF specification.

This is approximate, and throws away some information in the animation samplers. In the future we can consider re-sampling at a higher framerate (as described in the comment) or providing a mechanism for setting tangents with THREE.InterpolateSmooth.

/cc @zellski I have exactly one model to test this, any chance you have others?

@donmccurdy
Copy link
Collaborator Author

One clarification — this PR adds "basic support" in the sense that CUBICSPLINE animation will actually play now, and will interpolate smoothly.

However, it does not precisely match the authored splines — we'd need some changes to THREE.KeyframeTrack to do that. As a result, we also don't really get the efficiency benefits of cubic spline interpolation.

@takahirox
Copy link
Collaborator

takahirox commented Dec 15, 2017

So we need a custom interpolation to fully support CUBICSPLINE?

MMD also needs some special interpolations and I've been thinking if we can easily make custom interpolations.

/*
* extends existing KeyframeTrack for bone and camera animation.
* - use Float64Array for times
* - use Cubic Bezier curves interpolation
*/
THREE.MMDLoader.VectorKeyframeTrackEx = function ( name, times, values, interpolationParameterArray ) {
this.interpolationParameters = new Float32Array( interpolationParameterArray );
THREE.VectorKeyframeTrack.call( this, name, times, values );
};

@donmccurdy
Copy link
Collaborator Author

I believe that's correct. We'd need to write a version of CubicInterpolant that uses the tangents during evaluation, and set track.createInterpolant to a custom factory method returning the new interpolant. The existing runtime interface for interpolants, interpolant.evaluate( clipTime ), should be fine — is that also true for MMD?

@zellski
Copy link

zellski commented Dec 15, 2017

@donmccurdy I have no model other than what was committed to the model collection, but I’ve been meaning to hand-craft a very simplistic cubic-animated for my own dev work. I may as well accelerate that little undertaking.

@donmccurdy
Copy link
Collaborator Author

Ok, thanks - the Blender exporter can write CUBICSPLINE, that's probably where the current model came from. I'd be inclined to merge this in its current form and implement the more complex and precise interpolation when we have more models to test with, assuming that adding support for custom interpolants in THREE.KeyframeTrack sounds reasonable to everyone.

@mrdoob
Copy link
Owner

mrdoob commented Dec 16, 2017

Thanks!

@donmccurdy donmccurdy deleted the feat-gltfloader-cubicspline branch December 16, 2017 00:08
@takahirox
Copy link
Collaborator

takahirox commented Dec 16, 2017

@donmccurdy

Yup, what I've done for MMD is very similar to what you mentioned, inherited keyframe track and overrode setInterpolation to set the factory method which creates the custom interpolant to createInterpolant.

But this's a bit hacky so wandering if it'd be worth to provide a easier way to users for custom interpolants.

Anyways, I think that tech would be good enough for CUBICSPLINE so far.

@takahirox
Copy link
Collaborator

takahirox commented Dec 16, 2017

This's my Cubic Spline WIP repo. Probably the impl would be like this.

https://github.com/takahirox/three.js/blob/GLTFCubicSpline/examples/js/loaders/GLTFLoader.js#L2406-L2415

(I haven't tested because of no models tho!)

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Dec 16, 2017

@takahirox Nice! 😀 here's my one test model https://s3.amazonaws.com/bug-figures/v2-locrotscale-cube-embedded-buffers.gltf

you can make more with the Blender exporter too

@takahirox
Copy link
Collaborator

Just in case, have you confirmed the animation with this PR expectedly works?
I mean, can I use the animation behavior with this change as likely reference animation behavior?

@donmccurdy
Copy link
Collaborator Author

Not sure what you mean — all of the official animated models still work (they don't use CUBICSPLINE) and the only test model I have (above) animates approximately correctly — only approximate because the tangent data is thrown out. So I wouldn't consider this implementation a reference, but it should work well enough for a starting point.

@takahirox
Copy link
Collaborator

This is the animation of your model with this PR. Does this look ok?

ezgif com-optimize

@donmccurdy
Copy link
Collaborator Author

Yes, that's as expected.

@takahirox
Copy link
Collaborator

takahirox commented Dec 18, 2017

I think these outputAccessorValues[ j / 3 + ? ] should be outputAccessorValues[ j + ? ] outputAccessorValues[ j / 3 * itemSize + ? ], am I wrong?

https://github.com/donmccurdy/three.js/blob/5249a1b3a33e9ec6bffcc35c8e4fc94085a197ef/examples/js/loaders/GLTFLoader.js#L2318-L2322

@donmccurdy
Copy link
Collaborator Author

Using this:

outputAccessorValues[ j / 3 + ? ] = outputAccessor.get?( j + 1 );

Because outputAccessorValues has 1/3rd the length of outputAccessor.array:

var outputAccessorValues = new TypedArray( outputAccessor.count * itemSize / 3 );

We're intentionally throwing away 2/3rds of the data in the output sampler.

@takahirox
Copy link
Collaborator

takahirox commented Dec 18, 2017

for ( var j = 0, jl = outputAccessor.count; j < jl; j += 3 ) {

	outputAccessorValues[ j / 3 ] = outputAccessor.getX( j + 1 );
	if ( itemSize > 1 ) outputAccessorValues[ j / 3 + 1 ] = outputAccessor.getY( j + 1 );
	if ( itemSize > 2 ) outputAccessorValues[ j / 3 + 2 ] = outputAccessor.getZ( j + 1 );
	if ( itemSize > 3 ) outputAccessorValues[ j / 3 + 3 ] = outputAccessor.getW( j + 1 );

}

indices of outputAccessorValues in first loop are 0, 1, 2, and in second loop are 1, 2, 3 (assuming itemSize is 3). The following loop overrides outputAccessorValues set in the previous loop. Is this what you intended?

I suppose it should be...

outputAccessorValues[ j / 3 + ? ] = outputAccessor.get?( j + 1 );

outputAccessorValues[ j / 3 * itemSize + ? ] = outputAccessor.get?( j + 1 );

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Dec 18, 2017

Hm good point. 😅 I don't have the original Blender file for that model, so let me try making another model to test and verify this...

@takahirox
Copy link
Collaborator

This is the animation of your model with the fix.

ezgif com-optimize 1

My Cubic Spline impl is very close to this. And I saw the raw input/outputAccessor.array values and probably the animation should be like this.

@donmccurdy
Copy link
Collaborator Author

Ok, yes I think you are correct then. Will make a new test model anyway for good measure.

@donmccurdy
Copy link
Collaborator Author

Yeah here's another example. Definitely need to multiply by itemSize as you said.

SimpleCubicSpline.zip

16dae9a8-040e-4aa8-8d05-bec83453cc59-59594-00004993cf2cd481

@donmccurdy
Copy link
Collaborator Author

Here's another example, working nicely with the new fix:

CubicSplineMorphCube.zip

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.

4 participants