Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support 3D Tiles feature picking in Viewer #9121

Merged
merged 5 commits into from
Aug 31, 2020
Merged

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Aug 28, 2020

Viewernow has default pick handling forCesium3DTileFeaturedata and will display its properties in the defaultInfoBoxas well as setViewer.selectedEntity` to a transient Entity instance representing the data. While a little clunky, this is identical to how ImageryLayer picking has worked for ages.

@cesium-concierge
Copy link

Thanks for the pull request @mramato!

  • ✔️ Signed CLA found.
  • ❔ 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.

@mramato mramato requested a review from OmarShehata August 28, 2020 15:58
@mramato
Copy link
Contributor Author

mramato commented Aug 28, 2020

This area in general isn't super well unit tested and it didn't look like it was easy to test Cesium3DFeature without mocking a bunch of things. Given it's super easy to test with Sandcastle, I didn't worry about it for now.

Here's a link to the OSM demo which now showcases this behavior by default: http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/feature-picking-3d/Apps/Sandcastle/index.html?src=Cesium%20OSM%20Buildings.html

Copy link
Contributor

@OmarShehata OmarShehata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty amazing. Even though I already knew how to do this in my app, this is way more convenient.

One thing I don't get about the code is what happens to the new Entity that's constructed for this - does it need to be explicitly destroyed? Does it just get garbage collected when selectedEntity = undefined ?

Source/Widgets/Viewer/Viewer.js Show resolved Hide resolved
Source/Widgets/Viewer/Viewer.js Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
@mramato
Copy link
Contributor Author

mramato commented Aug 28, 2020

One thing I don't get about the code is what happens to the new Entity that's constructed for this - does it need to be explicitly destroyed? Does it just get garbage collected when selectedEntity = undefined ?

No need to explicitly destroy it, it will get garbage collected when the infobox is closed or programmatically unselected.

@mramato
Copy link
Contributor Author

mramato commented Aug 28, 2020

@OmarShehata I think I addressed your feedback, anything else?

@OmarShehata
Copy link
Contributor

I was considering if we need to update any Sandcastle examples to simplify them, like this one in particular: http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/feature-picking-3d/Apps/Sandcastle/index.html?src=3D%20Tiles%20Feature%20Picking.html, but that's doing more than just getting the properties on click (getting it on hover, adding an HTML overlay, highlighting with post processing).

It may be valuable to add a separate code example for extracting the selectedEntity.feature, perhaps something like this but I think we need to change/remove one of the existing ones, because we'll just be showing a lot of different ways to do the same thing and ideally we should show just the easiest way to do it.

@OmarShehata
Copy link
Contributor

I am going to merge this since I am happy with how the feature works, and because the .feature property is documented now, and we can revisit revising the Sandcastle example since we're planning on taking a look at improving how Sandcastle is organized soon.

@OmarShehata OmarShehata merged commit c963081 into master Aug 31, 2020
@OmarShehata OmarShehata deleted the feature-picking-3d branch August 31, 2020 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants