Ability to load z-up models #4962
Conversation
|
Is the option undocumented because it's only going to be used internally by 3D Tiles? |
|
At this point yes, but I could see its use as a public option too especially for georeferenced models. |
Source/Scene/Model.js
Outdated
| @@ -624,6 +624,9 @@ define([ | |||
| this._pickUniformMapLoaded = options.pickUniformMapLoaded; | |||
| this._ignoreCommands = defaultValue(options.ignoreCommands, false); | |||
|
|
|||
| // By default models are y-up according to the glTF spec, however geo-referenced models will typically be z-up | |||
| this._zUp = defaultValue(options.zUp, false); | |||
There was a problem hiding this comment.
Are you sure you want to expose it like this? Why not expose it the same way as it will be in the 3D Tiles spec using x, y, and z enums here.
Also, drop the underscore and mark it @private.
|
Just that comment. |
|
This supports all axes now via a pretty simple |
Source/Scene/Axis.js
Outdated
| * | ||
| * @exports Axis | ||
| */ | ||
| var Axis = { |
Source/Scene/Axis.js
Outdated
| * @type {String} | ||
| * @constant | ||
| */ | ||
| X : 'X', |
There was a problem hiding this comment.
Are we sure we should use strings here? That is fine for the format, but generally not something we would do for the runtime.
Perhaps make these 0, 1, 2, and provide a static creation function? Perhaps even move yUpToZUp and zUpToZUp here as frozen properties for better cohesion.
Specs/Scene/ModelSpec.js
Outdated
| @@ -85,7 +87,10 @@ defineSuite([ | |||
| var texturedBoxCustomUrl = './Data/Models/Box-Textured-Custom/CesiumTexturedBoxTest.gltf'; | |||
| var texturedBoxKhrBinaryUrl = './Data/Models/Box-Textured-Binary/CesiumTexturedBoxTest.glb'; | |||
| var boxRtcUrl = './Data/Models/Box-RTC/Box.gltf'; | |||
| var boxesEcefUrl = './Data/Models/Boxes-ECEF/ecef.glb'; | |||
| var boxEcefXUpUrl = './Data/Models/Box-ECEF/ecef-x-up.gltf'; | |||
There was a problem hiding this comment.
Instead of having three copies of the data, is it reasonable to have one copy (with squashed commits) and just have the test do a small change to the model at load? This will be less test data and easier to update to glTF 2.0.
|
Updated |
|
I may as well include all possible axis conversions... I'll update when ready. |
Source/Scene/Axis.js
Outdated
| * @returns {Number} The axis enum. | ||
| */ | ||
| fromName : function(name) { | ||
| return Axis[name]; |
There was a problem hiding this comment.
Really sorry, but name is required so this should throw an exception, right?
Source/Scene/Model.js
Outdated
| * @private | ||
| * @readonly | ||
| */ | ||
| this.upAxis = defaultValue(options.upAxis, Axis.Y); |
There was a problem hiding this comment.
Should this user an explicit getter so readonly is enforced?
|
Ok this should be everything. I changed the x-up to z-up transform to use a negative rotation instead. If someone could double check all the conversions that would be good - but I think they are all correct for a right-handed coordinate system. |
For CesiumGS/3d-tiles#175
Added
zUpundocumented option to Model. After I'm going to open a PR in the3d-tilesbranch for loading tilesets whose models are z-up.