Details

Reviewers

Campbell Barton (campbellbarton)
Brecht Van Lommel (brecht)
Julian Eisel (Severin)

Summary

This set of patches update the Wayland frontend.

use the screen scale to render all surfaces (window and cursor) at a higher resolution for HiDPI screen
fetch the current cursor theme via D-Bus to match the system settings
support server-side decorations via the 'xdg-decoration' protocol
select Wayland at runtime by setting environment variable 'GHOST_SYSTEM=WAYLAND'

Diff Detail

Repository

rB Blender

Branch

hidpi

Build Status

Buildable 14720
Build 14720: arc lint + arc unit

Event Timeline

Christian Rauch (christian.rauch) requested review of this revision.May 23 2021, 7:20 PM

Christian Rauch (christian.rauch) created this revision.

Harbormaster completed remote builds in B14720: Diff 37405.May 23 2021, 7:20 PM

Christian Rauch (christian.rauch) retitled this revision from This set of patches update the Wayland frontend. to Wayland HiDPI and server-side decoration support.May 23 2021, 7:24 PM

Christian Rauch (christian.rauch) edited the summary of this revision. (Show Details)

rebase, fix issue when switching windows, silent warnings

Harbormaster completed remote builds in B14722: Diff 37408.May 24 2021, 2:06 AM

fix issues with restoring hidden cursors and HiDPI pointer hotspots

Harbormaster completed remote builds in B14736: Diff 37434.May 25 2021, 1:26 AM

This seems like a great step towards completing Wayland support.

CMakeLists.txt
220	Best to leave this for a separate review after this one, where we can check what the impact will be for the buildbot, build instructions and runtime dependencies.
build_files/cmake/platform/platform_unix.cmake
582–597	Can this be left out entirely? We are removing usage of `link_directories` in favor of absolute paths, as recommended by CMake.
intern/ghost/intern/GHOST_ISystem.cpp
55	I don't think we need to use the internal name "Ghost" in environment variables. Something like `BLENDER_WINDOW_SYSTEM` would be better.
55	This makes it seems like you can runtime switch between these systems, but really you can only switch between X11 and Wayland. Everything else is mutually exclusive since enabling headless or SDL disables the other window systems. So I would just limit this to the X11 and Wayland switch and leave out the rest.
77	Capitalize this like the others? Though really this code does nothing so might as well remove it.
92–104	Removed this unused code.
intern/ghost/intern/GHOST_Window.cpp
90	What is this for, and why is it here? This affects all platforms.
intern/ghost/intern/cursor-settings.h
2	Files must have license headers.
2	Rename this file to something like `GHOST_CursorsWayland.h`
6	We don't use this namespace anywhere else in the code, let's not introduce it here. Since this is all in a header file, an anonymous namespace could be used, or left out entirely since all these functions are `static` anyway.
27	Code style is to always use braces, should be fixed here and other places in this file.

This revision now requires changes to proceed.May 25 2021, 3:12 PM

address comments about the cursor header file

Harbormaster completed remote builds in B14768: Diff 37487.May 25 2021, 9:57 PM

I guess the biggest question that is left, is if you want/need something like a runtime selection of the GHOST implementation. If you never expect to switch at runtime between e.g. SDL/Win32, or SDL/X11, then maybe it makes sense to remove this again and just keep it temporary for enabling Wayland at runtime.

build_files/cmake/platform/platform_unix.cmake
582–597	I am not sure. How does it find the library in case it is not on one of the default paths (/usr/lib/, ...)? The build still seems to work if I remove that.
intern/ghost/intern/GHOST_ISystem.cpp
55	I kept it generic, as you could use SDL also on Windows or macOS. I think even X11 would work on macOS. Of course, some settings would be invalid/ignored, like requesting "Win32" or "COCOA" as those are only supported on a single platform. I added the environment variable `GHOST_SYSTEM` mainly so that one can "enforce" a particular display server protocol or framework. If you do not expect that e.g. SDL will ever be used in parallel to Win32/COCOA/X11/WAYLAND, it would be better to only have a temporary variable for enabling Wayland at runtime. My intention with this is to be able to build Wayland by default but only activate at runtime if an environment variable is set. This will make it way more easier to test this more widely. Would you prefer to remove this runtime selection for all GHOST implementations and keep this temporary for the Wayland case (until it is deemed stable and enabled by default)?
92–104	I kept it so it becomes clear that only a single commit has to be "undone" to enable Wayland by default at runtime. But this might change anyway depending on how you want to handle the possible runtime selection of the GHOST implementation.
intern/ghost/intern/GHOST_Window.cpp
90	This is need when multiple windows are used and one of them is fully occluded or on another non-visible desktop. The reason for this is that, at least on Wayland, a fully hidden window will not receive any callback to save resources. Its content is not visible, so it does not make sense to update the content from the perspective of the display server. However, if multiple windows are used and all of them run in a single thread, then all windows will block if one of them is blocked. I guess the proper way to handle this is to run all windows in separate threads or use multiple event queues. But setting the swap interval to 0 also mitigates this, as the window will then never block.

Hans Goudey (HooglyBoogly) awarded a token.May 25 2021, 10:50 PM

remove runtime selection of GHOST implementation

Harbormaster completed remote builds in B14778: Diff 37509.May 26 2021, 1:47 PM

@Brecht Van Lommel (brecht) I removed the runtime selection of the GHOST implementation via GHOST_SYSTEM entirely.

So this patchset is now just about Wayland support for

supporting HiDPI screens,
fixing some cursor issues,
fetching the cursor theme via D-Bus, and
server-side decoration support.

Brecht Van Lommel (brecht) requested changes to this revision.May 26 2021, 3:57 PM

Brecht Van Lommel (brecht) added inline comments.

build_files/cmake/platform/platform_unix.cmake
582–597	Use `LINK_LIBRARIES` instead of `LIBRARIES` to get the absolute paths: https://cmake.org/cmake/help/latest/module/FindPkgConfig.html
intern/ghost/intern/GHOST_Window.cpp
90	My understanding is that disables v-sync, which leads to screen tearing. So if this change is required, it should be isolated to Wayland. If it causes screen tearing on Wayland, that's also something that should be documented in the code and indicated as a temporary workaround.

This revision now requires changes to proceed.May 26 2021, 3:57 PM

use absolute library paths, set swap interval 0 only for Wayland

Harbormaster completed remote builds in B14790: Diff 37531.May 26 2021, 10:42 PM

Christian Rauch (christian.rauch) added inline comments.May 26 2021, 10:42 PM

build_files/cmake/platform/platform_unix.cmake
582–597	Thanks. That's useful.
intern/ghost/intern/GHOST_Window.cpp
90	I don't know how this affects X11. But on Wayland there are no tearing effects since the compositor always decides when to render the window content. So the content will generally always represent only a single state. I placed that in the `GHOST_WindowWayland` constructor so it will only affect the Wayland implementation.

Brecht Van Lommel (brecht) accepted this revision.May 31 2021, 4:27 PM

This revision is now accepted and ready to land.May 31 2021, 4:27 PM

rebase on master

Harbormaster completed remote builds in B14907: Diff 37777.Jun 2 2021, 11:56 PM

@Campbell Barton (campbellbarton), @Brecht Van Lommel (brecht), @Julian Eisel (Severin) Is this ready to be merged? If possible, I would prefer to keep the commit history to better track down any issues I haven't noticed.

It can be merged.

For commit history, it's fine to split the patch up in multiple commits. But there should be no merge commits, and bug fixes should be squashed.

In D11368#293698, @Brecht Van Lommel (brecht) wrote:

It can be merged.

For commit history, it's fine to split the patch up in multiple commits. But there should be no merge commits, and bug fixes should be squashed.

Since I got access to commit to the upstream repo, should I generally just merge my own commits into master myself once any reviewer accepts the commits? Or does one of the reviewers have to do this?

You can commit changes to the master branch yourself, from what I can tell you were given commit rights to be able to do this.

The commits have been merged to master. Thanks for the review.

Christian Rauch (christian.rauch) mentioned this in T76428: GHOST/Wayland Support.Jun 3 2021, 7:34 PM

Christian Rauch (christian.rauch) mentioned this in T87962: "Toggle Window Fullscreen" on the new Fedora 34 (Wayland mode) makes mouse cursor disappear..Jul 3 2021, 7:51 PM

Omar Emara (OmarSquircleArt) mentioned this in T98612: Window Decorations Not Present on GNOME - Wayland.Jun 8 2022, 2:21 PM

Wayland HiDPI and server-side decoration support
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 37405

CMakeLists.txt

build_files/cmake/macros.cmake

build_files/cmake/platform/platform_unix.cmake

intern/ghost/CMakeLists.txt

intern/ghost/intern/GHOST_ISystem.cpp

intern/ghost/intern/GHOST_SystemWayland.h

intern/ghost/intern/GHOST_SystemWayland.cpp

intern/ghost/intern/GHOST_Window.cpp

intern/ghost/intern/GHOST_WindowWayland.h

intern/ghost/intern/GHOST_WindowWayland.cpp

intern/ghost/intern/cursor-settings.h

Wayland HiDPI and server-side decoration supportClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 37405

CMakeLists.txt

build_files/cmake/macros.cmake

build_files/cmake/platform/platform_unix.cmake

intern/ghost/CMakeLists.txt

intern/ghost/intern/GHOST_ISystem.cpp

intern/ghost/intern/GHOST_SystemWayland.h

intern/ghost/intern/GHOST_SystemWayland.cpp

intern/ghost/intern/GHOST_Window.cpp

intern/ghost/intern/GHOST_WindowWayland.h

intern/ghost/intern/GHOST_WindowWayland.cpp

intern/ghost/intern/cursor-settings.h

Wayland HiDPI and server-side decoration support
ClosedPublic
Actions

Revision Contents
Changeset List