feat(core): allow layer to specify which elements should be updatable by peppsac · Pull Request #789 · iTowns/itowns

peppsac · 2018-06-25T16:23:33Z

Some layers use separated objects for metadata and drawable objects.
For instance PointCloudProvider or 3dTilesProvider.

Attached layers (e.g texture layers) expect to receive a drawable object,
in their update function.

So this commit adds an optional metadataToElements function to layers,
allowing them to returns the to-be-updated object from metadata, and
its parent.

zarov · 2018-06-26T06:29:11Z

This needs a little more documentation, on what is expected to be returned from metadataToElements, in case someone writes this function as well. What about putting it in the documentation of Layer, or the Provider interface ?

gchoqueux · 2018-06-26T08:44:12Z

src/Core/MainLoop.js

        for (const attachedLayer of geometryLayer._attachedLayers) {
            if (attachedLayer.ready) {
-                attachedLayer.update(context, attachedLayer, element);
+                // Attached layers expect to receive the visual representation


An example would be needed to understand the use case.
it's necessary to add a new update?

You can find an example in the https://github.com/peppsac/itowns2/tree/colorisation branch.

It uses the ColorTextureProcessing to colorize a 3dtiles pointcloud.

But 3dTilesProcessing update function process metadata objects (= the content of the tileset.json file) while ColorTextureProcessing expects objects with a material (because it was initially built for TileMesh).

gchoqueux · 2018-06-26T08:50:37Z

src/Provider/3dTilesProvider.js

 function preprocessDataLayer(layer, view, scheduler) {
    layer.sseThreshold = layer.sseThreshold || 16;
    layer.cleanupDelay = layer.cleanupDelay || 1000;
+    layer.metadataToElements = (meta) => {


metadataToElements shouldn't be exposed in layer. We must avoid putting processing in the provider (especially that the providers should disappear)

why not do it in the attachedLayer.update?

metadataToElements shouldn't be exposed in layer
We must avoid putting processing in the provider

Why?

(especially that the providers should disappear)

I don't think so. While currently their scope/responsability is unclear, I don't think they'll ever disappear. For instance, in the 3dtiles case you'll always need "something" that understands the tileset.json structure and parse/prepare it for the processing code to operate on.

The purpose of this metadataToElements function is to allow Processing code to be independant of the providers.

We already had the same discussion for the picking feature where no good conclusion was reached.

This confirms that the Provider/Processing boundary isn't good enough, but IMHO this shouldn't prevent merging this.

As presented in #798, this PR proposes, not to couple the provider and the processing, as it makes the architecture more rigid and less modular. We must try to place this code in the processing not to increase this coupling

You missed the point again and again.

I won't repeat myself please read my previous comments.

peppsac · 2018-06-26T09:43:52Z

@zarov I shuffled the code a bit and moved the comment to Layer.js + added an error in MainLoop. WDYT?

zarov · 2018-06-26T09:48:21Z

src/Core/MainLoop.js

+
                if (sub) {
+                    if (__DEBUG__) {
+                        if (!(sub.isObject && sub.material)) {


Are you sure about this ? It will always throw an error here.

No the correct code is if (!(sub.element.isObject3D && sub.element.material)) {.

I'll push it, but first I need to resolve a bug that this assert shows.

zarov · 2018-06-26T13:35:22Z

src/Core/MainLoop.js

+
+                if (sub) {
+                    if (__DEBUG__) {
+                        if (!(sub.element.isObject3D && sub.element.material)) {


Some objects still go through this condition, for example the Scene of iTowns. Thus on the 3dtiles example I can't navigate and move at all.

It may actually not really a bug.

Do we really want to pass all the root of 3dtiles scene to attached layer? (in which case the attached layer needs to be aware that it may receive a hierarchy, rather than a single object). Or do we want to pass the drawable elements? (in this case the code in getObjectToUpdateForAttachedLayers should collect all objects in content that have a material).

I think I prefer the second option. WDYT?

Since I'm not sure about this, I removed the sub.element.material check for now.

I'll go with the second option as well.

I implemented it. Note that I had to adapt 3dTilesDebug to reflect this change - this part is a bit tedious due to the way we handle gltfUpAxis (= we rotate the matrix of the model but the boundingVolume must not be affected by this rotation).

mbredif

When do you need the parent ?

mbredif · 2018-06-26T23:33:53Z

src/Core/MainLoop.js

+                                Invalid object for attached layer to update.
+                                Must be a THREE.Object and have a THREE.Material`);
+                        }
+                    }


the above does not depend on attachedLayer and could be moved outside the loop

I missed this, thanks.

peppsac · 2018-06-27T06:52:03Z

When do you need the parent ?

e.g: for initializing a texture layer from the parent's texture = every usage of node.parent in LayeredMaterialNodeProcessing.js.

gchoqueux

I'm currently reviewing this PR
but first you have to add some tests

gchoqueux · 2018-06-27T16:23:50Z

src/Core/MainLoop.js

+                        attachedLayer.update(context, attachedLayer, sub.element, sub.parent);
+                    }
+                }
+            } else if (sub.elements) {


getObjectToUpdateForAttachedLayers can always return an Array?
it would simplify this part

This part is internal only (= no user of iTowns will have to write the same code in its application), so I don't mind having a bit more complicated code if it performs better.

I'll check, and simplify if possible.

I feel that it is more of a problem of structure of our elements. We should not complicate this part of the process to be able to adapt to all structures. I'll go deeper and I'll come back on the subject

I agree with @gchoqueux, even if it is hidden, it should always return elements as an array.

Keep in mind that this piece of code is executed multiple times during each update loop.

So creating an array by default, even if it's for a single element might pressure the GC or cause a perf hit.

Ok, that's a good point. We still have duplicated code though, could you move both part into another function ?

gchoqueux · 2018-06-27T16:42:43Z

src/Provider/PointCloudProvider.js

        layer.postUpdate = PointCloudProcessing.postUpdate;

+        // override the default method, since updated objects are metadata in this case
+        layer.getObjectToUpdateForAttachedLayers = (meta) => {


It should move this function in the processing, and assign it as above (same think for 3dtiles)

layer.getObjectToUpdateForAttachedLayers = PointCloudProcessing.getObjectToUpdateForAttachedLayers;

the provider.preprocess it looks like a layer's constructor. 3dTiles and pointCloud should inherit geometryLayer and overload its processing functions (if that is necessary)

It should move this function in the processing, and assign it as above

Where? Processing have no code that run once for a layer, while Provider have a preprocess step.
And processing code should try to be as independant as possible from providers so moving these functions that are highly dependent on the protocol/provider makes very little sense.

One of my mid-term goal is to have a single processing module for pointclouds, that would work with all kind of pointcloud sources.
To be able to do this, the provider of the said sources has to format/prepare the layer to meet the expectations of this processing module.

Basically: the processing code defines a contract, and will work with every layer that implements it.

the provider.preprocess it looks like a layer's constructor

Can you explain what you mean?

3dTiles and pointCloud should inherit geometryLayer and overload its processing functions (if that is necessary)

I assume that you meant 3dtiles layer and pointcloud layer?
What would be the benefits of inheriting + overriding GeometryLayer over the current situation?

(as there are no processing functions in GeometryLayer so I don't understand this either)

as there are no processing functions in GeometryLayer so I don't understand this either

geometryLayer.getObjectToUpdateForAttachedLayers is processing function

Ok I don't want to change this because I simply don't agree.

The problem currently is not this code, but the fact that Provider and Processing have strong coupling.

I and others are working on improving this and I would appreciate that you don't undermine this effort by bringing this topic on each review.

peppsac · 2018-06-27T20:59:17Z

I'm currently reviewing this PR

Ok, thanks.

but first you have to add some tests

This part is central in iTowns, so it's exercised by all functionnal tests.
I'll see if I can add some useful tests anyway - don't hesitate to suggest what kind of test you would like to have.

gchoqueux · 2018-06-28T10:19:13Z

src/Core/MainLoop.js

+                        attachedLayer.update(context, attachedLayer, sub.element, sub.parent);
+                    }
+                }
+            } else if (sub.elements) {


I feel that it is more of a problem of structure of our elements. We should not complicate this part of the process to be able to adapt to all structures. I'll go deeper and I'll come back on the subject

gchoqueux · 2018-06-28T10:28:23Z

src/Provider/3dTilesProvider.js

    layer.sseThreshold = layer.sseThreshold || 16;
    layer.cleanupDelay = layer.cleanupDelay || 1000;
+    // override the default method, since updated objects are metadata in this case
+    layer.getObjectToUpdateForAttachedLayers = (meta) => {


you should add a test will verify the function work as expected
and others clarify / document the data structure used for 3dTiles (as for example, the object, the userData and the content are structured)

I reverted part of the userData change.

The point was to store our objects in this dict created by three.js instead of patching the constructed objects (mainly because it's a good pratice and it might improve performance).

See this comment for a more info.

OK fine,
but what is the structure of the data?

what is the structure of the data?

I don't understand your question sorry.

Can you clarify the structure of objects used for 3dTiles, in itowns

I didn't change any the structure or its behavior in this PR. Why are you requesting this?

(also the structure is exactly the one from the 3dtiles spec)

gchoqueux · 2018-06-28T10:34:24Z

src/Provider/PointCloudProvider.js

        layer.postUpdate = PointCloudProcessing.postUpdate;

+        // override the default method, since updated objects are metadata in this case
+        layer.getObjectToUpdateForAttachedLayers = (meta) => {


you should add a test will verify the function work as expected
and others clarify / document the data structure used for cloudPoint (as for example, what is the difference this structure geometryLayer)

Test getObjectToUpdateForAttachedLayers is added, is it testing for geometryLayer?
What is data structure used for cloudPoint (as for example, what is the difference this structure geometryLayer)

getObjectToUpdateForAttachedLayers always return the same thing, as it's a contract between MainLoop and Layer.

The description is here https://github.com/iTowns/itowns/pull/789/files#diff-61e012c3e00bdc510be33d9eac684b2aR83, and the comment / ref implementation explains the returned types (it's not using jsdoc since it's not exposed anyway).

I can not find an explanation of data structure used for cloudPoind, in itowns

See #789 (comment)

gchoqueux · 2018-06-28T10:37:39Z

src/Provider/PointCloudProvider.js

        layer.postUpdate = PointCloudProcessing.postUpdate;

+        // override the default method, since updated objects are metadata in this case
+        layer.getObjectToUpdateForAttachedLayers = (meta) => {


as there are no processing functions in GeometryLayer so I don't understand this either

geometryLayer.getObjectToUpdateForAttachedLayers is processing function

peppsac · 2018-06-29T08:26:11Z

Pushed a commit to address the reviews + added some tests.

peppsac · 2018-07-02T09:35:39Z

@gchoqueux can you update your review please?

gchoqueux · 2018-07-09T16:23:46Z

for this PR:

What is the structure of instanced objects (THREE.js and itowns) and not the specification.
for example in node tile

tileA  // (THREE.Mesh->itowns.tileMesh)
     {
          children: [tileB, tileC] // (Array) tile children
          parent: tile0  // (THREE.Mesh->itowns.tileMesh) tile parent 
          obb: // (itowns.OBB)  oriented bouding box
     }

in 3dTiles, how are the objects linked: content, parent, bounding Volume, region, transform ...
in pointCloud, how are the objects linked: parent, obj, tightbbox ...

This information is important to understand the MainLoop.update and getObjectToUpdateForAttachedLayers (and for the maintenance of the code)

for getObjectToUpdateForAttachedLayers

As seen in the PR #798 , it is an error to place processing functions in the provider, to go in this direction we have to strive to place in the processing like process3dTilesNode or pointCloudProcessing.update

peppsac · 2018-07-09T16:48:46Z

As seen in the PR #798 , it is an error to place processing functions in the provider, to go in this direction we have to strive to place in the processing like process3dTilesNode or pointCloudProcessing.update

I disagree for the 1000th times. You got the logic backward: processing code should only depend on the data type (points or tiles or whatever). And 3dTilesProcessing should disappear, not 3dTilesProvider.

To achieve this, the Provider must hide away the specific of each protocols. And this is exactly the purpose of getObjectToUpdateForAttachedLayers.

With this logic, I'm able to implement pointcloud coloring using a WMS server (aka = I have a branch where it works). And the pointcloud texture selection uses the same code than the tile texturing code.

Do you have something similar with your branch? If not, then maybe you could let this PR in, so I can submit the next PR that depends on it. And then you might realize that you're wrong.

zarov

I disagree for the 1000th times. You got the logic backward: processing code should only depend on the data type (points or tiles or whatever). And 3dTilesProcessing should disappear, not 3dTilesProvider.

And I disagree too. Both should disappear, data type has nothing to do with the provider. It makes more sense to me to add this processing code in 3dTilesProcessing, which is more relevant to the 3dtiles format and data type.
Anyway, what @gchoqueux is saying (if I understand correctly) is that we should avoid adding methods like this if both are meant to disappear in the future.

gchoqueux 7 days ago Member
Can you clarify the structure of objects used for 3dTiles, in itowns
@peppsac
peppsac 5 days ago Member
I didn't change any the structure or its behavior in this PR. Why are you requesting this?
(also the structure is exactly the one from the 3dtiles spec)

Concerning this, I don't think the documentation of data structure is relevant to this PR, even if it could be good to have it.

zarov · 2018-07-10T09:18:56Z

src/Core/MainLoop.js

+                    }
+                }
+            } else if (sub.elements) {
+                for (let i = 0; i < sub.elements.length; i++) {


Use for (let element of sub.elements) instead.

I dislike more and more the for of syntax because it's harder to debug (lots of indirection). I'm also wondering on the perf cost of this (for the same reason: lots of indirection)...

Anyway I'll change it because for now we settled on using it everywhere.

Yeah it seems that performance wise it is the slowest (as tested quickly here http://jsben.ch/iEYTg)

And this test misses one of the cost: in itowns we never run native for of loops, but only transpiled for of.
For instance a simple:

for (const word of words) { console.log(word); }

becomes:

"use strict"; var _iteratorNormalCompletion = true; var _didIteratorError = false; var _iteratorError = undefined; try { for (var _iterator = words[Symbol.iterator](), _step; !(_iteratorNormalCompletion = (_step = _iterator.next()).done); _iteratorNormalCompletion = true) { var word = _step.value; console.log(word); } } catch (err) { _didIteratorError = true; _iteratorError = err; } finally { try { if (!_iteratorNormalCompletion && _iterator.return) { _iterator.return(); } } finally { if (_didIteratorError) { throw _iteratorError; } } }

(see it live)

zarov · 2018-07-10T09:24:32Z

src/Core/MainLoop.js

+                        attachedLayer.update(context, attachedLayer, sub.element, sub.parent);
+                    }
+                }
+            } else if (sub.elements) {


I agree with @gchoqueux, even if it is hidden, it should always return elements as an array.

peppsac · 2018-07-10T10:17:11Z

And I disagree too. Both should disappear, data type has nothing to do with the provider. It makes more sense to me to add this processing code in 3dTilesProcessing, which is more relevant to the 3dtiles format and data type.

The function here has no expectation on the data type. It only depends on the 3dtiles protocol (metadata would be more accurate).

To be clear: 3dtiles is not a data type, it's a protocol (in itowns current semantics at least which mix protocol and metadata).

So the provider is aware of the metadata, and build a data structure understood by itowns / the processing functions. This structure will evolve, but the point of this PR (and the next one) is to iterate on its definition (at the very least: getObjectToUpdateForAttachedLayers and .extent are part of it).

In the long term: the 3dtilesprocessing module should be gone, and be replaced either by a single processing.js module (see #674 for something that goes in this direction) or several datatypeprocessing.js (pointcloudprocessing.js, tileprocessing.js ...).

But the 3dtilesprovider must remain (even if its renamed 3dtilessource): because we need a place in itowns that is aware of the protocol/metadata of each supported sources.

Is it clearer?

zarov · 2018-07-10T10:56:42Z

It is clearer thanks, I was confused with the fact that 3dtiles is a protocol indeed. Could we have a glimpse of the next PR ?

Otherwise it seems good now to me, maybe @mbredif can give his opinion too ? I'm not the most aware contributor on 3dtiles.

peppsac · 2018-07-10T11:14:37Z

Could we have a glimpse of the next PR ?

Already posted elsewhere: https://github.com/peppsac/itowns2/commits/colorisation

My recent PRs are a split of the work done there. At the end the point is to be able to reuse the 'texture update processing' code to colorize pointclouds.

(and I have another feature built on top of this branch that leverage the cache to reduce the latency in itowns... but this will have to wait until all the previous PRs are merged)

zarov · 2018-07-10T12:40:20Z

Thanks ! I only took a quick look but I think there are some things I might disagree on later ;)

Anyway, could you rebase this PR ?

peppsac · 2018-07-10T12:45:06Z

Thanks ! I only took a quick look but I think there are some things I might disagree on later ;)

Sure :)

Anyway, could you rebase this PR ?

Done

Moving forward

zarov

Okay let's merge this, rebase and test before ! Thanks

Some layers use separated objects for metadata and drawable objects. For instance PointCloudProvider or 3dTilesProvider. Attached layers (e.g texture layers) expect to receive a drawable object, in their update function. So this commit adds an optional metadataToElements function to layers, allowing them to returns the to-be-updated object from metadata, and its parent.

gchoqueux reviewed Jun 26, 2018

View reviewed changes

zarov reviewed Jun 26, 2018

View reviewed changes

mbredif reviewed Jun 26, 2018

View reviewed changes

peppsac force-pushed the feat_metadata_to_element branch from 767b0cb to a9791bb Compare June 27, 2018 11:48

gchoqueux requested changes Jun 27, 2018

View reviewed changes

gchoqueux previously requested changes Jun 28, 2018

View reviewed changes

peppsac force-pushed the feat_metadata_to_element branch from af61677 to e8c2b32 Compare June 29, 2018 08:39

zarov reviewed Jul 10, 2018

View reviewed changes

peppsac force-pushed the feat_metadata_to_element branch from e8c2b32 to 3d8dfc4 Compare July 10, 2018 12:44

peppsac force-pushed the feat_metadata_to_element branch from 3d8dfc4 to f5290a2 Compare July 10, 2018 13:18

zarov approved these changes Jul 12, 2018

View reviewed changes

peppsac force-pushed the feat_metadata_to_element branch from f5290a2 to e0081e5 Compare July 12, 2018 07:48

peppsac force-pushed the feat_metadata_to_element branch from e0081e5 to b4a880b Compare July 12, 2018 07:51

peppsac merged commit 0cb47e4 into master Jul 12, 2018

peppsac deleted the feat_metadata_to_element branch July 12, 2018 09:27

Conversation

peppsac commented Jun 25, 2018

Uh oh!

zarov commented Jun 26, 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!

Choose a reason for hiding this comment

Uh oh!

peppsac commented Jun 26, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbredif left a comment

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!

peppsac commented Jun 27, 2018

Uh oh!

gchoqueux left a comment

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!

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!

peppsac commented Jun 27, 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