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: use bounding sphere when bounding box not available #340

Merged

Conversation

jesse-small
Copy link
Contributor

@jesse-small jesse-small commented Jun 1, 2023

  • getBounds now returns a bounding box from the bounding sphere when a cached bounding box is not available
  • calculateError now correctly applies the inverse transform matrix to the bounding sphere before calculating distance

// Sphere#distanceToPoint is negative inside the sphere, whereas Box3#distanceToPoint is
// zero inside the box. Clipping the distance to a minimum of zero ensures that both
// types of bounding volume behave the same way.
distance = Math.max( boundingSphere.distanceToPoint( tempVector ), 0 );
distance = Math.max( tempSphere.distanceToPoint( tempVector ), 0 );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we save a transformed bounding sphere this was causing the calculate error to always be wrong for bounding spheres when a transform matrix was present. This applies the inverseTransform to the bounding sphere before calculating the distance.

Copy link
Contributor

@gkjohnson gkjohnson left a comment

Choose a reason for hiding this comment

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

Awesome thank you! I've left a few comments.

src/three/TilesRenderer.js Outdated Show resolved Hide resolved
src/three/raycastTraverse.js Outdated Show resolved Hide resolved
src/three/TilesRenderer.js Show resolved Hide resolved
src/three/raycastTraverse.js Outdated Show resolved Hide resolved
@jesse-small
Copy link
Contributor Author

I added the raycasting logic along with an explanation to this PR #347

@gkjohnson
Copy link
Contributor

Great thank you!

@gkjohnson gkjohnson merged commit 67bf29c into NASA-AMMOS:master Jun 6, 2023
3 checks passed
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.

None yet

2 participants