Open ViLPy opened 8 years ago
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)?
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.
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.
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.
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?
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.
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
,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).
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