Time Dynamic Point Clouds #6721

lilleyse · 2018-06-22T14:50:12Z

Fixes #6397
This is built off of the Draco PR #6559, merge that first.

Adds the ability to playback time dynamic point cloud data. TimeDynamicPointCloud takes a TimeIntervalCollection, where each interval points to a 3D Tiles .pnts file and an optional transform.

Also added a new private class PointCloud that does the core loading and rendering of pnts content independent of a 3D Tileset. This was done so that PointCloud3DTileContent and TileDynamicPointCloud can both share the same core code even though one has a tileset and the other does not.

TileDynamicPointCloud has a lot of the same rendering features as a tileset - styling, clipping planes, attenuation, eye dome lighting, and maximum memory usage.

The playback works by calculating the average time it takes to load a frame and prefetching the next frame based on this average and how fast the clock is running. When that frame is reached it is rendered and another frame is prefetched. Any frames in-between are skipped. If the frames are sufficiently small or the clock is sufficiently slow then no frames will be skipped.

The alternative approach not implemented here is to load larger batches of frames and pause the simulation if a frame is not ready, similar to video buffering. This way no frames are skipped during playback. For the data I've been testing loading can rarely keep up with playback, so I expect the constant buffering would really hurt user experience.

The approach here is hopefully more flexible for a wider array of playback scenarios like scrubbing the timeline or setting the clock to a 5-100x speed. It will also adapt nicely to actual point cloud streaming, like real-time from a LiDAR sensor.

To do:

cesium-concierge · 2018-06-22T14:50:14Z

@lilleyse, thanks for the pull request! Maintainers, we have a signed CLA from @lilleyse, so you can review this at any time.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

pjcozzi · 2018-06-22T21:24:48Z

If you didn't already include them, many of the notes in the top of this PR would make good code comments.

pjcozzi · 2018-06-22T21:27:08Z

Apps/Sandcastle/gallery/Time Dynamic Point Cloud.html

@@ -0,0 +1,77 @@
+<!DOCTYPE html>


Trying to run this, I get "Request has failed. Status Code: 404"

Is this because the data is only in the specs?

http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/point-cloud-stream-new/Apps/Sandcastle/index.html?src=Time%20Dynamic%20Point%20Cloud.html

Yeah, that will be fixed.

pjcozzi · 2018-06-22T21:27:26Z

Apps/Sandcastle/gallery/Time Dynamic Point Cloud.html

+    var frames = properties.frames;
+    var intervals = frames.intervals;
+
+    // Update uris to be absolute


Is this a hack?

pjcozzi · 2018-06-22T21:28:07Z

Apps/Sandcastle/gallery/Time Dynamic Point Cloud.html

+var basePath = '../../SampleData/test/PointCloudTimeDynamic/';
+var czmlPath = basePath + 'frames.czml';
+var dataSource = viewer.dataSources.add(Cesium.CzmlDataSource.load(czmlPath));
+dataSource.then(function(dataSource) {


Should there be a helper for this?

What if a CZML has time-dynamic point clouds and other content - looks like it all just works, right?

pjcozzi · 2018-06-22T21:28:17Z

Apps/Sandcastle/gallery/Time Dynamic Point Cloud.html

+    var entities = dataSource.entities;
+    var entity = entities.getById('Time Dynamic Point Cloud');
+    var properties = entity.properties;
+    var frames = properties.frames;


Should the CZML spec be updated?

I didn't actually make any changes to CZML in this PR, I just use CZML to package the data for the demo. CZML conveniently creates a TimeIntervalCollection under the hood that can be passed to TimeDynamicPointCloud.

But maybe I should hardcode the time intervals and uris in the demo itself to avoid the confusion here and #6721 (comment) #6721 (comment).

Hardcoding sounds OK; if CZML can now point to a .pnts file, we should update the spec for that.

No CZML spec updates required.

The demo is just using CZML as a storage mechanism and helper.

There was some offline discussion that this use case is too niche to be added to core CZML, so I never built that in. But the demo shows that if you had a czml with a custom property you could get a TimeIntervalCollection and initialize a TimeDynamicPointCloud. You can do this without CZML too, which is what I plan on doing in this demo instead.

OK, I suppose. We have someone relying on this and we need to provide them guidance on the best practice whether that be a custom CZML property or a completely separate container. They are already a CZML user as you know but that does not imply that the point clouds needs to be part of it.

pjcozzi · 2018-06-22T21:31:36Z

Source/Scene/PointCloud.js

+
+        createUniformMap(pointCloud, frameState);
+
+        var vs = 'attribute vec3 a_position; \n' +


For the current use case, how does the shader cache perform? It doesn't thrash right - since a shader program isn't deleted right away? Nonetheless it has to do a ton of string processing - could be a good roadmap or PERFORMANCE_IDEA to improve just like reusing a vertex buffer.

Yeah, no shader cache thrashing but there is a bunch of string processing. This and other GPU resource optimizations are now in a PERFORMANCE_IDEA.

pjcozzi · 2018-06-22T21:34:54Z

Source/Scene/TimeDynamicPointCloud.js

+     *
+     * @param {Object} options Object with the following properties:
+     * @param {Clock} options.clock A {@link Clock} instance that is used when determining the value for the time dimension.
+     * @param {TimeIntervalCollection} options.intervals A {@link TimeIntervalCollection} with its data property being an object containing a uri to a Point Cloud (.pnts) tile and an optional transform.


For context, probably say "3D Tiles Point Cloud"

pjcozzi · 2018-06-22T21:41:27Z

Source/Scene/TimeDynamicPointCloud.js

+    'use strict';
+
+    /**
+     * Provides functionality for playback of time-dynamic point cloud data.


Style-wise I think "Provides functionality" is empty - could say that about anything.

pjcozzi · 2018-06-22T21:42:57Z

Source/Scene/TimeDynamicPointCloud.js

+     * @param {Boolean} [options.show=true] Determines if the point cloud will be shown.
+     * @param {Matrix4} [options.modelMatrix=Matrix4.IDENTITY] A 4x4 transformation matrix that transforms the point cloud.
+     * @param {ShadowMode} [options.shadows=ShadowMode.ENABLED] Determines whether the point cloud casts or receives shadows from each light source.
+     * @param {Number} [options.maximumMemoryUsage=256] The maximum amount of memory in MB that can be used by the point cloud.


Perhaps details for the property reference doc but how does this work if one frame exceeds the usage? I assume it is allowed. Reference doc should also briefly explain that this is streaming, prefectches, and uses an LRU cache like the GitHub comment mentions.

One frame that exceeds the usage will still be rendered.

I added a note that should clarify this (similar to the Cesium3DTileset wording):

Frames that are not being loaded or rendered are unloaded to enforce this.

pjcozzi · 2018-06-22T21:43:26Z

Source/Scene/TimeDynamicPointCloud.js

+     * @param {Matrix4} [options.modelMatrix=Matrix4.IDENTITY] A 4x4 transformation matrix that transforms the point cloud.
+     * @param {ShadowMode} [options.shadows=ShadowMode.ENABLED] Determines whether the point cloud casts or receives shadows from each light source.
+     * @param {Number} [options.maximumMemoryUsage=256] The maximum amount of memory in MB that can be used by the point cloud.
+     * @param {Object} [options.pointCloudShading] Options for constructing a {@link PointCloudShading} object to control point size based on geometric error and eye dome lighting.


Should "geometric error" be here?

pjcozzi · 2018-06-22T21:44:30Z

Source/Scene/TimeDynamicPointCloud.js

+         *
+         * @see TimeDynamicPointCloud#maximumMemoryUsage
+         */
+        totalMemoryUsageInBytes : {


Are you sure that some of these properties should be in MB and others in bytes? What does the rest of Cesium do?

I copied what Cesium3DTileset does. I think the premise is any option that is settable from the user is in MB and anything internal is bytes.

Perhaps submit an issue to add this to the Coding Guide? Or just update it.

pjcozzi · 2018-06-22T21:47:22Z

If this is fast enough for the current use case, some of the TODOs at the top of this PR and ideas in #6397 could be a new roadmap issue or PERFORMANCE_IDEA comments even if allocating a new vertex buffer per frame makes me hurt inside. 😢

…rage load time can't be determined

lilleyse · 2018-07-03T17:24:21Z

@pjcozzi @ggetz Updated

pjcozzi · 2018-07-03T20:42:09Z

@ggetz please run with it.

lilleyse · 2018-07-09T15:02:50Z

@ggetz Updated and synced up with master.

ggetz

I think there is an issue with bounding spheres when providing a transform. For instance, here's the initial zoomedTo view when I specify

function dataCallback(interval, index) {
    return {
        uri: uris[index],
        transform : Cesium.Matrix4.fromUniformScale(8.0)
    };
}

I'm having trouble specifying clipping planes because I need to get the bounding sphere center in order to transform them correctly.

Also nitpicky, but can we add more "time slices" to the Sandcastle thumbnail? It wasn't immediately obvious to me what it was trying to show with just the two.

ggetz · 2018-07-03T21:19:49Z

Apps/Sandcastle/gallery/Time Dynamic Point Cloud.html

+    <meta http-equiv="X-UA-Compatible" content="IE=edge">
+    <meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1, minimum-scale=1, user-scalable=no">
+    <meta name="description" content="Time Dynamic Point Cloud">
+    <meta name="cesium-sandcastle-labels" content="Showcases, 3D Tiles">


Totally just semantics but technically, though it uses .pnts files, this isn't really 3D Tiles, is it?

I was debating this too. I'll move it out of 3D Tiles.

ggetz · 2018-07-09T15:54:30Z

Source/Scene/PointCloud.js

+        byteOffset += sizeOfUint32;  // Skip magic
+
+        var version = view.getUint32(byteOffset, true);
+        if (version !== 1) {


Should have asked this in the last PR, but should we check supported extensions like we do in Model.js?

Hm.. I'm tempted to skip for now because PointCloud and TimeDynamicPointCloud don't have a list of extensions used by the tile/tileset. It would have to guess where to look for unsupported extension, like in the feature table and batch table extensions object. But it feels a bit weird to do that at the tile level.

Extension checking probably belongs at the Cesium3DTileset level. Maybe worth adding to #6780?

ggetz · 2018-07-09T16:20:17Z

Source/Scene/TimeDynamicPointCloud.js

+            var message = defined(error.message) ? error.message : error.toString();
+            if (that.frameFailed.numberOfListeners > 0) {
+                that.frameFailed.raiseEvent({
+                    url : uri,


Should this be uri as well?

ggetz · 2018-07-09T16:21:15Z

Source/Scene/TimeDynamicPointCloud.js

+         *     console.log('Error: ' + error.message);
+         * });
+         */
+        this.frameFailed = new Event();


I think the error properties be documented here as well.

ggetz · 2018-07-09T17:21:07Z

Source/Scene/TimeDynamicPointCloud.js

+        that._runningSamples[that._runningIndex] = loadTime;
+        that._runningLength = Math.min(that._runningLength + 1, that._runningSamples.length);
+        that._runningIndex = (that._runningIndex + 1) % that._runningSamples.length;
+        that._runningAverage = that._runningSum / that._runningLength;


Is there a reason we're replacing each frame with the last sample instead of just taking one long running average? I think it would be simpler to keep a running average with something like runningAverge = (previousAverage * n / (n + 1)) + loadTime / (n + 1) and you would only need to keep track of the runningAverage and the total number of samples.

I initially had a long running average but extra slow frames would really throw off the average. Less noticeable in a simple app, but it became a big problem in the app I was testing where lots of other network requests were being made.

So now it just tracks the last N frames, and adapts a lot better to variations in tile size, network speed, etc.

ggetz · 2018-07-09T18:10:34Z

Source/Scene/TimeDynamicPointCloud.js

+            return;
+        }
+
+        var clockMultiplierChanged = false;


You may have already considered this, but would it make more sense to handle the current/previous/next interval logic in a function that subscribes to the clock tick event instead of in the render loop itself? This is what TimeDynamicImagery does.

I think TimeDynamicImagery does that because it doesn't have an update function that gets called. For point clouds I like that all the update stuff is in the same place.

ggetz · 2018-07-09T18:12:26Z

Source/Scene/TimeDynamicPointCloud.js

+    function getUnloadCondition(frameState) {
+        return function(frame) {
+            // Unload all frames that aren't currently being loaded or rendered
+            return frame.touchedFrameNumber < frameState.frameNumber;


Does this also make sense if the clock is running in reverse?

FrameState's frameNumber is always incremented, not attached to the clock.

ggetz · 2018-07-09T18:14:59Z

Source/Scene/TimeDynamicPointCloud.js

+     *
+     * @param {Object} options Object with the following properties:
+     * @param {Clock} options.clock A {@link Clock} instance that is used when determining the value for the time dimension.
+     * @param {TimeIntervalCollection} options.intervals A {@link TimeIntervalCollection} with its data property being an object containing a <code>uri</code> to a 3D Tiles Point Cloud tile and an optional <code>transform</code>.


<code>data</code> property

ggetz · 2018-07-09T18:20:13Z

Source/Scene/TimeDynamicPointCloud.js

+    TimeDynamicPointCloud.prototype._getAverageLoadTime = function() {
+        if (this._runningLength === 0) {
+            // Before any frames have loaded make a best guess about the average load time
+            return 0.05;


What's this based on? Can we grab any properties from the browser or system to make a more specific guess?

It's basically arbitrary - tuned roughly to a 300kb Draco tile. We would need to know tile size before it's downloaded and network speed to make a proper guess. I don't think it matters too much since the average will get corrected once tiles start to load.

ggetz · 2018-07-09T18:26:56Z

Apps/Sandcastle/gallery/Time Dynamic Point Cloud.html

+    <meta charset="utf-8">
+    <meta http-equiv="X-UA-Compatible" content="IE=edge">
+    <meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1, minimum-scale=1, user-scalable=no">
+    <meta name="description" content="Time Dynamic Point Cloud">


The description shouldn't just repeat the name.

lilleyse · 2018-07-10T00:59:17Z

I think there is an issue with bounding spheres when providing a transform. For instance, here's the initial zoomedTo view when I specify

I think this is two problems:

The tile uses an RTC_CENTER of [1215012.8828876738,-4736313.051199594,4081605.22126042] which is applied before the transform, so the transform is scaling a really large translation and pushing it really far away from the globe. A translation only transform should look more correct. I think this is expected behavior though.
For pnts files that aren't quantized, the bounding volume is calculated by sampling 20 points (evenly spaced apart) in the point cloud here. The test data has an unlucky point layout which causes the bounding sphere to center near the corner. I didn't see this problem with real-world data. But to be safe I should probably sample randomly.

20 samples

Calculated from all points

lilleyse · 2018-07-10T03:08:51Z

@ggetz Updated.

ggetz

@lilleyse

Looking good! I have a few more comments, and you also have some failing tests.

ggetz · 2018-07-10T14:29:12Z

Source/Scene/PointCloud.js

+        // Use same random values across all runs
+        if (!defined(randomValues)) {
+            CesiumMath.setRandomNumberSeed(0);
+            randomValues = new Array(20);


Shouldn't this be new Array(samplesLength);?

ggetz · 2018-07-10T14:52:37Z

Source/Scene/TimeDynamicPointCloud.js

-     * Do not call this function directly.  This is documented just to
-     * list the exceptions that may be propagated when the scene is rendered:
-     * </p>
-     */


Either mark as @private or document, here and throughout.

ggetz · 2018-07-10T15:03:09Z

Source/Scene/TimeDynamicPointCloud.js

+        if (defined(frame) && (frame !== this._lastRenderedFrame)) {
+            if (that.frameChanged.numberOfListeners > 0) {
+                frameState.afterRender.push(function() {
+                    that.frameChanged.raiseEvent(that);


Works well, however, in requestRender mode, the frames stop advancing and this event stops firing after each frame has been loaded once. I think this may be an argument for handling the frame advancement in a clock listener rather than in the render loop.

That's true, and is probably the right thing to do, I'm just afraid that splitting the update logic into two areas might complicate the design, when in most cases request render mode won't be used for an app that uses this features.

I understand your point about wanting to keep it simple, but on the other hand, I think separating the update and render logic also has value.

The biggest concern I have is since the update is tied to the rendering, that event won't trigger when in request render mode, so there's no way to watch and explicitly render a new frame when needed. I can't attest to the use cases for this as much as you, but it seems like a good candidate for requestRender mode in that you only need to render a frame (assuming nothing else is changing) when the timeline advances enough. However if you think it won't get used, then this doesn't need to be a priority now.

I think its best to revisit later if it comes up. The render and update are just too linked together at the moment, sometimes by necessity, and it might take some time to get the separation right.

lilleyse · 2018-07-11T00:27:22Z

Updated. Hopefully b2b94f5 fixes the tests.

ggetz · 2018-07-11T15:13:47Z

Ok, this looks good to me. Thanks @lilleyse !

lilleyse added 14 commits May 30, 2018 16:41

Added PointCloud class

2dab089

Added PointCloudStream class

61065d8

Start of prefetching code

96cd741

Batch id uint32 fix

a618a7e

Rename PointCloudStream to TimeDynamicPointCloud

4704934

Updates in preparation for auto-skipping frames

ab37223

Merge branch 'point-clouds-draco' into point-cloud-stream-new

d2df499

More picking updates

cd219a8

Documentation and cleanup

45382e7

Added Sandcastle demo

0e44cf8

Frame skipping added

1c62498

Minor wording and organization changes

23fb235

Merge branch 'point-clouds-draco' into point-cloud-stream-new

45f20bc

Update Sandcastle demo

59e169a

Move uniform map creation so it's easier to merge in mutables

f505207

lilleyse force-pushed the point-cloud-stream-new branch from bd5a049 to f505207 Compare June 22, 2018 18:20

OmarShehata mentioned this pull request Jun 22, 2018

Don't post if nothing wrong? CesiumGS/cesium-concierge#107

Closed

pjcozzi reviewed Jun 22, 2018

View reviewed changes

lilleyse added 6 commits June 26, 2018 14:04

Avoid skipping requested frames

bd67113

Get the next interval more aggressively

f30e694

Calculate average load time from the last N frames

a4f2d21

Merge branch 'point-clouds-draco' into point-cloud-stream-new

c4a0620

Documentation updates

178003f

Fix edge case where first frame is not loaded sequentially and an ave…

b166766

…rage load time can't be determined

lilleyse force-pushed the point-cloud-stream-new branch from f79ff9a to f6dc8ee Compare June 27, 2018 01:24

lilleyse added 2 commits June 26, 2018 21:57

Initial work for computing bounding sphere

3455dc3

Add PERFORMANCE_IDEA for sharing GPU resources

70cac08

lilleyse added 4 commits July 2, 2018 21:42

Fix EDL test for webgl-stub

04a8797

Rename pointCloudShading to shading

a9d2be7

More description for frameFailed event

e1fddc0

Added viewer.zoomTo(timeDynamicPointCloud)

f531406

lilleyse force-pushed the point-cloud-stream-new branch from b767800 to f531406 Compare July 3, 2018 17:23

This was referenced Jul 5, 2018

Added Draco support to point clouds #6559

Merged

Generate Draco and Time Dynamic Point Cloud tilesets CesiumGS/3d-tiles-validator#150

Merged

ggetz self-assigned this Jul 9, 2018

lilleyse changed the base branch from point-clouds-draco to master July 9, 2018 14:46

lilleyse added 2 commits July 9, 2018 10:54

Merge branch 'master' into point-cloud-stream-new

dd7a59c

Comment about quantized range and scale

adf8f4e

lilleyse force-pushed the point-cloud-stream-new branch from 6161a96 to adf8f4e Compare July 9, 2018 15:00

ggetz requested changes Jul 9, 2018

View reviewed changes

lilleyse force-pushed the point-cloud-stream-new branch from 86f1dbb to 5476619 Compare July 10, 2018 03:03

frame changed event, ready promise, and other updates

cb9ee74

lilleyse force-pushed the point-cloud-stream-new branch from 5476619 to cb9ee74 Compare July 10, 2018 03:07

ggetz requested changes Jul 10, 2018

View reviewed changes

lilleyse added 3 commits July 10, 2018 13:10

Updates

a7baeff

Fix test

b2b94f5

Merge branch 'master' into point-cloud-stream-new

3294f35

ggetz approved these changes Jul 11, 2018

View reviewed changes

ggetz merged commit 4529694 into master Jul 11, 2018

lilleyse deleted the point-cloud-stream-new branch July 26, 2018 16:22


		createUniformMap(pointCloud, frameState);

		var vs = 'attribute vec3 a_position; \n' +

Time Dynamic Point Clouds #6721

Time Dynamic Point Clouds #6721

Conversation

lilleyse commented Jun 22, 2018 • edited Loading

cesium-concierge commented Jun 22, 2018

pjcozzi commented Jun 22, 2018

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

pjcozzi commented Jun 22, 2018

lilleyse commented Jul 3, 2018

pjcozzi commented Jul 3, 2018

lilleyse commented Jul 9, 2018

ggetz 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

lilleyse commented Jul 10, 2018

lilleyse commented Jul 10, 2018

ggetz 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

lilleyse commented Jul 11, 2018

ggetz commented Jul 11, 2018 • edited Loading

lilleyse commented Jun 22, 2018 •

edited

Loading

ggetz commented Jul 11, 2018 •

edited

Loading