feat(pointcloud): use 'changeSource' mechanism to avoid useless work by peppsac · Pull Request #719 · iTowns/itowns

peppsac · 2018-04-09T15:53:29Z

Implements a simple logic to avoid updating the full layer if we
already know that the result won't change.

This feature is already implemented for tile based layers
(see 3f340b9).

elemoine · 2018-04-10T10:46:09Z

src/Process/PointCloudProcessing.js

+                }
+            }
+        }
+        if (commonAncestorName && commonAncestorName.length > 0) {


(neat) I guess the second test (after &&) is useless, so this could be simplified to if (commonAncestorName).

Indeed: fixed.

elemoine · 2018-04-10T12:30:22Z

src/Process/PointCloudProcessing.js

-    preUpdate(context, layer) {
-        // TODO: use changeSource
-        layer.counters = {
-            pointCount: 0,


I see that the pointcloud.js and pointcloud_globe.js examples refer to pointcloud.counters.pointCount. Wouldn't that be a problem if you removed that count?

Also, if you just keep displayedCount you may want to go with layer.displayCount instead of having an intermediary layer.counters object.

elemoine

Looks good to me.

elemoine · 2018-04-11T08:46:26Z

Just a quick unrelated note:

I note that iTowns often dynamically adds properties to objects. For example, layer.displayedCount in this patch. For performance it is better to set properties at object construction time, and not dynamically add properties afterwards. See for example the Hidden Classes section in this HTML5 Rocks article. For that reason I think it's a good thing to more strictly type things, using specific object constructors/types. For example I think the Layer constructor should look like this:

function  Layer(options) {
    this.name = options.name;
    this.projection = options.projection;
    …
    this.displayedCount = 0;
}

It also makes the code more readable, and easier to document.

Just my 2 cents, and sorry for the unrelated comment here :)

zarov

LGTM, great improvement.

A question though (that have nothing to do with this PR): why is the node name is stored as a string ? Wouldn't be more efficient to store it with something as an octal ? I think it would simplify operation on it, and it could be simply converted to a string when needed. Nevermind, tried it up, not efficient.

zarov · 2018-04-11T09:14:37Z

@elemoine I agree completely on this, there could be improvement made here.

gchoqueux · 2018-04-11T09:52:00Z

src/Process/PointCloudProcessing.js

+                // some invisible tiles may now be visible
+                return [layer.root];
+            }
+            if (source.obj === undefined) {


I don't remember where this obj came from?
it's an Object3D?

See: https://github.com/iTowns/itowns/blob/master/src/Process/PointCloudProcessing.js#L228

gchoqueux · 2018-04-11T09:54:10Z

src/Process/PointCloudProcessing.js

+            if (source.obj === undefined) {
+                continue;
+            }
+            if (source.obj.isPoints && source.obj.layer == layer.id) {


you could add comment to explain this part with commonAncestorName

gchoqueux · 2018-04-11T09:58:15Z

src/Provider/PointCloudProvider.js

            // eslint-disable-next-line no-console
            console.log('LAYER metadata:', root);
            layer.root = root;
+            root.findChildrenByName = findChildrenByName.bind(root, root);


why you use bind?

To do currying and pre-set the first argument to root.

Implements a simple logic to avoid updating the full layer if we already know that the result won't change. This feature is already implemented for tile based layers (see 3f340b9).

This commit fixes a left-over from #719

peppsac force-pushed the feat_pointcloud_change_sources branch from 7ef054e to 9643de9 Compare April 10, 2018 07:59

elemoine reviewed Apr 10, 2018

View reviewed changes

elemoine approved these changes Apr 11, 2018

View reviewed changes

zarov approved these changes Apr 11, 2018

View reviewed changes

gchoqueux reviewed Apr 11, 2018

View reviewed changes

feat(pointcloud): use 'changeSource' mechanism to avoid useless work

38e7c11

Implements a simple logic to avoid updating the full layer if we already know that the result won't change. This feature is already implemented for tile based layers (see 3f340b9).

peppsac force-pushed the feat_pointcloud_change_sources branch from 32f104c to 38e7c11 Compare April 13, 2018 08:18

peppsac merged commit b67670a into master Apr 13, 2018

peppsac deleted the feat_pointcloud_change_sources branch April 13, 2018 08:53

peppsac added a commit that referenced this pull request Apr 16, 2018

fix(pointcloud): remove unused variable

1f679de

This commit fixes a left-over from #719

peppsac mentioned this pull request Apr 16, 2018

fix(pointcloud): remove unused variable #729

Merged

zarov pushed a commit that referenced this pull request Apr 16, 2018

fix(pointcloud): remove unused variable

e0d839d

This commit fixes a left-over from #719

peppsac mentioned this pull request May 30, 2018

Add support for other data types in potree pointclouds #767

Merged

peppsac mentioned this pull request Jun 29, 2018

feat(core): allow layer to specify which elements should be updatable #789

Merged

Conversation

peppsac commented Apr 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elemoine left a comment

Choose a reason for hiding this comment

Uh oh!

elemoine commented Apr 11, 2018

Uh oh!

zarov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zarov commented Apr 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zarov left a comment •

edited

Loading