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

AxisAlignedBox and OrientedBoundingBox improvements #608

Merged
merged 7 commits into from
Mar 20, 2023

Conversation

kring
Copy link
Member

@kring kring commented Mar 16, 2023

From the changelog:

Additions 🎉
  • Added getOrientedBoundingBoxFromBoundingVolume to the Cesium3DTilesSelection namespace.
  • Added transform and toAxisAligned methods to OrientedBoundingBox.
Fixes 🔧
  • Fixed a bug that caused the center field of AxisAlignedBox to be incorrect.

Copy link
Contributor

@j9liu j9liu left a comment

Choose a reason for hiding this comment

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

@kring this looks good to me, just two small suggestions that I can push a commit for.

@@ -78,4 +78,13 @@ estimateGlobeRectangle(const BoundingVolume& boundingVolume);
CESIUM3DTILESSELECTION_API const CesiumGeospatial::BoundingRegion*
getBoundingRegionFromBoundingVolume(const BoundingVolume& boundingVolume);

/**
* @brief Get an oriented bounding box that contains this bounding volume.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @brief Get an oriented bounding box that contains this bounding volume.
* @brief Returns an oriented bounding box that contains the given {@link BoundingVolume}.

Nitpick, just for consistency with the other documentation in this file.

@@ -49,8 +50,9 @@ class CESIUMGEOMETRY_API OrientedBoundingBox final {
}

/**
* @brief Gets the transformation matrix, to rotate and scale the box to the
* right position and size.
* @brief Gets he three orthogonal half-axes of the bounding box.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @brief Gets he three orthogonal half-axes of the bounding box.
* @brief Gets the three orthogonal half-axes of the bounding box.

@j9liu
Copy link
Contributor

j9liu commented Mar 20, 2023

merging now that CI has passed, thanks @kring !

@j9liu j9liu merged commit 1d7bfd0 into main Mar 20, 2023
@j9liu j9liu deleted the bounding-volume-operations branch March 20, 2023 20:31
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.

2 participants