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

Pixel ratio #7333

Closed
wants to merge 7 commits into from
Closed

Pixel ratio #7333

wants to merge 7 commits into from

Conversation

ryanhamley
Copy link
Contributor

Launch Checklist

Makes devicePixelRatio configurable and fixes #1953

By default, config gets the devicePixelRatio from window if no value is explicitly set.

tip of the 🎩 to @sniok

  • Add a devicePixelRatio setting to config.js which defaults to browser.devicePixelRatio
  • Add a devicePixelRatio getter and setter to index.js
  • Change all uses of browser.devicePixelRatio to config.devicePixelRatio
  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • manually test the debug page

@ChrisLoer
Copy link
Contributor

This sounds like a good idea, but I have questions:

  • What are the range of valid values for device pixel ratio, and what happens when someone sets it to something invalid? Does it make sense to have a validator?
  • We have a getter and a setter for the value, but what happens when you change the value at runtime? I'm just quickly eyeballing the code here, but it looks like normalizeTileURL will change behavior immediately based on the change, but the shaders won't regenerate with the new pixel ratio? Should we disallow changing this value at runtime?
  • What sort of manual tests have we done on this so far?
  • Can we do automated tests of the rendering at the supported pixel ratios? The mapbox.test.js part is just testing the normalizeTileURL logic, but the rendering seems like the really important part here.
  • How does changing the DEVICE_PIXEL_RATIO in the shaders jive with the devicePixelRatio in painter.js not changing? e.g.
    this.width = width * browser.devicePixelRatio;
    this.height = height * browser.devicePixelRatio;

@AskAlice
Copy link

AskAlice commented Aug 6, 2019

please merge this. When I resize my renderer with three, the mapbox gl resizes at double scale and looks very bugged

@Mechazawa
Copy link

Setting window.devicePixelRatio and calling map.resize() this seems to work fine for now untill this PR has been finished/merged. Do note that the devicePixelRatio can change if the browser is moved to a different display.

@asheemmamoowala
Copy link
Contributor

This PR has gotten very stale, and there are still many open questions from #7333 (comment). Closing for now, but @ryanhamley please re-open if/when this is planned to work on next.

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

Successfully merging this pull request may close these issues.

Make "devicePixelRatio" user-configurable
6 participants