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: Refactor animation methods to be accessible from extensions on parser #26126

Merged

Conversation

hybridherbst
Copy link
Contributor

@hybridherbst hybridherbst commented May 24, 2023

Related issue:

Description

As it seems that more granular extension callbacks for animation are not desired, this PR merely refactors some of the animation code into separate methods, which can then be called from other code. The methods stay internal; I hope that satisfies the wish to not increase the to-be-maintained API surface.

This allows to have a separate file that only uses the existing loadAnimation hook for KHR_animation_pointer, at the price of some code duplication in the extension compared to my earlier PR.

See https://github.com/needle-tools/three.js/blob/khr-animation-pointer-separation/examples/jsm/loaders/GLTFLoaderAnimationPointer.js

cc @donmccurdy @takahirox

This contribution is funded by Needle

@hybridherbst hybridherbst marked this pull request as ready for review May 24, 2023 00:07
@hybridherbst
Copy link
Contributor Author

cc @donmccurdy @mrdoob

@donmccurdy
Copy link
Collaborator

@hybridherbst I have no concerns with this PR — Based on #24193 (comment) I can understand this might not be ideal from your perspective ... would you be OK with our merging this PR, and we can think about ways to reduce duplication in later PR(s)?

@hybridherbst
Copy link
Contributor Author

Yes, would be happy with merging this PR as it would finally allow us to release KHRAnimationPointer support as separate file and it "just works" with three.js. The remaining duplication should be solved later when/if that is merged back in in one way or another. Might also be a candidate for testing a dynamic import of an entire extension implementation only when needed (only briefly touched on this with @mrdoob)

@Mugen87 Mugen87 added this to the r154 milestone Jun 1, 2023
@hybridherbst
Copy link
Contributor Author

This can be merged now, right? :)

@Mugen87 Mugen87 merged commit 598c1f7 into mrdoob:dev Jun 9, 2023
@hybridherbst hybridherbst deleted the gltfloader-refactor-animation-methods branch July 6, 2023 11:57
Mugen87 added a commit to Mugen87/three.js that referenced this pull request Jul 22, 2023
Mugen87 added a commit that referenced this pull request Jul 22, 2023
@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 22, 2023

Since this PR introduced a regression, I have reverted it for since r155 is already close (see #26476).

Please file a new PR with a fixed implementation of this refactoring.

@hybridherbst
Copy link
Contributor Author

@Mugen87 any ideas what this refactor should have broken?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 22, 2023

No, sorry. I did not have time to investigate this in detail. I can only say for sure that the InterpolationComparison test asset (donmccurdy/three-gltf-viewer#344) breaks with this PR.

@hybridherbst
Copy link
Contributor Author

I'll take a look asap. Thanks.

@hybridherbst
Copy link
Contributor Author

🤦 after doing the refactor again this is the only difference. Will make a new PR now.

image

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