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

Interpret zoom level in the same way mapbox does #138

Closed
pakb opened this issue Feb 19, 2019 · 20 comments
Closed

Interpret zoom level in the same way mapbox does #138

pakb opened this issue Feb 19, 2019 · 20 comments

Comments

@pakb
Copy link
Contributor

pakb commented Feb 19, 2019

OLMS internal zoom level for styling has not the same steps/interpretation as mapbox does.
This leads to features showing up too early like in this codepen : https://codepen.io/pakb/full/PVVeeJ

Could OLMS have the same zoom level for interpretation as mapbox does? We're styling everything according to mapbox way of interpreting the zoom level, it's very frustrating seeing the result altered by a misinterpretation (and have to quick and dirty try to fix this with hacks)

@ahocevar
Copy link
Member

What you're seeing is not really an interpretation of zoom levels. OpenLayers applies all styles for vector tiles at integer zoom levels. These styles are reused throughout the full range of an integer zoom level (which goes approximately from z - 0.6 to z + 0.4). So at integer zoom levels, the result will be the same with ol-mapbox-style and mapbox-gl. In the worst case, if a style is applied for z 9 and higher, OpenLayers would show that style already for z 8.4.

Applying styles at non-integer zoom levels would bring a significant performance penalty, because both pre-rendered images for polygons and lines and instruction groups for points and labels would have to be regenerated whenever the view settles at a non-integer zoom. This would lead to a noticable lag when the user e.g. starts panning after a zoom operation has finished.

Once openlayers/openlayers#9137 is merged, the map could be forced to integer zooms by configuring the view with constrainResolution: true. Not sure if that's what users want, but it would work around these minimal visual differences.

@ahocevar
Copy link
Member

ahocevar commented Feb 21, 2019

I see two possible approaches to improve the situation:

  1. Use more render zoom levels. This means that the render tile grid would have more resolutions than the powers of two. Then there could be e.g. rendered tiles for zooms 8, 8.4, 9, 9.4, 10 etc., instead of just 8, 9, 10. Means a bit more work for the renderer and higher cache load.

  2. Re-render hifi tiles when the view has settled on a resolution. This has no cache/memory impact, but means much more work for the renderer. When Refactor the way view constraints are handled & add a view extent constraint openlayers#9137 is merged, we can predict the resolution after an animation (e.g. zoom in) and save one render pass at the end of that animation. This approach would lead to a noticeable lag when the user starts interacting again with the map right after an animation (zoom, pan) ends, but it would ensure the map is rendered with the exact style instructions form the style document.

Of these two approaches, 1 is the more pragmatic one, with a manageable increase in resource demand. 2 would give even better rendered results, but likely at the cost of decreased performance and higher power drain.

Both approaches require changes to the OpenLayers library. The impact of 1 would be quite minimal, 2 would mean a more involved refactoring.

@ahocevar
Copy link
Member

ahocevar commented Mar 6, 2019

Investigating option 1, but currently blocked by #164, which appears to be a major issue.

@ahocevar
Copy link
Member

ahocevar commented Mar 6, 2019

I thought testing option 1 would be something that can easily be tested with a hack, i.e. by setting source.tileGrids_['EPSG:3857'] to a tile grid with more resolutions. But there is another bug in OpenLayers that prevents us from doing that: openlayers/openlayers#9296.

Edit: Things look better with resolutions.push(78271.51696402048 / Math.pow(Math.sqrt(2), i));, which is the special case where we just add another resolution exactly half way between two resolutions. I can put together a demo with that tomorrow.

@danduk82
Copy link

danduk82 commented Mar 7, 2019

@ahocevar yes please let's test this small hack

@ahocevar
Copy link
Member

ahocevar commented Mar 7, 2019

@danduk82 I have created an updated codepen with the hack: https://codepen.io/ahocevar/pen/XGMagr

@danduk82
Copy link

danduk82 commented Mar 7, 2019

Hi @ahocevar ,
mh it is slightly better, but with the labels we still see huge discrepancies. Is related to the other issues or is it still related to zoom level intepretation?
image

@ahocevar
Copy link
Member

ahocevar commented Mar 7, 2019

The partially hidden labels are due to #136. The other differences are implementation specific (e.g. decluttering algorithms, font measurement and rendering, etc.). As I said already, we won't be able to get exactly the same rendered results. If you can ticket other discrepancies separately, we'll be better able to assess if and with what effort we can improve.

@ahocevar
Copy link
Member

ahocevar commented Mar 7, 2019

#169 is another label difference that I just fixed.

@ahocevar
Copy link
Member

#173 improves the appearance of wrapped text.

@ahocevar
Copy link
Member

With the other issues now fixed, can I get another feedback on whether this hack is sufficient:

const renderResolutions = [];
for (let z = 0; z <= 36; ++z) {
  renderResolutions.push(resolutions[0] / Math.pow(Math.sqrt(2), z));
}

olms(ol6, json).then(function(map) {
  
  // Set up a denser render tile grid for vector tile sources
  const layers = map.getLayers().getArray();
  for (let i = 0, ii = layers.length; i < ii; ++i) {
    const source = layers[i].getSource();
    if (source.tileGrids_) {
      source.tileGrids_['EPSG:3857'] = new ol.tilegrid.TileGrid({
        origin: ol.extent.getTopLeft(ol.proj.get('EPSG:3857').getExtent()),
        resolutions: renderResolutions,
        tileSize: 512
      })
    }
  }

});

With this hack, we add additional render resolutions so we're not up to 0.6 zooms off, but only 0.3.

@danduk82
Copy link

@ahocevar sorry if I didn't give feedback: I am off 2 weeks on holidays.

@pakb can you test and give feedback rapidly to @ahocevar ?

@pakb
Copy link
Contributor Author

pakb commented Mar 11, 2019

I was trying to add this hack in our code base, but it’s not that simple. I’ll go for a simple codepen tomorrow morning

@pakb
Copy link
Contributor Author

pakb commented Mar 12, 2019

I've integrated this "hack" in this codepen : https://codepen.io/pakb/pen/YgrYRK?editors=0010
now zoom levels are identical between mapbox and ol/view.
Question : why make an interpolation of this zoom level when they're identical in both framework. We could use ol/view/View zoom level for mapbox style interpretation and that would be a perfect match.

@ahocevar
Copy link
Member

now zoom levels are identical between mapbox and ol/view.

But that's not the result of the hack, and also not relevant to the zoom level at which tiles are rendered. View zoom and tile zoom can be different. The resolutions are what counts, because that's what OpenLayers uses. The hack introduces more zoom levels for rendering, so we get more resolutions at which OpenLayers and mapbox-gl rendering matches.

Question : why make an interpolation of this zoom level when they're identical in both framework. We could use ol/view/View zoom level for mapbox style interpretation and that would be a perfect match.

Actually this would be my second option. But it would require a quite involved change: The vector tile renderer pre-renders images, and does so once per render tile resolution. So when a tile gets first rendered at zoom 16.1, and then you zoom to 16.4, it will still be rendered for 16.1. We'd need to re-render the pre-rendered images after each interaction/animation, to match the view resolution. This not only requires a significant refactoring, but also increases power consumption on mobile devices, because it requires much more renderer passes.

So I still don't know if you're happy with option 1 (as previewed with the hack). We could introduce even more render resolutions (e.g. 3 per zoom level) for option 1, but that's not possible at the moment because of openlayers/openlayers#9296. Or we go the more involved and device power draining route of option 2. I just need to know, because option 2 will keep me busy for at least a week.

@danduk82
Copy link

Hi @ahocevar, after looking at the codepen of @pakb I still notice slight differences especially at high zl (e.g. zl 13) like in the following screenshot. In fact, it really depends on what is defined in the style, and to have a full mapbox/OL parity I think (from what I understand), that option 2, apart from being very cpu consuming should mimic perfectly what is happening in mapbox, or am I wrong?

image

@ahocevar
Copy link
Member

"Perfectly" only in terms of what is defined for an exact resolution. I'd limit re-rendering to zoom level increments of 0.1 though. Renderer differences apart from resolution handling won't go away with that change.

@ahocevar
Copy link
Member

I think when openlayers/openlayers#9296 is addressed, the best approach will be to go for option 1, but create a tile grid with many resolutions. This way we get caching and pixel-sharp rendering, and can do so even in increments of approximately 0.1 or 0.2 zoom levels.

@ahocevar
Copy link
Member

ahocevar commented Mar 18, 2019

Could not persue option 1 further, due to openlayers/openlayers#9296. Instead, I implemented option 2 with openlayers/openlayers#9347. I think the result is quite good, and due to other optimizations alongside that change, render performance and power consumption were not too negatively affected.

@pakb
Copy link
Contributor Author

pakb commented Mar 19, 2019

Thanks a lot! I'll check this out as soon as your PR is merged into OL

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