Skip to content

Fix WebGL scaling of antialising by pixel ratio - #13783

Merged
ianthomas23 merged 2 commits into
branch-3.5from
ianthomas23/13692_webgl_pixel_ratio
Mar 26, 2024
Merged

ianthomas23 merged 2 commits into
branch-3.5from
ianthomas23/13692_webgl_pixel_ratio

Conversation

@ianthomas23

Copy link
Copy Markdown
Member

Fixes #13692, the incorrect scaling of WebGL antialiasing by pixel ratio.

The width and height of the canvas used for WebGL rendering is scaled by pixel_ratio:

target.width = width*this.pixel_ratio
target.height = height*this.pixel_ratio

whereas the antialias width is constant regardless of pixel_ratio. Following this through the WebGL rendering pipeline shows that this means the rendered antialiasing scales with pixel_ratio, and so is too large for pixel_ratio > 1 such as for retina screen or zooming (which is implemented in chrome dev tools using a larger pixel ratio).

The fix is to scale the antialiasing by dividing by pixel_ratio.

Test code:

from bokeh.plotting import figure, row, show

ps = []
for backend in ("canvas", "webgl"):
    p = figure(width=200, height=150, output_backend=backend, title=backend)
    p.line(x=[0, 1, 2, 3], y=[1, 2, 3, 2], width=5)
    ps.append(p)

show(row(ps))

before the fix on a retina screen (pixel ratio 2) at zoom level 300% (so the pixel ratio seen is 6):
Screenshot 2024-03-25 at 15 52 08

and after this fix:
Screenshot 2024-03-25 at 15 25 29

The fix scales the antialising that is passed to the WebGL vertex shaders. It could be performed in the vertex shaders instead, but then the antialias would have to be a GLSL attribute rather than uniform. Whilst here I realised that the pixel_ratio is passed to the shaders but only used to scale the canvas size, i.e. to remove the pixel_ratio multiple added in canvas.ts above. So I have moved this scaling to BaseGLGlyph instead which means we no longer need to pass pixel_ratio into any of the shaders. It is not a big deal either way, but I think this implementation is a bit simpler and cleaner.

@bryevdv bryevdv added this to the 3.5 milestone Mar 25, 2024
@codecov

codecov Bot commented Mar 25, 2024 •

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.65%. Comparing base (78d71c2) to head (3344c55).
Report is 2 commits behind head on branch-3.5.

Additional details and impacted files
@@             Coverage Diff             @@
##           branch-3.5   #13783   +/-   ##
===========================================
  Coverage       92.65%   92.65%           
===========================================
  Files             326      326           
  Lines           20734    20734           
===========================================
  Hits            19211    19211           
  Misses           1523     1523           

await display(row([plot("canvas"), plot("svg"), plot("webgl")]))
})

it.dpr(3)("with devicePixelRatio == 3", async () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if just using .dpr(N) is enough to produce a meaningful test. I think this needs scaling as well, but we don't have an API exposed for this currently, though it's possible at CDP (Chrome Devtools Protocol) level.

@ianthomas23 ianthomas23 Mar 25, 2024 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, really we should also be scaling the canvas size along with the pixel ratio. But it is meaningful as it is, here is the dpr=3 test image before and after this PR:

before
after

The WebGL line is "too blurry" before this change, but one might have to enlarge the images to convince oneself.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd happily remove the new test and save the disk space. But if we keep it we can at least identify when it changes and it will lead us back here where there are instructions for a manual test.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to keep this test, but I would also like to establish if we can make the difference more obvious. I did a little bit of exploration myself and there is a difference when comparing generated images between branch-3.5 and this PR, so this test is indeed valid, though hard on "the naked eye". I tried to enable scaling in CDP, but it doesn't seem to do anything useful. So I suppose this will have to do for now.

@ianthomas23

Copy link
Copy Markdown
Member Author

It looks to me like the test failures are unrelated.

Comment thread bokehjs/test/integration/regressions.ts Outdated
@ianthomas23
ianthomas23 merged commit d9a2f7a into branch-3.5 Mar 26, 2024
@ianthomas23
ianthomas23 deleted the ianthomas23/13692_webgl_pixel_ratio branch March 26, 2024 11:51
@mattpap mattpap modified the milestones: 3.5, 3.4.1 Apr 10, 2024
mattpap pushed a commit that referenced this pull request Apr 10, 2024
* Fix webgl handling of pixel ratio

* Fix spelling mistake
@mattpap mattpap mentioned this pull request Apr 10, 2024
17 tasks done
Chiemezuo pushed a commit to Chiemezuo/bokeh that referenced this pull request Aug 27, 2024
* Fix webgl handling of pixel ratio

* Fix spelling mistake
@github-actions

Copy link
Copy Markdown

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Oct 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Blurry plots with WebGL

3 participants