refactor: move Layer folder, extract geometric layers from views #815

zarov · 2018-07-23T09:15:59Z

This is a PR similar to #794 but more advanced. It is still missing documentation and test at the moment, but we can discuss the main goal of the PR while I add it.

Description

Several things have been done here:

move the content of src/Core/Layer to src/Layer
create several class for layers in this folder: GeometryLayer, TiledGeometryLayer, ColorLayer and ElevationLayer
from each type of view in src/Core/Prefab, remove the createXXXLayer in profit of a XXXLayer file

Motivation and Context

I feel that layers are one of the main aspect in itowns, and refactoring them like this can help us see clearer through it and help refactoring things around them (like #798 and #807).

peppsac · 2018-07-23T09:28:53Z

If we move away from layers being data-container + attached (external) functions to layers being classes, then we should also reconsider the existence of the process code.

For instance, all the code from GlobeTileProcessing is now only meant to be used from GlobeLayer so IMHO, it would make more sense to remove GlobeTileProcessing and move all of its code in GlobeLayer.

Other questions:

why GlobeLayer exists but not $3dtilesLayer or PointCloudLayer?
why not removing completely the view.addLayer({ type: 'color' }) API, and only allow the view.addLayer(new ColorLayer(...)) form?

zarov · 2018-07-23T09:38:52Z

For instance, all the code from GlobeTileProcessing is now only meant to be used from GlobeLayer so IMHO, it would make more sense to remove GlobeTileProcessing and move all of its code in GlobeLayer.

Completely agree, all layers should have their own update method. I didn't do it here to not clutter the PR.

why GlobeLayer exists but not $3dtilesLayer or PointCloudLayer?

It could, and it should, but those layers are for now more present in examples. But we could take the opportunity to create them here.

why not removing completely the view.addLayer({ type: 'color' }) API, and only allow the view.addLayer(new ColorLayer(...)) form?

I don't have an opinion here, I'm okay to leave it or remove it. It could be at least deprecated in this PR.

peppsac · 2018-07-23T12:23:48Z

I don't have any opinion on the proposed changes, so I'll keep asking questions :)

Is there a point to keep PlanarView, GlobeView, PanoramaView? Can't you move all the view type specific code to each Layer class?

It could, and it should, but those layers are for now more present in examples. But we could take the opportunity to create them here.

I'm not sure that theses layers would make sense actually.
Can you show the code to display a 3dtiles point cloud using itowns using both this PR and a source concept (the one from #798 or similar)? what about a pointcloud from PotreeConverter?

zarov · 2018-07-24T09:26:05Z

Is there a point to keep PlanarView, GlobeView, PanoramaView? Can't you move all the view type specific code to each Layer class?

There is some code that we could move from views to layers, but I don't really see the point of dropping views. To me, views are the basis of any project using itowns. Or do you meant we could only keep View and only use that ?

I'm not sure that theses layers would make sense actually.

OK, so maybe a 3dtiles layer would be added as a GeometryLayer/TiledGeometryLayer, and then the provider(/source ?) would change the specific update stuff ?

gchoqueux

the specific globe's part in GlobeTileProcessing is the horizonCulling, this part could be moved in globeLayer. We must strive to unify the other parts of the GlobeTileProcessing (if possible and justified)

Do you meant we could only keep View and only use that ?

Yes, it would be necessary to have only View, the different things are the controls / Camera and the fog / atmosphere (atmosphere should be a layer)

gchoqueux · 2018-07-26T11:41:07Z

src/Core/Layer/Layer.js

@@ -1,199 +0,0 @@
-import { EventDispatcher } from 'three';


move Layer.js in other PR to see history change

gchoqueux · 2018-07-26T11:43:05Z

src/Core/Prefab/Globe/GlobeLayer.js

+        this.builder = new BuilderEllipsoidTile();
+    }
+
+    pickObjectsAt(view, mouse, radius = 5) {


why he doesn't inherit from geometryLayer or TiledGeometryLayer.js?

Because of the default radius value which isn't the same, and I don't know a way of specifying this differently.

gchoqueux · 2018-07-26T11:51:40Z

src/Core/Prefab/Globe/GlobeLayer.js

+            this._latestUpdateStartingLevel = 0;
+        }
+
+        preGlobeUpdate(context, this);


Why globeLayer.preUpdate should be identical to planarLayer.preUpdate or panoramaLayer.preUpdate, exepted for preGlobeUpdate, prePlanarUpdate

gchoqueux · 2018-07-26T11:58:17Z

src/Layer/Layer.js

+
+        Object.assign(this, config);
+
+        Object.defineProperty(this, 'id', {


we could you use class's getter and setter?

Only using a getter would do the same job indeed, but does not emit an error: what is your take here ?

gchoqueux · 2018-07-26T12:01:58Z

src/Layer/Layer.js

+    //      * the minimum sequence will always be 0
+    //      * the maximum sequence will always be layers.lenght - 1
+    // the ordering of all layers (Except that specified) doesn't change
+    moveLayerToIndex: function moveLayerToIndex(layer, newIndex, imageryLayers) {


maybe this should be in geometryLayer because the color and elevation layers are only attached to geometric layers : geometryLayer.moveColorLayerToIndex(layer, index)?

This kind of makes sense, but this method is used in ColorLayersOrdering too; how do you see the integration with this one, very specific to color layers ?

zarov · 2018-08-07T15:05:51Z

PR updated, I moved the pickObjectsAt and added documentation.

Note that it is getting quite unbrowseable in the jsdoc page, so it may be interesting to regroup classes by theme (here we could have Layers for example) and only display their name and not the method names. But this is the matter of another PR.

gchoqueux · 2018-08-08T13:47:42Z

src/Core/Layer/Layer.js

+        this.options = config.options || {};
+
+        // Does this layer is attached to another one
+        this.isAttached = false;


it's necessary, because if (this.attachedLayers.length === 0) so this.isAttached === false

gchoqueux · 2018-08-08T13:52:07Z

src/Core/Layer/Layer.js

+        // Does this layer is attached to another one
+        this.isAttached = false;
+
+        if (!this.updateStrategy) {


Note
It's necessary to think where to place the strategy of loading.
The layer is a good place because it must independent of the source.

gchoqueux · 2018-08-08T14:01:44Z

src/Core/Layer/Layer.js

+        });
+
+        // Default properties
+        this.options = config.options || {};


It would be necessary, to write _ before private property name.
Could you tell me the properties that make sense to be accessible?

by example this.isAttached is not supposed to be modifiable by the user

So this line comes from View, so I kinda let it as it is. For private property name, I'm okay to write it with _, but I don't know a way to keep a property modifiable by us but not by the user.

gchoqueux · 2018-08-08T14:21:38Z

src/Core/Prefab/Globe/GlobeLayer.js

+
+        function subdivision(context, layer, node) {
+            if (TiledGeometryLayer.hasEnoughTexturesToSubdivide(context, node)) {
+                return globeSubdivisionControl(2,


Could you standardize the signature of globeSubdivisionControl, panoramaSubdivisionControl and planarSubdivisionControl, because these functions are pretty much the same

Could you move subdivision and this.update declaration to TiledGeometryLayer

pretty much yes, but the panorama one is kinda annoying... I'll see what I can do here

I don't agree with this, I'd prefer to transfer the called methods (from GlobeTiledProcessing for example) in layers in the PR following this one.

gchoqueux · 2018-08-08T14:23:26Z

src/Core/Prefab/Panorama/PanoramaLayer.js

+
+        this.update = processTiledGeometryNode(panoramaCulling, subdivision);
+        this.builder = new PanoramaTileBuilder(type, config.ratio || 1);
+        this.segments = 8;


move to this.options

gchoqueux · 2018-08-08T14:41:32Z

src/Core/View.js

-        defineLayerProperty(layer, 'visible', true, () => _syncGeometryLayerVisibility(layer, view));
-        defineLayerProperty(layer, 'frozen', false);
+    if (layer.type == 'geometry' || layer.type == 'debug') {
+        layer.defineLayerProperty('visible', true, () => _syncGeometryLayerVisibility(layer, view));


It's a shame that (layer.defineLayerProperty) this remains
It should use the notifyChange mechanism

How could I use it ?

gchoqueux · 2018-08-08T14:45:25Z

src/Layer/GeometryLayer.js

+     * @return {Array} An array containing all targets picked under the
+     * specified coordinates.
+     */
+    pickObjectsAt(view, coordinates, radius) {


why radius parameter don't use default value?
pickObjectsAt(view, coordinates, radius = this.config.defaultRadius)

Thanks, didn't think of it this way !

gchoqueux · 2018-08-08T14:49:07Z

src/Layer/TiledGeometryLayer.js

+     * @return {Array} An array containing all targets picked under the
+     * specified coordinates.
+     */
+    pickObjectsAt(view, coordinates, radius) {


you can declare in class geometryLayer

pickObjectsAt(view, coordinates, radius = this.defaultPickingRadius) { return Picking.pickTilesAt(view, coordinates, radius, this); }

and remove pickObjectsAt in TiledGeometryLayer

But TiledGeometryLayer uses pickTilesAt and GeometryLayer uses pickObjectsAt.

gchoqueux · 2018-08-08T14:51:37Z

src/Process/GlobeTileProcessing.js

@@ -42,15 +42,6 @@ export function preGlobeUpdate(context, layer) {

    // pre-sse
    context.camera.preSSE = _preSSE(context.view);
-


what is this deleted or moved part for?

gchoqueux · 2018-08-08T14:56:41Z

src/Process/PlanarTileProcessing.js

@@ -24,17 +24,6 @@ function _isTileBigOnScreen(camera, node) {
    return (dim.x >= 256 && dim.y >= 256);
 }

-export function prePlanarUpdate(context, layer) {


same part that here
There's the same preUpdate in globePreUpdate ?
It's used for elevation with color data , can use color data for elevation in the globeLayer?

It is the same indeed, so I moved it to TiledGeometryLayer.

Move all contents of `createXXXLayer` in XXXView to XXXLayer in Prefab/XXX/. This declutters the XXXView files. This change applies for Globe, Planar and Panorama. In the same time, GeometryLayer has been extract from Layer and given some new methods which were repeated through each View. TiledGeometryLayer, ColorLayer and ElevationLayer has been created as well, in the same spirit. All code present in src/Core/Layer has been moved to src/Layer.

gchoqueux · 2018-08-09T16:14:13Z

good job

zarov added wip 🚧 Still being worked on to validate labels Jul 23, 2018

zarov changed the title ~~refactor: move geometric layers off views~~ refactor: move Layer folder, extract geometric layers from views Jul 23, 2018

gchoqueux reviewed Jul 26, 2018

View reviewed changes

zarov force-pushed the refactor_layers branch from 3f5469c to 2559684 Compare August 7, 2018 15:02

zarov force-pushed the refactor_layers branch from 1ca9f1d to ad56118 Compare August 8, 2018 08:31

gchoqueux added validated 👌 validated by PSC and removed to validate labels Aug 8, 2018

gchoqueux requested changes Aug 8, 2018

View reviewed changes

zarov force-pushed the refactor_layers branch from 244cf6f to 76d784e Compare August 9, 2018 15:37

gchoqueux approved these changes Aug 9, 2018

View reviewed changes

gchoqueux merged commit 10decfd into iTowns:master Aug 9, 2018

zarov deleted the refactor_layers branch August 29, 2018 08:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move Layer folder, extract geometric layers from views #815

refactor: move Layer folder, extract geometric layers from views #815

zarov commented Jul 23, 2018

peppsac commented Jul 23, 2018

zarov commented Jul 23, 2018

peppsac commented Jul 23, 2018

zarov commented Jul 24, 2018

gchoqueux left a comment

gchoqueux Jul 26, 2018

zarov Jul 27, 2018

gchoqueux Jul 26, 2018

zarov Jul 27, 2018

gchoqueux Jul 26, 2018

zarov Jul 27, 2018

gchoqueux Jul 26, 2018

zarov Jul 27, 2018

gchoqueux Jul 26, 2018

zarov Jul 27, 2018

zarov commented Aug 7, 2018

gchoqueux Aug 8, 2018

gchoqueux Aug 8, 2018

gchoqueux Aug 8, 2018

zarov Aug 9, 2018

gchoqueux Aug 8, 2018

zarov Aug 9, 2018

gchoqueux Aug 8, 2018

zarov Aug 9, 2018

gchoqueux Aug 8, 2018

zarov Aug 9, 2018

gchoqueux Aug 8, 2018

zarov Aug 9, 2018

gchoqueux Aug 8, 2018

zarov Aug 9, 2018

gchoqueux Aug 8, 2018

gchoqueux Aug 8, 2018

zarov Aug 9, 2018

gchoqueux commented Aug 9, 2018


		Object.assign(this, config);

		Object.defineProperty(this, 'id', {

		@@ -42,15 +42,6 @@ export function preGlobeUpdate(context, layer) {

		// pre-sse
		context.camera.preSSE = _preSSE(context.view);

refactor: move Layer folder, extract geometric layers from views #815

refactor: move Layer folder, extract geometric layers from views #815

Conversation

zarov commented Jul 23, 2018

Description

Motivation and Context

peppsac commented Jul 23, 2018

zarov commented Jul 23, 2018

peppsac commented Jul 23, 2018

zarov commented Jul 24, 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

zarov commented Aug 7, 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

gchoqueux commented Aug 9, 2018