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

Refactor DPI scaling and fix the GTK implementation. #904

Merged
merged 29 commits into from
May 20, 2020

Conversation

xStrom
Copy link
Member

@xStrom xStrom commented May 7, 2020

Background

This work got started due to the bug report in #862. @makoConstruct ran into an issue with GTK where there would occasionally be a single line of unused space in the window. I confirmed that this was an issue related to DPI scaling. @makoConstruct had a DPI of 75.0 but the issue appears with a bunch of other DPIs too.

Reproducing the bug

On my Ubuntu 19.10 I could easily reproduce these issues by changing my DPI via gnome-tweaks. Changing the text scaling factor is what changes the DPI. Entering 0.78 will roughly match a DPI of 75.0. Other fractional numbers work too. In my testing the GTK DPI handling was completely broken.

The calculator with master and with this PR

before after

Getting more value out of time

As I already had researched the DPI code thoroughly to understand how to fix GTK, I figured I might as well improve the druid-shell DPI scaling system in general.

I added the Scale struct, which is a platform independent resolution scaling helper. It can do proper scaling per axis and has a bunch of helper methods for converting between logical and platform pixels.

Previously druid-shell was leaking some platform pixels via WinHandler::size, which were handled by e.g. druid. I moved that into druid-shell so that all DPI related work is a druid-shell implementation detail. If someone really wants then they can still get information about the platform pixels and the scaling by fetching the Scale via WindowHandle::get_scale.

I also updated the Windows and web backends to make use of the new Scale system.

Pixel terminology

Druid has been using the web terminology where a pixel and a pixel aren't the same thing. This is confusing to say the least. There is the CSS pixel unit (abbreviated px) and the device pixel (abbreviated px). I know we've been generally following web naming, but I'm convinced this is not a case where it makes sense. The situation in the web world with px and px isn't the result of some wise design, it's just an overloading of a term in the name of backwards compatibility because all the existing docs/code already use pixel/px. We have a chance to be much less ambiguous and confusing.

Unfortunately there isn't really industry consensus about what to call logical pixels.

Are there any others with good abbreviation?

I think it's important that we move away from calling both platform pixels and logical pixels - pixels. I think we should keep platform pixels as pixels and use something else for logical pixels, something that isn't confusing and has a good abbreviation.

The problem with dip/dp is that for a newcomer it's easy to imagine it meaning device pixel. It's not even that wrong, that's what the d truly stands for! We want the opposite effect.

The problem with pt is that it would be overloading a different existing unit, from the text/print world. It also has a bit of a clash with Point.

It's not super clear to me what the best choice is. Suggestions and discussion is welcome on this. What I do know is that I want to stop the use of pixel for both concepts.

For this PR I went with point/pt, but it can be changed if we think another choice is superior.

Future work

This PR addresses the DPI issue on the shell side, but there's still a remaining issue on the druid side. While the shell issue revealed itself as an empty pixel line at the edge, the druid issue reveals itself as a clipped pixel line at the same edges. It has to do with BoxConstraints expanding to integers. The solution however isn't as trivial as just removing the expansion, because some other widgets like Flex depend on that behavior. A more thorough solution is needed, which is out of scope for this PR here.

Fixes #862

@xStrom xStrom added shell/win concerns the Windows backend shell/gtk concerns the GTK backend shell/web concerns the web backend S-needs-review waits for review labels May 7, 2020
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

I think this makes a lot of sense!

On the naming front, I'd be tempted to split the difference and call these "display points" or "device independent points"; something unusual enough that when someone sees it and doesn't know what it means, they will ask us or otherwise look it up.

druid-shell/src/scale.rs Outdated Show resolved Hide resolved
@xStrom
Copy link
Member Author

xStrom commented May 9, 2020

I think having a Scalable trait is an improvement and will allow for easier extensions, so I implemented it.

I think display point / dp is a fine choice, so I changed everything to that. Using dp in method names will also allow for some backwards compatible rebranding if the need arises.

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

Okay, overall I think this makes sense.

A few questions:

  • Are non-uniform x/y scaling a common thing? This isn't something I've really thought of much, in the past.

  • We're going to have to figure out what this means for bitmap image resources. On apple platforms it's common to ship multiple resources for different resolutions, but there you also have a limited set of known resolutions. I guess this shouldn't be that different; you can have some set of resources, and then based on the scale you'll pick the best one and then scale that, if necessary?

  • I wonder if ultimately scale shouldn't be part of the Env in druid, sort of like locale; when scale changes we might need to change out image assets, similarly to how we might reload localized strings when locale changes.

druid-shell/src/mouse.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/web/window.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/web/window.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/web/window.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/windows/util.rs Outdated Show resolved Hide resolved
@luleyleo luleyleo added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels May 15, 2020
@xStrom
Copy link
Member Author

xStrom commented May 15, 2020

Non-uniform x/y scaling is definitely not common. It is supported by drawing, but the platform scale factor provided by Windows/macOS/web is always uniform from what I've gathered. However Linux supports it, although not via popular GUI configuration tools. So I guess this is mostly a question if we want to support uncommon Linux configurations. The cost of supporting it isn't that high as it's mostly an implementation detail of Scale, especially if you do conversions on things that implement Scalable.

I haven't really looked into DPI scaling images yet, because I felt this PR was getting big as it is already. However that's definitely the next significant step on the road to DPI success. In general yes the app could provide a set of images and then we should select the smallest that is equal or bigger than the target area.

Having Scale in Env might be useful. I'll look more into that and see what can be done and if it makes sense.

@xStrom
Copy link
Member Author

xStrom commented May 16, 2020

Okay I made changes based on the feedback. I added some more documentation around display points and a bunch of doc links pointing to it. I surfaced the Scale type in druid, which also surfaces that documentation.

I split out ScaledArea from Scale. Now Scale is only about scale factors and ScaledArea tracks the drawing area size in px/dp.

I looked into adding Scale to Env but there was no clear way to do it, because Env seems global. However Scale can be different per window. Not sure if Env should grow the capability to have per-window state or if we should have some sort of Event::Scale(Scale) event.

I did add the scale method to WinHandler so the druid-shell part of informing changes to this is done. The druid side will take more thinking and currently it will ignore that change. I think it may make more sense to figure out the bitmap scaling story first - as in does scaling images even work right now and how to do it. Then we can figure out how to best surface scale changes.

In that sense I think this PR can just be the shell part, which is done. We can deal with the druid side in a different PR as we have more answers.

@xStrom xStrom added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels May 16, 2020
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

Okay this looks comprehensive and good overall; my one lingering question is whether or not we can delete some code, but that is legitimately a question, not a request.

Other stuff: let's not worry about Env now, or otherwise try and add to this PR; it's easy enough to follow-up. It is totally reasonable though, to me, that modify items in the Env on a per-window basis, that seems totally reasonable.

druid-shell/src/scale.rs Outdated Show resolved Hide resolved

/// Returns `true` if the specified DPI is approximately equal to the `Scale` DPI.
#[inline]
pub fn dpi_approx_eq(&self, dpi_x: f64, dpi_y: f64) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

is there a specific rationale for this, instead of say making the caller create a Scale and having an approx_eq for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

The rationale has vanished. This used to be useful with an initial implementation that used a different strategy and attempted to always achieve integer logical display points. That no longer being the case combined with Scale now just being the scale factors, we can remove this. The new Scale implements PartialEq which will be just fine for our needs.

druid-shell/src/scale.rs Show resolved Hide resolved
druid-shell/src/scale.rs Show resolved Hide resolved

/// Returns the x axis platform DPI associated with this `Scale`.
#[inline]
pub fn dpi_x(&self) -> f64 {
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 probably just have a single dpi method that returns either a (f64, f64) or Vec2.

}
}

impl Scalable for Line {
Copy link
Member

Choose a reason for hiding this comment

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

Is this used? I would have thought that scaling is happening to points, sizes, and rects, but not other shapes.

}
}

impl Scalable for Insets {
Copy link
Member

Choose a reason for hiding this comment

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

ditto this; do we use insets to store like window chrome size or something?

@xStrom
Copy link
Member Author

xStrom commented May 18, 2020

I pushed a new version which fixes the typo and removes some code.

  • dpi_approx_eq removed because it's not really needed anymore.
  • px_to_dp_xy / px_to_dp_x / px_to_dp_y were adaptations of the old DPI methods in druid-shell. They're no longer used thanks to Scalable and so yeah we can delete them.

I think there's value in keeping the dpi_x and dpi_y methods. They're used right now like this:

let props = D2D1_RENDER_TARGET_PROPERTIES {
    _type: D2D1_RENDER_TARGET_TYPE_DEFAULT,
    pixelFormat: D2D1_PIXEL_FORMAT {
        format: DXGI_FORMAT_B8G8R8A8_UNORM,
        alphaMode: D2D1_ALPHA_MODE_IGNORE,
    },
    dpiX: scale.dpi_x() as f32,
    dpiY: scale.dpi_y() as f32,
    usage: D2D1_RENDER_TARGET_USAGE_NONE,
    minLevel: D2D1_FEATURE_LEVEL_DEFAULT,
};

It's possible to achieve this even with a unified method of course, but it would be an extra line.

Line and Insets are not used right now, but could be useful if say the X11 backend implements its own menu. I don't think the cost of having them is too high and they provide some completeness.

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

Okay, let's not let this sit around too long, it's clearly an improvement. Thanks for doing the deep diving here!

@xStrom xStrom merged commit e397801 into linebender:master May 20, 2020
@xStrom xStrom deleted the gtk-dpi branch May 20, 2020 22:25
@xStrom xStrom removed the S-needs-review waits for review label Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
shell/gtk concerns the GTK backend shell/web concerns the web backend shell/win concerns the Windows backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

every 4 or 5 pixels, widget gets sized slightly smaller than the window
3 participants