Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

HiDPI #329

Merged
merged 16 commits into from
Nov 10, 2017
Merged

HiDPI #329

merged 16 commits into from
Nov 10, 2017

Conversation

ddevault
Copy link
Contributor

@ddevault ddevault commented Oct 24, 2017

  • Accept output scale in the config
  • Send output enter/leave events
  • Render surfaces scaled up by their scale factor
  • Consider the new surface size accurately with respect to pointer events
  • Scale client cursors
  • Scale subsurfaces
  • Correctly handle surfaces whose scale factor does not match the output
  • Scale rootston cursor
  • Report bug to GTK: moving window from scale=1 -> scale=2 causes incorrect cursor scale
  • Input for rotated surfaces
  • Increase input fidelity

@@ -135,4 +135,8 @@ struct wlr_surface *wlr_surface_get_main_surface(struct wlr_surface *surface);
*/
struct wlr_subsurface *wlr_surface_subsurface_at(struct wlr_surface *surface,
double sx, double sy, double *sub_x, double *sub_y);

void wlr_surface_send_enter(struct wlr_surface *surface,
struct wlr_output *output);
Copy link
Member

@emersion emersion Oct 24, 2017

Choose a reason for hiding this comment

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

We should send leave too.

}
if (output && output != view->output) {
view->output = output;
wlr_surface_send_enter(view->wlr_surface, output->wlr_output);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we send one enter event for each output which intersects with the view?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question.

Copy link
Member

Choose a reason for hiding this comment

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

Spec: https://wayland.freedesktop.org/docs/html/apa.html#protocol-spec-wl_surface-event-enter

Note that a surface may be overlapping with zero or more outputs.

So yeah, we should send enter to all outputs that intersects with the view.

@@ -28,8 +28,7 @@ static void wl_output_send_to_resource(struct wl_resource *resource) {
if (version >= WL_OUTPUT_MODE_SINCE_VERSION) {
struct wlr_output_mode *mode;
wl_list_for_each(mode, &output->modes, link) {
// TODO: mode->flags should just be preferred
uint32_t flags = mode->flags;
uint32_t flags = mode->flags & ~WL_OUTPUT_MODE_PREFERRED;
Copy link
Member

Choose a reason for hiding this comment

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

Does this flag break things?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this backwards.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, so:

  • WL_OUTPUT_MODE_PREFERRED has only one bit on
  • ~WL_OUTPUT_MODE_PREFERRED has only one bit off, the preferred bit (all others on)
  • mode->flags & ~WL_OUTPUT_MODE_PREFERRED is mode->flags with the preferred bit off

But the comment says the contrary: mode->flags should only be able to change WL_OUTPUT_MODE_PREFERRED (ie. should not be able to set WL_OUTPUT_MODE_CURRENT).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I already said I did it backwards -_-

Copy link
Member

Choose a reason for hiding this comment

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

Oh, sorry. I though you were speaking about the ~, saying it inverts bits so it would be fine.

@ddevault
Copy link
Contributor Author

I'm done for today, but if anyone is reviewing my commits and sees the reason behind this bug, I would appreciate your insight:

https://sr.ht/_HSY.png

gnome-calculator should not show up at all on the right output - but it does.

@ddevault
Copy link
Contributor Author

I think I figured out an approach that will address that problem, writing it down here for poserity -

We need to treat HiDPI displays in the output layout as scaled up - that is, my 4K display should be arranged as if it were 1920x1080, and its x/y position relative to this. Then, at the renderer, we can scale it up or down depending on the scale of the surface and the genuine size of the output.

void (*activate)(struct roots_view *view, bool active);
void (*resize)(struct roots_view *view, uint32_t width, uint32_t height);
void (*set_position)(struct roots_view *view, double x, double y);
void (*close)(struct roots_view *view);
};

void view_get_size(struct roots_view *view, struct wlr_box *box);
void view_get_size(const struct roots_view *view, struct wlr_box *box);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename this to get_box now that this also returns the position?

@ddevault ddevault changed the title HiDPI [WIP] HiDPI Oct 27, 2017
A similar change should probably be applied to hardware cursors, though
more complicated. Also, this doesn't actually fix the issue where the
cursor is too small when over a scale=2 surface. Apparently they don't
set their cursor scales to 2. Seems like a client bug? idk
Something about my math is off, but I'm not certain what. Would
appreciate a second opinion.
Except for subsurfaces not rendering at the right scale. But that part
is (somewhat) easy.
@ddevault
Copy link
Contributor Author

ddevault commented Nov 3, 2017

Woo-hoo, got it working

set_view_focus(input, view->desktop, view);
wlr_seat_keyboard_notify_enter(input->wl_seat, view->wlr_surface);
Copy link
Member

Choose a reason for hiding this comment

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

This is already done in set_view_focus:

wlr_seat_keyboard_notify_enter(input->wl_seat, view->wlr_surface);

@@ -450,6 +451,8 @@ void wlr_output_cursor_set_surface(struct wlr_output_cursor *cursor,
}

bool wlr_output_cursor_move(struct wlr_output_cursor *cursor, int x, int y) {
x *= cursor->output->scale;
y *= cursor->output->scale;
Copy link
Member

Choose a reason for hiding this comment

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

Because of this, maybe we want to accept doubles, multiply them, and cast them to ints right away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is in my todo list

@emersion
Copy link
Member

emersion commented Nov 3, 2017

We could add a config field for XWayland DPI:

Xwayland
-dpi int               screen resolution in dots per inch

Also see https://bugs.freedesktop.org/show_bug.cgi?id=9331

@ddevault
Copy link
Contributor Author

ddevault commented Nov 3, 2017

Are you sure that's the right bugzilla link?

@emersion
Copy link
Member

emersion commented Nov 3, 2017

Whoops, here's the link: https://bugs.freedesktop.org/show_bug.cgi?id=93315

An interesting but complicated idea if we want to really support HiDPI in XWayland is this comment:

Could we run two xwayland instances? first display is lodpi, and second is hidpi?

@ddevault
Copy link
Contributor Author

ddevault commented Nov 3, 2017

Could we run two xwayland instances? first display is lodpi, and second is hidpi?

We wouldn't be able to move windows between them. We should run an Xwayland server at the highest DPI among our monitors and scale down views on some displays. But it will be very messy.

We now use doubles until the last minute, which makes it so we can move
the pointer more precisely. This also includes a fix for tablet tools,
which move absolutely and sometimes do not update the X or Y axis.
There was an issue where it would only work within the boundaries of the
unscaled resolution.
@ddevault
Copy link
Contributor Author

ddevault commented Nov 7, 2017

I have a proposal for how to deal with cursor scale here. Basically, we're going to need to load xcursor themes at multiple scales for them to look good on each output in a mixed-DPI setup. I want to change wlr_cursor to distinguish the surface cursor from the fallback cursor. We'll change the set pixels guy to wlr_cursor_set_fallback_image and stash the data away on the cursor somewhere, and add a scale parameter. wlr_cursor will pick the most appropriate scaled image for the output it's showing on. Then when rootston sets an outputs scale factor, it makes sure that the cursor has the necessary images.

@ddevault ddevault changed the title [WIP] HiDPI HiDPI Nov 9, 2017
@ddevault
Copy link
Contributor Author

ddevault commented Nov 9, 2017

Looking for feedback on this now. I'm going to spin out the remaining work into separate tickets.

@@ -231,14 +231,15 @@ void wlr_output_destroy(struct wlr_output *output) {

void wlr_output_effective_resolution(struct wlr_output *output,
int *width, int *height) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's worth it to have doubles here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I'm not sure we get any benefit from that. This isn't a place where loss of precision impacts anything.

Copy link
Member

Choose a reason for hiding this comment

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

Well, we use it below for the cursor code:

https://github.com/swaywm/wlroots/pull/329/files#diff-8c8783fe608815c9626588d85b00f2f4R273

What we basically do is: (int)(width / scale) * scale.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but there are no displays (or if there are, I've never heard of them) with odd dimensions so you are always going to be dealing with integers here anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, so we should be fine for now. We won't be anymore when we'll introduce fractional scaling.

@emersion
Copy link
Member

emersion commented Nov 9, 2017

  • New views are no longer centered on screen
  • weston-terminal is blurry (same for all weston demos)
  • Subsurfaces seem to work properly
  • QT5 works
  • (Those two TODOs from the PR description)

@ddevault
Copy link
Contributor Author

ddevault commented Nov 9, 2017

weston-terminal is blurry (same for all weston demos)

I'm not convinced that weston supports hidpi

New views are no longer centered on screen

Genuine bug

@ddevault ddevault merged commit aafb00a into master Nov 10, 2017
@ddevault ddevault deleted the hidpi branch November 10, 2017 13:27
@main-- main-- mentioned this pull request Nov 22, 2017
10 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants