Support for different types of bounding volumes for 3D tiles #3325

e-andersson · 2015-12-10T10:10:12Z

Bounding volume is now specified as boundingVolume.box or boundingVolume.sphere where it was previously only box. boundingVolume.box is the same as the old box for now.

PR for issue Different bounding volumes.

Somewhat rudimentary tests are included, feedback welcome.

…ox (same as before) and sphere (new)

e-andersson · 2015-12-10T11:34:52Z

Source/Scene/Cesium3DTile.js

-        this._debugBox = undefined;
-        this._debugcontentBox = undefined;
-        this._debugOrientedBoundingBox = undefined;
-        this._debugContentsOrientedBoundingBox = undefined;


I kept only one debug volume for the tile and one debug volume for the tile contents. Not sure whether there is any need to keep them separated.

This seems like the right approach to me. We will have to modify Cities.html to remove some of the extra buttons, but that can be a later commit.

pjcozzi · 2015-12-10T12:47:36Z

@lilleyse can you do the initial review?

@e-andersson we'll want to use box for an AABB. What is a good name for the current box? region?

e-andersson · 2015-12-10T13:19:56Z

@pjcozzi Could we call it something along the lines of curved_box or perhaps WGS84_box (unless other coordinates may be used)? A little bit more verbose than region but possible clearer?

pjcozzi · 2015-12-10T13:33:59Z

A lot of KML use cases will be served by served by 3D Tiles. In particular, KML regions, which call this LatLonAltBox. So "region" will be familiar to many users (I would not use LatLonAltBox, by Cesium conventions, it would be longitudeLatitudeHeightBox).

By the way, we use camel case for property names so curved_box would be curvedBox. There is an (incomplete) Spec Writing Guide here: CesiumGS/3d-tiles#26

@lilleyse is there a more precise graphics term for this that we are over looking?

lilleyse · 2015-12-10T14:04:39Z

I'm not sure what the proper term is. Maybe tileBox if that isn't too general.

lilleyse · 2015-12-10T14:11:57Z

Source/Core/BoundingSphere.js

+    BoundingSphere.distanceToCamera = function(sphere, frameState) {
+        return Math.max(0.0, Cartesian3.distance(sphere.center, frameState.camera.positionWC) - sphere.radius);
+    };
+


Add a comment above this function, but add @private at the end for now.

Throw DeveloperError if sphere or frameState is not defined, similar to what the other functions do.

e-andersson · 2015-12-10T14:30:16Z

@lilleyse box and tileBox may be a bit too similar, it's probably easier to see the difference between box and region (at least if one is familiar with regions from KML).

lilleyse · 2015-12-10T14:51:00Z

Source/Scene/Cesium3DTile.js

+            0, 1, 0, 0,
+            0, 0, 1, 0,
+            0, 0, 0, 1
+        );


Cleaner to do var modelMatrix = Matrix4.clone(Matrix4.IDENTITY);

Thanks -- I stumbled on the immutability of Matrix4.IDENTITY, didn't figure out I should clone it :)

lilleyse · 2015-12-10T16:18:58Z

Those are all my comments. I did a quick test and it works fine. When everything here is final, I'll update the test data sets to reflect the new tiles.json changes, as well as add some sphere and AABB tests (once we include AABB support).

When we pick a name for region, we may want to change the name of TileBoundingBox as well.

e-andersson · 2015-12-10T16:24:09Z

Thanks @lilleyse. I'll update the code in line with the comments tomorrow morning.

lilleyse · 2015-12-10T16:28:14Z

I added a boundingVolume getter to replace orientedBoundingBox. The getter will have to change once we add new box types.

e-andersson · 2015-12-21T14:57:17Z

@lilleyse Would you mind having a look at why the Instanced3DTileContentProvider and Composite3DTileContentProvider resolve tests don't pass (timeout)? The sample data looks fine and there isn't any problem instantiating the bounding volumes, so I'm not sure where the problem is. Do you have an idea?

lilleyse · 2015-12-21T16:58:51Z

It looks like the tests are failing because ModelInstanceCollection expects the bounding volume to have a center property in order to do relative to center rendering. So a quick fix would be to to add a center getter in TileBoundingRegion, or the boundingVolume getter could return this._orientedBoundingBox instead.

e-andersson · 2015-12-22T10:46:37Z

@lilleyse Thanks, that indeed seems to be it.

…bounding box as the bounding volume

e-andersson · 2015-12-22T11:50:40Z

I added a temporary solution for creating an oriented bounding box from points representing the corners when creating a TileBoundingRegion. We'll probably not want when there is a OrientedBoundingBox.fromCorners method, but until then it's nice to have a real box.

e-andersson · 2015-12-22T11:51:36Z

@pjcozzi I think this is ready. EDIT: Updating a few tests... done!

pjcozzi · 2015-12-22T18:32:05Z

@e-andersson can you merge the 3d-tiles branch into this? Looks like there are some minor conflicts from @lilleyse's test branch that we merged today.

pjcozzi · 2015-12-22T18:37:06Z

Source/Scene/TileBoundingRegion.js

+         *
+         * @private
+         */
+        this._orientedBoundingBox = OrientedBoundingBox.fromPoints(getBoxPoints(this.rectangle, this.minimumHeight, this.maximumHeight));


I believe we can use OrientedBoundingBox.fromRectangle here. For example, see https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Workers/createVerticesFromQuantizedTerrainMesh.js#L121

pjcozzi · 2015-12-22T18:39:15Z

@e-andersson can you merge the 3d-tiles branch into this? Looks like there are some minor conflicts from @lilleyse's test branch that we merged today.

Also, when you merge in master, please change the new tiles.json files for the tests in Specs/Data/Cesium3DTiles so they follow the latest schema for bounding volumes

pjcozzi · 2015-12-22T18:42:21Z

Just those comments. Once the 3d-tiles branch is merged in, I'll test this locally, and then we should be good to merge.

e-andersson · 2016-01-13T14:09:21Z

Merged in 3d-tiles into this branch. Note that I seem to have introduced two test failures, possibly due to the _orientedBoundingBox in TileBoundingRegion being incorrectly created using OrientedBoundingBox.fromPoints. It seems this is also manifested as error-prone culling/tile selection in the viewer.
@pjcozzi and @lilleyse, does the above seem correct? If so, it will be fixed by introduction of a way to create oriented bounding boxes as discussed in #3325 (comment), right?

lilleyse · 2016-01-13T15:33:10Z

Maybe you can try OrientedBoundingBox.fromRectangle from this comment #3325 (comment)

e-andersson · 2016-01-14T08:32:03Z

Thanks for the pointer @lilleyse, I seem to have missed that comment. Works fine now.

This is ready now @pjcozzi.

EDIT: Currently there are no destroy functions for the the bounding volumes of a Cesium3DTile -- there doesn't seem to be much we could destroy, no GL resources held by the bounding volumes which if I've understood it correctly would be a reason to explicitly destroy objects. (I put a TODO note in Cesium3DTile about this.)

pjcozzi · 2016-01-14T15:31:41Z

Source/Scene/Cesium3DTile.js

-        this._debugcontentBox = this._debugcontentBox && this._debugcontentBox.destroy();
-        this._debugOrientedBoundingBox = this._debugOrientedBoundingBox && this._debugOrientedBoundingBox.destroy();
-        this._debugContentsOrientedBoundingBox = this._debugContentsOrientedBoundingBox && this._debugContentsOrientedBoundingBox.destroy();
+        // TODO: Is there a reason to destroy the bounding volumes (and thus to implement destroy() functions for them)? The destroyObject function description says it should be used to for objects that have GL resources. 


This is only needed for the debug volumes since they contain Cesium primitives, which contain GL resources.

Ah, got it. (Sean has fixed it.)

pjcozzi · 2016-01-14T15:39:12Z

Thanks again @e-andersson! I'm going to merge this since we have some changes we want to make that depend on this. Can you please open a separate PR into the 3d-tiles branch to address these two minor comments: #3325 (diff) and #3325 (diff)

Support for different types of bounding volumes for 3D tiles

pjcozzi · 2016-01-14T15:40:08Z

@tfili and anyone else using 3D Tiles - note that next time you update to the latest 3d-tiles branch that this is a small breaking change.

added support for different types of bounding volumes for 3D tiles, b…

e8b5244

…ox (same as before) and sphere (new)

e-andersson reviewed Dec 10, 2015
View reviewed changes

lilleyse reviewed Dec 10, 2015
View reviewed changes

Create boundingVolume getter

5ce15ee

e-andersson added 4 commits December 21, 2015 12:29

added distance and plane intersection tests to TileOrientedBoundingBox

06389ce

added simple camera distance and plane intersection tests

2007190

added tests for TileBoundingSphere

b6ed7f4

merged in changes from 3d-tiles

2d2b857

e-andersson added 2 commits December 22, 2015 11:56

fixed issue with missing properties needed. Now returns the oriented …

d7f107e

…bounding box as the bounding volume

temporary solution to create oriented bounding box from points

884cefb

updated tests

c56a909

pjcozzi reviewed Dec 22, 2015
View reviewed changes

lilleyse mentioned this pull request Jan 6, 2016

Standardize object declarations for 3d-tiles #3389

Merged

merged in 3d-tiles

989449a

now creating OBB from rectangle

a19cd70

pjcozzi reviewed Jan 14, 2016
View reviewed changes

pjcozzi added a commit that referenced this pull request Jan 14, 2016

Merge pull request #3325 from e-andersson/3d-tiles_bounding_volumes

65ca279

Support for different types of bounding volumes for 3D tiles

pjcozzi merged commit 65ca279 into CesiumGS:3d-tiles Jan 14, 2016

pjcozzi deleted the 3d-tiles_bounding_volumes branch January 14, 2016 15:39

lilleyse mentioned this pull request Jan 14, 2016

3D Tiles Coverage and Cleanup #3427

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for different types of bounding volumes for 3D tiles #3325

Support for different types of bounding volumes for 3D tiles #3325

e-andersson commented Dec 10, 2015

e-andersson Dec 10, 2015

lilleyse Dec 10, 2015

pjcozzi commented Dec 10, 2015

e-andersson commented Dec 10, 2015

pjcozzi commented Dec 10, 2015

lilleyse commented Dec 10, 2015

lilleyse Dec 10, 2015

lilleyse Dec 10, 2015

e-andersson commented Dec 10, 2015

lilleyse Dec 10, 2015

e-andersson Dec 10, 2015

lilleyse commented Dec 10, 2015

e-andersson commented Dec 10, 2015

lilleyse commented Dec 10, 2015

e-andersson commented Dec 21, 2015

lilleyse commented Dec 21, 2015

e-andersson commented Dec 22, 2015

e-andersson commented Dec 22, 2015

e-andersson commented Dec 22, 2015

pjcozzi commented Dec 22, 2015

pjcozzi Dec 22, 2015

pjcozzi commented Dec 22, 2015

pjcozzi commented Dec 22, 2015

e-andersson commented Jan 13, 2016

lilleyse commented Jan 13, 2016

e-andersson commented Jan 14, 2016

pjcozzi Jan 14, 2016

e-andersson Jan 15, 2016

pjcozzi commented Jan 14, 2016

pjcozzi commented Jan 14, 2016

Support for different types of bounding volumes for 3D tiles #3325

Support for different types of bounding volumes for 3D tiles #3325

Conversation

e-andersson commented Dec 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Dec 10, 2015

e-andersson commented Dec 10, 2015

pjcozzi commented Dec 10, 2015

lilleyse commented Dec 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e-andersson commented Dec 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Dec 10, 2015

e-andersson commented Dec 10, 2015

lilleyse commented Dec 10, 2015

e-andersson commented Dec 21, 2015

lilleyse commented Dec 21, 2015

e-andersson commented Dec 22, 2015

e-andersson commented Dec 22, 2015

e-andersson commented Dec 22, 2015

pjcozzi commented Dec 22, 2015

Choose a reason for hiding this comment

pjcozzi commented Dec 22, 2015

pjcozzi commented Dec 22, 2015

e-andersson commented Jan 13, 2016

lilleyse commented Jan 13, 2016

e-andersson commented Jan 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Jan 14, 2016

pjcozzi commented Jan 14, 2016