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

Fix globe.pick for 2D/CV #6859

Merged
merged 7 commits into from
Aug 8, 2018
Merged

Fix globe.pick for 2D/CV #6859

merged 7 commits into from
Aug 8, 2018

Conversation

hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Jul 30, 2018

This makes globe.pick return a position in 3D coordinates instead of world coordinates based on the scene mode, which is what users are expecting. We did a similar thing for scene.pickPosition.

This fixes imagery layer feature picking in 2D/Columbus View.

@cesium-concierge
Copy link

Thanks for the pull request @hpinkos!

  • ✔️ 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.
  • Works (or fails gracefully) in IE11.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@hpinkos
Copy link
Contributor Author

hpinkos commented Jul 31, 2018

This is ready for review. I updated the first comment now that I have a better understanding of how our coordinates are supposed work. We did something similar for scene.pickPosition at some point, so I'm adding the same behavior to globe.pick.

@hpinkos
Copy link
Contributor Author

hpinkos commented Aug 8, 2018

@bagnell can you review please?

@bagnell
Copy link
Contributor

bagnell commented Aug 8, 2018

The code looks good to me. Since this is a breaking change, are we sure we don't want to add a new function and deprecate the old one?

@hpinkos
Copy link
Contributor Author

hpinkos commented Aug 8, 2018

I think keeping it this way makes it consistent with what we did for pickPosition and I'm sure it's what users are expecting anyway. I would guess pretty much anyone using globe.pick now thinks it's broken in 2D/CV instead of knowing it's in the wrong projection. I can add a note to CHANGES though

@bagnell bagnell mentioned this pull request Aug 8, 2018
@hpinkos hpinkos changed the title Fix globe.pick for Columbus View Fix globe.pick for 2D/CV Aug 8, 2018
@bagnell
Copy link
Contributor

bagnell commented Aug 8, 2018

Since this has some of my code changes, someone else should review and merge. @mramato?

@hpinkos
Copy link
Contributor Author

hpinkos commented Aug 8, 2018

@bagnell this is ready

@hpinkos hpinkos closed this Aug 8, 2018
@hpinkos hpinkos reopened this Aug 8, 2018
@mramato
Copy link
Contributor

mramato commented Aug 8, 2018

I'm only somewhat familiar with this code, but nothing jumped out as terrible to me. If both @bagnell and @hpinkos are happy and this passes, one of you can just hit merge.

@bagnell bagnell merged commit 04b99aa into master Aug 8, 2018
@bagnell bagnell deleted the globe-pick branch August 8, 2018 21:14
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.

4 participants