Conversation
|
The question of "why PMTiles" was raised in a previous comment. I think a nice answer lies in this article Open Source Mapping for News: Reuters. |
|
Hi @ymoisan |
airnez
left a comment
There was a problem hiding this comment.
Thank you for this contribution ! PMTiles is indeed a very interesting format.
I tested your branch, and it looked fine to me.
With one data source, it seems that some tiles are broken, do you know why ? I suspect an iTowns-related problem, after reading your code.
https://r2-public.protomaps.com/protomaps-sample-datasets/tilezen.pmtiles
| super(source); | ||
|
|
||
| this.isPMTilesVectorSource = true; | ||
| this.isVectorSource = true; |
There was a problem hiding this comment.
Is setting isVectorSource necessary ? Source.js defines
this.isVectorSource = (source.parser || supportedParsers.get(source.format)) != undefined;
I guess this is enough considering that you set
source.format = 'application/x-protobuf;type=mapbox-vector';
What do you think ?
| getDataKey(extent) { | ||
| if (extent.isTile) { | ||
| return `z${extent.zoom}r${extent.row}c${extent.col}`; | ||
| } | ||
| return `z${extent.zoom}r${extent.row}c${extent.col}`; | ||
| } |
There was a problem hiding this comment.
This might not be necessary. It always returns the same value whether extent.isTile is true or false. You could rely on Source.js code that does use the same key scheme.
`z${extent.zoom}r${extent.row}c${extent.col}`
| .then((data) => { | ||
| if (!data) { | ||
| // No tile data - return empty collection | ||
| return Promise.resolve(null); |
There was a problem hiding this comment.
I think it could be safer to return an empty FeatureCollection here.
Promise.resolve(new FeatureCollection(out));
examples/source_pmtiles_vector.html
Outdated
|
|
||
| const opacityToUse = userOpacity; | ||
|
|
||
| pmtilesSource.clearCache(); |
There was a problem hiding this comment.
I don't think that you need to empty the source cache to reload the layer. It seems to work after commenting this line. In Itowns, it seems that clearing the cache is more a View / Layer responsability than source.
| /** | ||
| * Called when layer is added. Sets up caching. | ||
| * | ||
| * @param {Object} options - Options | ||
| */ | ||
| onLayerAdded(options) { | ||
| super.onLayerAdded(options); | ||
|
|
||
| // Setup cache for features | ||
| if (!this._featuresCaches[options.out.crs]) { | ||
| this._featuresCaches[options.out.crs] = new LRUCache({ max: 500 }); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think that Source.js already handles that for you. Calling super will always instanciate features cache for this layer. You could simpy not implement this method, what do you think ?
| * @property {number} zoom.min - The minimum zoom level of the source. | ||
| * @property {number} zoom.max - The maximum zoom level of the source. | ||
| */ | ||
| class PMTilesSource extends Source { |
There was a problem hiding this comment.
I understand that this parent source will be useful later to specify a Raster PMTiles Source ?
| this.styles = source.styles || {}; | ||
|
|
||
| // Parser for MVT tiles | ||
| this.parser = VectorTileParser.parse; |
There was a problem hiding this comment.
I dont think this is needed, as Source.js already sets this parser for application/x-protobuf;type=mapbox-vector WDYT ?
| * @property {boolean} isPMTilesVectorSource - Used to checkout whether this | ||
| * source is a PMTilesVectorSource. Default is true. You should not change this, | ||
| * as it is used internally for optimisation. | ||
| * @property {Object} layers - Object containing layer definitions for styling. |
There was a problem hiding this comment.
This differs from VectorTilesSource, but I think this makes sense as PMTiles are not advertised through mapbox styles. This directly corresponds to the format expected by the parser
| /** | ||
| * Set visibility of a specific source layer. | ||
| * | ||
| * @param {string} layerName - The name of the source layer | ||
| * @param {boolean} visible - Whether the layer should be visible | ||
| */ | ||
| setLayerVisibility(layerName, visible) { | ||
| if (!this._layerVisibility) { | ||
| this._layerVisibility = {}; | ||
| } | ||
| this._layerVisibility[layerName] = visible; | ||
|
|
||
| // Update layers object based on visibility | ||
| if (this._originalLayers && this._originalLayers[layerName]) { | ||
| if (visible) { | ||
| this.layers[layerName] = [...this._originalLayers[layerName]]; | ||
| } else { | ||
| // Set to empty array so parser skips this layer | ||
| this.layers[layerName] = []; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Set visibility of all source layers. | ||
| * | ||
| * @param {boolean} visible - Whether all layers should be visible | ||
| */ | ||
| setAllLayersVisibility(visible) { | ||
| if (this._originalLayers) { | ||
| Object.keys(this._originalLayers).forEach((layerName) => { | ||
| this.setLayerVisibility(layerName, visible); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Get visibility state of a layer. | ||
| * | ||
| * @param {string} layerName - The name of the source layer | ||
| * @returns {boolean} Whether the layer is visible | ||
| */ | ||
| getLayerVisibility(layerName) { | ||
| return this._layerVisibility ? this._layerVisibility[layerName] !== false : true; | ||
| } |
There was a problem hiding this comment.
I understand that this code is very helpful to build your example page (which is great by the way!). I think that this is something that should be managed by the Layer, not the source. What you are actually doing here is modifying source.layers in order that the parser filters the unwanted layers. You are updating the layer filtering at the source and re-instanciating the layer, not updating visibilities. There are discussions about off-screen GPU rendering for rasterized tiles. I think this is the way to implement true layer visibility switching. For now, I suggest that you could keep all this code by moving it to your example page. What do you think ? @Desplandis I can't find any issue related to off-screen ColorLayer rendering, is there one existing already ? I think this PR gives another reason to work on this great idea.
|
|
||
| // If no layers were provided, try to auto-detect from metadata | ||
| if (Object.keys(this.layers).length === 0 && metadata.vector_layers) { | ||
| this._setupLayersFromMetadata(metadata.vector_layers); |
There was a problem hiding this comment.
It's great to be able to parse headers directly. Thank you for implementing that part. It makes iTowns easier to use :)
|
Thanx for the thorough review @airnez. I'll be honest up front : although I have coding experience, everything in this PR is "vibe coded". I could never think of implementing all this in JS, a language I don't work with normally. However, I knew what I wanted so I guided the implementation best I could. So, I submitted your comments to AI : <begin-AI> What I'll fix:
Questions:1. Layer visibility UI implementation (addressing your comment on lines 140-184): I'd like to keep the layer visibility UI in the example since it's helpful for demonstrating PMTiles capabilities. As you suggested, I'll move all that code from the Source to the example. However, I'm wondering about the best approach for implementing this in the example - should I:
Since you mentioned that true layer visibility switching will come with off-screen GPU rendering, what would be an acceptable interim approach for the example? 2. Error handling strategy (addressing your comment on line 274): You suggested using 3. Metadata parsing: Should I keep the automatic layer setup from metadata ( 4. PMTilesSource parent class (addressing your comment on line 25): Yes, exactly! The parent 5. Broken tiles issue (addressing your comment about https://r2-public.protomaps.com/protomaps-sample-datasets/tilezen.pmtiles): I suspect the broken tiles might be related to the error handling issues you identified. Currently, the code:
Looking at how I suspect the broken tiles might be caused by:
With the error handling fixes I mentioned above (returning empty
<end-AI> If that suits you I can provide code fixes. Otherwise let's continue the discussion. |
- Remove redundant code (isVectorSource, parser, getDataKey, onLayerAdded) - Move visibility methods to example level (Layer responsibility) - Use this.handlingError for error handling - Return empty FeatureCollection instead of null - Simplify example: constant style, remove unnecessary clearCache
|
I tested my example and it works fine. Summary of ChangesPMTilesVectorSource.js (simplified from 284 to 181 lines)
|
|
@ymoisan Sorry for taking this long to answer. We talked about it a few weeks ago with other contributors, and I forgot to keep you in touch. For this one, I'll review your code and provide the code modifications if needed. Your contribution seems safe to merge because it's a new independant feature. I already spend time on it, let's push it. I'll then hand the final review to another maintainer. I'll keep you in touch :) Thank you for contributing. You are welcome to contribute again, at the sole condition that you are able to understand provided code and apply review suggestions by yourself. |
I open this PR as per this issue and this discussion.
As I stated in the discussion item, I would like to be able to use PMTiles instead of OGC web services, provided that all "flavours" of PMTiles (MVT, raster, elevation) cover the requirements for iTowns to work. If so, it is my opinion that functional requirements for iTowns could be dramatically relaxed : no web server needed, just simple http access to files. Simplicity in using different basemaps and other map artifacts than what is currently provided would make iTowns easier to personnalize for other organizations, especially those that do not have the breadth of OGC web services currently needed to accommodate iTowns.
I would like this PR to serve as a starting point for discussing the integration of "cloud-native" file formats in iTowns. The example provided is just to show PMTiles of MVT type can be overlaid in iTowns. My hope is that a well prepared PMTiles could provide the same output than OGC services and therefore could be a substitue rather than mererly an overlay.