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

Gltf node instancing not supported in Cesium. #1754

Closed
vinjn opened this issue May 28, 2014 · 16 comments
Closed

Gltf node instancing not supported in Cesium. #1754

vinjn opened this issue May 28, 2014 · 16 comments
Assignees

Comments

@vinjn
Copy link

vinjn commented May 28, 2014

KhronosGroup/glTF#276

Dae file uploaded as https://cloud.githubusercontent.com/assets/558657/3093329/694702ee-e5b1-11e3-9c48-138430dd89c6.jpg
You can download the jpg, and rename the extension to 7z and extract it.

The expected rendering result should be like

However the result from Cesium is like

@pjcozzi
Copy link
Contributor

pjcozzi commented May 28, 2014

Thanks for the report @vinjn

@vinjn
Copy link
Author

vinjn commented May 29, 2014

@pjcozzi Do you have clues on how to debug this issue? I can look into this problem myself with your instructions.

@pjcozzi
Copy link
Contributor

pjcozzi commented May 29, 2014

Thanks for the kind offer. In Model.js, have a look at parseNodes() and createRuntimeNodes().

@vinjn
Copy link
Author

vinjn commented Jun 1, 2014

I have investigated these methods, however no clues have been found so far.

@vinjn
Copy link
Author

vinjn commented Jun 3, 2014

https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Scene/Model.js#L1833-L1845

// Node hierarchy is a DAG so a node can have more than one parent so it may already exist
var runtimeNode = runtimeNodes[n.id];
if (runtimeNode.parents.length === 0) {
    if (defined(gltfNode.matrix)) {
        runtimeNode.matrix = Matrix4.fromColumnMajorArray(gltfNode.matrix);
    } else {
        // TRS converted to Cesium types
        axis = Cartesian3.fromArray(gltfNode.rotation, 0, axis);
        runtimeNode.translation = Cartesian3.fromArray(gltfNode.translation);
        runtimeNode.rotation = Quaternion.fromAxisAngle(axis, gltfNode.rotation[3]);
        runtimeNode.scale = Cartesian3.fromArray(gltfNode.scale);
    }
}

It seems the transformation fileds of runtimeNode is only updated when gltfNode is parsed for the first time runtimeNode.parents.length === 0.
@pjcozzi is the logic correct here?

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 5, 2014

This could be the issue. If a node has more than one parent, we should create a new runtimeNode for it with a different set of transforms leading up to the root.

It could be a small fix, but I don't have the bandwidth to look at it right now. Do you want to experiment?

@vinjn
Copy link
Author

vinjn commented Jun 5, 2014

I would love to have that challenge. When shall we create new runtimeNode then?
My initial plan is:

  • create new runtimeNode in parseNodes()
  • update transforms in createRuntimeNodes()

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 5, 2014

I think that will do it. Essentially, in runtimeNode, we will transform the DAG into a tree, then createRuntimeNodes will just work on a tree (no need to check number of parents because it will always be one).

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 28, 2015

Thanks again @vinjn. This is a good catch since Cesium was trying to handle this case but was not doing so correctly; it had a separate draw call for each instanced node, but they all used the "last" parent's transform.

I hacked up a partial fix in gltf-instancing but it is ugly and only works for multiple immediate parents, not older ancestors. Instead, I'm pretty sure I am going to expand the glTF DAG into a tree, but this will be some work since I will need to map the original glTF node ids (e.g., for animation and show/hide) to the new nodes. @fabrobinet I imagine this is how your renderer works?

image

@fabrobinet
Copy link

I confirm but it's not a big overhead as you can share the same geometry.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 28, 2015

Agreed, I'm not concerned about the overhead, just the time I need to update the code :)

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 2, 2015

@satishkashyap also reported this bug with a different model: KhronosGroup/glTF#363

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 20, 2015

Fixing this might also fix #2387.

@slozier
Copy link
Contributor

slozier commented Nov 30, 2015

I'm also running into this issue when trying to convert some models. From the glTF issue it seems like the plan is to fix this at the converter level?

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 30, 2015

Exactly, this will be fixed in the converter. We decided that the glTF spec will not allow DAGs, so DAGs will be converted to a tree offline. See KhronosGroup/glTF#401

This conversion will be part of CesiumGS/gltf-pipeline#1. Work will begin in the new year.

@lilleyse
Copy link
Contributor

The updated converter that should be deployed soon fixes this model.
tree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants