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

3D Tiles - Support tile transform #4130

Closed
wants to merge 13 commits into from
Closed

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Jul 18, 2016

For CesiumGS/3d-tiles#98

Each tile now has a transformLocal and transformGlobal property. transformLocal is equal to the transform property that a tile can have in tileset.json, but the user can also change it on the fly. transformGlobal is the computed transform when taking into account ancestor transforms. When the value changes, the bounding volumes and content are updated.

Questions:

  1. The transform doesn't change the region bounding volume, but should it still change the contents?
  2. If a tile pointing to an external tileset has a transform property, does the root node of the external tileset also need a matching transform property? Right now the code will apply the transform twice if both are present.

To do:

  • Upgrade Points3DTileContent to render more accurately with the tile transform. Right now it renders at the same location as the transform, but not rotated or scaled with the transform. Short term - add 3x3 transform uniform to the shader. Long term - build shader from scratch. Letting Primitive construct high/low positions buffers could be a bottleneck anyways and is probably more precision than we need considering we are already rendering relative to center. In either case, the point data is uploaded as-is and transformed in the shader.
  • Test Instanced3DModel3DTileContent more once I3dm updates #4101 is in. This will give more room for trying OOB/BoundingSphere setups.
  • Write tests

captu3r2e
The wooden tower and the points are defined relative to (0,0,0) but are transformed to their position with the transform property.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 18, 2016

Each tile now has a transformLocal and transformGlobal property

In the context of just Cesium, this is perfect. In the broader context of 3D Tiles, are these better named transform and globalTransform (or transform and computedTransform)?

* The local transform of this tile
* @type {Matrix4}
*/
this.transformLocal = defined(header.transform) ? Matrix4.unpack(header.transform, 0) : Matrix4.clone(Matrix4.IDENTITY);
Copy link
Contributor

Choose a reason for hiding this comment

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

0 is the default. fromArray (which is the same as unpack) may also be more clear in this context.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 18, 2016

Part of #3241.

Matrix4.clone(tile.transformGlobal, tile._transformGlobal);

// Update the bounding volumes
tile._boundingVolume = createBoundingVolume(tile._header.boundingVolume, tile.transformGlobal, tile._boundingVolume);
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be cleaner with

var header = tile._header;
var content = header.content;

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 18, 2016

If a tile pointing to an external tileset has a transform property, does the root node of the external tileset also need a matching transform property? Right now the code will apply the transform twice if both are present.

I think they need to match, like the other properties:

Is semantically the same as the external tileset's root tile so

  • root.geometricError === tile.geometricError,
  • root.refine === tile.refine, and
  • root.boundingVolume === tile.content.boundingVolume (or root.boundingVolume === tile.boundingVolume when tile.content.boundingVolume is undefined).

See https:/AnalyticalGraphicsInc/3d-tiles#external-tilesets

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 18, 2016

The transform doesn't change the region bounding volume, but should it still change the contents?

I guess. I'm tempted to say that the region bounding volume and a non-identity transform are mutually exclusive.

However, I think that is too limiting; the may be transform up the tree, for example, that are convenient for the tileset generator. For some use cases, the region may also be defined conservatively so the content could move around and still be spatially coherence.

this._boundingVolume = defined(options.boundingVolume) ? options.boundingVolume : createBoundingSphere(this);
this._boundingVolumeExpand = !defined(options.boundingVolume); // Expand the bounding volume by the radius of the loaded model

this._center = defined(options.center) ? options.center : this._boundingVolume.center;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use defaultValue?

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 18, 2016

@lilleyse As I mentioned in CesiumGS/3d-tiles#98 (comment), I think it is useful for the 3D Tileset primitive to have a modelMatrix to allow apps to do registration, e.g., a user manually drags around a point cloud to match it up with imagery, but I am not sure that each tile needs the ability for its bounding volume to change...so if this is hard to test/debug/finish, feel free to revert to the simplest solution.

@lilleyse
Copy link
Contributor Author

@lilleyse As I mentioned in CesiumGS/3d-tiles#98 (comment), I think it is useful for the 3D Tileset primitive to have a modelMatrix to allow apps to do registration, e.g., a user manually drags around a point cloud to match it up with imagery, but I am not sure that each tile needs the ability for its bounding volume to change...so if this is hard to test/debug/finish, feel free to revert to the simplest solution.

It should be easy to add a modelMatrix property to the tileset that changes the root's transform. Right now tile bounding volumes automatically update when the computed transform changes, so it's easiest to just leave that as it is.

@lilleyse
Copy link
Contributor Author

Closing - now included in #4183

@lilleyse lilleyse closed this Aug 30, 2016
@lilleyse lilleyse deleted the 3d-tiles-transform branch September 9, 2016 13:42
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