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

Account for degenerate axes of OrientedBoundingBox in distanceSquaredTo #9670

Merged
merged 5 commits into from
Jul 15, 2021

Conversation

j9liu
Copy link
Contributor

@j9liu j9liu commented Jul 9, 2021

Fixes #9658. Before normalizing the halfAxes, the function will check if their magnitude is nonzero. If not, it will generate new substitute axes based on which ones are degenerate, then proceed with the same algorithm. I added a bunch of specs to account for these changes.

I'm sure the code organization / style can be cleaned up a bit. @lilleyse - do you mind taking a look?

@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.

Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

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

Overall the approach makes sense and it fits into the current algorithm cleanly, though it feels a little verbose and I wonder if specialized distance functions for each degenerate axes case would be less code and/or faster. The non-degenerate case ought to run fast still which is most important.

Since distanceSquaredTo is a hotspot in the code it would be worth benchmarking. The benchmark should create some randomly sized, non-degenerate OBBs, and call distanceSquaredTo multiple times for each. You can look at @sanjeetsuhag's sandcastles in #9599 for reference. Make sure to test with the built version of CesiumJS (npm run minifyRelease npm run combineRelease, npm run buildApps, then go to http://localhost:8080/Build/Apps/Sandcastle/index.html). Run the benchmark on this branch and master.

Other than that, very robust tests like usual!

Source/Core/OrientedBoundingBox.js Outdated Show resolved Hide resolved
Source/Core/OrientedBoundingBox.js Outdated Show resolved Hide resolved
Source/Core/OrientedBoundingBox.js Outdated Show resolved Hide resolved
Source/Core/OrientedBoundingBox.js Outdated Show resolved Hide resolved
Source/Core/OrientedBoundingBox.js Outdated Show resolved Hide resolved
Source/Core/OrientedBoundingBox.js Outdated Show resolved Hide resolved
Source/Core/OrientedBoundingBox.js Outdated Show resolved Hide resolved
Source/Core/OrientedBoundingBox.js Outdated Show resolved Hide resolved
@j9liu
Copy link
Contributor Author

j9liu commented Jul 12, 2021

EDIT: truncated the numbers for easier reading.

@lilleyse - Here's the benchmark sandcastle (localhost) I used to get the following numbers:

Run # master obb-distancesquared times faster than master
1 0.0042592 0.0033573 1.2686x
2 0.0041313 0.0034534 1.1963x
3 0.0041913 0.0034055 1.2307x
4 0.004239 0.0034258 1.2373x
5 0.0042231 0.0034183 1.2354x

It seems that right now, obb-distancesquared is slightly faster than master -- maybe this is because of the use of Cartesian3.normalize (and thus the duplicate call to Cartesian3.magnitude) in the original code.

@lilleyse
Copy link
Contributor

lilleyse commented Jul 12, 2021

I'm seeing similar results on Firefox Linux: 0.013468 (master) 0.012589 (obb-distancesquared) 👍

@lilleyse
Copy link
Contributor

I confirmed this fixes the particle system crash in #9658 when TileBoundingRegion uses an obb distance check.

@kring @baothientran would you like to review as well? Assuming this makes its way back to cesium-native.

@lilleyse
Copy link
Contributor

I'm going to merge - but any more feedback is welcome after.

Thanks @j9liu!

@lilleyse lilleyse merged commit a76c1ae into master Jul 15, 2021
@lilleyse lilleyse deleted the obb-distancesquared branch July 15, 2021 15:22
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.

OrientedBoundingBox.distanceSquaredTo fails when halfAxes have magnitude near 0
3 participants