Pick/Drillpick Imagery Layer #9651

srothst1 · 2021-06-30T22:02:29Z

Based on the discussion from this post, there is clearly demand for functionality allowing users to select an imagery layer (or imagery layers) from our viewer. Currently, in demos like this one, if a user selects an imagery layer and not a feature they are presented with the message "No features found." The final goal of this pull request is to give users easy access to imagery layer data.

I have spent the last few days looking through ImageryLayer.js, ImageryLayerCollection.js and Viewer.js. I believe that this functionality should be added to a new method pickImageryLayer in ImageryLayerCollection.js and eventually integrated into either viewer.pickEntity or viewer.pickImageryLayerFeature.

Here is a sandcastle demo that showcases how this functionality currently works. When selecting a position on the globe, we see that pickImageryLayers returns an ImageryLayer and pickImageryLayerFeatures returns nothing since there are no features. I think this is a step in the right direction.

While I am looking for overall feedback and clarification, I do have some more specific questions:

Should ImageryLayerCollection.prototype.pickImageryLayers return a ImageryLayerCollection or a promise to ImageryLayerFeatureInfo?
How should ImageryLayerCollection.prototype.pickImageryLayers be integrated into ImageryLayer.js and Scene.js?

When I get a better understanding of what needs to be done, I will update ImageryLayerCollectionSpec.js and CHANGES.md. I do not expect this to be part of Thursday's release.

@lilleyse
@ebogo1

cesium-concierge · 2021-06-30T22:02:33Z

Thanks for the pull request @srothst1!

✔️ 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.
❔ Unit tests were not updated.
- Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

Cesium Viewer works.
Works in 2D/CV.
Works (or fails gracefully) in IE11.

lilleyse

Not too many comments on the code itself which looks pretty good. The major limitation of this approach is that it isn't pixel perfect. Still I think it's would be valuable until someone implements the pixel perfect approach. I wrote some starter notes on that here: #8692 (comment).

Probably not worth tackling now unless you are up for it.

Source/Scene/ImageryLayerCollection.js

srothst1 · 2021-07-07T14:56:08Z

@lilleyse Thank you for the feedback. I am happy to hear that the code looked mostly good. I implemented some of your suggestions:

pickImageryLayers returns an array of ImageryLayer objects instead of an ImageryLayerCollection. Following the precedent set by drillPick and pickImageryLayerFeatures.
Created a helper function (pickImageryLayersHelper) for pickImageryLayers and pickImageryLayerFeatures, removing a lot of duplicated code.
Updated the documentation to include information about the caveats of the rectangle-based approach currently implemented.

Before I consider implementing a pixel-perfect version of pickImageryLayers, I want to ensure that the code I currently have is correct.

A few questions:

On my local machine, all specs pass. Why are some checks not successful on GitHub? How should I debug when this occurs?
How do I generate the documentation to make sure everything is generated properly?
Should I create a spec for pickImageryLayersHelper or pickImageryLayers?

Sandcastle Demos:
#1 #2

lilleyse · 2021-07-09T20:14:39Z

It could be a fluke. I restarted the build and CI passed. If you see similar issues open an issue. (EDIT: still running actually)
Run npm run generateDocumentation. If you're curious about the various build commands see the Build Guide
Add tests for pickImageryLayers. It's fine not to test pickImageryLayersHelper. On the topic of tooling, you can check how well the unit tests cover the the code by running npm run coverage and seeing the coverage results. If you're only concerned with a single suite of tests you can change describe to fdescribe and it will only run those tests.

Source/Scene/ImageryLayerCollection.js

lilleyse · 2021-07-09T21:01:35Z

The new approach is better but there's still a lot of code duplication. One idea is to pass a callback function to the helper so that once overlapping imagery is found it can let the callback do the rest of the work. This lets pickImageryLayers and pickImageryLayerFeatures define their own custom behavior with little code duplication.

It was a bit hard to annotate the idea on github so I mocked something up: callback.txt. I haven't actually tested it so double check that there aren't any mistakes. What do you think?

lilleyse · 2021-07-09T21:05:00Z

On my local machine, all specs pass. Why are some checks not successful on GitHub? How should I debug when this occurs?

I submitted an issue for this #9672

Co-authored-by: Sean Lilley <[email protected]>

…back function to pickImageryHelper

srothst1 · 2021-07-13T01:43:08Z

@lilleyse Thanks for the feedback! I added a series of commits. Here is a quick summary:

Added specs for pickImageryLayers. I modeled these specs after the tests for pickImageryLayerFeatures. Let me know if the specs that I added are comprehensive enough.
Updated the documentation for pickImageryLayers according to your specifications.
Removed an unnecessary if statement from pickImageryLayers.
Updated the helper function pickImageryHelper, removing it from the ImageryLayerCollection class.
Implemented your suggestion of using a callback function to minimize code duplication. Upon further consideration, I agree with this approach. I think it makes the code is a lot more intuitive and ImageryLayerCollection.js now matches the rest of our codebase more closely.

lilleyse

Thanks @srothst1 - the main code looks good - I have some comments on the tests. Make sure to run code coverage (see #9651 (comment)) - there are some code paths that would be frequently hit that could be better tested. I marked some of those in red.

Specs/Scene/ImageryLayerCollectionSpec.js

srothst1 · 2021-07-14T20:50:54Z

@lilleyse a few updates:

We now see that the ImageryLayerCollection spec passes. In addition, the following individual specs in pickImageryLayers now pass:

returns undefined when pick ray does not intersect surface
returns undefined when globe has no pickable layers
returns imagery from one layer
returns imagery from two layers
returns undefined if there are zero imagery layers (covers imageryLayers.length === 0)

I created a spec providing coverage for imageryLayers.length === 0 in pickImageryLayers. (see above)
The following paths can be found in pickImageryHelper:

line 356 -> undefined pickedTile
line 370 -> pickFeatures && !defined(provider.pickFeatures)
line 374 -> !Rectangle.contains(imagery.rectangle, pickedLocation)

Should specs covering these paths go in a separate describe, or should they be implemented in "pickImageryLayers" or "pickImageryLayerFeatures"? Do you have any suggestions for how to cover these paths?

I moved the updateUntilDone helper function out of the describe block of "pickImageryLayers" and "pickImageryLayerFeatures". updateUntilDone is now a stand-alone function, making the code cleaner looking.
I removed tests from pickImageryLayers that referenced ImageryProvider.pickFeatures and pickFeatures references from "returns imagery from one layer" and "returns imagery from two layers" in "pickImageryLayers".

lilleyse · 2021-07-16T18:23:53Z

Should specs covering these paths go in a separate describe, or should they be implemented in "pickImageryLayers" or "pickImageryLayerFeatures"? Do you have any suggestions for how to cover these paths?

The specs can be put in the group they're most relevant to. If there's overlap I think pickImageryLayers is fine because it's the more general capability. This is how I would do it:

pickImageryLayers describe block

line 356 -> undefined pickedTile
line 374 -> !Rectangle.contains(imagery.rectangle, pickedLocation)

pickImageryLayerFeatures describe block

line 370 -> pickFeatures && !defined(provider.pickFeatures)

… better coverage

… location to improve coverage

… to improve coverage

srothst1 · 2021-07-21T20:20:19Z

@lilleyse I apologize for the delay - I just added three specs that should address the cases outlined above.

"continues if imagery rectangle does not contain picked location" covers the following code in ImageryLayerCollection.js when run individually (checked using DevTools)

    if (!Rectangle.contains(imagery.rectangle, pickedLocation)) {
      continue;
    }

However, coverage is not shown when running npm run coverage.

Any ideas of why we are seeing this behavior?

lilleyse

"pickImageryHelper returns if undefined tile picked"
"continues if imagery rectangle does not contain picked location"

These descriptions could be improved. They should be describing the behavior, not how the code is written. Maybe instead:

pickImageryLayers returns undefined if no tiles are picked
pickImageryLayers skips imagery layers that don't overlap the picked location

"pickImageryHelper continues if provider.pickFeatures is undefined"

This ought to already be covered by returns undefined when ImageryProvider does not implement pickFeatures...

"continues if imagery rectangle does not contain picked location" covers the following code in ImageryLayerCollection.js when run individually (checked using DevTools)
...
However, coverage is not shown when running npm run coverage.

This is most likely related to #9651 (comment). You'll need to get to the bottom of why. This could also explain why other tests aren't hitting the right code.

lilleyse · 2021-07-21T23:51:54Z

Specs/Scene/ImageryLayerCollectionSpec.js

+      var customScene;
+      var customGlobe;
+      var customCamera;

      beforeAll(function () {
        scene = createScene();
        globe = scene.globe = new Globe();
        camera = scene.camera;

        scene.frameState.passes.render = true;
+        //NEW:
+        customScene = createScene();
+        customGlobe = customScene.globe = new Globe();
+        customCamera = customScene.camera;
+
+        customScene.frameState.passes.render = true;


What are these custom variables used for? Feels like a strange workaround.

I originally added the set of custom variables because the spec "pickImageryLayers returns undefined if no tiles are picked" modifies globe._surface._tilesToRender and I do not want this change to impact the other specs.

…s are picked

lilleyse

@srothst1 just one comment and then this PR can be merged.

lilleyse · 2021-07-26T15:57:00Z

Source/Scene/ImageryLayerCollection.js

- * Asynchronously determines the imagery layer features that are intersected by a pick ray.  The intersected imagery
- * layer features are found by invoking {@link ImageryProvider#pickFeatures} for each imagery layer tile intersected
- * by the pick ray.  To compute a pick ray from a location on the screen, use {@link Camera.getPickRay}.
+ * Helper function for `pickImageryLayers` and `pickImageryLayerFeatures`.


It's fine to remove the documentation above pickImageryHelper since it's a local function.

Also some of the doc is misformatted (I think in @return) which is causing CI to fail, but no need to worry about since the doc will be removed.

@lilleyse I just removed the documentation above pickImageryHelper. CI now passes.

lilleyse · 2021-07-26T16:48:49Z

@srothst1 Sorry one last thing - could you add an entry to CHANGES.md?

srothst1 · 2021-07-26T16:55:33Z

@lilleyse done!

lilleyse · 2021-07-26T17:17:49Z

Added the ability to select imagery layers. #9651

This could use a little more detail, maybe:

Added ImageryLayerCollection.pickImageryLayers which determines the imagery layers that are intersected by a pick ray. #9651

pick imagery layer

342f0eb

removed reference to promise

ced8594

lilleyse mentioned this pull request Jul 4, 2021

Pick/drillpick imagery layer #8692

Closed

lilleyse reviewed Jul 4, 2021

View reviewed changes

Source/Scene/ImageryLayerCollection.js Outdated Show resolved Hide resolved

Source/Scene/ImageryLayerCollection.js Outdated Show resolved Hide resolved

Source/Scene/ImageryLayerCollection.js Outdated Show resolved Hide resolved

srothst1 added 3 commits July 6, 2021 09:57

pickImageryLayers returns an array of ImageryLayer objects

047409e

created a helper function, updated documentation

67f9e15

updated documentation, removed console.log() calls

e3dafcb

lilleyse reviewed Jul 9, 2021

View reviewed changes

Source/Scene/ImageryLayerCollection.js Outdated Show resolved Hide resolved

Source/Scene/ImageryLayerCollection.js Outdated Show resolved Hide resolved

Source/Scene/ImageryLayerCollection.js Outdated Show resolved Hide resolved

lilleyse and others added 7 commits July 9, 2021 17:47

Merge branch 'master' into drillpick-imagery-layer

d791aff

Update Source/Scene/ImageryLayerCollection.js

dc6d21a

Co-authored-by: Sean Lilley <[email protected]>

removed unessesary check from pickImageryLayers

ccf35cf

removed helper function from the ImageryLayerCollection class

61fc141

removed code duplication from ImageryLayerCollection by adding a call…

92cb7df

…back function to pickImageryHelper

updated ImageryLayerCollectionSpec.js to include pickImageryLayers

d9bcbc9

cleaned up ImageryLayerCollectionSpec.js

348fc4b

updated spec

40a8555

srothst1 assigned srothst1 and unassigned srothst1 Jul 13, 2021

lilleyse reviewed Jul 14, 2021

View reviewed changes

Specs/Scene/ImageryLayerCollectionSpec.js Outdated Show resolved Hide resolved

Specs/Scene/ImageryLayerCollectionSpec.js Outdated Show resolved Hide resolved

Specs/Scene/ImageryLayerCollectionSpec.js Outdated Show resolved Hide resolved

updated ImageryLayerCollectionSpec.js

1b49533

removed console.log statement

2cea953

starting to add specs

2f16339

srothst1 added 5 commits July 21, 2021 08:32

added the spec pickImageryHelper returns if undefined tile picked for…

0d5a571

… better coverage

added the spec continues if imagery rectangle does not contain picked…

fb74484

… location to improve coverage

added the spec helper continues if provider.pickFeatures is undefined…

2783af5

… to improve coverage

cleaned up specs

3f3a4e4

updated spec name

3a023b7

Merge branch 'main' into drillpick-imagery-layer

da1c254

lilleyse reviewed Jul 22, 2021

View reviewed changes

srothst1 added 4 commits July 21, 2021 17:45

updated spec names and pickImageryLayers returns undefined if no tile…

37b3d06

…s are picked

updated spec ordering

bd82e88

group work

2d49b53

removed three new specs

9ff2243

lilleyse reviewed Jul 26, 2021

View reviewed changes

removed documentation above pickImageryHelper

a3b08eb

srothst1 added 2 commits July 26, 2021 09:53

updated CHANGES.md

4620659

updated CHANGES.md

bf24654

srothst1 and others added 3 commits July 26, 2021 10:18

added detail to CHANGES.md

9d598ed

added markup

9c17f9b

Merge branch 'main' into drillpick-imagery-layer

aca8921

lilleyse merged commit 1cf9d5b into main Jul 26, 2021

lilleyse deleted the drillpick-imagery-layer branch July 26, 2021 18:01

lilleyse mentioned this pull request Jul 26, 2021

Pixel perfect pick/drillpick for imagery layers #9695

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pick/Drillpick Imagery Layer #9651

Pick/Drillpick Imagery Layer #9651

srothst1 commented Jun 30, 2021 •

edited

Loading

cesium-concierge commented Jun 30, 2021

lilleyse left a comment

srothst1 commented Jul 7, 2021 •

edited

Loading

lilleyse commented Jul 9, 2021 •

edited

Loading

lilleyse commented Jul 9, 2021

lilleyse commented Jul 9, 2021

srothst1 commented Jul 13, 2021 •

edited

Loading

lilleyse left a comment

srothst1 commented Jul 14, 2021 •

edited

Loading

lilleyse commented Jul 16, 2021 •

edited

Loading

srothst1 commented Jul 21, 2021

lilleyse left a comment •

edited

Loading

lilleyse Jul 21, 2021

srothst1 Jul 22, 2021

lilleyse left a comment

lilleyse Jul 26, 2021

srothst1 Jul 26, 2021

lilleyse Jul 26, 2021

lilleyse commented Jul 26, 2021

srothst1 commented Jul 26, 2021

lilleyse commented Jul 26, 2021

Pick/Drillpick Imagery Layer #9651

Pick/Drillpick Imagery Layer #9651

Conversation

srothst1 commented Jun 30, 2021 • edited Loading

cesium-concierge commented Jun 30, 2021

lilleyse left a comment

Choose a reason for hiding this comment

srothst1 commented Jul 7, 2021 • edited Loading

lilleyse commented Jul 9, 2021 • edited Loading

lilleyse commented Jul 9, 2021

lilleyse commented Jul 9, 2021

srothst1 commented Jul 13, 2021 • edited Loading

lilleyse left a comment

Choose a reason for hiding this comment

srothst1 commented Jul 14, 2021 • edited Loading

lilleyse commented Jul 16, 2021 • edited Loading

srothst1 commented Jul 21, 2021

lilleyse left a comment • edited Loading

Choose a reason for hiding this comment

lilleyse Jul 21, 2021

Choose a reason for hiding this comment

srothst1 Jul 22, 2021

Choose a reason for hiding this comment

lilleyse left a comment

Choose a reason for hiding this comment

lilleyse Jul 26, 2021

Choose a reason for hiding this comment

srothst1 Jul 26, 2021

Choose a reason for hiding this comment

lilleyse Jul 26, 2021

Choose a reason for hiding this comment

lilleyse commented Jul 26, 2021

srothst1 commented Jul 26, 2021

lilleyse commented Jul 26, 2021

srothst1 commented Jun 30, 2021 •

edited

Loading

srothst1 commented Jul 7, 2021 •

edited

Loading

lilleyse commented Jul 9, 2021 •

edited

Loading

srothst1 commented Jul 13, 2021 •

edited

Loading

srothst1 commented Jul 14, 2021 •

edited

Loading

lilleyse commented Jul 16, 2021 •

edited

Loading

lilleyse left a comment •

edited

Loading