Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework maxNativeZoom #74

Open
IvanSanchez opened this issue Feb 13, 2017 · 11 comments
Open

Rework maxNativeZoom #74

IvanSanchez opened this issue Feb 13, 2017 · 11 comments

Comments

@IvanSanchez
Copy link
Member

As @jkuebart pointed out:

It's interesting to note that by using maxNativeZoom as implemented by GridLayer, vector tiles will not be re-rendered at higher zoom levels. Consequently, the stroke width is scaled up along with the tile, while with the previous solution given above the stroke width remained constant.

If maxNativeZoom is only used because there is sufficient detail at a particular zoom level, this might not be the intended effect. If the stroke width should remain visually constant across all zoom levels, a vector tile layer might still need to reimplement getTileSize() and createTile() accordingly.

I think this is a valid concern, and deserves its own issue.

A tentative approach would be to:

  • Override L.GridLayer._clampZoom and replace it with an identity function
  • Calculate the "data tile" to be fetched for a given "display tile"
  • Multiply pxPerExtent by (2 to the power of the zoom delta)
  • Calculate some pxOffset for when the "data tile" and "display tile" are not aligned at their top left corners
  • Use that pxOffset in all coordinate calculations in PointLayer, PolylineLayer and PolygonLayer

Another approach would be:

  • Keep the current _clampZoom logic
  • On a zoomend event, iterate through all the tiled renderers (there's a reference in this._vectorTiles) and
    • Reset the size of the containers of the tiled renderers by a factor of (2 to the power of zoom delta)
    • Trigger an update of the tiled renderers

First approach doesn't sound too easy, and might need re-requesting tiles. Second approach sounds simpler, almost too simple.

@jkuebart
Copy link
Contributor

jkuebart commented Feb 15, 2017

Do I understand correctly that for your second solution step 2.2 the tiles would need to keep hold of the feature geometries in order to update the path coordinates for the new dimensions? I.e. for the case of SVG, 2.1 would update the viewBox and step 2.2 would recalculate the d attributes for the paths?

@IvanSanchez
Copy link
Member Author

The fact is that the tiled renderers already keep an internal copy of the feature geometries (i.e. see the references to this._layers in the /src/layer/vector/SVG.js file).

So step 2.1 would be setting the viewBox as in the constructor of SVG.Tile.js, and hopefully step 2.2 would be a simple renderer._update().

Please keep in mind that this is a conjecture right now, I haven't spent the time to make an actual prototype and see if the methods would work.

@jkuebart
Copy link
Contributor

You're right, the renderers have references to the layers (i.e. PolylineLayer and siblings). However, renderer._update() isn't currently called for tile renderers, and I think it would be problematic because it attempts to translate its contents according to the visible area of the map, which is definitely not what we need tiles to do.

Firing an 'update' event at the tile renderer should have the effect of running _updatePaths() which will in turn _update() all the contained PolylineLayers and siblings. But this would be equally problematic since it performs clipping and _simplifyPoints() which might not be desirable for tile features.

I do like the basic idea of adjusting the tiles' viewport and then rescaling the feature coordinates, and I think the first solution would be troublesome not least because extra clipping would be needed on the »sub-tiles«.

I think what basically needs to be done is that the FeatureLayer constructor stores the vector-tile's extent and the feature geometry unmodified (currently it is adjusted to the tile size using pxPerExtent) and the scaling is done before rendering, perhaps re-purposing _project() which serves a similar purpose for latLngs. If this sounds sane, I can start taking a look at implementing this.

@IvanSanchez
Copy link
Member Author

I'm not 100% sure, but maybe we can refactor pxPerExtent away? It looks like it should become a property of each tile (i.e. of each tiled renderer), maybe it stays constant through all the tiles (but this is not guaranteed).

Maybe the tiled renderers can just scale everything with a CanvasRenderingContext2D.currentTransform or SVG transform? I wonder if that will resize the line widths and so on as expected.

If this sounds sane, I can start taking a look at implementing this.

For me, every last bit of this sounds completely crazy 🤣 🤣 🤣

@jkuebart
Copy link
Contributor

I think the vector-tile extent need not be constant within one tile, since each vector-tile layer seems to have its own extent – but I don't know enough about vector-tiles to be sure.

@jkuebart
Copy link
Contributor

https://github.com/mapbox/vector-tile-spec/tree/master/2.1

A layer MUST contain an extent that describes the width and height of the tile in integer coordinates.

No wording about extents being identical for every layer in a vector-tile.

@egemon
Copy link

egemon commented Sep 4, 2018

@IvanSanchez @jkuebart is anybody here?

@IvanSanchez
Copy link
Member Author

@egemon Yes, there's somebody here. What's not here is spare time to work on this.

@jkuebart
Copy link
Contributor

jkuebart commented Sep 5, 2018

@egemon I've switched to https://gitlab.com/jkuebart/Leaflet.VectorTileLayer, the readme details differences to Leaflet.VectorGrid.

@IvanSanchez
Copy link
Member Author

@jkuebart I don't see that listed in https://leafletjs.com/plugins#vector-tiles - Could you consider adding it to the plugins list?

@jkuebart
Copy link
Contributor

jkuebart commented Sep 5, 2018

@IvanSanchez done, although I haven't done any development on it recently either. I am however using it successfully in one project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants