Pointcloud improvement #732

peppsac · 2018-04-16T13:54:50Z

2 commits improving pointcloud support for itowns.

The first one fixes the point budget handling for .bin format.

The second reworks SSE: it's similar to #674 but simpler and only for pointclouds.

Full commits message:

refactor(pointcloud): rework point budget implementation

Since .bin and .cin have different repartitions of the points inside a node,
they must use different algorithms.
For .cin we want to draw a %-age of each node because points are evenly
distributed.
For .bin, we sort the nodes by 'importance', and stop drawing nodes when we
have enoug points.

refactor(pointcloud): simplify SSE computation for pointcloud

Project average point spacing on screen and compare to sse threshold.
It's a simplification of the approach proposed in #674.

This allows to drop a dependency (convexhull) and brings the SSE
computation for pointcloud closer to what is done for other geometries.

Edit:
The SSE value computed here is the estimation of the maximum distance between 2 points in a given node.
PotreeConverter guarantees that the distance between each pair of points is >= metadata.spacing / node.level in all nodes. This is easy to visualize if you display successive children (using the 'sticky' debug tool):

The SSE is the estimation of this distance projected on the screen.

peppsac · 2018-04-20T09:54:30Z

Edited description + added image

mbredif

Looks good and simplifies things, thanks !
I added a few comments to improve it even further :)
(I have not tested it though)

mbredif · 2018-04-20T16:33:09Z

src/Process/PointCloudProcessing.js

@@ -155,7 +78,7 @@ function markForDeletion(elt) {

    if (!elt.notVisibleSince) {
        elt.notVisibleSince = Date.now();
-        elt.shouldBeLoaded = -1;
+        elt.sse = -1;


Maybe you could add a comment here stating what -1 means

mbredif · 2018-04-20T16:34:51Z

src/Process/PointCloudProcessing.js

@@ -169,6 +92,11 @@ export default {
            return [];
        }

+        context.camera.preSSE =


You could link here to an existing technical paper to explain the derivation

mbredif · 2018-04-20T16:36:16Z

src/Process/PointCloudProcessing.js

+            } else if (!elt.promise) {
+                const distance = Math.max(0.001, bbox.distanceToPoint(context.camera.camera3D.position));
+                const priority = computeScreenSpaceError(context, layer, elt, distance) / distance;
+                elt.promise = context.scheduler.execute({


Why do you divide by distance ?

To load closest node first.

If I understand correctly, minimizing SSE is what you want eventually (as things settle), but you prefer to bias the ordering of the fetches towards closer nodes, even if those have a smaller SSE. Is that correct ? I think a comment should be placed here for future reference (eg, we might want to enable the customization of this strategy)

mbredif · 2018-04-20T16:44:14Z

src/Process/PointCloudProcessing.js

-                    const count = Math.max(1.0, Math.floor(pts.geometry.drawRange.count * reduction));
-                    pts.geometry.setDrawRange(0, count);
+            // 2 different point count limit implementation, depending on the pointcloud source
+            if (layer.metadata.customBinFormat) {


I would rather have the processing be agnostic of formats, nut depend rather on format properties or strategies. Could not you have reordered bin files as in cin files, or the other way around?

What about renaming customBinFormat to something like prefixSubsampling ?

I added a layer.supportsProgressiveDisplay property. (Note that the rest of the file relies on the potreeconverter format, there's more work to do to be able to use this processing code on any pointcloud source)

gchoqueux · 2018-04-26T09:59:57Z

this commit : refactor(pointcloud): rework point budget implementation
Does it allow PointCloud to be fully compliant with the data potree (from potree converter)? Especially in the organization of points in the data structure

peppsac · 2018-04-26T10:01:07Z

this commit : refactor(pointcloud): rework point budget implementation
Does it allow PointCloud to be fully compliant with the data potree (from potree converter)? Especially in the organization of points in the data structure

Yes.

mbredif

LGTM

mbredif · 2018-04-26T09:57:45Z

src/Process/PointCloudProcessing.js

+            } else if (!elt.promise) {
+                const distance = Math.max(0.001, bbox.distanceToPoint(context.camera.camera3D.position));
+                const priority = computeScreenSpaceError(context, layer, elt, distance) / distance;
+                elt.promise = context.scheduler.execute({


If I understand correctly, minimizing SSE is what you want eventually (as things settle), but you prefer to bias the ordering of the fetches towards closer nodes, even if those have a smaller SSE. Is that correct ? I think a comment should be placed here for future reference (eg, we might want to enable the customization of this strategy)

mbredif · 2018-04-26T10:00:51Z

src/Process/PointCloudProcessing.js

-        return { shouldBeLoaded, surfaceOnScreen: 0 };
+function computeScreenSpaceError(context, layer, elt, distance) {
+    if (distance <= 0) {
+        return layer.sseThreshold;


is it equivalent to returning Infinity ? (which would be parameterless and thus more explicit)

Thanks for the review. I addressed both comments.

gchoqueux · 2018-04-26T10:08:54Z

Ok thanks, it's good to be completely compliant with Potree data

Since .bin and .cin have different repartitions of the points inside a node, they must use different algorithms. For .cin we want to draw a %-age of each node because points are evenly distributed. For .bin, we sort the nodes by 'importance', and stop drawing nodes when we have enoug points.

Project average point spacing on screen and compare to sse threshold. It's a simplification of the approach proposed in #674. This allows to drop a dependency (convexhull) and brings the SSE compuation for pointcloud closer to what is done for other geometries.

mbredif

LGTM

peppsac · 2018-04-27T12:01:57Z

Thanks for the review @mbredif - and welcome to the @iTowns/reviewers team !

autra · 2018-04-27T12:04:38Z

Thanks for the review @mbredif - and welcome to the @iTowns/reviewers team !

actually he just has write access. The convention is : a review from someone from itowns-reviewers group.

Nevermind, I'll make some comments on this if needed and we'll do a followup, ok for you?

peppsac · 2018-04-27T12:13:12Z

Oops my bad. I'm reverting this PR then, until your approval.

peppsac · 2018-04-27T12:22:23Z

PR reverted in #745 and reopened as #746.

peppsac force-pushed the pointcloud_improvment branch 2 times, most recently from 279a857 to 181d1ee Compare April 16, 2018 16:02

peppsac changed the title ~~Pointcloud improvment~~ Pointcloud improvement Apr 17, 2018

peppsac force-pushed the pointcloud_improvment branch from 181d1ee to e4592df Compare April 20, 2018 09:54

mbredif reviewed Apr 20, 2018

View reviewed changes

mbredif reviewed Apr 26, 2018

View reviewed changes

peppsac added 2 commits April 27, 2018 11:31

peppsac force-pushed the pointcloud_improvment branch from 9af92a7 to 24c618b Compare April 27, 2018 09:36

mbredif approved these changes Apr 27, 2018

View reviewed changes

peppsac merged commit 739f053 into master Apr 27, 2018

peppsac deleted the pointcloud_improvment branch April 27, 2018 12:02

peppsac mentioned this pull request Apr 27, 2018

Revert "Pointcloud improvement" #745

Merged

peppsac restored the pointcloud_improvment branch April 27, 2018 12:20

peppsac deleted the pointcloud_improvment branch April 27, 2018 12:20

peppsac restored the pointcloud_improvment branch April 27, 2018 12:20

peppsac mentioned this pull request Apr 27, 2018

Pointcloud improvement #746

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pointcloud improvement #732

Pointcloud improvement #732

peppsac commented Apr 16, 2018 •

edited

Loading

peppsac commented Apr 20, 2018

mbredif left a comment

mbredif Apr 20, 2018

peppsac Apr 23, 2018

mbredif Apr 20, 2018

peppsac Apr 23, 2018

mbredif Apr 20, 2018

peppsac Apr 23, 2018

mbredif Apr 26, 2018

mbredif Apr 20, 2018

mbredif Apr 20, 2018

peppsac Apr 23, 2018

gchoqueux commented Apr 26, 2018

peppsac commented Apr 26, 2018

mbredif left a comment

mbredif Apr 26, 2018

mbredif Apr 26, 2018

peppsac Apr 26, 2018

gchoqueux commented Apr 26, 2018

mbredif left a comment

peppsac commented Apr 27, 2018

autra commented Apr 27, 2018

peppsac commented Apr 27, 2018

peppsac commented Apr 27, 2018

Pointcloud improvement #732

Pointcloud improvement #732

Conversation

peppsac commented Apr 16, 2018 • edited Loading

peppsac commented Apr 20, 2018

mbredif 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

gchoqueux commented Apr 26, 2018

peppsac commented Apr 26, 2018

mbredif 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

gchoqueux commented Apr 26, 2018

mbredif left a comment

Choose a reason for hiding this comment

peppsac commented Apr 27, 2018

autra commented Apr 27, 2018

peppsac commented Apr 27, 2018

peppsac commented Apr 27, 2018

peppsac commented Apr 16, 2018 •

edited

Loading