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

HiDPI screen support #102

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

HiDPI screen support #102

wants to merge 2 commits into from

Conversation

ViLPy
Copy link

@ViLPy ViLPy commented Oct 9, 2016

Added resolution parameter in order to support HiDPI screens. More information about HiDPI can be found here https://www.html5rocks.com/en/tutorials/canvas/hidpi/

‘Tile’ layout is not updated because current API implementation should be modified in order to support variable tilemap size

Added resolution parameter in order to support HiDPI screens.
See more here https://www.html5rocks.com/en/tutorials/canvas/hidpi/
‘Tile’ layout is not updated because current API implementation should
be modified in order to support variable tilemap size
@ondras
Copy link
Owner

ondras commented Oct 9, 2016

I am not sure I understand what is this PR about.

What problem is it trying to solve? As far as I know, high-density ("retina") devices are completely opaque to web pages, hiding their hardware resolution. An iPhone, for instance, is 640 physical pixels wide, but as far as logical pixels go, the size is 320. A canvas with width=320 would cover the whole device's width and every single JS-accessible pixel would render using 2x2 physical pixels. Also, rendering with fontSize = 15px would draw individual glyphs using height of 30 hardware pixels, consistently with the (fake) reported height of 15 logical (software) pixels.

Is there a particular scenario where the existing ROT.Display does an incorrect job with respect to the resolution? Also, does the value of resolution:2 pass all current unit tests (eventToPosition comes to mind)?

@ViLPy
Copy link
Author

ViLPy commented Oct 9, 2016

Yes, it is opaque and you're right - every single JS-accessible pixel would render using 2x2 physical pixels. So because of that, canvas will be blurred on HiDPI screens like it is described in article.

eventToPosition resolution transform is actually covered here. Haven't found any other places where this should also be applied.

@ondras
Copy link
Owner

ondras commented Oct 10, 2016

The article is interesting, but:

  1. as far as I can tell, there is no such property webkitBackingStorePixelRatio (tested on mobile webkit and desktop Chrome),

  2. the upscaling issue is related to images only,

  3. I do not see any visible issues even on high-dpi devices.

I created a very simple testbed page, hosted at http://bespin.cz/~ondras/canvas/ -- obviously, results for a CSS-styled <div> element shall be the same as JS-styled canvas. If I understand your patch correctly, you propose making the <canvas> two times smaller. which is rather weird: we shall maintain size consistency with CSS-styled elements.

So in order to render in device-dpi, we would need to double the canvas first and scale it down (via CSS) afterwards. But that would influence basically every rendering method in ROT.Display.*... and such adjustment would obviously need to be done on a feature-testing basis, not explicitely mentioned in a JS constant.

@ViLPy
Copy link
Author

ViLPy commented Oct 10, 2016

  1. Yea, article is four years old and now we can use window.devicePixelRatio in every modern browser, but this PR doesn't add such calls. It is only about adding resolution parameter like in PixiJS and developers can set to to whatever they like.
  2. Upscale issue is related to any raster-based object and canvas is raster-based
  3. Pardon me, but canvas version is definitely blurred
    screenshot_2016-10-10-08-32-13

In PR I multiply fontSize by resolution, so this increase canvas size and then I set CSS for downscale, also this mitigate most of possible calculation issues in ROT.Display. You're right, I might miss some code parts where I should recalculate some parameters, but I think this PR is a nice thing to start with.

@ondras
Copy link
Owner

ondras commented Oct 10, 2016

I see. To sum this up:

  1. the proper constant for this is devicePixelRatio (dPR),
  2. to achieve sharp rendering, we need to multiply canvas size by dPR and scale it down (by 1/dPR) via CSS,
  3. doing so implies scaling every canvas operation by dPR (because the canvas is larger than before),
  4. forcing custom CSS breaks any userland CSS scaling that might be applied outside of rot.js.

I think that fixing the blurry rendering (you are correct -- it looked okay on my phone, because of the high pixel density -- which is kinda the point of hi-dpi) is a generally good idea. But:

a) it should be done automatically, by reading the dPR value and adjusting accordingly (you want your code to work with all dPR values out there, right?);

b) it should not break custom CSS (people should be able to do canvas { width: 300px; } if they want).

Are you aware of any approach that maintains both assertions?

@ViLPy
Copy link
Author

ViLPy commented Oct 11, 2016

I see no way how to make resolution setup completely automatic and support all possible custom CSS settings at same time. One possible solution is to keep inline styling and just mention in docs that explicit width/height can be set via min-/max-width or with !important keyword.

@ondras
Copy link
Owner

ondras commented Oct 17, 2016

Hmm, inline styling cannot be overriden with !important. Also, using !important is generally considered a bad practice.

What we really need is to make the canvas twice (or any other dPR-based value) as large as... I have no idea. Suppose we have the following setup:

  • ROT.Display({width:50}) which results in 500px,
  • devicePixelRatio = 2,
  • CSS styling that has canvas { width:400px; }

So the author clearly wants to have a canvas 400px (logical pixels) wide. What size do we want to render in? 2*400 for retina displays, 500px for regular displays? This is doable, as we know the ideal size (500px, computed by us) as well as the scaled-down size (clientWidth, retrievable via DOM queries). And we can compute the proper scaling factor from these values (800/500 in this example).

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.

3 participants