Fix gltf pipeline for some 3d Tilesets #5783
Conversation
- only assign name to value if value is an object - if gltf.assets.extras is already defined and not an object change to assets.extras.extras
|
@jbo023, thanks for the pull request! I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to I noticed that a file in one of our ThirdParty folders ( I am a bot who helps you make Cesium awesome! Thanks again. |
|
Thanks @jbo023. Can you upload any problematic b3dm's so we can verify these fixes? |
|
@lilleyse Since this is third-party code, shouldn't these changes go into the |
| @@ -56,6 +56,12 @@ define([ | |||
| gltf.extras._pipeline = defaultValue(gltf.extras._pipeline, {}); | |||
| gltf.asset = defaultValue(gltf.asset, {}); | |||
| gltf.asset.extras = defaultValue(gltf.asset.extras, {}); | |||
| if(defined(gltf.asset.extras) && typeof(gltf.asset.extras) !== 'object'){ | |||
There was a problem hiding this comment.
In the glTF 1.0 spec extras should be an object, so I think the source data is invalid. However this change is ok for now, and the newer code on the cleanup branch of gltf-pipeline doesn't have this problem..
There was a problem hiding this comment.
Yeah we are working on a new version of our 3d-tiles converter where we will fix these two bugs. But there are hundreds of legacy datasets we would still like to support.
When we finish the rewrite of our converter to GLTF 2.0 we are planning to tell our customers to reexport existing datasets.
| @@ -225,7 +225,7 @@ define([ | |||
| var value = object[id]; | |||
| mapping[id] = array.length; | |||
| array.push(value); | |||
| if (!defined(value.name)) { | |||
| if (!defined(value.name) && typeof(value) === 'object') { | |||
There was a problem hiding this comment.
Looking at a glTF in http://hosting.virtualcitysystems.de/demos/temp/b3dm there is a samplerRepeat property at the top level of the glTF which isn't support by the spec. Ideally the source data should be fixed. Also fine with this change for now as this area too is changed in the cleanup branch.
| @@ -17,6 +17,9 @@ define([ | |||
| * @returns {Number} The byte stride of the accessor. | |||
| */ | |||
| function getAccessorByteStride(gltf, accessor) { | |||
| if(defined(accessor.byteStride) && accessor.byteStride !== 0){ | |||
| return accessor.byteStride; | |||
| } | |||
There was a problem hiding this comment.
Good catch here. This change probably belongs in updateVersion instead.
var bufferView = clone(bufferViews[oldBufferViewId]);
var accessorByteStride = (accessor.byteStride === 0) ? getAccessorByteStride(gltf, accessor) : accessor.byteStride;
if (defined(accessorByteStride)) {
There was a problem hiding this comment.
I moved the check into updateversion, but i kept the defined check because I am not sure if accessor.byteStride is always defined?
|
I merged these changes into Thanks @jbo023 |
Hi,
we had some problems with our 3D Tilesets with cesium cesium 1.36.
This pullrequest fixes these problems. The problems only concerns the GLTFPipeline code in Source/Thirdparty, so I am not sure if I should also create a pullrequest in the gltf-pipeline repo?`
Jannes