Skip to content

Support fractional HiDpi scaling with Qt backends #15656

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

Merged
merged 1 commit into from
Jun 15, 2020

Conversation

timhoffm
Copy link
Member

PR Summary

We need to use the floating point versions devicePixelRatioF instead of devicePixelRatio to scale the canvas correctly when the system uses a fractional scaling.

Since we support older Qt versions, setting and getting devicePixelRatioF is wrapped in helper functions that fall back to the integer version or even no scaling.

Tested with a scaling factor of 1.2:

before:
image

after:
image

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@timhoffm timhoffm added this to the v3.3.0 milestone Nov 10, 2019
@timhoffm timhoffm changed the title Support fractional HiDpi scaling with Qt bakcends Support fractional HiDpi scaling with Qt backends Nov 10, 2019
@timhoffm
Copy link
Member Author

There's an issue with

Traceback (most recent call last):
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/backends/backend_qt5cairo.py", line 25, in paintEvent
    surface = cairo.ImageSurface(cairo.FORMAT_ARGB32, width, height)
TypeError: integer argument expected, got float
Fatal Python error: Aborted
Current thread 0x00007f065e8d2700 (most recent call first):
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/backends/backend_qt5.py", line 1044 in mainloop
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/backend_bases.py", line 3395 in show
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/cbook/deprecation.py", line 398 in wrapper
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/pyplot.py", line 266 in show
  File "<string>", line 58 in <module>

While I will try to fix this in isolation, we must check if other places also need an integer conversion. Also, should we cast to int or round to int?

@anntzer
Copy link
Contributor

anntzer commented Nov 10, 2019

Probably do the same as what the qt version does?

@timhoffm timhoffm force-pushed the qt-fractional-hidpi branch from ea8dfdb to c205b60 Compare November 11, 2019 01:34
obj.setDevicePixelRatioF(val)
if hasattr(obj, 'setDevicePixelRatio'):
# Not available on Qt4 or some older Qt5.
obj.setDevicePixelRatio(val)
Copy link
Member

Choose a reason for hiding this comment

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

do we want to cast to int here?

Copy link
Member

Choose a reason for hiding this comment

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

I would think so, given your other Qt PR, though that's for new Qt that wouldn't call this. Though I think it would come from getDevicePixelRatio, so we may expect int anyway?

@@ -19,8 +19,8 @@ def draw(self):
def paintEvent(self, event):
self._update_dpi()
dpi_ratio = self._dpi_ratio
width = dpi_ratio * self.width()
height = dpi_ratio * self.height()
width = int(dpi_ratio * self.width())
Copy link
Member

Choose a reason for hiding this comment

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

We may want to use round here rather than int to move the jumps away from the integers.

However, this may have issues with not being in sync with what we do else were in the code (which is int).

@QuLogic QuLogic modified the milestones: v3.3.0, v3.4.0 May 1, 2020
@timhoffm timhoffm mentioned this pull request Jun 4, 2020
@tacaswell tacaswell force-pushed the qt-fractional-hidpi branch from c205b60 to eb0068c Compare June 11, 2020 22:34
@tacaswell tacaswell modified the milestones: v3.4.0, v3.2.2 Jun 11, 2020
@tacaswell tacaswell added Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. and removed status: needs revision status: work in progress labels Jun 11, 2020
@tacaswell
Copy link
Member

It turns out Qt now has an enum to control how it rounds to pixels (https://doc.qt.io/qt-5/qt.html#HighDpiScaleFactorRoundingPolicy-enum).

You can test this behavior by setting the QT_SCALE_FACTOR=1.5 ENV in linux and running

import matplotlib.pyplot as plt
import numpy as np

th = np.linspace(0, 2*np.pi, 512)

fig, ax = plt.subplots()
ax.plot(th, np.sin(th))
ax.plot(th, np.cos(th))

ax.set_title(fig.canvas._dpi_ratio)

plt.show()

Without this patch it rounds down via truncating for non-integer ratios. I think what is going on is that we are getting the integer version of the dpi scaling and render the figure at (N, M) size. However, the widgets are seeing the fractional part and actually taking up (N1.2, M1.2) pixels on screen. When we paint the rendered figure Qt helpfully re-samples to the actual number of pixels and our anti-aliasing interacts really badly with that.

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

Take my review as only 1/2 as I did the rebase.

@tacaswell tacaswell force-pushed the qt-fractional-hidpi branch from eb0068c to c056bfa Compare June 12, 2020 01:12
Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

This test always fails for me when run with everything else, so I'm not 100% on this, but it does pass when run by itself.

obj.setDevicePixelRatioF(val)
if hasattr(obj, 'setDevicePixelRatio'):
# Not available on Qt4 or some older Qt5.
obj.setDevicePixelRatio(val)
Copy link
Member

Choose a reason for hiding this comment

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

I would think so, given your other Qt PR, though that's for new Qt that wouldn't call this. Though I think it would come from getDevicePixelRatio, so we may expect int anyway?

@timhoffm
Copy link
Member Author

I don‘t have the time to look into this in the next couple of days. Anybody can take over and push to this PR if necessary.

Author:    Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@tacaswell tacaswell force-pushed the qt-fractional-hidpi branch from c056bfa to 17ff6ba Compare June 15, 2020 19:43
if hasattr(obj, 'setDevicePixelRatioF'):
# Not available on Qt<5.6
obj.setDevicePixelRatioF(val)
if hasattr(obj, 'setDevicePixelRatio'):
Copy link
Member

Choose a reason for hiding this comment

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

This should be elif, no? Or you'll set pixel ratio two different ways.

Copy link
Member

Choose a reason for hiding this comment

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

I opened #17640 with this change.

@jklymak jklymak merged commit 77f23da into matplotlib:master Jun 15, 2020
@jklymak
Copy link
Member

jklymak commented Jun 15, 2020

arghh.sorry @QuLogic

@lumberbot-app
Copy link

lumberbot-app bot commented Jun 15, 2020

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
$ git checkout v3.2.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 77f23da286def97e4f05f4c59f2d5c4f0bfe7af2
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #15656: Support fractional HiDpi scaling with Qt backends'
  1. Push to a named branch :
git push YOURFORK v3.2.x:auto-backport-of-pr-15656-on-v3.2.x
  1. Create a PR against branch v3.2.x, I would have named this PR:

"Backport PR #15656 on branch v3.2.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

@QuLogic QuLogic mentioned this pull request Jun 15, 2020
2 tasks
tacaswell pushed a commit to tacaswell/matplotlib that referenced this pull request Jun 16, 2020
…t backends

Merge pull request matplotlib#15656 from timhoffm/qt-fractional-hidpi

Support fractional HiDpi scaling with Qt backends

Conflicts:
	lib/matplotlib/backends/backend_qt5.py
	lib/matplotlib/backends/qt_compat.py
          - only backport relevant changes
@timhoffm timhoffm deleted the qt-fractional-hidpi branch June 16, 2020 11:39
@anntzer
Copy link
Contributor

anntzer commented Oct 20, 2020

@timhoffm Actually, is the setDevicePixelRatioF shim ever used? AFAICT there's only setDevicePixelRatio, which directly takes a qreal (https://doc.qt.io/qt-5/qpixmap.html#setDevicePixelRatio), but no such thing as setDevicePixelRatioF. (On the getter side there's indeed both devicePixelRatio and devicePixelRatioF https://doc.qt.io/qt-5/qpaintdevice.html#devicePixelRatio.)

@timhoffm
Copy link
Member Author

timhoffm commented Oct 20, 2020

@anntzer You are probably right. I probably did the getter first and assumed that the setter follows the same pattern.

td;dr: Additional findings on the getter devicePixelRatio()

Digging a bit deeper: It seems that QPaintDevice initially only had int QPaintDevice::devicePixelRatio() but grew qreal QPaintDevice::devicePixelRatioF() in Qt 5.6. Note that QPaintDevice itself does not support setting the pixel ratio.

However, the derived classes QImage and QPixmap seemed to always have had floating point support via qreal devicePixelRatio() and void setDevicePixelRatio(qreal scaleFactor). (at least since Qt 5.5 - The Qt archive does not seem to have docs for older versions). I find that a little surprising in Qt - I would not have expected that they overload the API in the subclasses.

I was about to propose that we could replace _devicePixelRatioF(obj) by obj.devicePixelRatio(). That would work for QPixmap and QImage. But we also use _devicePixelRatioF in FigureCanvasQt, which inherits -> QWidget -> QPaintDevice but does not overload devicePixelRatio for qreal.

So indeed we could use devicePixelRatio() directly on all QPixmap, QImage and move the contents of _devicePixelRatioF to it's only needed usage in FigureCanvasQt._dpi_ratio.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI: Qt Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants