Fix Cesium ion external asset handling #6140
Conversation
`CesiumIonResource` should only be used for ion hosted assets, so create a standard Resource instance for external 3D Tiles and Terrain. This also means that `CesiumIonResource.createImageryProvider` can't exist and needs to just be on `CesiumIon` directly instead. Also add `Instrumented` directory to .eslintignore becuase it was linting the results of `instrumentForCoverage`
|
@mramato, thanks for the pull request! Maintainers, we have a signed CLA from @mramato, so you can review this at any time.
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
Source/Scene/CesiumIon.js
Outdated
| } | ||
|
|
||
| //External imagery assets have additional configuration that can't be represented as a Resource | ||
| throw new RuntimeError('CesiumIon.createResource does not support external imagery assets.'); |
There was a problem hiding this comment.
Add a comment to use CesiumIon.createImageryProvider instead. Or can't we just make a call to that from this function instead of throwing an error?
There was a problem hiding this comment.
I agree on updating the error message, but I think just calling it from here would be confusing since it's a completely different return type. I also expect external assets to change a lot in the future, so they'll remain experimental for longer.
Source/Scene/CesiumIon.js
Outdated
| /** | ||
| * @private | ||
| */ | ||
| CesiumIon.createEndpointResource = function (assetId, options) { |
There was a problem hiding this comment.
createEndpointResource -> _createEndpointResource?
|
Just those comments, the rest of the changes look fine to me 👍 |
1. `CesiumIon.createEndpointResource` -> `CesiumIon._createEndpointResource` 2. Improve `createResource` error message.
|
Thanks @hpinkos, changes made. |
Source/Scene/CesiumIon.js
Outdated
| * creation and automatic refresh token handling. This object | ||
| * should not be created directly, use CesiumIonResource.create | ||
| * creation and automatic refresh token handling. This object | ||
| * should not be created directly, use CesiumIonResource.create. | ||
| * | ||
| * @private | ||
| */ |
There was a problem hiding this comment.
Should CesiumIonResource be in its own file? Usually in Cesium if we have a class with a few functions, we usually split it out.
There was a problem hiding this comment.
I moved it, my main reason for keeping it off is because it ends up on the Cesium object in the combined version, but that's par for the course for us anyway. This should be good to go.
|
@tfili bump |
|
Looks good to me. |
I was so excited about #6136 that I totally glossed over some important details of external assets and for the most part, they didn't work.
CesiumIonResourceshould only be used for ion hosted assets, so create a standard Resource instance for external 3D Tiles and Terrain. This also means thatCesiumIonResource.createImageryProvidercan't exist and needs to just be onCesiumIondirectly instead.It's probably easier to review this as a whole file again rather than looking at the diff.
I also add
Instrumenteddirectory to .eslintignore becuase it was linting the results ofinstrumentForCoverageCC @hpinkos @tfili