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

Skinned model fix #3224

Merged
merged 3 commits into from
Nov 20, 2015
Merged

Skinned model fix #3224

merged 3 commits into from
Nov 20, 2015

Conversation

lilleyse
Copy link
Contributor

While testing a simple skinned model exported from Blender and then going through the Collada-glTF converter, I noticed a bug with the skeleton generation.

When searching for a joint in searchForest, it loops through the children of the runtime node. The problem is the runtime nodes don't have children yet, since createSkins comes before createRuntimeNodes in createResources.

The solution I have loops over the children of the gltf node instead of the runtime node. Another solution is to change the order of calls in createResources, but I was worried this would create unintended problems. Or maybe the runtime node's children should be set in parseNodes.

The reason this wasn't noticed before is our Cesium guy lists all its joints in the skeletons property, so searchForest never had to loop over children.

@lilleyse
Copy link
Contributor Author

Tested with: simple_anim.txt

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 20, 2015

Great catch, thanks @lilleyse!

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 20, 2015

Please update CHANGES.md.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 20, 2015

Can you add a test? Put the .dae/.gltf here: https://github.com/AnalyticalGraphicsInc/cesium/tree/master/Specs/Data/Models

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 20, 2015

@tparisi @julien-moreau @deltakosh here is a simple skinned glTF model for testing: #3224 (comment)

@julien-moreau @deltakosh this is a glTF 1.0 model. If you need to upgrade your importer from glTF 0.8, see https://github.com/KhronosGroup/glTF/wiki/glTF-0.8-to-1.0-Guide

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 20, 2015

@lilleyse can you also open a pull request into the glTF repo to include this sample model? See https://github.com/KhronosGroup/glTF/tree/master/sampleModels

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 20, 2015

Please update CHANGES.md.

Ah, nevermind. I see.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 20, 2015

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 20, 2015

The solution I have loops over the children of the gltf node instead of the runtime node.

OK with me. Given how subtle this is, can you please add a comment?

@lilleyse
Copy link
Contributor Author

Updated.

@lilleyse
Copy link
Contributor Author

glTF repo pull request: KhronosGroup/glTF#480

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 20, 2015

Looks great, thanks!

pjcozzi added a commit that referenced this pull request Nov 20, 2015
@pjcozzi pjcozzi merged commit 06be660 into master Nov 20, 2015
@pjcozzi pjcozzi deleted the skinned-model-fix branch November 20, 2015 16:18
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.

None yet

2 participants