Skip to content
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

Fix screen grabbing zones coordinates when screen scaling is used #378

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kdsx
Copy link

@kdsx kdsx commented Aug 3, 2020

When the app is running on a system which uses screen scaling factor other
than "1.0", then grabber widget logical coordinates will differ from physical
coordinates. Physical coordinates could be calculated by multiplying logical
coordinates to a device pixel ratio.

When the app is running on a system which uses screen scaling factor other
than "1.0", then grabber widget logical coordinates will differ from physical
coordinates. Physical coordinates could be calculated by multiplying logical
coordinates to a device pixel ratio.
@psieg-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

1 similar comment
@psieg-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@psieg
Copy link
Owner

psieg commented Aug 8, 2020

Thanks for the PR!

Which OS are you testing on? On windows this value seems to be always 1 even when using scaling.

ok to test

@kdsx
Copy link
Author

kdsx commented Aug 8, 2020

When device pixel ratio is 1, then probably everything should work as expected, shouldn't it?

In my case it was Linux/KDE and screen scaling was set to 2 manually in KDE settings.

Also please note that I haven't tested it on multi-screen multi-DPI environment. BTW does Lightpack support such configurations properly?

@psieg
Copy link
Owner

psieg commented Aug 8, 2020

When device pixel ratio is 1, then probably everything should work as expected, shouldn't it?

Oh yes, I didn't mean to imply this would break something.

In my case it was Linux/KDE and screen scaling was set to 2 manually in KDE settings.

SG. I'm not familiar with scaling on linux, but I assume that's the standard way people would change the scaling.

Also please note that I haven't tested it on multi-screen multi-DPI environment. BTW does Lightpack support such configurations properly?

At least on windows, Prismatik is kind of scaling agnostic. It uses desktop coordinates for the widgets and those match with the screenshot coordinates when adjusting for the screen position. The windows scaling doesn't change the resolution.

In your case different screens could have different ratios I assume, so rather than using the QApp's ratio, you should use the ratio given by the Window, i.e. the Widget: grabWidget->devicePixelRatio()

@kdsx
Copy link
Author

kdsx commented Aug 8, 2020

Oh, my bad. I've taken the ratio from the wrong place. Will fix.

@kdsx
Copy link
Author

kdsx commented Aug 8, 2020

At least on windows, Prismatik is kind of scaling agnostic. It uses desktop coordinates for the widgets and those match with the screenshot coordinates when adjusting for the screen position. The windows scaling doesn't change the resolution.

Hmm. Qt scaling works pretty similar on each platform. Does UI look properly scaled when you use it on high-DPI screens?

@psieg
Copy link
Owner

psieg commented Aug 8, 2020

Windows scaling is a mess and most things look acceptable only after restarting them after changing the scale.
For example, this is Prismatik when I set my windows scaling to 200%:
image

I'm not sure why most of the text size does not scale properly in Prismatik...

@kdsx
Copy link
Author

kdsx commented Aug 8, 2020

Ah, got it. App does not use Qt scaling in your case. That's why devicePixelRatio is 1. Instead you tell Windows to
scale the app. So the app is rendered in low-resolution and then up-scaled. To llok good, the app should enable auto-scaling. Also the devicePixelRatio will become 2 :) In my case KDE enables Qt scaling through the environment. In Windows the same approach is also possible, but the better way is to enable scaling in the code. However this is another task. I'll take a look at it later.

@psieg
Copy link
Owner

psieg commented Aug 8, 2020

I'm pretty sure it's not the case that windows scales the app. If that were the case, proportions of the elements in the UI would remain the same, but I guess that's not really relevant for this change.

@zomfg
Copy link

zomfg commented Aug 8, 2020

In your case different screens could have different ratios I assume, so rather than using the QApp's ratio, you should use the ratio given by the Window, i.e. the Widget: grabWidget->devicePixelRatio()

probably grabWidget->screen()->devicePixelRatio()
also I think this might break the widget scaling on macOS, where it currently works (or did anyway) as is, so maybe wrap it in #ifdef MACOS or something for now

@kdsx
Copy link
Author

kdsx commented Aug 8, 2020

I'll try to check it there.

@psieg
Copy link
Owner

psieg commented Aug 9, 2020

probably grabWidget->screen()->devicePixelRatio()

Docs for QGuiApplication: "Use this function only when you don't know which window you are targeting. If you do know the target window, use QWindow::devicePixelRatio() instead."
Docs for QWindow: "Note: For windows not backed by a platform window, ..., the function will fall back to the associated QScreen's device pixel ratio."

The widget's method should be preferred IIUC.

also I think this might break the widget scaling on macOS, where it currently works (or did anyway) as is, so maybe wrap it in #ifdef MACOS or something for now

Indeed, docs: "Common values are 1.0 on normal displays and 2.0 on Apple "retina" displays."

I believe it's currently working on MacOS because we're adjusting for the pixel ratio in the grabber. We might want to do the same for the X grabber instead of changing the maths here, WDYT?

@zomfg
Copy link

zomfg commented Aug 9, 2020

Docs for QGuiApplication: "Use this function only when you don't know which window you are targeting. If you do know the target window, use QWindow::devicePixelRatio() instead."
Docs for QWindow: "Note: For windows not backed by a platform window, ..., the function will fall back to the associated QScreen's device pixel ratio."

The widget's method should be preferred IIUC.

I just tested app, widget, widget->screen and widget returned 1.00 while the other two 1.25 (as per settings), but I only have 1 screen, so it'll be whichever works for multiple screens :D

Indeed, docs: "Common values are 1.0 on normal displays and 2.0 on Apple "retina" displays."

macOS allows (in some cases?) for intermediate values (with some blurriness), I don't know if Qt returns the right values now, I used the native api at the time

I believe it's currently working on MacOS because we're adjusting for the pixel ratio in the grabber. We might want to do the same for the X grabber instead of changing the maths here, WDYT?

the scale is used in mac grabber so we capture a 1x image (since the capture allows almost any target scaling), but the scale returned to GrabberBase isn't adjusted, so it's as if it was already multiplied
the other grabbers aren't as flexible (speaking of which, maybe we can bump DDupl DownscaleMipLevel to 4 when 200% is used?)
So IF Qt method works everywhere and the native apis aren't required maybe it's better to just centralise in GrabberBase
But.. I tested in XFCE, and the scaling doesn't affect Qt apps (you indeed need to set some specific env vars), but if Prismatik is the only Qt app you run in background maybe you won't bother/think scaling properly, so maybe that'll need a native call?... way too hot to think right now for me!

@psieg
Copy link
Owner

psieg commented Apr 12, 2024

Qt6 has changed the coordinate system to a scaling dependent one, so things need to be adjusted. I did that on master meanwhile but could use help testing this on Mac on Linux systems

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants