Don't require KMLDataSource canvas+camera

[thw0rted]

The KmlDataSource constructor (and static load method) fail when being called without a Canvas or Camera, but all accesses to these properties are actually guarded, and they only even do anything in the event the KML you're trying to load is in fact coming from a NetworkLink.

KmlDataSource is the only DataSource implementation that needs to know anything about the Viewer to which it will be attached at construction-time. In my program, we have to significantly complicate our startup logic to accommodate not being able to create the DataSource until the Viewer (and thus canvas and Camera) are ready. Allowing the caller to construct a KmlDataSource, then the Viewer, then (eventually) assign camera and canvas before calling instance load would greatly simplify things for us, with no ill effect for those who were using the current behavior.

I altered tests to stop passing canvas+camera for those cases where they're not used, and I edited one test that previously called KmlDataSource.load({canvas, camera}) to instead create an instance with no options, then set the properties separately before calling src.load(), just to show that this approach also works.

I did have to make a small change to the eslint config to allow the use of Object.assign. If there's another means of combining plain objects that you'd prefer I use, I'm happy to switch. Alternately, maybe it would make sense to get rid of the "plugin:es/restrict-to-es5" in Source/eslintrc.json, now that you're on native Promises and using other ES6 syntax -- of course, I figured that should be a separate PR, and the team would want to discuss it ahead of time.

[cesium-concierge]

Thanks for the pull request @thw0rted!

• ✔️ Signed CLA found.
• ❔ CHANGES.md was not updated.
  • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

• Cesium Viewer works.
• Works in 2D/CV.

[thw0rted]

Re the concierge comment, the only change to the public API is that some required parameters aren't anymore, and the values of src.camera / src.canvas are public read/write. I don't think it matters enough to mention, but I can make the edit if desired.

I did update the docs, which I think maybe weren't 100% correct before anyway, to be as consistent as possible. It can be a little confusing because some options are written to instance properties while others are passed through to the file-scope load function invocation without being saved.

[ggetz]

Thanks @thw0rted! A few comments:

[ggetz]

Since this exposes new properties, I think that warrants a line in CHANGES.md. Additionally, changing the docs for the options affects the typescript definitions.

[ggetz]

What's the implication of updating the canvas or camera (or un-assigning either of these) after the after a kml is loaded? Would it be confusing if someone 1) set a camera, 2) loaded a kml, 3) changed the camera, 4) loaded another kml?

[thw0rted]

From what I can tell, these two are used to compute a NetworkLink URL -- camera for bounding-box, canvas for resolution (I guess to supply raster imagery at appropriate DPI?). If they're undefined, they won't be used; if they change between loads, or NetworkLink updates, the next load will get the query values (bounding-box and canvas size) from the new properties.

[ggetz]

maybe it would make sense to get rid of the "plugin:es/restrict-to-es5" in Source/eslintrc.json, now that you're on native Promises and using other ES6 syntax -- of course, I figured that should be a separate PR, and the team would want to discuss it ahead of time.

We agree most of these rules are no longer needed due to dropping IE support. We have plans to clean this up our (many) .eslint files in this project holistically very soon.

[thw0rted]

I'm adding an entry to the changelog now, including a link to the relevant section of the KML spec. I was mistaken above, it's not just for NetworkLink, rather for any Link, including requests for images or 3D models. I don't think any changes are required, though, since changes between loads -- or even "during", i.e. after a request is initiated and before the response resolves -- would simply produce a different template-string replacement value on the next request, which would be the expected behavior.

[thw0rted]

Looks like the changelog is a merge conflict -- do you want to resolve or should I rebase and force-push this branch? (I do it all the time on my own projects but last time I did that here, somebody was upset about it, so I thought I should ask.)

[ggetz]

Thanks @thw0rted! You can go ahead and merge main into your branch and resolve the conflicts.

Unless you're trying to squash your commits, there shouldn't be a need to force push 👍

[thw0rted]

Ah, see, our workflow is to rebase then force push, which results in a cleaner history (no merge commits), but at the danger of conflicts between alternate histories. I'll do the merge commit, no problem.

[ggetz]

Thanks @thw0rted!