Handle HiDPI displays in WebAgg/NbAgg backends #5383

Merged
merged 9 commits into from Dec 10, 2015

Conversation

Projects
None yet
5 participants
Owner

mdboom commented Nov 2, 2015

Fix #5381.

Tested on Firefox/Chrome/Safari on Linux/Mac. Anyone able to do Windows testing?

mdboom added the needs_review label Nov 2, 2015

mdboom added this to the next bug fix release (2.0.1) milestone Nov 2, 2015

@QuLogic QuLogic and 1 other commented on an outdated diff Nov 2, 2015

lib/matplotlib/backends/backend_webagg_core.py
@@ -342,7 +355,7 @@ def handle_event(self, event):
def handle_resize(self, event):
x, y = event.get('width', 800), event.get('height', 800)
- x, y = int(x), int(y)
+ x, y = int(x) * 2, int(y) * 2
@QuLogic

QuLogic Nov 2, 2015

Member

Should there be this hard-coded 2 here, not self.canvas._dpi_ratio?

@mdboom

mdboom Nov 2, 2015

Owner

Indeed. Good catch.

Owner

mdboom commented Nov 6, 2015

Rebased

Member

WeatherGod commented Nov 6, 2015

Does this work correctly if the user changes the DPI (not even really sure if that is possible anyway). The way I see it, the original dpi could never get updated?

Owner

mdboom commented Nov 6, 2015

With Chrome, I can't find a way to change the "devicePixelScale" dynamically. It can be set with a commandline argument, but that implies restarting the browser.

With Firefox, it can be set dynamically through about:config:layout.css.devPixelsPerPx. In that case, the plot scales up with pixelation, which is fine. It's theoretically possible to catch that and start regenerating the images at a higher resolution, but that seems like a real corner case given that the user has to visit the "void your warranty" about:config page.

This also performs fine under Ctrl + and Ctrl - scaling, though the plot is pixelated at the higher scales. That might be something worth trying to fix, but it doesn't exactly feel broken as it is, either.

Owner

jenshnielsen commented Nov 6, 2015

If I move a window from my external Non high dpi to my internal Retina monitor
window.devicePixelRatio changes from 1 to 2 as expected I guess you would need to regenerate the image if you change monitor. I.e. unplug external monitor

Owner

mdboom commented Nov 6, 2015

@jenshnielsen: That's a good point. I only have HiDPI monitors. I'd be curious what happens for you if you're able to test that. My best guess is that if you move from lodpi to hidpi the image will be pixelated. If you move from hidpi to lodpi the image will be downsampled (but appear at a reasonable total size). Neither of which are the end of the world.

I guess we could try to trap the change and adapt on the fly. It seems at least theoretically possible: https://stackoverflow.com/questions/28905420/window-devicepixelratio-change-listener

Owner

jenshnielsen commented Nov 8, 2015

If I understand the code correctly it will get the DPI every time a new figure is created?
I think that recreating the figure to get the correct dpi when changing is perfectly fine as long as it doesn't require restarting the notebook or some such thing. Unfortunately I only have a non hiDPI monitor in the office so I will have to wait with testing that till tomorrow.

Otherwise this is really nice 👍 however there seems to be a bug in the conversion from low to high when zooming on a highDPI screen. When drawing a zoom box it seems to get the coordinates in the mixed up and zoom to the corner. I guess there is a multiplication by dpi missing somewhere

Owner

jenshnielsen commented Nov 8, 2015

This also manifests it self in that the coordinates in the lower right corner are wrong.
Also when stopping a figure on a highDP screen the static figure is much larger than the original

Owner

mdboom commented Nov 17, 2015

I've fixed the issues with the coordinates and the rubberband.

This still has potential issues when moving the browser from a lo to hi-res screen, but I don't have a way of testing that.

mdboom added some commits Nov 2, 2015

@mdboom mdboom Handle HiDPI displays in WebAgg/NbAgg backends 5156948
@mdboom mdboom Remove hardcoding 249f19e
@mdboom mdboom Fix nbagg 68cdfa9
@mdboom mdboom Only send ratio if needed 256e906
@mdboom mdboom PEP8 0c1ea5a
@mdboom mdboom Add what's new 17c7259
@mdboom mdboom Fix scale of mouse events when in HiDPI mode
c08e251
Owner

jenshnielsen commented Nov 17, 2015

Thanks I will test out the moving when I'm back with my external monitors

Owner

jenshnielsen commented Nov 18, 2015

Did some testing.

  • When you move a non highdp figure to a high dp display it is non highdp until the figure is recreated.
  • When you move a highdp figure to a non highdp display it is slightly more fuzzy until the figure is recreated

I think this is fine and there is no need to do anything more elaborate.

Did you push the fix for rubberband? I dont see the any commits newer than d0b16a1 which is 12 days old

Owner

mdboom commented Nov 18, 2015

Sorry -- recent changes pushed now.

Owner

jenshnielsen commented Nov 18, 2015

The rubberband and coordinates look right now.

I am still seeing the issue where the figure grows in size when stopped and converted to a static figure. I guess the right dpi is not set for the static figure.

Owner

mdboom commented Nov 18, 2015

Ah -- See that DPI change now, too. Let me see what can be done. At least for me, the static figure is scaled up by the browser (i.e. not using the full DPI of the display). We should fix that, but I suspect that might require tweaking something in IPython.

Owner

jenshnielsen commented Nov 18, 2015

IPython at least used to have some hidpi support ipython/ipython#3381 not sure where that lives now

Owner

mdboom commented Nov 18, 2015

I see -- maybe we can add that flag for the user, since we already know they are retina. (Of course, multiple clients connected to the same kernel will be a problem -- but we have that problem in NbAgg anyway).

Owner

jenshnielsen commented Dec 9, 2015

I was playing around with this and it seems to me that we can solve it by doing something like
changing handle_close to:

mpl.figure.prototype.handle_close = function(fig, msg) {
    var width = fig.canvas.width/mpl.ratio
    fig.root.unbind('remove')

    // Update the output cell to use the data from the current canvas.
    fig.push_to_output();
    var dataURL = fig.canvas.toDataURL();
    // Re-enable the keyboard manager in IPython - without this line, in FF,
    // the notebook keyboard shortcuts fail.
    IPython.keyboard_manager.enable()
    $(fig.parent_element).html('<img src="' + dataURL + '" width="' + width + '">');
    fig.close_ws(fig, msg);
}

I.E explicitly setting the width according to the dpi ratio. This obviously needs some more work

Owner

jenshnielsen commented Dec 9, 2015

And a similar modification in push_to_output

mpl.figure.prototype.push_to_output = function(remove_interactive) {
    // Turn the data on the canvas into data in the output cell.
    var width = this.canvas.width/mpl.ratio
    var dataURL = this.canvas.toDataURL();
    this.cell_info[1]['text/html'] = '<img src="' + dataURL + '" width="' + width + '">';
}
@jenshnielsen jenshnielsen Set witdh of non interactive figure in nbagg backend.
This ensures that the figure size remains the same following a closure of the figure regardless of display dpi
e338687
Owner

jenshnielsen commented Dec 10, 2015

I put in a pull request with the changes in mdboom/matplotlib#15

With those changes I think this is ready to go

@mdboom mdboom Merge pull request #15 from jenshnielsen/hidp
Set witdh of non interactive figure in nbagg backend.
9b3b733
Owner

jenshnielsen commented Dec 10, 2015

Im happy to merge this now. It is tagged agains 2.0 but I'm not sure if we want to backport it?

@jenshnielsen jenshnielsen added a commit that referenced this pull request Dec 10, 2015

@jenshnielsen jenshnielsen Merge pull request #5383 from mdboom/hidpi-nbagg
Handle HiDPI displays in WebAgg/NbAgg backends
0a08ceb

@jenshnielsen jenshnielsen merged commit 0a08ceb into matplotlib:master Dec 10, 2015

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.4%) to 67.781%
Details

tacaswell removed the needs_review label Dec 10, 2015

Owner

jenshnielsen commented Dec 10, 2015

Happy to backport if anyone thins we should

Owner

mdboom commented Dec 10, 2015

I can see this going either way for backporting. @tacaswell: Wanna be the tie-breaker?

Owner

tacaswell commented Dec 10, 2015

👍 to backporting. It isn't a new plotting feature, it is fixing what is arguably a bug in nbagg on retina

@jenshnielsen jenshnielsen added a commit to jenshnielsen/matplotlib that referenced this pull request Aug 18, 2016

@jenshnielsen @jenshnielsen jenshnielsen + jenshnielsen Merge pull request #5383 from mdboom/hidpi-nbagg
Handle HiDPI displays in WebAgg/NbAgg backends
5bf037e

@tacaswell tacaswell added a commit that referenced this pull request Aug 20, 2016

@tacaswell tacaswell Merge pull request #6956 from jenshnielsen/backport5383
Backport #5383 Needed for retina support in ipympl
0bf79ea

@QuLogic QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Oct 16, 2016

@jenshnielsen @QuLogic jenshnielsen + QuLogic Merge pull request #5383 from mdboom/hidpi-nbagg
Handle HiDPI displays in WebAgg/NbAgg backends
9111ee2
Member

QuLogic commented Oct 16, 2016

Backported to v2.x via #6956.

@QuLogic QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Oct 16, 2016

@QuLogic QuLogic Revert backport of "Merge pull request #5383 from mdboom/hidpi-nbagg"
This reverts commit 9111ee2. The
individual changelog file was already merged into the main one.
734b702

QuLogic added the GUI/webagg label Oct 18, 2016

QuLogic referenced this pull request Nov 24, 2016

Merged

Qt5Agg HiDPI support #7507

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment