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

Allow setting a custom pixel ratio #769

Closed
vanilla-lake opened this issue Jan 13, 2022 · 11 comments
Closed

Allow setting a custom pixel ratio #769

vanilla-lake opened this issue Jan 13, 2022 · 11 comments

Comments

@vanilla-lake
Copy link
Contributor

Motivation

When exporting a map to an image, there are many cases where we might want the image to have a different size in pixels than the in-app map all while keeping the same extent and zoom level. In other words, we want to scale the map. One reason to do this is to allow exporting at a certain DPI, like https://github.com/watergis/maplibre-gl-export does.

Design Alternatives

The way maplibre-gl-export addresses this problem is by overriding the Window#devicePixelRatio property: https://github.com/watergis/maplibre-gl-export/blob/3977f96e829bf589d9988270da26444fdab807d9/lib/map-generator.ts#L148-L151

While it works, it relies on changing some global state, which could easily have some unintended consequences, especially when the code is asynchronous.

Design

We could add a new MapOptions#pixelRatio property along with a new Map#setPixelRatio method. Instead of using the Window#devicePixelRatio property, maps would now keep their own pixel ratio value. In addition to making the above use case much less brittle, this would make testing easier as there would be no more need to override the Window#devicePixelRatio property.

One drawback is that it adds a bit more complexity to the Map interface. However, since maps' pixel ratio would default to the value stored in the Window#devicePixelRatio property when MapOptions#pixelRatio is not specified and Map#setPixelRatio has not been called, users would not have to think about this unless they need the new functionality, so existing code could remain unchanged.

Mock-Up

As an example, maplibre-gl-export could get rid of the code snippet linked above and do something like the following at https://github.com/watergis/maplibre-gl-export/blob/3977f96e829bf589d9988270da26444fdab807d9/lib/map-generator.ts#L162-L175:

const renderMap = new MapLibreMap({
    /* ... */
    pixelRatio: this._dpi / 96
});

Concepts

Some documentation will have to be written for the above additions to the public interface. The terminology we should use is "pixel ratio" as that will already be familiar to Web developers because of the Window#devicePixelRatio property. It is also already used internally by MapLibre GL JS.

Implementation

We could add a new MapOptions#pixelRatio property, along with some new Map#setPixelRatio and Map#getPixelRatio methods.

Internally, the code would need to keep its own pixel ratio value instead of reading the Window#devicePixelRatio property.

vanilla-lake added a commit to vanilla-lake/maplibre-gl-js that referenced this issue Jan 13, 2022
@HarelM
Copy link
Collaborator

HarelM commented Jan 13, 2022

I'm not sure this should be a map property and not a global property like base url and access token were...?

@vanilla-lake
Copy link
Contributor Author

I'm not sure... Having it as a global property would be a lot less useful in my opinion, since we wouldn't be able to set it on a per-map basis. It can be useful to create a separate map for exporting to an image, and having it as a global property would unnecessarily affect all maps.

@vanilla-lake
Copy link
Contributor Author

FWIW, this seems to have been requested for some time in Mapbox GL's issue tracker: mapbox/mapbox-gl-js#1953. Someone even proposed a similar solution: mapbox/mapbox-gl-js#1953 (comment).

@vanilla-lake
Copy link
Contributor Author

@vanilla-lake
Copy link
Contributor Author

Another possibility could be to have a global setting, but also allow it to be overridden on a per-map basis.

@HarelM
Copy link
Collaborator

HarelM commented Jan 15, 2022

I see you point, both global and per map is a bit confusing I think.
I'll review the PR later this week.

@vanilla-lake
Copy link
Contributor Author

Thank you!

@astridx
Copy link
Contributor

astridx commented Jan 18, 2022

Thank you very much @vanilla-lake for this PR. I like the possibility to set devicePixelRatio myself. Not only to be able to print a map in a defined resolution. You might want to lower the value for performance reasons.

However, it is also the case that one has to be careful when changing the devicePixelRatio value, as the size of the image increases and this can lead to performance issues and a WebGL error in Mozilla Firefox. If the WebGL error occurs, a smaller section of the map is displayed. See the example below. I have this problem in Firefox where as chrome browsers are rendering fine.

Should we provide a fallback mechanism for these? I know that the value 9 for devicePixelRatio is quite high. But as it is at the moment, it can simply be entered. You can even use 100 as a value. In the latter case, I am also shown a map section that is too small in Chrome without seeing an error in the browser console.

If we leave it like this, then I would integrate the function with the note "at your own risk", like #574.


<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8" />
    <title>Quickstart MapLibre GL</title>
    <meta
      name="viewport"
      content="initial-scale=1,maximum-scale=1,user-scalable=no"
    />
    <script src="../maplibre-gl-js/dist/maplibre-gl.js"></script>
    <link href="../maplibre-gl-js/dist/maplibre-gl.css" rel="stylesheet" />
  </head>

  <body>
    <div id="map1" style="width: 50vw; height: 40vh"></div>
    <hr />
    <div id="map2" style="width: 50vw; height: 40vh"></div>
    <span id="test"></span>

    <script>
      var map1 = new maplibregl.Map({
        container: "map1",
        style:
          "https://api.maptiler.com/maps/streets/style.json?key=" +
          config.MAPTILER_TOKEN,
        center: [7.726761111111138, 50.254269444444], // Startposition [lng, lat]
        zoom: 12, // Zoom
      });

      var map2 = new maplibregl.Map({
        container: "map2",
        style:
          "https://api.maptiler.com/maps/streets/style.json?key=" +
          config.MAPTILER_TOKEN,
        center: [7.726761111111138, 50.254269444444], // Startposition [lng, lat]
        zoom: 12, // Zoom
      });
      map2.setPixelRatio(9);

      document.getElementById("test").innerHTML =
        "<br>map1.painter.pixelRatio: " +
        map1.painter.pixelRatio +
        "<br>map1.painter.width: " +
        map1.painter.width +
        "<br>map1.painter.height: " +
        map1.painter.height +
        "<br>map2.painter.pixelRatio: " +
        map2.painter.pixelRatio +
        "<br>map2.painter.width: " +
        map2.painter.width +
        "<br>map2.painter.height" +
        map2.painter.height;
    </script>
  </body>
</html>

WebGL warning: width/height: Requested size 8316x3141 was too large, but resize to 4158x1570 succeeded.

1

@vanilla-lake
Copy link
Contributor Author

Great catch @astridx! Thanks for pointing that out. I can't really think of a fallback beyond rendering into multiple smaller canvases, but please let me know if you have any suggestions. However, since not all browsers display a warning, maybe we could display one ourselves when the canvas' width or height exceed the value returned by gl.getParameter(gl.MAX_RENDERBUFFER_SIZE)?

@astridx
Copy link
Contributor

astridx commented Jan 21, 2022

Is it ok if I close the issue because we have a PR #772 ?
Then the conversation is in one place.

@vanilla-lake
Copy link
Contributor Author

@astridx That is fine with me.

@astridx astridx closed this as completed Jan 21, 2022
astridx pushed a commit that referenced this issue Jan 26, 2022
As I mentioned in #769 (comment), we should add this feature. The problems I had there have nothing to do with this PR directly. Since others have also commented positively here and no one has anything against it, I am now merging the PR. I hope this is fine.
Thank you very much @vanilla-lake
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