MapBox VectorTile: bug fixes using an official MapBox stream #2469

ftoromanoff · 2024-11-26T15:38:55Z

Using the mapbox style file "mapbox://styles/mapbox/streets-v8" on mapLib and on itowns, I noticed the result as not at all comparable.

Thus I looked layers by layers to understand what were we doing wrong in itowns and tried to correct a maximum of bugs. The example using the mapbox stream has been added. (examples/vector_tile_2d_mapbox.html)

What this PR does:

new feature:
- support relative urls in style file (ArcGIS style file)
- add a warn properties in source to add warnings and send them only ones
itowns bug fix:
- Mapbox stream
  - enter url in {z}/{y}/{x} instead of ${z}/${y}/${x}
  - wrong selection of metadata when "{id}" in sprite
  - gestion of MVT layer using the 'ref' properties
  - gestion of line and polygon Label (simplified -> we use the first point of the geometry and sending a warning)
  - good gestion of the layer.order properties
- Style
  - take into account the zoom in style when displaying layer
  - no display of polygon when fill.color is undefined
  - no display of line when stroke.width is 0
  - no recoloring of icon when icon.color is white (to avoid a lightning of the icon)
  - allow multiple label with same text (for contour lines)
  - no display of icon if size is 0

…udes {}

…exture

…in VT

jailln

Thanks for the debug and the fixes :)

jailln · 2024-11-27T19:51:44Z

examples/vector_tile_raster_2d_mapbox.html

@@ -0,0 +1,98 @@
+<html>
+  <head>
+    <title>Itowns - Globe</title>


Can you update the title please?

jailln · 2024-11-27T19:53:30Z

examples/vector_tile_raster_2d_mapbox.html

+      const source = new itowns.VectorTilesSource({
+        style: "mapbox://styles/mapbox/streets-v8",
+        accessToken:
+          "pk.eyJ1IjoiZ2VvdXJmbyIsImEiOiJjbHdyZWVrd2owOW1rMmlxdmx2Z3Fwb2JhIn0.sg5en26yZ6aSEg6CsfUByA",


is that your personal access token?

jailln · 2024-11-27T19:56:37Z

examples/vector_tile_raster_2d_mapbox.html

+      rel="stylesheet"
+      type="text/css"
+      href="https://www.itowns-project.org/itowns/examples/css/widgets.css"
+    />


we don't need widgets in this example

jailln · 2024-11-27T19:56:42Z

examples/vector_tile_raster_2d_mapbox.html

+    <script src="../dist/itowns.js"></script>
+    <script src="../dist/debug.js"></script>
+    <!-- Import iTowns Widgets plugin -->
+    <script src="../dist/itowns_widgets.js"></script>


we don't need widgets in this example

jailln · 2024-11-27T20:10:30Z

src/Source/Source.js

@@ -108,6 +108,7 @@ class Source extends InformationsData {
    constructor(source) {
        super(source);
        this.isSource = true;
+        this.warn = new Set();



Aggregating logs is a good idea and needed in itowns. I think we should not store aggregated logs in objects and pass them around as function parameters. An alternative implementation would be to implement a LogAggregator that stores logs, aggregates them and can log info, warn and errors. A global instance could be used by all itowns components then. If we do so, we need to be careful with memory usage and empty the log cache frequently. And if we don't want to implement it ourselves, I'm sure there are libraries available. However I think that's out of scope of this PR, so I'd be for removing this log aggregation for now and implement it differently later. @Desplandis what is your opinion on this?

Yeah, we could have some kind of heap with a max size to aggregate those warnings. I think there should be some kind of library to do it. However, I agree that this is out of scope of this PR.

jailln · 2024-11-28T09:10:03Z

src/Core/Style.js

@@ -919,17 +958,16 @@ class Style {
     * @param {Boolean} canBeFilled - true if feature.type == FEATURE_TYPES.POLYGON.
     */
    applyToCanvasPolygon(txtrCtx, polygon, invCtxScale, canBeFilled) {
-        const context = this.context;
        // draw line or edge of polygon
        if (this.stroke) {
            // TO DO add possibility of using a pattern (https://github.com/iTowns/itowns/issues/2210)


should we check for stroke properties too, like for fill ?

jailln · 2024-11-28T09:13:02Z

src/Core/Style.js

@@ -820,6 +826,8 @@ class Style {
            style.point.opacity = opacity;
            style.point.radius = readVectorProperty(layer.paint['circle-radius']);
        } else if (layer.type === 'symbol') {
+            // if symbol we shouldn't draw stroke but defaut value is 1.
+            style.stroke.width = 0.0;


no need for this comment I think, the code is clear enough

jailln · 2024-11-28T09:17:39Z

src/Renderer/Label2DRenderer.js

-            })) {
+            label.horizonCullingPoint && GlobeLayer.horizonCulling(label.horizonCullingPoint)
+            // Why do we might need this part ?
+            // || // Check if content isn't present in visible labels


I think it was to avoid overlapping but I'm not sure we need it too so let's remove it completely in this case. We still have git if we need it later. @Desplandis WDYT?

If we can't reproduce the overlapping and it seems to be the case here. I think we could totally remove this.

jailln · 2024-11-28T09:21:23Z

src/Parser/VectorTileParser.js

-                } else if (!collection.features.find(f => f.id === layer.id)) {
-                    feature = collection.newFeatureByReference(feature);
-                    feature.id = layer.id;
-                    feature.order = layer.order;


This seems like some kind of optimization, can you explain in more details why you remove it please?

jailln · 2024-11-28T09:23:14Z

src/Core/Style.js

@@ -464,8 +465,6 @@ function _addIcon(icon, domElement, opt) {
 * The first parameter of functions used to set `Style` properties is always an object containing
 * the properties of the features displayed with the current `Style` instance.
 *
- * @property {Number} order - Order of the features that will be associated to
- * the style. It can helps sorting and prioritizing features if needed.
 * @property {Object} fill - Polygons and fillings style.
 * @property {String|Function|THREE.Color} fill.color - Defines the main color of the filling. Can be
 * any [valid color


can you provide more details about why you remove it please?

ftoromanoff added 3 commits November 22, 2024 17:54

feat(VectorTile): add support for relative url in style

06804f1

fix(VectorTile): fix {z}/{y}/{x}

5c62908

fix(MVTStyle): icon properties -> fix return of function when id incl…

f903a1a

…udes {}

ftoromanoff force-pushed the mapBoxVT branch from d9300ac to 5581f1e Compare November 26, 2024 16:51

ftoromanoff added 6 commits November 26, 2024 17:54

feat(Source): add propertie 'warn' for avoiding multiple console.warn

328cc2c

test(VectorTileSource): fix test

a96f21b

refactor(VectorTileParser): cleanup

d756acb

fix(MVTLayers): add MVTLayer where MVTStyle.layer has 'ref' properties

a881fac

fix(Style): take style.zoom into account for LabelLayer and Feature2T…

b77965b

…exture

fix(examples): fix linked with zoom properties well used

7f27ef8

ftoromanoff force-pushed the mapBoxVT branch from 5581f1e to 9fcfdee Compare November 26, 2024 16:57

ftoromanoff marked this pull request as ready for review November 27, 2024 08:48

ftoromanoff added 4 commits November 27, 2024 10:57

fix(MVTParser): supp use of layer.style.zoom in parser

c7a1c32

fix(Style): Don't draw Polygon when fill.color is undefined

6d568fa

fix(Style): Don't draw stroke when width is 0

5f63ee2

fix(LabelLayer): gestion simplified of line and polygon Label

af2e0f1

ftoromanoff force-pushed the mapBoxVT branch from 9fcfdee to f1bd8aa Compare November 27, 2024 09:59

ftoromanoff changed the title ~~MapBox VectorTile: bug fixes using an official MapBox flux~~ MapBox VectorTile: bug fixes using an official MapBox stream Nov 27, 2024

ftoromanoff requested a review from jailln November 27, 2024 10:01

ftoromanoff force-pushed the mapBoxVT branch from f1bd8aa to bc8515d Compare November 27, 2024 14:15

ftoromanoff added 6 commits November 27, 2024 15:15

fix(MVTStyle): Doing recoloring only with sdf icons.

049dd6a

fix(Label): Multiple labels with same textContent

efa1777

refactor(MVTParser): 1 feature per vtfeature

36bcd33

fix(VectorTile): supp order in Style as it's only a Label properties …

151e9af

…in VT

example(MVT): add example with official MapBox style file

bb8f57d

fix(Style): dont draw icon when size is 0

18b1d2c

ftoromanoff force-pushed the mapBoxVT branch from bc8515d to 18b1d2c Compare November 27, 2024 14:18

jailln requested changes Nov 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MapBox VectorTile: bug fixes using an official MapBox stream #2469

MapBox VectorTile: bug fixes using an official MapBox stream #2469

ftoromanoff commented Nov 26, 2024 •

edited

Loading

jailln left a comment

jailln Nov 27, 2024

jailln Nov 27, 2024

jailln Nov 27, 2024

jailln Nov 27, 2024

jailln Nov 27, 2024

Desplandis Dec 3, 2024

jailln Nov 28, 2024

jailln Nov 28, 2024

jailln Nov 28, 2024

Desplandis Dec 3, 2024

jailln Nov 28, 2024

jailln Nov 28, 2024

MapBox VectorTile: bug fixes using an official MapBox stream #2469

Are you sure you want to change the base?

MapBox VectorTile: bug fixes using an official MapBox stream #2469

Conversation

ftoromanoff commented Nov 26, 2024 • edited Loading

jailln 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

ftoromanoff commented Nov 26, 2024 •

edited

Loading