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 incorrect distanceSquaredTo calculation in BoundingSphere #9686

Merged
merged 5 commits into from
Jul 20, 2021

Conversation

j9liu
Copy link
Contributor

@j9liu j9liu commented Jul 20, 2021

As noted in the forum, the distanceSquaredTo calculation of BoundingSphere was incorrect. It was calculated as (position - center)**2 - radius**2, instead of ((position - center) - radius)**2. This PR fixes the algorithm and the unit test that corresponds to it.

cc @lilleyse @ebogo1

@cesium-concierge
Copy link

Thanks for the pull request @j9liu!

  • ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@lilleyse
Copy link
Contributor

Thanks @j9liu for the fix, and thanks @donhatch for letting us know about the problem in the forum post and in #9534

Looks like this problem goes way back: febb47b

Turns out this function is only used for sorting translucent commands and sorting tiles for globe picking which have managed to get away with less than perfect results. Thankfully SSE calculations are correct since they use TileBoundingSphere.prototype.distanceToCamera which is actually implemented correctly.

Just a couple suggestions

  • It should return 0.0 if the point is inside the sphere. OrientedBoundingBox does the same thing. This should be mentioned in both classes' documentation like it is in TileBoundingSphere.prototype.distanceToCamera. Add a test for this.
  • In the BoundingSphere doc remove the word "estimated". It can just be "The distance squared from the bounding sphere to the point"
  • Looks like the OrientedBoundingBox doc copied the same line "The estimated distance squared from the bounding sphere to the point." It should be "The distance squared from the oriented bounding box to the point."

@j9liu
Copy link
Contributor Author

j9liu commented Jul 20, 2021

@lilleyse - Thanks for the review. I just made some changes that addressed your points, let me know if I missed anything!

@lilleyse
Copy link
Contributor

Thanks! Looks good.

@lilleyse lilleyse merged commit cc92866 into main Jul 20, 2021
@lilleyse lilleyse deleted the bounding-sphere-distancesquared branch July 20, 2021 22:50
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