Add console errors when a tileset contains unsupported extensions #9586
Conversation
…r unsupported extensions
|
Thanks for the pull request @srothst1!
Reviewers, don't forget to make sure that:
|
Source/Scene/Cesium3DTileset.js
Outdated
| @@ -1751,6 +1751,8 @@ Cesium3DTileset.prototype.loadTileset = function ( | |||
| throw new RuntimeError("The tileset must be 3D Tiles version 0.0 or 1.0."); | |||
| } | |||
|
|
|||
| Cesium3DTileset.checkSupportedExtensions(this._extensions); | |||
There was a problem hiding this comment.
A few things here:
this._extensionsis not yet initialized (see lines 965 and 974) at this point in the code, asloadTileset()gets called first. UsetilesetJson.extensionsRequiredinstead.- Also it would be good to check
if (defined(tilesetJson.extensionsRequired)) { /* check supported extensions */ }just in case the array is undefined (which will be the case most of the time) - To answer your question about whether we need to add
this._extensionsRequired, I don't think we need it, as this is the only place the required extensions list is used.
Changing the extensions so that they match the official extension names. Co-authored-by: Peter Gagliardi <[email protected]>
…esium into handle-unsupported-extensions
|
@ptrgags I believe that I have implemented all of the changes - let me know what you think! |
There was a problem hiding this comment.
@srothst1 almost there, just two more little things.
I also put together a local Sandcastle to double check the three main cases:
- A tileset with no extensions loads
- A tileset with a supported extension loads
- A tileset with an unsupported extension throws an error which will cause the readyPromise to reject
If you want to try this yourself, you have to make sure Cesium is running first before clicking the link above:
npm install
npm run build
npm start
(this Build Guide)
Source/Scene/Cesium3DTileset.js
Outdated
| for (var extension in extensionsRequired) { | ||
| if (extensionsRequired.hasOwnProperty(extension)) { |
There was a problem hiding this comment.
😮 My bad, I had forgotten that extensionsRequired is an array, not an object. This should be changed to a for (var i = 0; i < extensionsRequired.length; i++) loop instead.
There was a problem hiding this comment.
I think ModelUtility.js has the same mistake but works accidentally. @srothst1 could you open a PR to fix that as well?
Co-authored-by: Peter Gagliardi <[email protected]>
|
@ptrgags just added an update to |
Source/Scene/Cesium3DTileset.js
Outdated
| if (!Cesium3DTileset.supportedExtensions[extensionsRequired[i]]) { | ||
| throw new RuntimeError("Unsupported 3D Tiles Extension: " + extension); |
There was a problem hiding this comment.
One last tiny thing, extension here is undefined in the error message, so it ends up throwing a ReferenceError.
Fix this and I'll merge
|
@srothst1 changes look good, I'll merge once Travis passes. |
|
Merged! thanks @srothst1! |
Fixes #9552
There is no
_extensionsRequiredproperty in Cesium3DTileset so this pull request uses the_extensionsproperty - should we add_extensionsRequired?Updated
CONTRIBUTORS.md.@ptrgags