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

Configurable volume width for sampleHeight and clampToHeight #7287

Merged
merged 5 commits into from
Jan 2, 2019

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Nov 26, 2018

Fixes #7248

The user can now provide their own volume width when sampling height which helps get better results when sampling sparse point clouds. Screenshots of St. Helens with this option are in #7248.

Also the default width changed from 0.01 meters to 0.1 meters to help improve the common case.

To do:

  • Open deprecation issue after merge

@cesium-concierge
Copy link

Thanks for the pull request @lilleyse!

  • ✔️ Signed CLA found.

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.

🌍 🌎 🌏

@lilleyse lilleyse force-pushed the configurable-pick-width branch from 535877f to e70d371 Compare November 26, 2018 23:11
@cesium-concierge
Copy link

Thanks again for your contribution @lilleyse!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.


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

🌍 🌎 🌏

@lilleyse lilleyse force-pushed the configurable-pick-width branch 2 times, most recently from e9236ec to 4eb6d1e Compare January 1, 2019 20:35
@lilleyse
Copy link
Contributor Author

lilleyse commented Jan 1, 2019

Updated changes and added some tests. This is ready to review now. @bagnell could you check this out?

@lilleyse lilleyse requested a review from bagnell January 1, 2019 20:38
@@ -777,16 +778,18 @@ define([
camera.frustum.far = 10000000000.0;
}

var pickOffscreenDefaultWidth = 0.1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this a constant or is it meant to be configurable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hadn't thought about making it configurable, but I might as well. I marked it as private though since the preferred way is to pass the width to the function.

@@ -3898,15 +3904,15 @@ define([
(objectsToExclude.indexOf(object.id) > -1);
}

function getRayIntersection(scene, ray, objectsToExclude, requirePosition, async) {
function getRayIntersection(scene, ray, objectsToExclude, width, requirePosition, async) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you rename the async argument since it'll be a reserved word?

@@ -3957,22 +3963,22 @@ define([
}
}

function getRayIntersections(scene, ray, limit, objectsToExclude, requirePosition, async) {
function getRayIntersections(scene, ray, limit, objectsToExclude, width, requirePosition, async) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment throughout about async.

@lilleyse
Copy link
Contributor Author

lilleyse commented Jan 2, 2019

@bagnell Ready

@bagnell bagnell merged commit d5753b7 into master Jan 2, 2019
@bagnell bagnell deleted the configurable-pick-width branch January 2, 2019 23:45
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