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

glb model was rotated 90 degree in current master branch version #6713

Closed
Jeongyong-park opened this issue Jun 21, 2018 · 12 comments
Closed

Comments

@Jeongyong-park
Copy link

Jeongyong-park commented Jun 21, 2018

Developed using glb and czml, a source for master branch is being built and used.
All 3D models were rotated CCW direction in master branch.

using Cesium 1.46.1 version

1

using 488ca67 version

2

@lilleyse
Copy link
Contributor

This change was added in #6632 to orient glTF 2.0 models with Cesium's forward axis. But it looks like this is a case where the conversion is not desired.

@emackey do you know if there might be a clean way to control the forward axis setting from within the CZML and entity layers?

@mramato
Copy link
Contributor

mramato commented Jun 21, 2018

@lilleyse If it's a 2.0 model, isn't this desired behavior? Didn't we list #6632 in breaking changes?

@lilleyse
Copy link
Contributor

Yes it's expected behavior, however the forward axis conversion is really intended for objects in motion #6632 (comment).

There is a gray area for buildings and other things that don't have a concept of a forward axis, which is why we disabled the rotation for 3D Tiles. This case is more similar to the 3D Tiles case, just that its czml + glb instead.

@Jeongyong-park
Copy link
Author

I attach some of the glb and czml I am using.

output.zip

@hpinkos
Copy link
Contributor

hpinkos commented Jun 25, 2018

@lilleyse should we close this issue? Can you update the comment in CHANGES.md to include what our recommendation is for our users with existing models with the wrong forward axis? We need an answer here before the release

@mramato
Copy link
Contributor

mramato commented Jun 25, 2018

@emackey any thoughts here? I'm of the opinion that this is a breaking change regarding Cesium's interpretation of glTF 2.0 model orientation and should be listed as such in CHANGES. However, the change itself is good and I think we just need to close this issue.

If we can't come to a consensus, then we might have to back out the orientation changes until we can.

@hpinkos
Copy link
Contributor

hpinkos commented Jun 25, 2018

@mramato It is already listed in under Breaking Changes, but I think we need to add a recommendation for a path forward for existing models using an incorrect orientation to meet our previously wrong implementation. That's why I labeled this next release, doc. After that I agree, this issue can be closed. Unless I'm missing some other detail.

@mramato
Copy link
Contributor

mramato commented Jun 25, 2018

This comment from @lilleyse is what I was concerned about:

There is a gray area for buildings and other things that don't have a concept of a forward axis, which is why we disabled the rotation for 3D Tiles. This case is more similar to the 3D Tiles case, just that its czml + glb instead.

I don't think we should special case orientation anywhere (3D Tiles doesn't count because the spec allow you to override up axis specifically).

@Jeongyong-park
Copy link
Author

i was rotated 90 degrees on the y-axis all models . But it seems that everyone needs to turn around all of the models they've built for the previous version, but they need instructions and tools.

@lilleyse
Copy link
Contributor

It's unfortunate that is a breaking change for models that aren't necessarily wrong. They just don't have an implicit forward axis (for example, this model has heading and altitude baked in).

view1

view2

The breaking changes entry might be like:

  • glTF 2.0 models corrected to face +Z forwards per specification. Internally Cesium uses +X as forward, so a new +Z to +X rotation was added for 2.0 models only. To fix models that are oriented incorrectly after this change, apply a +X to +Z rotation [0,0,1,0,0,1,0,0,-1,0,0,0,0,0,0,1] to the root node of the glTF.

The fix feels a bit arbitrary and I hope not too many models need it, but it might be the best we can do.

@lilleyse
Copy link
Contributor

CHANGES.md edits are here #6738.

@kladess since this problem affects your data, do you think the updated instructions are clear?

@Jeongyong-park
Copy link
Author

@lilleyse thank you for update documents. It seems to be clearer and easier to understand.

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

4 participants