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

Add Wayland support #3142

Merged
merged 19 commits into from
Jun 26, 2024
Merged

Add Wayland support #3142

merged 19 commits into from
Jun 26, 2024

Conversation

joakimono
Copy link
Contributor

This PR fixes issue #2432

Implement support for Wayland with EGL in GLSupport.
The functionality is enabled at compile type by cmake option OGRE_GLSUPPORT_USE_WAYLAND, provided that EGL headers are found. Note that when Wayland is enabled, X11 is unavailable.
Some new packages will need to be installed to compile it (tested on Debian 12): libwayland-dev libwayland-egl wayland-protocols pkg-config
For Qt (in addition to default Core and Gui): At least also qtwayland5 and qttools5-private-dev. Tested with Qt 5.15.8.

Once compiled and installed, the feature can be tested with the sample browser.
It is important to explicitly select wayland when starting the application as follows:
SDL_VIDEODRIVER=wayland ./SampleBrowser

Likewise when using ApplicationContextQt: ./MyOgreQt -platform wayland or QT_QPA_PLATFORM=wayland ./MyOgreQt. Note: Ogre Qt and Wayland does not work yet.

This new feature has been tested on Debian 12 on WSL2. There are several things that should be tested and fixed before merging.

  • The SampleBrowser looses mouse capture after a while for some unknown reason. Keyboard events are still functional.
  • Maximising the window causes window decorations (shadows) to not update. It may be an external issue or WSL-specific.
  • I do not understand the parent window mechanisms used in X11 and whether this should be properly replicated with wayland.
  • How should the window creation/deletion etc. be tested for correctness/robustness?
  • The Wayland support and window impl files use a different order of initialisation compared to the X11 counterpart. It may not be an issue, but it should be checked. The reason for reorganisation is due to both the expected order of initialisation by wayland, but also SDL. In particular, there is a workaround also used by bgfx to be able to create egl surface external to SDL (see link to issue in ApplicationContextSDL.cpp below).

joakimono and others added 4 commits May 31, 2024 08:33
    - This is WIP and is largely incomplete
    - It marginally works in case of SDL and has not been tested for Qt or barebones
      `SDK_VIDEODRIVER=wayland ./SampleBrowser` is able to start a wayland window with
      contents, but there is several issues.
    - The [Bites] WindowEventUtilities only has X11 impl and needs a wayland alternative
    - No implementation of getVideoModes (resolution and refresh rates)
BuildingOgre.md Outdated Show resolved Hide resolved
    - Link to wayland stuff in OgreBites also if SDL2 is not found
    - This removes dependency on wayland-protocols since xdg_shell is not used
    - Also removes other unneeded implementations as suggested by reviewer
@joakimono
Copy link
Contributor Author

Regarding loosing mouse motion events: It is lost if the user clicks on the window title line (or moves the window) and then clicks somewhere inside the window.

    - Remove externalWindowHandle from application contexts, unused
    - Add suggestions regarding assert if wayland and where
    - Remove unused render target variables, such as WINDOW
    - Do not override getCustomAttributes, since no additions are made
    - Rename externalDisplay to externalWlDisplay
@joakimono

This comment was marked as resolved.

BuildingOgre.md Outdated Show resolved Hide resolved
@paroj
Copy link
Member

paroj commented Jun 23, 2024

I think this is in a pretty good shape for merging now. The comment spam is mostly a checklist for myself as it might take me a few weeks until I can fix this up. Of course you can do the remaining fixes yourself too :)

other things to watch out:

  • scroll over the diff and check if everything looks good
  • remove commented out code
  • check that all docstrings are correct

    - Remove superfluous code
    - Remove comments
    - Update readme
    - Address some more reviewer comments
    - Also move location for checking of X11 window handle in wayland code
@joakimono
Copy link
Contributor Author

TestContext fails with a segmentation fault at RenderSystem::_createRenderWindow "OGRE VTest Context". It should be resolved before merging.

@joakimono
Copy link
Contributor Author

ApplicationContextQt is still not functional. An error similar to: https://bugreports.qt.io/browse/QTBUG-123866
But it may be some missing configuration in our end. I want to try a newer version of Qt also.

@paroj
Copy link
Member

paroj commented Jun 24, 2024

ApplicationContextQt is still not functional. An error similar to: https://bugreports.qt.io/browse/QTBUG-123866 But it may be some missing configuration in our end. I want to try a newer version of Qt also.

it would be totally fine by me if Wayland support would be only available with Qt >= 6.5

@joakimono
Copy link
Contributor Author

ApplicationContextQt is still not functional. An error similar to: https://bugreports.qt.io/browse/QTBUG-123866 But it may be some missing configuration in our end. I want to try a newer version of Qt also.

it would be totally fine by me if Wayland support would be only available with Qt >= 6.5

Ok. I will investigate further to see if a newer Qt version is beneficial

@@ -129,7 +135,27 @@ namespace OgreBites
p.width = window->width();
p.height= window->height();

p.miscParams["externalWindowHandle"] = std::to_string(size_t(window->winId()));
if (QGuiApplication::platformName() != "wayland") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to investigate whether other platform names may be relevant too.
https://github.com/qt/qtwayland/blob/dev/README

@paroj
Copy link
Member

paroj commented Jun 24, 2024

TestContext fails with a segmentation fault at RenderSystem::_createRenderWindow "OGRE VTest Context". It should be resolved before merging.

I guess the problem is that it does not pass any miscParams:

mWindow = mRoot->createRenderWindow(desc);

which is a nice test case actually. We should assert instead of segfaulting in this case as on Vulkan:

OgreAssert( mWindowHandle, "externalWindowHandle required" );

but yeah.. for merging we should either revert the CI to X11 or fix that

I think maintaining CI with X11 could be advantageous since X11 will likely stay prevalent for a few more years. For now, we could modify the "custombuild" target to just compile-test Wayland.

    + This commit also removes wayland from the regular build
    + Add wayland to custombuild to test build it
@joakimono
Copy link
Contributor Author

Ok. I will investigate further to see if a newer Qt version is beneficial

Some of the interface is nicer for Qt 6.5+, but there will still be a need for including a private header, even at Qt 6.7. Specifically the wl_surface is not publically exposed in the new native interface.
ref: https://planet.kde.org/david-redondo-2022-12-09-wayland-native-interface-in-qt-6-5/

Since an implementation need to use QtGui::Private and deal with version-specific includes anyways, I do not see the point of restricting Ogre with Wayland to 6.5+ (unless there are other features unknown to me).
Qt 6 also requires c++17.

    - This also configures a qt version-specific include for native interface
@paroj paroj marked this pull request as ready for review June 26, 2024 14:21
@paroj
Copy link
Member

paroj commented Jun 26, 2024

tested this locally on Ubuntu 22.04 and added some final fixes. I would merge this next, if it is done from your side.

Out of curiosity as I did not test: does Wayland on Qt work in the current state?

@joakimono
Copy link
Contributor Author

Out of curiosity as I did not test: does Wayland on Qt work in the current state?

No it does not. I suspect there is something missing regarding handling of the wl_surface but I do not know what. Maybe somehow initalise xdg_surface or indicate that it is a parent window. But these are guesses. I don't have time to look more into this for the time being.

@paroj paroj merged commit bb37697 into OGRECave:master Jun 26, 2024
6 checks passed
@paroj paroj mentioned this pull request Jun 26, 2024
@paroj
Copy link
Member

paroj commented Jun 27, 2024

No it does not.

I just tested it on Ubuntu 22.04 & Qt 6.2.4 and I get an undecorated window with the wrong resolution. However input is already working. So likely not much is missing.

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.

2 participants