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

Add width and height parameters to Scene.drillPick #6363

Closed
wants to merge 2 commits into from

Conversation

sir-chaos
Copy link
Contributor

On #6361

@cesium-concierge
Copy link

Please sign the CLA before we review this PR.

Welcome to the Cesium community @sir-chaos!

Can you please send in a Contributor License Agreement (CLA) so that we can review and merge this pull request?

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.


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

🌍 🌎 🌏

@hpinkos
Copy link
Contributor

hpinkos commented Mar 22, 2018

Thanks for the pull request @sir-chaos! When you have a moment, can you send over a CLA? We need that in order to review and merge your changes. Instructions are in the cesium-concierge post above.

Also, we'll probably want some unit tests before merging this. Thanks!

@sir-chaos
Copy link
Contributor Author

sir-chaos commented Mar 22, 2018

@hpinkos sure, just did it. Hope the tests will pass :)

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 13, 2018

Thanks again @sir-chaos, we received your CLA, sorry for the slow response.

@hpinkos
Copy link
Contributor

hpinkos commented Apr 17, 2018

Hi @sir-chaos, did you add unit tests? If you did you may have forgotten to push them up, I don't see it in this pull request

@sir-chaos
Copy link
Contributor Author

Hi @hpinkos, probably I misunderstood your last comment, so no tests added yet, but I can try and do that.

@cesium-concierge
Copy link

Thanks again for the pull request!

Looks like this pull request hasn't been updated in 30 days since I last commented.

To keep things tidy should this be closed? Perhaps keep the branch and submit an issue?

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

🌍 🌎 🌏

@chris-cooper
Copy link
Contributor

Hi, what is the status of this PR @sir-chaos ?

@hpinkos
Copy link
Contributor

hpinkos commented Aug 15, 2018

Thanks for the PR @sir-chaos! @chris-cooper went ahead and finished up these changes in #6922. These changes will be included in Cesium 1.49 available on September 3rd.
Hope to see more contributions from you in the future! =)

@hpinkos hpinkos closed this Aug 15, 2018
@sir-chaos
Copy link
Contributor Author

Glad to hear that.
Unfortunately I wasn't able to provide the unit tests as was suggested, but it's good the features are now in trunk.

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.

5 participants