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

Fix Vulkan renderer on wayland #3143

Merged
merged 2 commits into from
Aug 2, 2023
Merged

Conversation

belegdol
Copy link
Contributor

This fixes the outstanding problems mentioned in #2416. I do not really know C++ so tips how to determine wl_surface dynamically are welcome.
In order to test this, --with-sdl --with-wayland needs to be added to makefile line 116.

@belegdol
Copy link
Contributor Author

belegdol commented Jul 26, 2023

To be precise: we need a way of passing the wmi.info.wl.surface from

struct wl_surface* surface = wmi.info.wl.surface;

to
https://github.com/belegdol/bgfx/blob/1628329a12fdaa74ccfd6eeca5b176a1b7ddf689/src/renderer_vk.cpp#L6782
I do not know how to do that, the existing structures appear to only be able to pass display and window handles but not the surface one. For the proof-of-concept I inspected the value with the debugger and hardcoded it. It works in gdb only.

@belegdol
Copy link
Contributor Author

Would something like this be acceptable?

@belegdol belegdol marked this pull request as ready for review July 31, 2023 12:21
@belegdol belegdol requested a review from bkaradzic as a code owner July 31, 2023 12:21
@belegdol belegdol marked this pull request as draft July 31, 2023 12:44
Copy link
Owner

@bkaradzic bkaradzic left a comment

Choose a reason for hiding this comment

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

There won't be Wayland surface... PR needs to work as it would be with X, Windows, etc. nothing special won't be added for Wayland.

@belegdol belegdol marked this pull request as ready for review August 1, 2023 10:03
@belegdol
Copy link
Contributor Author

belegdol commented Aug 1, 2023

Turns out it was simpler than originally thought.

@belegdol belegdol requested a review from bkaradzic August 1, 2023 10:04
@belegdol belegdol changed the title Initial proof-of-concept of vulkan renderer working on wayland Fix Vulkan renderer on wayland Aug 1, 2023
@bkaradzic
Copy link
Owner

Much better... what is setting this WL_EGL_PLATFORM?

@belegdol
Copy link
Contributor Author

belegdol commented Aug 2, 2023

It is set by genie.lua:

bgfx/scripts/genie.lua

Lines 207 to 209 in 8b6a6bd

if _OPTIONS["with-wayland"] then
defines { "WL_EGL_PLATFORM=1" }
end

@bkaradzic bkaradzic merged commit 3101a0d into bkaradzic:master Aug 2, 2023
13 checks passed
@belegdol belegdol deleted the wayland_vulkan branch August 2, 2023 04:46
mipek pushed a commit to mipek/bgfx that referenced this pull request Mar 2, 2024
* Initial proof-of-concept of vulkan renderer working on wayland

* Get wayland surface from the window handle
@bkaradzic
Copy link
Owner

@belegdol Check this: https://github.com/libsdl-org/SDL/pull/9345 (intentionally in code quotes to not notify the issue, since it has nothing to do with bgfx, just notifying you about the issue).

@belegdol
Copy link
Contributor Author

@belegdol Check this: https://github.com/libsdl-org/SDL/pull/9345 (intentionally in code quotes to not notify the issue, since it has nothing to do with bgfx, just notifying you about the issue).

Thanks for the info, this is a bit disappointing. In any case, slow is better than crashing in my opinion. Additionally, for cases like mame - which motivated me to submit this PR in the first place - getting every last bit of GPU performance is not that critical. mame runs fast enough on my machines at least.

@bkaradzic
Copy link
Owner

In any case, slow is better than crashing in my opinion.

Yes. Anyhow, the point of notifying you is just to keep you informed. No action needed on bgfx.

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