Screen overlay #9864

krupkad · 2021-10-12T03:58:00Z

This provides an MVP of KML ScreenOverlay support.

both external/URL images and bundled/kmz images are supported
rotation is not supported
enabled by passing a container element to KMLDataSource.load
"Cleaning up" the container is up to the user - no management of the created image elements is done here once they are created/positioned.

cesium-concierge · 2021-10-12T03:58:04Z

Thank you so much for the pull request @krupkad! I noticed this is your first pull request and I wanted to say welcome to the Cesium community!

The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.

❌ Missing CONTRIBUTORS.md entry.
- Please add yourself to the contributors file!
❌ Missing CLA.
- Please send in a Contributor License Agreement (CLA) and comment back here to let us know to check this!
- Maintainers, here's how to check for a submitted CLA.
❔ 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.
Works (or fails gracefully) in IE11.

IanLilleyT

Looks good - mostly small stuff to fix. Wondering if anyone else has thoughts on whether the screen overlay should be cleaned up by the caller vs automatic. @mramato @hpinkos

CHANGES.md

package.json

Source/DataSources/KmlDataSource.js

Specs/DataSources/KmlDataSourceSpec.js

IanLilleyT · 2021-10-12T14:34:49Z

Source/DataSources/KmlDataSource.js

@@ -3333,6 +3514,7 @@ function load(dataSource, entityCollection, data, options) {
 * @property {Boolean} [clampToGround=false] true if we want the geometry features (Polygons, LineStrings and LinearRings) clamped to the ground.
 * @property {Ellipsoid} [ellipsoid=Ellipsoid.WGS84] The global ellipsoid used for geographical calculations.
 * @property {Credit|String} [credit] A credit for the data source, which is displayed on the canvas.
+ * @property {String} [screenOverlayContainer] A container for ScreenOverlay images. Note: this container is not managed, the caller is responsible for styling and removing unused overlay images.


Is String the right type?

seems to be what other docs use for "pass in an element" options

ah ok I see how getElement.js will accept a string or an element

Source/DataSources/KmlDataSource.js

krupkad · 2021-10-13T13:36:07Z

@mramato I added the destroy method as discussed - turns out there is existing cleanup machinery in Source/DataSources/DataSourceCollection.js

Source/DataSources/KmlDataSource.js

mramato · 2021-10-13T14:57:30Z

@mramato I added the destroy method as discussed - turns out there is existing cleanup machinery in Source/DataSources/DataSourceCollection.js

Awesome, I thought we added something like that but wasn't completely sure.

mramato · 2021-10-13T14:58:03Z

Can we update the viewerDragDropMixin and Sandcastle demos to support screen overlays? I'm pretty sure at least one of our sample files already has one.

hpinkos · 2021-10-13T14:59:49Z

Do you think we should add a container for screen overlays that the Viewer creates that KmlDataSource can use by default if a container is not specified? How does Google Earth work?

mramato · 2021-10-13T15:16:43Z

Do you think we should add a container for screen overlays that the Viewer creates that KmlDataSource can use by default if a container is not specified?

viewerDragDropMix could do this but I'm not sure how it would work in Viewer itself since it doesn't know anything about KmlDataSource and isn't the one that creates it.

How does Google Earth work?

The web version doesn't have an API (and I'm not even sure it supports screen overlays). The desktop version has no customization either for similar reasons.

hpinkos · 2021-10-13T15:30:39Z

I'm not sure how it would work in Viewer itself since it doesn't know anything about KmlDataSource and isn't the one that creates it.

Right, I was thinking the viewer could create an element and KmlDataSource could getElementById to see if it happens to exist, which I'm assuming it would most of the time since I imagine most users create a Viewer. Or KmlDataSource could document.getElementByClassName('cesium-viewer') and add it's own screen overlay container to that div if it exists.

Just a thought. If we decide not to do this we might want to show a one time warning if the KML has a screen overlay and the user did not supply a screen overlay container.

mramato · 2021-10-13T15:32:37Z

I think that too closely couples everything together.

hpinkos · 2021-10-13T15:52:01Z

eh, maybe. I just thought it might make it easier for folks and make the default functionality more similar to google earth.

krupkad · 2021-10-13T15:56:40Z

enabled screen overlays in both apps by passing the main container - seems to be working (though the overlay is under the toolbar in sandcastle)

CHANGES.md

Apps/CesiumViewer/CesiumViewer.js

Source/DataSources/KmlDataSource.js

Specs/DataSources/KmlDataSourceSpec.js

Source/DataSources/KmlDataSource.js

Specs/DataSources/KmlDataSourceSpec.js

Source/DataSources/KmlDataSource.js

IanLilleyT · 2021-10-15T14:29:56Z

@hpinkos @mramato any final comments on the PR? I think it's just about ready to go

hpinkos · 2021-10-15T14:35:43Z

Nope, looks good to me

IanLilleyT · 2021-10-15T19:17:43Z

Merged. Congrats on your first CesiumJS PR @krupkad!

Make sure to update #873 with any new information

krupkad added 7 commits October 11, 2021 07:37

Add KML screen overlay support

c8dcad9

Add KML screen overlay unit testing

800886d

Clear screen overlay at KML load

7b76f12

Merge remote-tracking branch 'origin/main' into screen-overlay

7b8c0ea

Fix linter errors

a61bd4b

Force overlay sizing to occur after image load

d04b0a2

Fix ScreenOverlay tests

fd4ad2d

krupkad added 2 commits October 12, 2021 06:47

Update CHANGES.md/CONTRIBUTORS.md

3215048

Document screenOverlayContainer management

acf9235

IanLilleyT suggested changes Oct 12, 2021

View reviewed changes

krupkad added 3 commits October 12, 2021 10:48

Fix screen overlay insetPixels handling

4cdd844

KML ScreenOverlay cleanup

50eba46

Move screen overlay cleanup to afterEach in tests

1270377

krupkad requested a review from IanLilleyT October 12, 2021 14:58

krupkad added 3 commits October 13, 2021 09:01

Add cleanup method to KMLDataSource

1cfaa5d

Update documentation for KMLDataSource.cleanup

78f01e9

Rename KMLDataSource.cleanup to KMLDataSource.destroy

a701859

hpinkos reviewed Oct 13, 2021

View reviewed changes

Source/DataSources/KmlDataSource.js Outdated Show resolved Hide resolved

Fix screenOverlayContainer type in docs

b3b8a05

Enable screenOverlay in demo apps

043724e

krupkad requested a review from hpinkos October 13, 2021 15:56

IanLilleyT suggested changes Oct 14, 2021

View reviewed changes

krupkad added 3 commits October 14, 2021 13:40

Use viewer.container in demo apps

ef68d58

Clean up KML documentation

584cc4e

Update CHANGES.md

d899014

krupkad requested a review from IanLilleyT October 14, 2021 17:44

krupkad added 3 commits October 14, 2021 14:00

Fix linter errors

d90ae7f

Fix drag-and-drop screen overlays

faf6985

Fix screen overlay cleanup in tests

093f36f

IanLilleyT suggested changes Oct 15, 2021

View reviewed changes

Source/DataSources/KmlDataSource.js Outdated Show resolved Hide resolved

Specs/DataSources/KmlDataSourceSpec.js Outdated Show resolved Hide resolved

Specs/DataSources/KmlDataSourceSpec.js Outdated Show resolved Hide resolved

Clean up linter errors

f8d94c2

IanLilleyT suggested changes Oct 15, 2021

View reviewed changes

Source/DataSources/KmlDataSource.js Outdated Show resolved Hide resolved

Stray screen overlay option cleanup

1b08638

IanLilleyT approved these changes Oct 15, 2021

View reviewed changes

IanLilleyT merged commit 4f557e6 into main Oct 15, 2021

IanLilleyT deleted the screen-overlay branch October 15, 2021 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Screen overlay #9864

Screen overlay #9864

krupkad commented Oct 12, 2021 •

edited by IanLilleyT

cesium-concierge commented Oct 12, 2021

IanLilleyT left a comment

IanLilleyT Oct 12, 2021

krupkad Oct 12, 2021

IanLilleyT Oct 12, 2021

krupkad commented Oct 13, 2021

mramato commented Oct 13, 2021

mramato commented Oct 13, 2021

hpinkos commented Oct 13, 2021

mramato commented Oct 13, 2021 •

edited

hpinkos commented Oct 13, 2021

mramato commented Oct 13, 2021

hpinkos commented Oct 13, 2021

krupkad commented Oct 13, 2021

IanLilleyT commented Oct 15, 2021

hpinkos commented Oct 15, 2021

IanLilleyT commented Oct 15, 2021

Screen overlay #9864

Screen overlay #9864

Conversation

krupkad commented Oct 12, 2021 • edited by IanLilleyT

cesium-concierge commented Oct 12, 2021

IanLilleyT left a comment

Choose a reason for hiding this comment

IanLilleyT Oct 12, 2021

Choose a reason for hiding this comment

krupkad Oct 12, 2021

Choose a reason for hiding this comment

IanLilleyT Oct 12, 2021

Choose a reason for hiding this comment

krupkad commented Oct 13, 2021

mramato commented Oct 13, 2021

mramato commented Oct 13, 2021

hpinkos commented Oct 13, 2021

mramato commented Oct 13, 2021 • edited

hpinkos commented Oct 13, 2021

mramato commented Oct 13, 2021

hpinkos commented Oct 13, 2021

krupkad commented Oct 13, 2021

IanLilleyT commented Oct 15, 2021

hpinkos commented Oct 15, 2021

IanLilleyT commented Oct 15, 2021

krupkad commented Oct 12, 2021 •

edited by IanLilleyT

mramato commented Oct 13, 2021 •

edited