MapBox VectorTile: bug fixes using an official MapBox stream

[ftoromanoff]

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

[jailln]

Thanks for the debug and the fixes :)

[jailln]

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?

[Desplandis]

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]

@ftoromanoff can you remove this part from this PR then please ? You can open an issue on this matter if you want to keep track of it

[jailln]

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?

[Desplandis]

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

[ftoromanoff]

As for now, it seems that it is not needed on what I tested and I asked @mgermerie why he used it in the first place.

[jailln]

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

[ftoromanoff]

I removed it to simplified. It is also linked to a trouble to link a specifique feature.style to it's properties.

With this optimization I got these error:

linked with

itowns/src/Core/Style.js

Lines 903 to 914 in 18cc3ee

	style.icon.cropValues = function _(p) {
	const id = iconImg.replace(/\{(.+?)\}/g, (a, b) => (p[b] || '')).trim();
	if (sprites[id] === undefined) {
	const warning = `WARNING: "${id}" not found in sprite file`;
	if (!warn.has(warning)) {
	warn.add(warning);
	console.warn(warning);
	}
	sprites[id] = cropValueDefault;// or return cropValueDefault;
	}
	return sprites[id];
	};

The properties of the current featreu doesn't seems to be the right one when the function is called...

That I don't get when each feature as it's own existence. It might probalby be solved other wise to avoid the probable border effect induce by newFeatureByReference...

[jailln]

It's a bit weird that you get this warning since we look for features with the same id that would already have been parsed.
If we remove this code I'm not sure we need to features collection at all since from my understanding it is to avoid creating multiple times the same feature in memory. It would be interresting to check if it is common that same features are in different pbf and therefore what if this optimization is really needed. It seems a bit unrelated to this PR though so what I propose is to:

• keep this code but comment it for now
• add a comment above explaining what we've said here: this optimization is not fully working and needs to be re-assessed. It has been deactivated for now, see: https://github.com/iTowns/itowns/pull/2469/files#r1861802136