Various OrientedBoundingBox, AxisAlignedBoundingBox, BoundingSphere, and Rectangle functions #10130
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request @IanLilleyT!
Reviewers, don't forget to make sure that:
|
IanLilleyT
force-pushed
the
obbConversion
branch
from
af980d9
to
d509826
Compare
|
Thanks @IanLilleyT! @sanjeetsuhag Could you please review? |
IanLilleyT
force-pushed
the
obbConversion
branch
2 times, most recently
from
34bb6c5
to
41105a3
Compare
IanLilleyT
force-pushed
the
obbConversion
branch
from
41105a3
to
496ec0b
Compare
There was a problem hiding this comment.
@IanLilleyT The code here mostly looks good. The only suggested change I have is in the JSDoc.
Regarding computeTransformation, could you give a use case for this function? I think that will help us decide if it's the right name for this or not.
Regarding subsection, would inset work here? Semantically, would that be more appropriate?
sanjeetsuhag
approved these changes
Feb 24, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Merge #10124 first.
This could have been split up into 6 small PRs, but they're all kinda similar and hopefully non-controversial. These functions aren't used in CesiumJS yet, but I have a private branch that does use all of them. Plus, I think these functions will be useful for other things. They all have 100% coverage.
I put a ❓ where I wasn't sure about naming. Let me know.
AxisAlignedBoundingBox.fromCornersBasically identical to the
AxisAlignedBoundingBoxconstructor. It exists so that you can use a result parameter instead of allocating.BoundingSphere.fromTransformationPretty similar to
BoundingSphere.fromOrientedBoundingBox, but avoids the intermediate conversion to anOrientedBoundingBox.OrientedBoundingBox.fromTransformationThe box's center becomes the translation and the half axes become the top left
Matrix3(but with half the scale).OrientedBoundingBox.computeTransformationOpposite of
OrientedBoundingBox.fromTransformation. Is this a good name ❓OrientedBoundingBox.computeCornersComputes the eight corners of the
OrientedBoundingBoxand stores them in an array. Useful for debug visualizations or as an intermediate step for other algorithms.Rectangle.subsectionReturns a subsection (aka patch / subregion / etc) of a rectangle given interpolation factors for west, south, east, and north. Is this a good name ❓