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

Additional OrientedBoundingBox functionality #12178

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Sep 1, 2024

Description

Added a few functions to the OrientedBoundingBox class:

  • fromMinMax computes an OBB from given min/max points
  • transform transforms an OBB with a matrix
  • intersect tests for intersection between two OBBs

The first ones are rather simple "convenience" functions. I used these functions, for example, in #12112 , but there, only 'drafted' them, still outside of the OrientedBoundingBox class.

The intersect function is a bit more involved. It implements the "Separating Axis Theorem", as described in https://gamma.cs.unc.edu/users/gottschalk/main.pdf . Strangely, this function already did exist, back when there was still a class called ObjectOrientedBoundingBox. This class was deprecated in #2782 and eventually removed, without a replacement for the intersect function. So I'm not even 100% sure whether it should be added again 😬

Issue number and link

No specific issue, but ...

Testing plan

In addition to the Specs/UnitTests, the following is a sandcastle that can be used for testing the intersect function this when running this branch on localhost:

http://localhost:8080/Apps/Sandcastle/#c=3Vhtb9s2EP4rhL9ExlzZWpqt0FxvdZJlBtq4SJMtMgwUlETbQijJkKjEbpH/viMpipL8kqSxjWFFapA88l6eO94d5cVRytB9QB5Igt6jiDygU5IGWWj+LdaMccMT89M4YjiISDJutND3cYTQlMYusdEE05S0xtFj87dxNI4mWeSxII6QlxDMyOckCAMW3BMjdt0W8mIaJ0153hOipyQOCUuWVeH9eDHMGAV5FzndEGcQCoMoCLPQLu8+xQmDEY6OjTeW2Wkh/dts5cfwYtsxsV/9yDPCHqVlGPuEfsIsCRagaH5ezt+akyQOr2KGudnXCY5SKoaGlAx2mzNMJx8WJG3pJY9EDMCEeVlOAD848kgVDYXBIKcqLBR2djFS1mp17fIkJ2MGEzcD422Us0LSNXaBDZ/V5X5Q54TJYoshPZozfiwjlxCWJVHZDh0MNQOUANBHIaBUnc8JTvhCxXWfSaLOCDU+FNuMwiAmXJFxoHWUStIEPGTDhkwtPZZ8/sjDWLrj7Lx/c/H19uvwanAxuASn/HL867vjTmddpJ8RN5sOXffJiJeer3q4FIr1EDEX6KeaIithZC5Xl77VY8vPFaxKHiYB3+/34yzyg2gKN8+QHFqV0F3HqTAVWNavu9qjECiHxMp5hXoBKVxyFmD6J11exxzH/xmAbr73y3xGEkBPyi5yX5korto6GUobyVdmcDMFycT0cEgSbE44elVuRlVyS11/P0tEzrJRp3oP1gU5t6YS5f8d71RUAx0qsMwVITWx7+ealLQUxn3S2XIlXdUS64vKQK5+p14LROqV4aIXSkrl3I2mojaLfZREUzazkdXpQOVSqw+BLxeLtcemxqp8AytgbXA4BJgwIA2+kduWGjnFaCRGAArDt3ro6KHcgKMpheCZ3lZmTmU2GkcvCaRcphKYSytHhEK6XrGPTY/GETFqi4Oz88vrwbUjWdSIYUZZMKfL3Js1J9Y2lyPhtiRmZrL4CvugfmoUiDSVQxVP7axd6eA8oYNzAB1GT+gw2qMOXzxMibE+jGRg51EtQ3qLJjKw4ufUAHmgXAHgjBoWtCLI5aB8R4GsLuU4arff7/Af54c+QTuPAAa4aZDVbCQWT8WtR0c8VR3B1fTF0DoyBfWfvwgcETULw39vBt4DuwKxiG4GLZTN/dL5h4DNxEFO5njdY5qBp9Q2vqyzsuIjk3ZLHOQawI6IIYxCkqZ4StDDjHAdxN5hv8/PAXQp8Zju3CScnY3ZAzp9/sedVE0WndVsoUOIe2fDA4KnW/Sz+D3uqDfESh4/5nlcC+UoPbebAMaFjvKpVbbUerml1oEttZ5rqaUttaSllAgORbHiDXh9zcrXcK2qKQ14VIniDmp8L0oaL5S6rNlgly5tNthXKm/QG5UqnI1O3paqnCKKdAZb357oqVOd5nsfhW45GHdR7N3FGTPh0eLdGYWuZUezOKYu5rnCj70sBKjMKWHnlPBhfzmAjqaR7xk3+ME6b3hL0WU/EGinWkZLca494OUdNSo1WWa+amuA0GUWuiTRHE0Brcqia6nOVupoI1W6YjvZ2U7ezFw6bzvZ2U4elXqtSrsHsJVrgd5QVABdDoQfik8C+pquuTJmkft4I94RTw5L16qYEpPGU4iMgdqHxg3ouINmLmRTh5yQMJbdvb51hVl6qYiG1edv0bKrPjRAv1c+L5hX52eo+sXB7H+8OdcIbmvfVzR7oTXWqjXWFms4rq2qrhdX5+eXL1PUeqailSQmReDa02bbg6xTvN/qOQAyxtBNSXKPXcjypRwwbohLC5nDTDM39ZLAJYZMAetyyRN8nB3xGe2Aj0wZu2Lk7IrRLkyTCWtXjJxdMdpo2jhSRQUm9W88ImgbrUY3ZUtKejzm/wjCeZwwlCXUMM02I+EcHtUkbbuZd0eY6aXis0q3rY50/eAeBf77NV+tkUdxmgJlklH6BWJr3Oh127C/cozGmGfW4T1JKF7yLTOr91EumqbZbcN09VRRdoXWXcZx6cmk12Vu7C976nXeZUlPv/O7zO+Ja9dtw6i6rmcwD6J5Bh3Ack5AWsK7bjAoDCKYQQvGx3gBY/ElAGYpI3M+hSHAjd+4UPRhLrpvO3/Ly178RvjDRkdCwlFuwXq5jCwYZw7HYXayhXmFTcU2mCRPgOEcHAxnn2A4rwJjdHAwRvsEY/TDYMgsflA01Bem/cChytJr8HAOj4ezVzyc1+ExOjweo73i8eP3RbYGu8Lj3dNoSIH7QkN1Oq9Bwzk0Gs5e0XBeh8bo0GiM9orG824KDIt2DMZ5n6Yaun8B

It uses one fixed bounding box, and allows editing the size, position, and orientation of the other one, showing the other one in red when they intersect (and printing infos to the console)

Cesium OBB Sandcastle

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

github-actions bot commented Sep 1, 2024

Thank you for the pull request, @javagl!

✅ We can confirm we have a CLA on file for you.

@ggetz
Copy link
Contributor

ggetz commented Sep 10, 2024

@lukemckinstry Could you please give this a first review pass?

@lukemckinstry
Copy link
Contributor

@javagl can you help connect the dots for me on how these functions are helping solve the core issue? #12108

@ggetz
Copy link
Contributor

ggetz commented Sep 19, 2024

@javagl To clarify what we spoke about offline, you mentioned that the intersect functionality is not strictly needed in support of #12112, is that correct?

If so, what do you think of removing that for the time being in the interest of getting this merged?

@javagl
Copy link
Contributor Author

javagl commented Sep 20, 2024

@lukemckinstry Sorry, I seem to have overlooked that. question/notification.

The intersect function is not required to solve any of the linked issues. I thought that doing intersection checks would be one of the more fundamental things that one might do with bounding volumes. But in CesiumJS, this was apparently "emulated"/replaced with intersectPlane.

The other functions, fromMinMax and transform are useful for fixing the linked issues, because...

  • bounding volumes have to be created from the min/max properties of the POSITION accessor of the glTF
  • they have to be transformed with the transform of the node that the respective mesh is attached to (which is currently not done at all, and at the core, the main reason for the issue)

Both of these functions could also be implemented locally, without adding them to the public API of OrientedBoundingBox. (In fact, they are implemented like that in the current state of the corresponding PR).

So, the bottom line: I think that the functions could be useful. But they are not "required" for anything. So if this is preferred, then this PR could be closed as it is.

@lukemckinstry
Copy link
Contributor

@javagl What you are saying about implementing fromMinMax and transform on the OrientedBoundingBox class as done here makes sense. We agree these functions are better implemented on this class than locally to the glTF specific code.

We think it is best to remove intersect from this PR (since as you point out the fuctionality is already implemented elsewhere) and then move forward with just fromMinMax and transform.

Let us know when this is ready for review again. Thanks!

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