Refacto provider and add Sources by gchoqueux · Pull Request #798 · iTowns/itowns

gchoqueux · 2018-06-29T17:18:12Z

Description

add Source in layer : all the parameters to get the data from a source
add convert in layer : fonction to convert data from source to layer's elements

The layer does

const layer = {
  id, 
  type,
  source,
  convert,
}

The provider does

function executeCommand() {
      const extent = command.extent; //  the datas's extent  to fetch
      const dataFetchedFromSource = await fetcher(source.urlFromExtent(extent));
      const dataParsed = await parse(dataFetchedFromSource);
      const layerElement = convert(dataParsed);
      return layerElement;
}

Motivation

Isolate sources and converters/parsers from providers and use them for other layers
simplify providers and merge certain providers (WMTS, WMS, Rasterizer), see delete them

mbredif

It looks better than the current state IMHO.
Some remarks after a quick review :

layer.source || layer looks odd, is it temporary ? can't we get rid of it ?
converts and parsers are 2 different things (eg: WMTS source -> Geojson parser -> Mesh converter)

I however feel that tiledCRS version of extents should be an implementation detail of quadtree-based sources and that other parts of itowns should only handle extents as a (BBox3 + CRS).

Likewise instead of relative pitches / offsetscales, we could use absolute coordinates (BBox2 + CRS) for textures so that downscaling a texture would be a noop and uvs in the shader would be well defined coordinates with a proper crs instead of normalized coordinates.

mbredif · 2018-06-29T21:36:13Z

src/Core/TileMesh.js

@@ -205,8 +204,9 @@ TileMesh.prototype.changeSequenceLayers = function changeSequenceLayers(sequence

 TileMesh.prototype.getCoordsForLayer = function getCoordsForLayer(layer) {
    if (layer.protocol.indexOf('wmts') == 0) {


layer.source.protocol ?

mbredif · 2018-06-29T21:38:49Z

src/Core/View.js


    // probably not the best place to do this
    if (layer.type == 'color') {
+        layer.fx = layer.fx || 0.0;


why not a defineLayerProperty as for other properties?

defineLayerProperty is a bad practice imho, and achieves nothing besides reducing a little the duplicated code, executing a callback in really rare usecases and emitting events on properties change. But I wouldn't change it here, and in the meantime keep this property declaration as it is.

zarov

I think this is a good idea. The introduction of a Source object (kind of like OpenLayers ?) is a good step towards decluttering providers. Could you add a more accurate description (maybe documentation like the current Provider interface ?) to this object ?

However I don't really like a few things:

the fetching in geoFileSource: no operation like should happen in a Source
the convert method in layer: what about your initials thoughts with source, processing, style only ?

zarov · 2018-07-05T13:38:35Z

src/Parser/textureConverter.js

+};
+
+export default {
+    textureColorLayer(texture, extent, layer) {


I wouldn't call this textureColorLayer. Why not have two methods, color and elevation, which gives textureConverter.color and textureConverter.elevation calls.

Also by introducing this you could drop the getColor/XBilTextureByUrl in OGCWebServiceHelper which have nothing to do with OGC.

zarov · 2018-07-05T13:57:05Z

src/Provider/Fetcher.js

@@ -1,14 +1,24 @@
-import { TextureLoader } from 'three';
+import { TextureLoader, DataTexture, AlphaFormat, FloatType, LinearFilter } from 'three';


My opinion: we should always import * as THREE, I don't get the appeal behind selecting what we are importing.

zarov · 2018-07-05T13:58:09Z

src/Provider/Fetcher.js

-            checkResponse(response);
-            return response.arrayBuffer();
+    arrayBuffer,
+    textureFloat(url, options = {}) {


This part should be in the elevationTexture part imho. Let's keep the Fetcher simple.

zarov · 2018-07-05T14:02:27Z

src/Core/Scheduler/Scheduler.js

-    if (typeof (provider.preprocessDataLayer) !== 'function') {
-        throw new Error(`Can't add provider for ${protocol}: missing a preprocessDataLayer function.`);
-    }
+    // if (typeof (provider.preprocessDataLayer) !== 'function') {


zarov · 2018-07-05T14:10:54Z

src/Core/View.js

+
+    // temp
+    // devrait etre supprime et placer dans source
+    if (provider && !layer.source) {


Yep, a provider shouldn't be the one defining the limit.

zarov · 2018-07-05T14:13:29Z

src/Core/View.js

            if (!(providerPreprocessing && providerPreprocessing.then)) {
                providerPreprocessing = Promise.resolve();
            }
+        } else if (layer.source) {


I don't like having this here, as much as I don't like having all other things below (conditions on type of layer). I know though that you hope to move it to the layer itself, so I'm ok with this here for now.

zarov · 2018-07-05T14:18:35Z

src/Core/View.js


    // probably not the best place to do this
    if (layer.type == 'color') {
+        layer.fx = layer.fx || 0.0;


defineLayerProperty is a bad practice imho, and achieves nothing besides reducing a little the duplicated code, executing a callback in really rare usecases and emitting events on properties change. But I wouldn't change it here, and in the meantime keep this property declaration as it is.

peppsac · 2018-07-09T09:04:25Z

I'm all for refactoring this part, and the chosen solution looks similar to what was previously discussed.
(the only downside is that we already have almost 30 open PR and that updating/reviewing/merging others would be more relevant)

Random notes:

IMHO, processing can depend on the data type (points or textures) but should not depend on the source type
provider (or source) implements the protocol specific knowledge and don't necesarly need to do network accesses themselves
a big refactoring must like this one:
- not lose features (obviously)
- bring new features (not necessarly in the same PR) to show its usefulness.

The last point is important. For instance, if this PR doesn't help implement vector-tiles support easily, then something is wrong.

nosy-b · 2018-07-24T14:58:14Z

Sounds like the way to go!

zarov · 2018-08-22T08:24:27Z

I tested the examples, here's what I saw:

cubic_planar: there seems to have artifact on the sides, see those two comparisons screenshots (first actual version, second this PR)
globe_vector: picking is not working anymore
syncCameras: not working, I have a Cannot read property 'protocol' of undefined

gchoqueux · 2018-08-22T10:07:27Z

@zarov all examples are fixed

New change:

root tile are build in constructor
Layer.projection are determined in addLayer

zarov · 2018-08-22T15:36:30Z

src/Source/Source.js

+ */
+class Source {
+    /**
+     * @param  {sourceParams}  source  object to set source


You should remove the @class declaration above, and write a @constructor here, to avoid duplication in the generated page of the documentation.

You should also write an example of declaring a random Source. Best would be to have it for all Sources files.

zarov

Added some comments on the Sources files !

zarov · 2018-08-23T08:08:33Z

src/Source/FileSource.js

+     */
+    constructor(source, crsOut) {
+        super(source);
+        if (!source.url) {


No need to repeat this as it is already been checked in Source.

zarov · 2018-08-23T08:18:50Z

src/Source/FileSource.js

+            throw new Error('source.url is required in FileSource');
+        }
+
+        if (!source.projection) {


You should specify in the documentation above that the projection is mandatory for this type of Source. This check can also be moved before calling super(source).

zarov · 2018-08-23T08:23:57Z

src/Source/Source.js

+     * @param      {Extent}  extent  extent to convert in url
+     * @return     {string}  url from extent
+     */
+    // eslint-disable-next-line


This is a problem I encountered as well, there is three solutions:

eslint-disable-next-line

declaring this method as static: static urlFromExtent(extent), that can still be called with this.urlFromExtent(extent) or source.urlFromExtent(extent)

removing the class-method-use-this eslint check from the configuration

yes, there are errors lint too for jsdoc

zarov · 2018-08-23T08:32:34Z

src/Source/StaticSource.js

+    constructor(source) {
+        super(source);
+
+        if (!source.extent) {


Same as above, you can move this check before calling the parent constructor and specify in the documentation that the extent is required here.

zarov · 2018-08-23T09:01:03Z

src/Source/WFSSource.js

+     * @param {Object} [source.zoom]
+     * @param {number} [source.zoom.min] layer's zoom minimum
+     * @param {number} [source.zoom.max] layer's zoom maximum
+     * @param {string} [source.zoom.max] layer's zoom maximum


Did you mean something else ?

zarov · 2018-08-23T09:01:16Z

src/Source/WFSSource.js

+    */
+    constructor(source) {
+        super(source);
+        if (!source.typeName) {


Same, move the super below

zarov · 2018-08-23T09:02:37Z

src/Source/WMSSource.js

+            }
+        }
+
+        let crsPropName = 'SRS';


Why not directly const crsPropName = (this.version === '1.3.0') ? 'CRS' : 'SRS'; ?

zarov · 2018-08-23T09:04:33Z

src/Source/WMTSSource.js

+
+/**
+ * Options to wtms protocol
+ * @typedef {Object} OptionsWmts


Will it be detailed ?

Add new object source: - Source have the properties to fetch the data to display - Remove all source abstractions from provider

gchoqueux · 2018-08-23T14:14:49Z

@zarov PR updated

zarov

OK let's validate this, I think I didn't see any problem.

For fixing your failing test, either remove it, or install a polyfill for URL (like this one https://github.com/webcomponents/URL) as it is not a component supported in nodejs.

zarov · 2018-08-23T16:07:11Z

It is even occurring because the URL component has only been added in node 10.

gchoqueux added wip 🚧 Still being worked on to validate labels Jun 29, 2018

gchoqueux mentioned this pull request Jul 2, 2018

FileProvider #705

Closed

mbredif reviewed Jul 2, 2018

View reviewed changes

gchoqueux mentioned this pull request Jul 3, 2018

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

Merged

zarov reviewed Jul 5, 2018

View reviewed changes

gchoqueux force-pushed the Source branch from b458129 to f440096 Compare July 6, 2018 11:58

gchoqueux force-pushed the Source branch from 8419149 to e00513b Compare July 9, 2018 13:49

zarov mentioned this pull request Jul 9, 2018

Test on Windows through AppVeyor, and use NPM prepublishOnly rather than prepublish #139

Merged

gchoqueux force-pushed the Source branch 7 times, most recently from 86a4f54 to a69dd40 Compare July 20, 2018 13:31

zarov mentioned this pull request Jul 23, 2018

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

Merged

gchoqueux mentioned this pull request Jul 26, 2018

refactor(core): rework texture update process #807

Closed

gchoqueux force-pushed the Source branch 10 times, most recently from 3506ba2 to 7592921 Compare August 2, 2018 11:14

gchoqueux force-pushed the Source branch 4 times, most recently from 1a22624 to 24adf87 Compare August 21, 2018 17:48

gchoqueux force-pushed the Source branch from 24adf87 to 331d6f5 Compare August 22, 2018 09:59

gchoqueux force-pushed the Source branch 3 times, most recently from d6fcdb4 to dd1006a Compare August 22, 2018 14:26

zarov reviewed Aug 22, 2018

View reviewed changes

gchoqueux force-pushed the Source branch 2 times, most recently from bcb8f07 to 15c1612 Compare August 22, 2018 16:37

zarov reviewed Aug 23, 2018

View reviewed changes

gchoqueux force-pushed the Source branch 2 times, most recently from c8cecb8 to 228116e Compare August 23, 2018 12:26

gchoqueux added 2 commits August 23, 2018 16:11

feat(protocol): add Source

8c51581

Add new object source: - Source have the properties to fetch the data to display - Remove all source abstractions from provider

examples(core): add Source examples

25c6021

gchoqueux force-pushed the Source branch from 228116e to 1d5261a Compare August 23, 2018 14:12

zarov approved these changes Aug 23, 2018

View reviewed changes

gchoqueux force-pushed the Source branch from 1d5261a to 9b81769 Compare August 23, 2018 16:07

gchoqueux added 3 commits August 23, 2018 18:13

test(core): add source unit test

ca0bdeb

test(core): add unit test on providers and sources

30f86d4

test(core): add new source examples

d68ffa6

gchoqueux force-pushed the Source branch from 9b81769 to d68ffa6 Compare August 23, 2018 16:14

gchoqueux merged commit c7fd059 into iTowns:master Aug 23, 2018

gchoqueux deleted the Source branch August 23, 2018 16:35

gchoqueux mentioned this pull request Aug 28, 2018

Plugins ? #529

Closed

		@@ -205,8 +204,9 @@ TileMesh.prototype.changeSequenceLayers = function changeSequenceLayers(sequence

		TileMesh.prototype.getCoordsForLayer = function getCoordsForLayer(layer) {
		if (layer.protocol.indexOf('wmts') == 0) {

		@@ -1,14 +1,24 @@
		import { TextureLoader } from 'three';
		import { TextureLoader, DataTexture, AlphaFormat, FloatType, LinearFilter } from 'three';

Conversation

gchoqueux commented Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation

Uh oh!

mbredif 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!

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!

zarov 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!

peppsac commented Jul 9, 2018

Uh oh!

nosy-b commented Jul 24, 2018

Uh oh!

zarov commented Aug 22, 2018

Uh oh!

gchoqueux commented Aug 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zarov 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!

gchoqueux Aug 23, 2018 • 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!

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!

gchoqueux commented Aug 23, 2018

Uh oh!

zarov left a comment

Choose a reason for hiding this comment

Uh oh!

zarov commented Aug 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

gchoqueux commented Jun 29, 2018 •

edited

Loading

mbredif left a comment •

edited

Loading

gchoqueux Aug 23, 2018 •

edited

Loading