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

Animation id should refer to top-level animation, not sampler #607

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lasalvavida
Copy link
Contributor

@RemiArnaud, could you take a look at this when you get a chance?

Currently the id of the animation object is created from the sampler, not the animation, so references (from animation clips, for example), are broken.

@lasalvavida
Copy link
Contributor Author

Bump, I'd like to get KhronosGroup/COLLADA2GLTF#227 merged this week, and it is currently blocked by this.

@lasalvavida
Copy link
Contributor Author

This should not be merged yet. I have been doing some work on automated validation in COLLADA2GLTF, and this causes some problems with models like VirtualCity who have multiple samplers within a single animation element, which is probably why the code was written this way originally.

This does still need to be fixed, but it's a little more complicated than just using the animation id. Animations are not samplers and the appropriate objects need to be added so that we can get the samplers belonging to an animation - accurately reflecting the structure of the COLLADA.

I have a temporary work-around for now on my end, and will update this with a more complete solution when I get a chance.

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.

1 participant