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

Support for loading 3D Tiles via CZML and Entity API #8580

Merged
merged 4 commits into from
Feb 7, 2020

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Jan 30, 2020

This change enables CZML and the Entity API to load 3D Tiles tilesets. Since Cesium3DTileset has dozens of options, I only exposed uri/show/maximumScreenSpaceError to start but I'm open to adding more if we think they're needed. We also use the entity position/orientation (if specified) to compute the tileset.modelMatrix allowing for moving tilesets that can be tracked with the camera. There seems to be an issue with 3D Tiles here where a moving tileset never actually gets loaded unless you pause animation to the camera stops. @loshjawrence seems to know how to fix this. (Also CC @lilleyse)

oz

Details:

  1. New Sandcastle example, CZML 3D Tiles.html
  2. New class, Cesium3DTilesetGraphics for representing a tileset via the Entity API.
  3. New class, Cesium3DTilesetVisualizer for creating/managing the primitives.
  4. Add Entity.tileset which is an instance of Cesium3DTilesetGraphics
  5. Specs for everything

This can wait until after Monday's release to merge so we aren't stuffing in a feature at the last moment.

This change enabled CZML and the Entity API to load 3D Tiles tilesets.
Since `Cesium3DTileset` has dozens of options, I only exposed
uri/show/maximumScreenSpaceError to start.  We also use the entity
position/orientation (if specified) to compute the tileset.modelMatrix
allowing for moving tilesets that can be tracked with the camera.

Details:

1. New Sandcastle example, `CZML 3D Tiles.html`
2. New class, `Cesium3DTilesetGraphics` for representing a tileset via
the Entity API.
3. New class, `Cesium3DTilesetVisualizer` for creating/managing the
primitives.
4. Add `Entity.tileset` which is an instance of `Cesium3DTilesetGraphics`
5. Specs for everything
@cesium-concierge
Copy link

Thanks for the pull request @mramato!

  • ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@mramato
Copy link
Contributor Author

mramato commented Jan 30, 2020

Another use case for this (in addition to flying houses) is that you can have an interval show property that swaps static tilesets over time, in case you wanted to do something like show interval-based collects over time and be able to scrub back and forth between them with the timeline.

@mramato
Copy link
Contributor Author

mramato commented Feb 4, 2020

@lilleyse just a reminder to look at the 3D Tiles loading issue in this branch. Thanks!

@loshjawrence
Copy link
Contributor

For whoever has the time to look at the tile loading issue:
it will require having the tilesets track their frame position delta and subtract it from the camera delta before finding the magnitude of change this will give a relative-to-tileset magnitude of change. This camera delta stuff occurs in Cesium3DTilesetTraversal.js

@OmarShehata
Copy link
Contributor

@mramato have you tried turning cullRequestsWhileMoving to false?

https://cesium.com/docs/cesiumjs-ref-doc/Cesium3DTileset.html#cullRequestsWhileMoving

@mramato
Copy link
Contributor Author

mramato commented Feb 4, 2020

No, but if that is the case it seems like something that shouldn't default to true since that means broken by default (or it needs to be made smarter)

@loshjawrence
Copy link
Contributor

loshjawrence commented Feb 5, 2020

cullRequestsWhileMoving turns the code path I mentioned on and off, This code path needs to be aware of the tilesets movement which it is not right now.

There is also a "multipler" flag with a similar name and if you set it to 0 it will turn the code path off so the true/false flag is redundant.

@mramato
Copy link
Contributor Author

mramato commented Feb 5, 2020

To reproduce the problem:

  1. Download FlyingHouse.zip and unpack it into the Cesium directory so you have a /FlyingHouse directory at the top level.

  2. Run this Sandcastle using this branch: http://localhost:8080/Apps/Sandcastle/index.html#c=ZZBPT8MwDMW/StRLW2lKxBW6CRggJiFxqMQpl6zxtojUQYmzqUN8d9xtMP7kZvu951+8NVFsHewgiqlA2Ik5JJd7+XLoVWV3KOcByTiEWE7Eu0bBL21C9vYGXW8IxKWgmEHjR32lUeMxUVpDpg05cog01lan7Pm+93ffI+mDsVWpHvzgcP0YcgKVXP/mQXYsLOtao6QNYLXK2JELWJ1z6xPNaSFF072CvUdyNPCHzkIJY88xyBrodljwxpbBvXcEatG25Qh+oC8mRZNo8DA7Rl8zS4gkcvSVlIqA0diZ1DLzLpJdSqOtUV+mxrqtcHaqiz/H04XovEmJJ6vsfev2oItZo1j/yzYehE/xvIXozTBKNhezp2NTStkoLv+7KAS/NPFHosZP

You'll see a satellite marker flying like the typical simple.czml, but it's not until you hit pause that the house comes in.

@mramato
Copy link
Contributor Author

mramato commented Feb 5, 2020

And just to confirm (though @loshjawrence already mentioned it) setting cullRequestsWhileMoving = true doesn't fix this problem.

@lilleyse
Copy link
Contributor

lilleyse commented Feb 6, 2020

@mramato actually it should be cullRequestsWhileMoving = false. That fixed it for me, for debugging purposes at least.

I'm going to go with a simple solution where it treats cullRequestsWhileMoving as false if the tileset is determined to be moving.

@loshjawrence
Copy link
Contributor

loshjawrence commented Feb 6, 2020

@lilleyse while your at it we should lower the cullRequestWhileMovingMultiplier because its too aggressive for static tilesets (0.1 or something)

@lilleyse
Copy link
Contributor

lilleyse commented Feb 6, 2020

@loshjawrence I kept it out of #8598 but you can open a separate PR. 60 to 0.1 is a huge difference though...

@OmarShehata
Copy link
Contributor

Adding the 3D Tileset to the Entity API is kind of big news! It solves this 3D Tiles tracking/camera flying issue for example: #7086 since you can now add a 3D Tileset like this:

var viewer = new Cesium.Viewer('cesiumContainer');
var tilesetEntity = viewer.entities.add({
    tileset: {
        uri: "../../SampleData/Cesium3DTiles/Batched/BatchedColors/tileset.json"
    }
});
viewer.flyTo(tilesetEntity);

Or from ion:

var viewer = new Cesium.Viewer('cesiumContainer');
var tilesetEntity = viewer.entities.add({
    tileset: {
        uri: Cesium.IonResource.fromAssetId(5741)
    }
});
viewer.flyTo(tilesetEntity);

The code, docs, specs and Sandcastle example all look good to me. My only quandary here is: inheriting the modelMatrix from the entity's position & orientation seems like the wrong approach. This would only produce the expected result when the 3D Tileset is located at the center of the Earth, right?

I almost feel like Entity.tileset needs to be treated more like Entity.polygon, which is positioned independently of Entity.position. But then to create the flying house example you have, you would need to expose a position and orientation on Entity.tileset that would follow the Entity.position & orientation?

I would be fine with merging this as-is assuming the common approach would be users creating a tileset on an entity with no position, and only using position when that tileset is located at the center of the Earth and is meant to be time dynamic.

/**
* Gets or sets the tileset.
* @memberof Entity.prototype
* @type {TilesetGraphics}
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no such type as TilesetGraphics. This should be Cesium3DTilesetGraphics. This produces an unclickable type in the doc here: http://localhost:8080/Build/Documentation/Entity.html#tileset

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, though party of me thought about removing the Cesium3D prefix from everywhere, but I guess keeping the two consistent makes more sense.

@OmarShehata
Copy link
Contributor

We should also remember to update https://github.com/AnalyticalGraphicsInc/czml-writer/wiki/CZML-Guide

@mramato
Copy link
Contributor Author

mramato commented Feb 6, 2020

I would be fine with merging this as-is assuming the common approach would be users creating a tileset on an entity with no position, and only using position when that tileset is located at the center of the Earth and is meant to be time dynamic.

This is exactly what they should do.

@mramato
Copy link
Contributor Author

mramato commented Feb 6, 2020

@OmarShehata ready

@OmarShehata
Copy link
Contributor

Thanks @mramato . I just merged in master and resolved the conflict in CHANGES.md. Will merge when green.

@mramato
Copy link
Contributor Author

mramato commented Feb 7, 2020

Awesome, thanks @OmarShehata!

@OmarShehata OmarShehata merged commit dbb3ab5 into master Feb 7, 2020
@OmarShehata OmarShehata deleted the czml-tilesets branch February 7, 2020 14:38
show: createPropertyDescriptor('show'),

/**
* Gets or sets the string Property specifying the URI of the glTF asset.
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't refer to a glTF asset. Copy/paste error from ModelGraphics?

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.

6 participants