Wmts support #386
Wmts support #386
Conversation
fschmenger
left a comment
fschmenger
left a comment
There was a problem hiding this comment.
Hi @enricofer,
thanks a lot for providing this!
I have to admit I`m not the OL expert on the team but i decided to have a brief look:
Since the main branch has progressed quite a bit:
- Could you merge it again please?
I also noticed that you commited 8ae5d21 - which probably does not belong into here - and then got rid of it later on in merge. This is a bit confusing. If possible it would be preferred if you could just rebase this PR on the current tip. - Also, could you replace fetch by axios please?
What I can see currently missing is:
- A documentation of your enhancement of the
TileGridDefsindocs/wegue-configuration.md - Some unit tests for the WMTS option in
tests/unit/specs/factory/Layer.spec.js
I made some more comments inline.
From a structural point of view, I'm trying to make up my mind whether we should make the createTileWmtsLayer function async, since an incomplete layer may be returned until fetching resolves. ( This however would infect the whole LayerFactory ). We could also just document on this in the description or put a TODO there and resolve it later. @chrismayer @sronveaux Any ideas welcome here.
I`ll be off for the next 3 weeks, so take your time!
Cheers, Felix
| | transparent | Boolean value, whether the WMS layer should be queried with a transparent background | `"transparent": true` | | ||
| | tileGridRef | Identifier of the tile grid to use for this layer (has to be defined in `tileGridDefs`) The grid has to be correctly identified with `"type": "WMTS"` | `"tileGridRef": "usgs"` | | ||
| | crossOrigin | Provides support for CORS, defining how the layers source handles crossorigin requests. For more information and the supported values see [HTML attribute: crossorigin](https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/crossorigin) | `"crossOrigin": "anonymous"` | | ||
| | cacheSize, interpolate, reprojectionErrorThreshold, tilePixelRatio, version, matrixSet, urls, wrapX, transition, zDirection | The module wraps ol/source/WMTS layer options: https://openlayers.org/en/latest/apidoc/module-ol_source_WMTS.html | `"cacheSize": 16` | |
There was a problem hiding this comment.
Could you be a bit more verbose regarding the documentation here and document fields 1 by 1?
| | crossOrigin | Provides support for CORS, defining how the layers source handles crossorigin requests. For more information and the supported values see [HTML attribute: crossorigin](https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/crossorigin) | `"crossOrigin": "anonymous"` | | ||
| | cacheSize, interpolate, reprojectionErrorThreshold, tilePixelRatio, version, matrixSet, urls, wrapX, transition, zDirection | The module wraps ol/source/WMTS layer options: https://openlayers.org/en/latest/apidoc/module-ol_source_WMTS.html | `"cacheSize": 16` | | ||
| | optionsFromCapabilities | In WMTS layers options can be retrieved parsing the service capabilities document, the option is an object containing at least the following keys: `url`, `layer`, `matrixSet` or `projection` | `"optionsFromCapabilities": {"url": "https://idt2.regione.veneto.it/gwc/service/wmts?request=GetCapabilities", "layer": "rv:DTM_RV_5m_3003", "matrixSet": "EPSG:4326"}` | | ||
| | hoverAttribute | Attribute to be shown if a feature of the layer is hovered. Only has an effect if `hoverable` is set to `true`. | `"hoverAttribute": "name"` | |
There was a problem hiding this comment.
The hoverable attribute is missing here.
| | optionsFromCapabilities | In WMTS layers options can be retrieved parsing the service capabilities document, the option is an object containing at least the following keys: `url`, `layer`, `matrixSet` or `projection` | `"optionsFromCapabilities": {"url": "https://idt2.regione.veneto.it/gwc/service/wmts?request=GetCapabilities", "layer": "rv:DTM_RV_5m_3003", "matrixSet": "EPSG:4326"}` | | ||
| | hoverAttribute | Attribute to be shown if a feature of the layer is hovered. Only has an effect if `hoverable` is set to `true`. | `"hoverAttribute": "name"` | | ||
| | hoverOverlay | ID of a custom map overlay to display when a feature of the layer is hovered. Only has an effect if `hoverable` is set to `true`. For more information on how to implement a map overlay see the [reusable components](reusable-components?id=map-overlay) section. | `"hoverOverlay": "my-custom-overlay"` | | ||
| | params | This allows to inject custom HTTP parameters to the GetMap request of the layer. | `"params": {"FEATUREID": 1}"` | |
There was a problem hiding this comment.
It seems like params is not supported on the WMTS source ( and isn`t set by your code).
| | hoverAttribute | Attribute to be shown if a feature of the layer is hovered. Only has an effect if `hoverable` is set to `true`. | `"hoverAttribute": "name"` | | ||
| | hoverOverlay | ID of a custom map overlay to display when a feature of the layer is hovered. Only has an effect if `hoverable` is set to `true`. For more information on how to implement a map overlay see the [reusable components](reusable-components?id=map-overlay) section. | `"hoverOverlay": "my-custom-overlay"` | | ||
| | params | This allows to inject custom HTTP parameters to the GetMap request of the layer. | `"params": {"FEATUREID": 1}"` | | ||
|
|
There was a problem hiding this comment.
Some fields supported by your code are not documented here, e.g. style and layer
| hoverable: lConf.hoverable, | ||
| hoverAttribute: lConf.hoverAttribute, | ||
| hoverOverlay: lConf.hoverOverlay |
There was a problem hiding this comment.
These attributes must be set on the layer, not source.
| source: new TileWmsSource({ // fake source, will be replaced | ||
| url: '' | ||
| }) |
There was a problem hiding this comment.
Can you just remove source and leave it unitialized here?
| layer: lConf.optionsFromCapabilities.layer, | ||
| matrixSet: lConf.optionsFromCapabilities.matrixSet, | ||
| projection: lConf.optionsFromCapabilities.projection, | ||
| requestEncoding: lConf.optionsFromCapabilities.requestEncoding, | ||
| style: lConf.optionsFromCapabilities.style, | ||
| format: lConf.optionsFromCapabilities.format, | ||
| crossOrigin: lConf.optionsFromCapabilities.crossOrigin |
There was a problem hiding this comment.
What I´m wondering from a structural point of view: Would it be sufficient to replace the optionsFromCapabilities configuration by e.g. a boolean and use all the properties with the same name from the layer level? Currently the definitions look somewhat redundant (but I could be entirely wrong here)
|
Hi there and sorry for the very long time since this PR was written first. I have to admit I looked at it very briefly when it was published and thought I had to look more in details later then completely forgot to do it... I also have to admit I haven't used WMTS layers that often and so I'm not aware of the peculiarities you can encounter when using them. I'm a bit out of time to make a complete review now and will also be off for a while but there's nothing more that comes up in my mind that was not already listed by @fschmenger. As stated, the very first step though would be to ensure this still works under current Vue3 codebase as OpenLayers was also updated a few times since that time. I hope I'll find some time to look at it more thoroughly next month... Cheers |
3 participants
this PR includes support for WMTS layer