Calculate oriented bounding box from bounding region for 3D tiles of Cesium's OSM buildings #40

sraimund · 2025-01-04T12:52:01Z

Here the ported code from Cesium to fix the calculation of the bounding box from a region on the WGS84 ellipsoid (see issue visgl/loaders.gl#3144). The function signature fromRegion() in oriented-bounding-box.ts (math.gl/culling) is similar to the previous createObbFromRegion function in bounding-volume.ts (loaders.gl/tiles). The other functions are nearly a 1:1 mapping from Cesium, except that these are now class methods.

Test:
https://stackblitz.com/edit/stackblitz-starters-pode72re (takes ~2mins to build and load)

Notes:

I could only build the culling module when adding // @ts-ignore // eslint-disable-next-line import/no-unresolved above import {Ellipsoid} from '@math.gl/geospatial'; in oriented-bounding-box.ts and import {Ellipsoid} from '@math.gl/geospatial'; in ellipsoid-tangent-plane.ts.
I'm not sure whether the 'lint' command worked correctly since it only showed some warnings for the dev-docs.

- mod - zeroToTwoPi - negativePiToPi

- PI

- multiplyByVector - multiplyByScale

- fromRegion - fromPlaneExtents

- intersectWithRay (originally in IntersectionTests)

ibgreen

@sraimund Thanks for doing this relatively extensive work, that was definitely a good amount of code!

Apart from some smaller nits (optional suggestion) in my comments, the main missing pieces are docs and tests, at least for the publicly exported functions.

I understand if you have limits on the amount of time you can put on this PR, just let me know what you can do, and then we will get it landed and published.

ibgreen · 2025-01-04T17:28:11Z

modules/core/src/classes/matrix3.ts

+   * @param {Vector3} result The object onto which to store the result.
+   * @returns {Vector3} The modified result parameter.
+   */
+  multiplyByVector(cartesian: Vector3, result?: Vector3): Vector3 {


This is the same as m3.transform(v3) below? The name comes from viewing the matrix as a transform on vectors.

It's indeed the same. I removed the method 2cb1e6b

ibgreen · 2025-01-04T17:30:00Z

modules/core/src/classes/matrix3.ts

+   * @param {Matrix3} result The object onto which to store the result.
+   * @returns {Matrix3} The modified result parameter.
+   */
+  multiplyByScale(scale: Vector3, result?: Matrix3): Matrix3 {


This method is inherited from Matrix3 < Matrix < MathArray. Called m3.scale(v3)

I now reused the parent scale method, see 801904f The overridden scale method in Matrix3 multiplies it with a Vector2. There I'm not sure if adding another check whether the array length is 2 or 3 impacts the performance in case the method is called many times.

ibgreen · 2025-01-04T17:31:21Z

modules/core/src/lib/common.ts

+/**
+ * The modulo operation that also works for negative dividends.
+ *
+ * @param {number} m The dividend.


Nit: Avoid adding types in the TSDoc, they are inferred from the typescript types.

Done. See 2222cc5

ibgreen · 2025-01-04T17:34:00Z

modules/culling/src/lib/bounding-volumes/oriented-bounding-box.ts

+   * @returns {OrientedBoundingBox} The modified result parameter or a new OrientedBoundingBox instance if none was provided.
+   */
+  // eslint-disable-next-line max-statements
+  fromRegion(region: number[]): OrientedBoundingBox {


Nit: From tree shaking (bundle size) perspective, it would be preferable to offer this as a global function rather than a method on the OBB class. That way we could also avoid this file from growing too big.

Done. See 5c2bd42. I renamed it to makeOrientedBoundingBoxFromRegion() similarly to the other function names in the algorithms folder.

ibgreen · 2025-01-04T17:34:12Z

modules/culling/src/lib/bounding-volumes/oriented-bounding-box.ts

+   * Computes an OrientedBoundingBox that bounds a region on the surface of the WGS84 ellipsoid.
+   * There are no guarantees about the orientation of the bounding box.
+   *
+   * @param {number[]} region The cartographic region ([west, south, east, north, minimum height, maximum height])


Nit: Remove type annotations here.

Done. See 2222cc5

ibgreen · 2025-01-04T17:35:02Z

modules/core/src/index.ts

@@ -58,6 +58,9 @@ export {
  // math.gl "GLSL"-style functions
  radians,
  degrees,
+  mod,


At least a trivial, minimal test case for every global export is normally expected.

Done. See 0f89190

In bounding-box-from-region.spec.ts, however, I added only two tests since there are too many to port from Cesium. Moreover, these tests are based on a unit sphere and math.gl supports only a WGS84 ellipsoid currently.

ibgreen · 2025-01-04T17:36:47Z

modules/culling/src/lib/ellipsoid-tangent-plane.ts

+const scratchDirection = new Vector3();
+
+/** A plane tangent to the WGS84 ellipsoid at the provided origin */
+export class EllipsoidTangentPlane {


Are there any tests for this class that could be ported? (even if most are commented out, still nice to have them)

Done. See 0f89190

ibgreen · 2025-01-04T17:38:02Z

modules/culling/src/lib/plane.ts

+   * @param {Vector3} [result] The object onto which to store the result.
+   * @returns {Vector3} The intersection point or undefined if there is no intersections.
+   */
+  intersectWithRay(ray: Ray, result?: Vector3): Vector3 {


Nit: Might have preferred as a global function to avoid pulling in Ray for every app.

Done. See 5c2bd42

ibgreen · 2025-01-04T17:39:26Z

modules/geospatial/src/rectangle.ts

+import {Vector3, negativePiToPi, _MathUtils} from '@math.gl/core'
+
+/** A two dimensional region specified as longitude and latitude coordinates. */
+export class Rectangle {


Nit: Maybe this should be called LngLatRectangle given its special handling of geospatial coordinates (it is certainly not a generic Rectangle class).

I renamed it to GeoRectangle ae5cef2 (similarly to https://doc.qt.io/qt-6/qgeorectangle.html). Concerning your proposition, I would be a bit confused if it is called LngLatRectangle or LatLngRectangle.

…neWithRay() to prevent large files and improve tree shaking

sraimund · 2025-01-06T18:00:29Z

I've also updated https://stackblitz.com/edit/stackblitz-starters-pode72re with a new build from the recent commits.

Do the docs need to be manually added or can they be somehow automatically transferred from TSdoc?

sraimund added 11 commits December 26, 2024 19:18

ported methods:

1e0b40a

- mod - zeroToTwoPi - negativePiToPi

partly ported class 'EllipsoidTangentPlane' from Cesium

37ea8d8

ported constant from Cesium:

798782e

- PI

ported methods from Cesium:

ad9d391

- multiplyByVector - multiplyByScale

ported methods from Cesium:

2e836a7

- fromRegion - fromPlaneExtents

partly ported class 'Rectangle' from Cesium

fc7bbcc

partly ported class 'Ray' from Cesium

dc5e8dc

ported method from Cesium:

fc2d667

- intersectWithRay (originally in IntersectionTests)

export ported classes

361f7c5

added geospatial module for Rectangle calculations

a4c6bfb

fix: reuse OBBs properties

bd15973

sraimund mentioned this pull request Jan 4, 2025

Calculate oriented bounding box from bounding region for 3D tiles of Cesium's OSM buildings visgl/loaders.gl#3176

Open

ibgreen approved these changes Jan 4, 2025

View reviewed changes

Merge branch 'master' into master

1f6013c

This was referenced Jan 4, 2025

feat(geospatial): Add makeOBBFromRegion() #43

Merged

chore(geospatial) makeOBBFromRegion docs #44

Open

sraimund added 8 commits January 6, 2025 11:44

remove types from TSdoc

2222cc5

reuse parent method

801904f

global functions makeOrientedBoundingBoxFromRegion() and intersectPla…

5c2bd42

…neWithRay() to prevent large files and improve tree shaking

rename Rectangle to GeoRectangle

ae5cef2

tests ported

0f89190

minimal tests added

d283930

Merge remote-tracking branch 'origin/master'

c8f8f7a

replace multiplyByVector() by transform()

2cb1e6b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calculate oriented bounding box from bounding region for 3D tiles of Cesium's OSM buildings #40

Calculate oriented bounding box from bounding region for 3D tiles of Cesium's OSM buildings #40

sraimund commented Jan 4, 2025

ibgreen left a comment

ibgreen Jan 4, 2025

sraimund Jan 6, 2025

ibgreen Jan 4, 2025

sraimund Jan 6, 2025

ibgreen Jan 4, 2025

sraimund Jan 6, 2025

ibgreen Jan 4, 2025

sraimund Jan 6, 2025

ibgreen Jan 4, 2025

sraimund Jan 6, 2025

ibgreen Jan 4, 2025

sraimund Jan 6, 2025

sraimund Jan 6, 2025

ibgreen Jan 4, 2025

sraimund Jan 6, 2025

ibgreen Jan 4, 2025

sraimund Jan 6, 2025

ibgreen Jan 4, 2025

sraimund Jan 6, 2025

sraimund commented Jan 6, 2025

Calculate oriented bounding box from bounding region for 3D tiles of Cesium's OSM buildings #40

Are you sure you want to change the base?

Calculate oriented bounding box from bounding region for 3D tiles of Cesium's OSM buildings #40

Conversation

sraimund commented Jan 4, 2025

ibgreen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sraimund commented Jan 6, 2025