Detect glTF 2.0 and change forward to match Cesium's convention. #6632
Conversation
|
@emackey, thanks for the pull request! Maintainers, we have a signed CLA from @emackey, so you can review this at any time.
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
|
Fixes #6591. |
| @@ -53,7 +53,7 @@ | |||
| viewer.scene.globe.depthTestAgainstTerrain = true; | |||
|
|
|||
| var position = new Cesium.Cartesian3(-1371108.6511167218, -5508684.080096612, 2901825.449865087); | |||
| var heading = Cesium.Math.toRadians(90); | |||
| var heading = Cesium.Math.toRadians(180); | |||
There was a problem hiding this comment.
Point of trivia, this change actually puts the heading back to what is was prior to #6592, which is not a coincidence. The model was updated to the correct glTF 2.0 orientation in that PR, but Cesium wasn't yet handling the 2.0 orientations correctly, so this was a workaround that is now being removed.
|
For glTF 2.0 in general this is probably the right thing to do. I'm a little worried about how this affects 3D Tiles, where unlike a standalone model there isn't really a concept of a forward axis. Maybe b3dm/i3dm should always pass in |
Source/Scene/Model.js
Outdated
| @@ -579,6 +579,7 @@ define([ | |||
| this._ignoreCommands = defaultValue(options.ignoreCommands, false); | |||
| this._requestType = options.requestType; | |||
| this._upAxis = defaultValue(options.upAxis, Axis.Y); | |||
| this._forwardAxis = defaultValue(options.upAxis, Axis.Z); | |||
There was a problem hiding this comment.
Should this be:
this._forwardAxis = defaultValue(options.forwardAxis, Axis.Z);
|
I think that's the right strategy. The rotation here is for the sake of Cesium's motions, like the |
|
We definitely need to address 3D Tiles before this gets merged. It looks completely broken. |
|
OK, I added the new constructor option to I believe this to be ready, but folks invested in 3D Tiles should probably do some testing of their own before merging this. |
|
@emackey I'll aim to look at this closer after the release today. |
|
Thanks @emackey, the Sandcastle demos look good. I substituted the ground vehicle for the milk truck in Multi-part CZML, which had the wrong orientation in master but is fixed here. I moved the breaking change to the 1.47 section to CHANGES.md. |
|
@emackey what do you think about having i3dm tiles also pass in their forward axis as |
|
Yes, certainly. Sadly I'm not a 3D Tiles expert (yet) so I'm sure I just overlooked some of the constructors. Are there any others? |
|
That should be it. Thanks! |
Missed a constructor in #6632.
I'm opening this a bit early to get Travis running on the tests, they seem to have problems. Also, feedback welcome, particularly on the approach here.
I've added a new
forwardAxissetting, similar to the existing privateupAxissetting, on Model.js. A change to the underlying gltf-pipeline marks models as having been upgraded from 1.0 (which includes models upgraded from 0.8 as they pass through 1.0 as an intermediate step). Models which are not marked as upgraded are presumed to be native glTF 2.0 or higher, and thus must use the new +Z forward convention. This is accomplished by adding an additional rotation, after the upAxis rotation for such models, to set the forward vector to align with what Cesium uses internally.TODO: