Skip to content
New issue

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

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

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants