Native MGL integration #886

Jesus89 · 2018-08-22T14:13:38Z

This branch adds a re-implementation with some refactors for the MGL integration interface using the new Custom Layers API. Since this feature is not already in a stable version I have released from our fork the version v0.48.0-carto1 (branch), from the stable v0.48.0 tag and the changes from custom-layers.

This is a pretty big PR. However, the main activity is in the files Layer, Renderer and Map. Here is a list of changes in this branch:

Use onAdd, onRemove, prerender and render functions of Custom Layers
Implement Layer.removeFrom
Refactor maps/Map to match the new interface
Allow layer.update(source) using current viz by default
Get zoom and center from the matrix
Request data when matrix changes
Remove integrators. Instead of a set of integrators, keep a set of renderers associated to each map.
Clean unused functions and attributes in class Layer
Minor fixes in the test framework
Use local basemaps for testing
Update examples to the new MGL version
Fix animation example
Fix interactivity examples

If we are interested in testing MGL upstream I can release another version (v0.48.0-carto2) pulling the changes from master.

…tegrator

davidmanzanares

Good work! But, pls, try to avoid huge PR like this one, at least some changes were outside the scope of the PR like the movement of some docs. Thx!

davidmanzanares · 2018-08-23T09:58:55Z

examples/basics/add-layer.html

@@ -37,10 +37,7 @@ <h1>Add layer</h1>
      container: 'map',
      style: 'https://basemaps.cartocdn.com/gl/voyager-gl-style/style.json',
      center: [0, 30],
-      zoom: 2,
-      scrollZoom: false,


Just for testing. I'll restore it

davidmanzanares · 2018-08-23T10:08:39Z

src/Layer.js

-        });
-
+        this.id = id;
+        this.type = 'custom';


Why is this added?

The idea is to convert carto.Layer in a native MGL CustomLayer. I'll add a comment there

davidmanzanares · 2018-08-23T10:12:24Z

src/Layer.js

@@ -138,7 +115,7 @@ export default class Layer {
    }

    /**
-     * Add this layer to a map.
+     * Add this layer to a map. It waits for the map to be loaded.


I would change the wording

Wait for the map to load?

I would say something like: If the map's style is not loaded yet it will wait for it to add the layer as soon as possible

davidmanzanares · 2018-08-23T10:14:06Z

src/Layer.js

+     * @api
+     */
+    removeFrom (map) {
+        map.removeLayer(this);


This will throw an error if the map hasn't been loaded yet and the layer hasn't been added yet

Yes and MGL this is the expected behaviour IMO.

Yes, but since we are making things easier in addTo for a similar problem, we could improve this, but I realize that this feature request is not simple, I think we could create an issue with it, are you ok?

davidmanzanares · 2018-08-23T10:16:02Z

src/Layer.js

     * @api
     */
    async update (source, viz) {
+        if (viz === undefined) {
+            // Use current viz
+            viz = this._viz;


I don't remember well but I think this wasn't done before for a reason, this should be tested well

I found an example using layer.getViz() as a second argument, so this could be the default behaviour. I'll add tests for this.

davidmanzanares · 2018-08-23T10:33:36Z

src/renderer/Renderer.js

+        const b = map.getBounds();
+        const nw = b.getNorthWest();
+        const sw = b.getSouthWest();
+        const z = (util.projectToWebMercator(nw).y - util.projectToWebMercator(sw).y) / util.WM_2R;


I don't think this is a good idea either, the Renderer didn't know about webmercator or maps at all before

Yes, I have decoupled the map so this functions are not in the Renderer anymore

davidmanzanares · 2018-08-23T10:33:43Z

src/renderer/Renderer.js

-        this._zoom = zoom;
+    _initCenter (map) {
+        const c = map.getCenter();
+        this.setCenter(c.lng / 180.0, util.projectToWebMercator(c).y / util.WM_R);


#886 (comment)

davidmanzanares · 2018-08-23T10:34:47Z

src/utils/util.js

+    if (m1 && m2 && m1.length === m2.length) {
+        let ret = true;
+        for (let i = 0; i < m1.length; i++) {
+            ret &= m1[i] === m2[i];


this would be simpler and faster without the ret variable, early returning a false when the values mismatch

Yes, I think I changed it already 🤔

davidmanzanares · 2018-08-23T10:35:43Z

test/acceptance/e2e/cluster-aggr/scenario.js

@@ -1,8 +1,6 @@
 const map = new carto.Map({
    container: 'map',
-    background: 'black',
-    center: [-75, 40],


this was set to avoid draw ordering issues

But this test uses carto.Map that has no center, zoom attributes...

ah, true, yes, I probably copy pasted them from the example I was thinking about

davidmanzanares · 2018-08-23T10:38:41Z

test/integration/util.js

@@ -57,6 +57,6 @@ function project (coordinates) {
    const wm = projectToWebMercator(coordinates);
    return {
        x: mapSize * (0.5 + wm.x / WM_2R),
-        y: mapSize * (0.5 - wm.y / WM_2R + 0.03 /* offset */)
+        y: mapSize * (0.5 - wm.y / WM_2R)


Could you explain this since you introduced the magic 0.03? Why was it added? The addition was done on: 7b46dee

This changed was required to match the bounding box in the simulated mouse events. I have updated the bounding box in the carto.Map so this is not required anymore.

I don't really understand it, but if you are sure... 👍

Jesus89 · 2018-08-23T15:16:08Z

Suggestions and more improvements added.

davidmanzanares

I think this is ready to be merged, but I also think this is the most dangerous PR of this release cycle, please wait for the merging of the other PRs to leave this PR as the last PR of the new release

elenatorro

Minor code style issues

elenatorro · 2018-08-24T07:17:32Z

src/Layer.js

- * @type {LayerEvent}
- * @api
-*/
+function getRenderer (map, gl) {


This is a private function that should be placed at the bottom (after the class) and should be _getRenderer instead of getRenderer.

elenatorro · 2018-08-24T07:22:51Z

src/Layer.js

+            // TODO: the best solution is to use the matrix at the shader
+            // level and remove the aspect and scale logic from the renderer
+            this.renderer.setCenter(
+                -(1 + 2 * matrix[12] / matrix[0]),


Could we move this center logic to an external utils file or similar?

elenatorro · 2018-08-24T07:23:15Z

src/Layer.js

+
+    _setRendererCenter () {
+        const c = this.map.getCenter();
+        this.renderer.setCenter(c.lng / 180.0, util.projectToWebMercator(c).y / util.WM_R);


Use constant instead of a number (180.0)

This is legacy code. It can be done with projectToWebMercator(c).x, etc but this is shorter. However, I'll move this to an utils file

Jesus89 added 23 commits August 21, 2018 11:17

Update MGL to 0.48.0-carto1

a0b1cf3

Update MGL version in the examples

122a2a3

Implement new CustomLayers interface. Remove MGL integrator

fb0b376

Simplify Layer.addTo function

ad29f3e

Refactor internal Map to match the new interface. Remove carto map in…

3110b6b

…tegrator

Clean class Layer. Add removeFrom function

973a19e

Add unit tests

81a501c

Rename apiUtils to events

33f07e7

Update addTo function to wait for the map to be loaded

6cfca5f

Fix render tests

88f6d83

Fix user tests

2b71ecc

Add local basemaps for e2e testing

cc972ce

Fix e2e tests

742bdac

Update example

42c2dd1

Get zoom and center from the render matrix

645ce3b

Create fake matrix for internal Map

3cd4011

Use one renderer per map

1778614

Refactor Layer to include CustomLayer interface functions

ef69d97

Restore add-layer example

3cc98b8

Refactor interactivity subscription functions

362f23e

Fix animation examples headers

6699ca2

Fix interactivity examples

716be56

Request data when matrix changes

056f191

Jesus89 mentioned this pull request Aug 22, 2018

Add layer.removeFromMap() method #428

Closed

Jesus89 added the status: do-not-merge DO NOT MERGE... label Aug 22, 2018

Jesus89 added 3 commits August 23, 2018 11:21

Remove unused events

55b0720

Fix non animated layers

4236a69

Clean comments

60509c3

Jesus89 mentioned this pull request Aug 23, 2018

Fix MGL integration inconsistencies #847

Closed

davidmanzanares reviewed Aug 23, 2018

View reviewed changes

Jesus89 added 4 commits August 23, 2018 13:07

Apply CR suggestions

dec19cb

Decouple the map from the Renderer

8cad3d8

Update layer.addTo docs

1090c94

Improve equalArrays function

a1de080

Jesus89 force-pushed the mgl-integration branch from 4e1186c to a1de080 Compare August 23, 2018 12:36

Jesus89 added 4 commits August 23, 2018 16:17

Improve editor example

a230897

Add RenderLayer set functions

dc70190

Set zoom center in requireData using the matrix if provided

c1d4151

Add comment in render

1ebc537

davidmanzanares approved these changes Aug 23, 2018

View reviewed changes

elenatorro self-requested a review August 24, 2018 07:13

elenatorro suggested changes Aug 24, 2018

View reviewed changes

Jesus89 added 3 commits August 24, 2018 09:56

Add CR suggestion

012327e

Refactor setZoomCenter function in Layer

5698911

Move computeZoom/Center functions to util

71ac5e0

elenatorro approved these changes Aug 24, 2018

View reviewed changes

Jesus89 removed the status: do-not-merge DO NOT MERGE... label Aug 24, 2018

Jesus89 added 2 commits August 24, 2018 10:41

Update CHANGELOG

22381cb

Merge branch 'master' into mgl-integration

41869c4

Jesus89 force-pushed the mgl-integration branch from 093fbdb to 41869c4 Compare August 24, 2018 10:28

Jesus89 added 2 commits August 24, 2018 12:32

Update layer.remove() function. Add render test

f2f7b08

Use prerender function to request data

edaef0b

Jesus89 force-pushed the mgl-integration branch from 91a4c57 to edaef0b Compare August 24, 2018 12:39

Jesus89 merged commit 2a13c0b into master Aug 24, 2018

Jesus89 deleted the mgl-integration branch August 24, 2018 12:46

Jesus89 mentioned this pull request Aug 24, 2018

Mapbox GL Custom WebGL Layers API integration #700

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Native MGL integration #886

Native MGL integration #886

Jesus89 commented Aug 22, 2018 •

edited

Loading

davidmanzanares left a comment

davidmanzanares Aug 23, 2018

Jesus89 Aug 23, 2018

davidmanzanares Aug 23, 2018

Jesus89 Aug 23, 2018

davidmanzanares Aug 23, 2018

Jesus89 Aug 23, 2018

davidmanzanares Aug 23, 2018

davidmanzanares Aug 23, 2018

Jesus89 Aug 23, 2018

davidmanzanares Aug 23, 2018

davidmanzanares Aug 23, 2018

Jesus89 Aug 23, 2018

davidmanzanares Aug 23, 2018

Jesus89 Aug 23, 2018

davidmanzanares Aug 23, 2018

Jesus89 Aug 23, 2018

davidmanzanares Aug 23, 2018

Jesus89 Aug 23, 2018

davidmanzanares Aug 23, 2018

Jesus89 Aug 23, 2018

davidmanzanares Aug 23, 2018

davidmanzanares Aug 23, 2018

Jesus89 Aug 23, 2018

davidmanzanares Aug 23, 2018

Jesus89 commented Aug 23, 2018

davidmanzanares left a comment

elenatorro left a comment

elenatorro Aug 24, 2018

elenatorro Aug 24, 2018

elenatorro Aug 24, 2018

Jesus89 Aug 24, 2018

Native MGL integration #886

Native MGL integration #886

Conversation

Jesus89 commented Aug 22, 2018 • edited Loading

davidmanzanares 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

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

Jesus89 commented Aug 23, 2018

davidmanzanares left a comment

Choose a reason for hiding this comment

elenatorro 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

Jesus89 commented Aug 22, 2018 •

edited

Loading