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

Supports ScaleFactor for GTK 3 #3003

Closed

Conversation

jairbubbles
Copy link

@jairbubbles jairbubbles commented Sep 18, 2024

GTK has a simple mechanism to scale the UI on high dpi screens. It's just an int. On windows if you set the scaling to 200%, ScaleFactor will return 2. At 125%, you still get 1. (see https://docs.gtk.org/gtk3/method.Widget.get_scale_factor.html)

I modified the code so that we create a bigger surface and then scale down when we render in the Cairo context. I added a IgnorePixelScaling to mimic what has be done on other views, I set the default to true so that my changes are completely harmless. You need to opt-in to get the new behavior.

My GTK skills are pretty close to zero, so please look two times at the proposed changes 😅

Tested on Windows only on the GTK sample.

Before (the text is blurry):
image

After (the text is sharp):
image

@jairbubbles jairbubbles marked this pull request as ready for review September 18, 2024 15:38
@mattleibow
Copy link
Contributor

mattleibow commented Sep 18, 2024

This looks good. The only issue is that the IgnorePixelScaling is false by default in all other platforms and the meaning is flipped.

It is wrong and what you have is technically correct, but when I originally implemented the code, the property was to tell the drawing system that if IgnorePixelScaling == true then we want to ignore the fact that the OS has a different scale to the number on the screen and just stretch the image. I have no idea why that was ever an option anyone would ever want.

So, false is the default and would typically be blurry on high res monitors.

One day we can have a new property that just inverts this one. Maybe we can add a bool ApplyPixelScaling => IgnorePixelScaling and hide this old one. But it is the backwards world in which we live.

@mattleibow
Copy link
Contributor

mattleibow commented Sep 18, 2024

Another thing I also do in other platforms is lie about the canvas size if this is set.

For example for android:

var userVisibleSize = IgnorePixelScaling
	? new SKSizeI((int)(info.Width / density), (int)(info.Height / density))
	: info.Size;

CanvasSize = userVisibleSize;

if (IgnorePixelScaling)
{
	var skiaCanvas = surface.Canvas;
	skiaCanvas.Scale(density);
	skiaCanvas.Save();
}

The reason to use the smaller/scaled down canvas size is that someone drawing the image with this option still assumes the canvas is the same size as the view.

@jairbubbles
Copy link
Author

The only issue is that the IgnorePixelScaling is false by default in all other platforms and the meaning is flipped.

Honestly, it was not clear to me at all what was the meaning of it. 😅

I wrote a small prototype based on SkiaShap and I tested with Blazor/WPF/GTK.

Blazor/WPF did apply the scaling to the surface passed to the paint callback.

Though on Blazor I'm using

<SKGLView OnPaintSurface="OnPaint" IgnorePixelScaling="false"

And on WPF I didn't touch the default value, which should be false ?

(I pushed the requested modification)

@jairbubbles
Copy link
Author

(in fact I'm using SkiaSharp.Views.WPF.SKGLElement which has no such option)

@jairbubbles
Copy link
Author

On Blazor, if set the value to false I get the full width (minus the browser borders):

image

(my screen is 2560x1440, scaling is 125%)

If I set it to true I get a smaller size:

image

I don't get my pixel perfect rendering and I'm sad 😿

@mattleibow
Copy link
Contributor

There is also the e.RawInfo property that should always be the size of the actual pixel canvas. When you set to true, this is where the lies come in. The info says 2000, but we just applied the scaling for you.

@mattleibow
Copy link
Contributor

For blazor, I think the size needs to be set via CSS/style or something. If you run the skia sample in the repo, it should be setting the property to true and also be crisp.

I think I did what this article said: https://web.dev/articles/canvas-hidipi and this mozilla: https://developer.mozilla.org/en-US/docs/Web/API/Window/devicePixelRatio#correcting_resolution_in_a_canvas

@@ -49,7 +50,7 @@ private void OnPaintSurface(object sender, SKPaintSurfaceEventArgs e)
};
using var font = new SKFont
{
Size = 24
Size = 24 * ScaleFactor
Copy link
Contributor

Choose a reason for hiding this comment

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

The sample should maybe not have the scaling. The IgnorePixelScaling = true should mean "i want to work in "scaled pixels" where the 100 canvas pixels == 100 UI pixels. So this font size should match the same size of a label.

Copy link
Author

Choose a reason for hiding this comment

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

I scaled the text size so that on screen I get the same physical dimension.

When I set the option to true the canvas is twice bigger. If I don't scale the text will be really small.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this is a different issue in gtk... I think what is supposed to happen after my rewrite of the property, false is not actually supposed to be blurry but small. Then the ignore will auto scale up

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you able to test any other platforms?

Copy link
Author

Choose a reason for hiding this comment

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

Maybe this is a different issue in gtk... I think what is supposed to happen after my rewrite of the property, false is not actually supposed to be blurry but small. Then the ignore will auto scale up

Maybe I should rename the property so that there's no ambiguity. UseScaleFactor ?

Are you able to test any other platforms?

I can test WSL.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean can you test android/ios/mac/windows? Not using GTK that is.

I think the issue is the property was fixed in the other platforms be be better, and gtk is probably still blurry because I have no idea how gtk works.

I think the correct fix should be when the property is false, everything is small and crisp, and when it is true, everything is big and crisp. Basically, the property just enables the autoscaling to match the UI sizes as opposed to using raw pixels.

If we change the property name then it probably should be in a new PR and do it for all the controls. Right now I think the bug is that gtk never got the property fix to scale with the screen density because I did not know how to do it and no one ever complained.

I don't mind fixing it so that false "breaks" things as long as it matches the rest of the sdks. No one wants a blurry image, so that is never a happy path.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @mattleibow, I won't have time to work more on this. I believe the best is to close the PR.

@jairbubbles
Copy link
Author

There is also the e.RawInfo property that should always be the size of the actual pixel canvas

Yes indeed I get my 2500 px

I think I did what this article said: https://web.dev/articles/canvas-hidipi and this mozilla: https://developer.mozilla.org/en-US/docs/Web/API/Window/devicePixelRatio#correcting_resolution_in_a_canvas

I'll look into it but when running the WPF / Blazor (IgnorePixelScaling="false") I was getting the same results.

Side note but what would be great is to have the scaling info in the SurfaceEventArgs. In my case, I would expect to get 1.25% so that I know I need to scale the rendering of my components.

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

Successfully merging this pull request may close these issues.

2 participants