Refacto limit dependency on layeredmaterial #792

peppsac · 2018-06-26T13:01:37Z

2 commits, related to the same context: stop leaking internal attributes (WMTS level, LayeredMateiral texture indexing schema) in all iTowns.

First commit content:

fix: stop using 'level' property where it's not mandatory

Currently the 'level' concept from WMTS has leaked everywhere in iTowns
codebase.

This commits replace its usage in one part of iTowns, by something
equivalent but that doesn't force texture Providers to mock WMTS level
mechanism.

2nd one is:
refacto(core): avoid manipulating LayeredMaterial internal state

This commits is a step forward stopping to rely on LayeredMaterial
internal representation of textures (using an index and such) in other
parts of iTowns.

This will allow to reuse all this code for objects that don't use a
LayerMaterial instance as their material.

mbredif

This definitely goes in the right direction !
I made a few comments.

mbredif · 2018-06-26T13:30:10Z

src/Core/TileMesh.js

@@ -224,8 +219,10 @@ TileMesh.prototype.getCoordsForLayer = function getCoordsForLayer(layer) {
        } else {
            return OGCWebServiceHelper.computeTMSCoordinates(this, layer.extent, layer.origin);
        }
-    } else {
+    } else if (layer.extent.crs() == this.extent.crs()) {
        return [this.extent];


Shouldn't this test go into Extent.as ?

It could but actually the as() function should be improved to support a 'no-clone' argument (currently we always clone the extent, even if the requested crs is the same).

add some comment to explain your choice

mbredif · 2018-06-26T22:10:32Z

src/utils/DEMUtils.js

@@ -169,7 +169,7 @@ function tileAt(pt, tile) {
                return t;
            }
        }
-        if (tile.getLayerTextures(l_ELEVATION)[0].coords.zoom > -1) {
+        if (tile.material.textures[l_ELEVATION][0].coords.zoom > -1) {


Is it equivalent to isElevationLayerLoaded ? (which would be clearer)

Yes.

I'll use this but I'll remove it from TileMesh and move it where it belongs = LayeredMaterial.

mbredif · 2018-06-26T22:16:08Z

src/Core/TileMesh.js

@@ -11,10 +11,12 @@ import RendererConstant from '../Renderer/RendererConstant';
 import OGCWebServiceHelper, { SIZE_TEXTURE_TILE } from '../Provider/OGCWebServiceHelper';
 import { is4326 } from './Geographic/Coordinates';

-function TileMesh(geometry, params) {
+function TileMesh(layer, geometry, params) {
    // Constructor
    THREE.Mesh.call(this);


the call to super could pass directly the geometry and material : super(geometry, material) (with a material constructed at the call site) and the constructor could be more explicit with : TileMesh(geometry, material, extent, level), as in 6dd1ba5

mbredif · 2018-06-26T22:16:48Z

src/Core/TileMesh.js

    // Constructor
    THREE.Mesh.call(this);

+    this.layer = layer;
+


What is the semantics of this layer ? which one is it ? I would rather keep it outside of TileMesh as with other meshes in itowns.

It's the GeometryLayer, and as stated above it was already stored in TileMesh (and I think it's useful).

where tile.layer is used?

For instance here.

And I'll repeat myself: this isn't a new thing. I simply moved the layer assignement inside the constructor.

mbredif · 2018-06-26T22:19:42Z

src/Core/TileMesh.js

@@ -37,6 +39,7 @@ function TileMesh(geometry, params) {
    this.frustumCulled = false;

    this.updateGeometricError();
+    this.wmtsCoords = {};


I think it makes more sense to move this to Extent.

Yeah... I moved it here, again because it was assigned somewhere else (https://github.com/iTowns/itowns/pull/792/files#diff-5e0253bd7132719c7fe2322c297efe16L48).

I propose to keep it that way for this PR, and use your PR #785 as an opportunity to fix this since it's more focused on the extent side. WDYT?

Why not rename it to a more appropriate name in the same time ?

mbredif · 2018-06-26T22:24:17Z

src/Process/LayeredMaterialNodeProcessing.js

@@ -66,7 +66,7 @@ function initNodeElevationTextureFromParent(node, parent, layer) {
            elevation.max = max;
        }

-        node.setTextureElevation(elevation);
+        node.setTextureElevation(layer, elevation);
        node.material.elevationLayersId =
            parent.material.elevationLayersId;


you removed it...

What do you mean?

do you still support material.elevationLayersId ? I tought you proposed to remove it.

Ah ok. I pushed a commit removing this line.

mbredif · 2018-06-26T22:28:56Z

src/Renderer/LayeredMaterial.js

+        opacity: layer.opacity,
+        fx: layer.fx,
+        idLayer: layer.id,
+    };


why do you create this intermediate object instead of using layer directly ?

I moved this intermediate object from elsewhere, but you're right it's useless. I'll remove it.

mbredif · 2018-06-26T22:29:52Z

src/Renderer/LayeredMaterial.js

 };

-LayeredMaterial.prototype.setLayerVisibility = function setLayerVisibility(index, visible) {
+LayeredMaterial.prototype.setLayerVisibility = function setLayerVisibility(layer, visible) {
+    const index = Number.isInteger(layer) ? layer : this.indexOfColorLayer(layer.id);
    this.uniforms.visibility.value[index] = visible;
 };


could we deprecate the index-based api (with a console.warn) ? is it still used elsewhere ?

It's only used internally in LayeredMaterial.

There are a bunch of other index based method in LayeredMaterial and I think they should all be removed. I'll do a separate PR for this stuff.

mbredif · 2018-06-26T22:35:32Z

src/Core/TileMesh.js

-    return mat.getColorLayerLevelById(layerId) > -1;
+    const textures = mat.getLayerTextures(layer);
+    if (textures.length) {
+        return textures[0].coords.zoom > -1;


should not it be textures.every(texture => texture.coords.zoom == this.level) or textures.every(texture => somefun(layer, this.extent, texture.coords) for a suitable function ?

Actually it doesn't matter, since we always set textures in one shot (this is what getColorLayerLevelById was doing anyway)

mbredif · 2018-06-26T22:42:52Z

src/utils/DEMUtils.js

@@ -371,7 +371,7 @@ function _readZ(layer, method, coord, nodes, cache) {
    }

    const tile = tileWithValidElevationTexture;
-    const src = tileWithValidElevationTexture.getLayerTextures(l_ELEVATION)[0];
+    const src = tileWithValidElevationTexture.material.textures[l_ELEVATION][0];


tileWithValidElevationTexture.getLayerTextures(layer)[0] ?

Yep, thanks.

Ah no, because layer here is the geometry layer. And the calling site is not aware of which elevation layer we can use.

So for now I'll leave it like that.

and with the famous tile.layer?
you could write:
tileWithValidElevationTexture.getLayerTextures(tile.layer)[0]

and with the famous tile.layer?

Are you sure the famous bit is needed?

you could write: tileWithValidElevationTexture.getLayerTextures(tile.layer)[0]

And it would be incorrect. tile.layer is the GeometryLayer, not the Layer used for the elevation.

Ok,
So we don't know in elevation layer the data is searched for?

we should add tests to DEMUtils, if it doesn't exist

Not the scope of this PR. I'm only changing this file to remove one useless abstraction (and it's a 2 line diff)

peppsac · 2018-06-27T07:33:29Z

Pushed a commit which should fix the above issues. Thanks for the review @mbredif :)

gchoqueux

we should also add the tests on the layeredMaterial

gchoqueux · 2018-06-28T08:46:02Z

src/Core/TileMesh.js

    // Constructor
    THREE.Mesh.call(this);

+    this.layer = layer;
+


where tile.layer is used?

gchoqueux · 2018-06-28T08:48:18Z

src/Core/TileMesh.js

-    return this.material.loadedTexturesCount[l_ELEVATION] > 0;
+    const textures = mat.getLayerTextures(layer);
+    if (textures.length) {
+        return textures[0].coords.zoom > -1;


use EMPTY_TEXTURE_ZOOM instead of -1

gchoqueux · 2018-06-28T08:52:31Z

src/Core/TileMesh.js

@@ -224,8 +219,10 @@ TileMesh.prototype.getCoordsForLayer = function getCoordsForLayer(layer) {
        } else {
            return OGCWebServiceHelper.computeTMSCoordinates(this, layer.extent, layer.origin);
        }
-    } else {
+    } else if (layer.extent.crs() == this.extent.crs()) {
        return [this.extent];


add some comment to explain your choice

gchoqueux · 2018-06-28T08:52:43Z

src/Process/GlobeTileProcessing.js

-            currentElevationLevel >= 0 &&
-            (node.level - currentElevationLevel) >= maxDeltaElevationLevel) {
-            return false;
+        const currentTexture = node.material.textures[0][0];


use l_color instead of 0

gchoqueux · 2018-06-28T09:03:56Z

src/Process/GlobeTileProcessing.js

+            const offsetScale = node.material.offsetScale[0][0];
+            const ratio = offsetScale.z;
+            // ratio is node size / texture size
+            if (ratio < 1 / Math.pow(2, maxDeltaElevationLevel)) {


it's a ratio, between the tile's extent and texture's extent?

gchoqueux · 2018-06-28T09:14:05Z

src/Process/PlanarTileProcessing.js

-            currentElevationLevel >= 0 &&
-            (node.level - currentElevationLevel) >= maxDeltaElevationLevel) {
-            return false;
+        const currentTexture = node.material.textures[0][0];


this part of this process is identical to the globe processing, it should be encapsulated in one place. Duplicate code is harder and longer to maintain

Duplicate code is harder and longer to maintain

This is not an absolute truth, and here I disagree. This is a 5 line copy-paste so I don't think it hurts to keep them separate.

If you really want to improve code sharing between part of itowns I opened a PR sometimes ago: #674

the duplicate code is technical debt

Duplicate code is harder and longer to maintain

Abusive factorization too, by introducing another layer of abstraction.

the duplicate code is technical debt

The program without technical debt is the one with 0 codes. I agree duplicate code has a cost, but introducing another function has one too. I call it more or less even here. But as it is not the scope of this PR and as this snippet is quite short, maybe you can let this one go?

This function can be factorized:

as an OGC conversion tool. there is WMTS_WGS84Parent. Why not have the opposite function?

it's an identified conversion tool, so no code duplication

Actually this code is a hack to workaround a bug in the SSE logic (= computing the geometric error using a sphere - except that a sphere is a poor approximation for a square tile).

So I'd rather get rid of it, instead of transforming it in a API that would be used in other places of iTowns.

gchoqueux · 2018-06-28T09:15:44Z

src/Process/SubdivisionControl.js

@@ -28,7 +28,7 @@ export default {
            if (node.layerUpdateState[c.id] && node.layerUpdateState[c.id].inError()) {
                continue;
            }
-            if (c.tileInsideLimit(node, c) && !node.isColorLayerLoaded(c.id)) {
+            if (c.tileInsideLimit(node, c) && !node.isColorLayerLoaded(c)) {


if isElevationLayerLoaded is material function, why isColorLayerLoaded is tileMesh function?

Mainly because this PR is a part of a bigger PR. I tried to extract a meaningful part of it, but there are left over like this one that doesn't make sense.

I'm moving isColorLayerLoaded to LayeredMaterial.

gchoqueux · 2018-06-28T09:18:04Z

src/Provider/TileProvider.js

@@ -80,8 +80,7 @@ function executeCommand(command) {
    };

    geometry._count++;
-    const tile = new TileMesh(geometry, params);
-    tile.layer = layer;
+    const tile = new TileMesh(layer, geometry, params);


same question, you remember where tile.layer is used?

Answered above.

gchoqueux · 2018-06-28T09:35:03Z

src/Renderer/LayeredMaterial.js

-            } else {
-                this.setLayerVisibility(index, false);
-                break;
+LayeredMaterial.prototype.setLayerTextures = function setLayerTextures(layer, textures) {


you saw @mbredif's proposal for the LayeredMaterial

gchoqueux · 2018-06-28T09:43:13Z

src/utils/DEMUtils.js

@@ -371,7 +371,7 @@ function _readZ(layer, method, coord, nodes, cache) {
    }

    const tile = tileWithValidElevationTexture;
-    const src = tileWithValidElevationTexture.getLayerTextures(l_ELEVATION)[0];
+    const src = tileWithValidElevationTexture.material.textures[l_ELEVATION][0];


and with the famous tile.layer?
you could write:
tileWithValidElevationTexture.getLayerTextures(tile.layer)[0]

peppsac · 2018-06-29T07:29:53Z

Updated.

peppsac · 2018-07-02T09:34:33Z

@gchoqueux could you update your review please?

peppsac · 2018-07-05T13:55:27Z

Ok, so IMHO this PR improves greatly itowns and will allow to reuse the texture update process for other types of data.

@gchoqueux: I don't think your Request Changes on this PR is justified.

gchoqueux · 2018-07-09T16:55:58Z

Ok, so IMHO this PR improves greatly itowns and will allow to reuse the texture update process for other types of data

yes, but you also need the missing tests of impacted objects

peppsac · 2018-07-10T07:08:39Z

yes, but you also need the missing tests of impacted objects

Most of them already have some tests or are pretty well exercised during with the test-examples target. So unless you have a specific test case in mind, I won't add any.

Stop blocking PRs for random reasons: being a reviewer is a responsability, you can't just click Request Changes without valid concerns about the code being merged.

peppsac · 2018-07-10T15:05:52Z

Rebased.

Moving forward

zarov

I see a lot of good things here, thanks for this !

zarov · 2018-07-12T13:23:55Z

examples/split.html

@@ -92,11 +92,8 @@
                globeView.scene.traverse(function _(obj) {
                    if (obj.material && obj.material.setLayerVisibility && obj.material.visible) {
                        material = obj.material;
-                        orthoIndex = material.indexOfColorLayer(orthoLayer.id);


You can remove their declaration above.

zarov · 2018-07-12T13:43:17Z

src/Core/TileMesh.js

@@ -37,6 +39,7 @@ function TileMesh(geometry, params) {
    this.frustumCulled = false;

    this.updateGeometricError();
+    this.wmtsCoords = {};


Why not rename it to a more appropriate name in the same time ?

zarov · 2018-07-12T13:45:40Z

src/Renderer/LayeredMaterial.js

            }
+        } else if (textures.texture !== null) {
+            this._setTexture(textures.texture,
+                layer.type == 'color' ? l_COLOR : l_ELEVATION, (slotOffset || 0), textures.pitch);


Condition unnecessary, result is always l_COLOR.

zarov · 2018-07-13T08:31:06Z

src/Core/TileMesh.js

@@ -32,11 +28,10 @@ function TileMesh(geometry, params) {
    this.boundingSphere = new THREE.Sphere();
    this.OBB().box3D.getBoundingSphere(this.boundingSphere);

-    this.material = new LayeredMaterial(params.materialOptions);


zarov · 2018-07-13T08:34:59Z

src/Provider/TileProvider.js

    geometry._count++;
-    const tile = new TileMesh(geometry, params);
-    tile.layer = layer;
+    const material = new LayeredMaterial(layer.materialOptions);


Could you add somewhere a documentation to what is materialOptions ? Even if it is a quick comment, so we do no forget when we'll generate a better documentation on layers.

Ok. Note that these options are a workaround to limitations in the current code.

Hopefully these limitations will be removed soon and so will the options :)

zarov · 2018-07-13T08:40:52Z

src/Renderer/LayeredMaterial.js

-LayeredMaterial.prototype.getLayerTextures = function getLayerTextures(layerType, layerId) {
-    if (layerType === l_ELEVATION) {
+LayeredMaterial.prototype.getLayerTextures = function getLayerTextures(layer) {
+    if (layer.type === 'elevation') {


Side note: shouldn't it be more practical to change the constants in LayeredMaterialConstants to elevation, color and all ?

IMHO, LayeredMaterialConstants should be removed soon because that should be an implementation details.

But we'll also need to have valid layer types as constants somewhere (elevation, color, etc).

peppsac · 2018-07-13T09:04:30Z

Why not rename it to a more appropriate name in the same time ?

I can't answer inline for this one so I'll reply here: the PR #785 tackles this part, so I limited myself to moving code around.

peppsac · 2018-07-13T09:05:55Z

@zarov PR updated.

zarov · 2018-07-13T12:32:12Z

Ok this is looking good to me. One last thing: do you have a suggestion for removing the tile.layer that I don't feel too is really useful (apart from the case you quote) ?

peppsac · 2018-07-13T12:34:02Z

do you have a suggestion for removing the tile.layer that I don't feel too is really useful (apart from the case you quote) ?

Not really, and I feel it's useful (another use case: in the picking code where we need to filter if an mesh belongs to the requested layer).

zarov

Ok good to go then, squash your last commit please.

Currently the 'level' concept from WMTS has leaked everywhere in iTowns codebase. This commits replace its usage in one part of iTowns, by something equivalent but that doesn't force texture Providers to mock WMTS level mechanism.

This commits is a step forward stopping to rely on LayeredMaterial internal representation of textures (using an index and such) in other parts of iTowns. This will allow to reuse all this code for objects that don't use a LayerMaterial instance as their material.

peppsac force-pushed the refacto_limit_dependency_on_layeredmaterial branch from ed80474 to 9bb0578 Compare June 26, 2018 13:16

mbredif reviewed Jun 26, 2018

View reviewed changes

gchoqueux previously requested changes Jun 28, 2018

View reviewed changes

peppsac force-pushed the refacto_limit_dependency_on_layeredmaterial branch from 5176049 to 906720d Compare July 10, 2018 15:05

peppsac force-pushed the refacto_limit_dependency_on_layeredmaterial branch from 906720d to 2fb058e Compare July 12, 2018 09:30

zarov reviewed Jul 13, 2018

View reviewed changes

zarov approved these changes Jul 13, 2018

View reviewed changes

peppsac added 2 commits July 13, 2018 14:42

peppsac force-pushed the refacto_limit_dependency_on_layeredmaterial branch from a460af0 to 182ba92 Compare July 13, 2018 12:42

peppsac merged commit d40385f into master Jul 13, 2018

peppsac deleted the refacto_limit_dependency_on_layeredmaterial branch July 13, 2018 13:09

AnthonyGlt mentioned this pull request Mar 29, 2024

Terrain tiles not dividing if not covered by elevation data #2159

Open

Refacto limit dependency on layeredmaterial #792

Refacto limit dependency on layeredmaterial #792

Conversation

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbredif Jun 26, 2018 • edited Loading

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

peppsac Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

mbredif Jun 26, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peppsac commented Jun 27, 2018

gchoqueux 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peppsac commented Jun 29, 2018

peppsac commented Jul 2, 2018

peppsac commented Jul 5, 2018

gchoqueux commented Jul 9, 2018

peppsac commented Jul 10, 2018

peppsac commented Jul 10, 2018

zarov 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

mbredif Jun 26, 2018 •

edited

Loading

peppsac Jun 29, 2018 •

edited

Loading

mbredif Jun 26, 2018 •

edited

Loading