Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
jklymak merged 1 commit intomatplotlib:masterfromtimhoffm:qt-fractional-hidpi
Jun 15, 2020

Conversation

timhoffm
Copy link
Member

PR Summary

We need to use the floating point versionsdevicePixelRatioF instead ofdevicePixelRatio to scale the canvas correctly when the system uses a fractional scaling.

Since we support older Qt versions, setting and gettingdevicePixelRatioF 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 isFlake 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

@timhoffmtimhoffm added this to thev3.3.0 milestoneNov 10, 2019
@timhoffmtimhoffm changed the titleSupport fractional HiDpi scaling with Qt bakcendsSupport fractional HiDpi scaling with Qt backendsNov 10, 2019
@timhoffm
Copy link
MemberAuthor

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 floatFatal Python error: AbortedCurrent 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

Probably do the same as what the qt version does?

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 fromgetDevicePixelRatio, so we may expectint 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 useround here rather thanint 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 isint).

@QuLogicQuLogic modified the milestones:v3.3.0,v3.4.0May 1, 2020
@timhoffmtimhoffm mentioned this pull requestJun 4, 2020
@tacaswelltacaswell modified the milestones:v3.4.0,v3.2.2Jun 11, 2020
@tacaswelltacaswell added Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. and removed status: needs revision status: work in progress labelsJun 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 theQT_SCALE_FACTOR=1.5 ENV in linux and running

importmatplotlib.pyplotaspltimportnumpyasnpth=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

@tacaswelltacaswell 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.

Copy link
Member

@QuLogicQuLogic 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 fromgetDevicePixelRatio, so we may expectint anyway?

@timhoffm
Copy link
MemberAuthor

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>
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 beelif, 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.

@jklymakjklymak merged commit77f23da intomatplotlib:masterJun 15, 2020
@jklymak
Copy link
Member

arghh.sorry@QuLogic

@lumberbot-app
Copy link

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 tosuggest an improvement.

@QuLogicQuLogic mentioned this pull requestJun 15, 2020
2 tasks
tacaswell pushed a commit to tacaswell/matplotlib that referenced this pull requestJun 16, 2020
…t backendsMerge pull requestmatplotlib#15656 from timhoffm/qt-fractional-hidpiSupport fractional HiDpi scaling with Qt backendsConflicts:lib/matplotlib/backends/backend_qt5.pylib/matplotlib/backends/qt_compat.py          - only backport relevant changes
@timhoffmtimhoffm deleted the qt-fractional-hidpi branchJune 16, 2020 11:39
@anntzer
Copy link
Contributor

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

@timhoffm
Copy link
MemberAuthor

timhoffm commentedOct 20, 2020
edited
Loading

@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 getterdevicePixelRatio()

Digging a bit deeper: It seems thatQPaintDevice initially only hadint QPaintDevice::devicePixelRatio() but grewqreal QPaintDevice::devicePixelRatioF()in Qt 5.6. Note thatQPaintDevice itself does not support setting the pixel ratio.

However, the derived classesQImage andQPixmap seemed to always have had floating point support viaqreal devicePixelRatio() andvoid 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) byobj.devicePixelRatio(). That would work forQPixmap andQImage. But we also use_devicePixelRatioF inFigureCanvasQt, which inherits-> QWidget -> QPaintDevice but does not overloaddevicePixelRatio for qreal.

So indeed we could usedevicePixelRatio() directly on allQPixmap,QImage and move the contents of_devicePixelRatioF to it's only needed usage inFigureCanvasQt._dpi_ratio.

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

@QuLogicQuLogicQuLogic left review comments

@tacaswelltacaswelltacaswell approved these changes

@anntzeranntzeranntzer approved these changes

Assignees
No one assigned
Labels
GUI: QtRelease criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Milestone
v3.2.2
Development

Successfully merging this pull request may close these issues.

5 participants
@timhoffm@anntzer@tacaswell@jklymak@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp