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 an EGL driver and port X11 OpenGL handling to it #68188

Closed
wants to merge 1 commit into from

Conversation

Riteo
Copy link
Contributor

@Riteo Riteo commented Nov 2, 2022

This uncouples Godot from GLX, effectively allowing other DisplayServers like Wayland to use OpenGL with the new display server agnostic glvnd stack.

This is done by replacing the libGL dependency with libglvnd's own libEGL (which shouldn't be a problem since it has been around for quite a while now), adding a new EGL_ENABLED define (defined in drivers/egl/SCsub) and a new EGLManager driver, which wraps the EGL API in a platform-agnostic way.

This new driver is based almost completely on the original GLManager_X11 (the creators of which I thank a lot), cleaning it up and making it follow more closely the coding conventions I learned while working on the rest of the engine. Most notably it removes all window size handling logic (as it's not needed and already did nothing) and, for the sake of being platform-agnostic, moves all X11 VisualInfo handling to its DisplayServer. Other platforms using it will have to implement their own specific quirks on their own, like Wayland having to manipulate its own wayland_egl handles. Note that this driver currently asks only for OpenGL, but it should be easy to adapt to also ask GLES, depending on the platform.

To use it, all platforms that wish to do so must implement a platform-specific child class which extends its two _get_platform_extension_* methods and make all calls through it.

All OpenGL methods get loaded by GLAD through a code path selected by the EGL_ENABLED define in RasterizerGLES3.

Discrete GPU detection has been blindly ported (I can't test it) and moved to the EGL driver directory (although it uses OpenGL stuff to get the renderer name).

Note that, although AFAIK EGL is supported (or may even be already used) on Android, I haven't ported anything other than X11 stuff to this driver.

I got also told that currently VR handling depends on GLX, but as I got no VR equipment I'm unable to work on this this myself. I'd be glad to assist any VR developer that wishes to help me with this problem.

@Calinou Calinou added this to the 4.0 milestone Nov 2, 2022
Riteo added a commit to Riteo/godot that referenced this pull request Nov 2, 2022
This should be all, but I'll only have the confirmation when it'll get
merged by looking at the final diff.
@dsnopek
Copy link
Contributor

dsnopek commented Nov 3, 2022

Thanks for splitting out this PR!

Note that, although AFAIK EGL is supported on Android, I haven't ported anything other than X11 stuff.

AFAIK, Android is already using EGL.

I got also told that currently VR handling depends on GLX, but as I got no VR equipment I'm unable to work on this this myself. I'd be glad to assist any VR developer that wishes to help me with this problem.

I'll paraphrase what I wrote over on #57025:

I'm working on a OpenXR with OpenGL PR (#67775), and the only way in the core standard to initialize OpenXR on X11 is using the XrGraphicsBindingOpenGLXlibKHR struct, which needs to take some pointers from GLX. I did discover that there is a vendor extension from Monado (XR_MNDX_egl_enable) that appears to allow initializing via EGL. If we can make the assumption that anyone on Linux is using Monado, we could perhaps rely on this extension? I haven't dug into it any deeper than looking at the docs, though, so this would need to be tested.

However, if we can't rely on that extension, then for OpenXR we'll need to stick with GLX for X11. There's perhaps a way to use GLX, but load libGL.so dynamically to prevent the dependency for folks who only have Wayland (without xwayland)?

@BastiaanOlij What are your thoughts?

@clayjohn Are there any other considerations with switching from GLX to EGL on X11?

@Riteo
Copy link
Contributor Author

Riteo commented Nov 3, 2022

AFAIK, Android is already using EGL.

Oh, I had no idea. Perhaps we could let it use this driver (in another PR) if all goes well.

@Riteo Riteo force-pushed the egl-manager branch 2 times, most recently from 0e74dc5 to 729c9fb Compare November 3, 2022 21:38
@Riteo
Copy link
Contributor Author

Riteo commented Nov 3, 2022

I just (blindly) ported the GPU detection code, but I have no way of testing it. Please let me know if it works.

@BastiaanOlij
Copy link
Contributor

@dsnopek I guess for OpenXR we can simply detect that extension and fail initialization if that extension is not available and EGL is used. Hopeful OpenXR runtimes on Linux will implement the extension.

@dsnopek
Copy link
Contributor

dsnopek commented Nov 4, 2022

@BastiaanOlij so, your inclination is to go for the switch to EGL on X11, and not worry that we'd be incompatible with SteamVR? As I said on RocketChat, my inclination would be to stick with GLX for now (but dynamically load libGL.so somehow) to retain compatibility. But I'm fine to go with whatever the consensus is!

@Riteo Riteo force-pushed the egl-manager branch 2 times, most recently from 0288ba5 to 99d52cd Compare November 4, 2022 14:34
@Riteo
Copy link
Contributor Author

Riteo commented Nov 4, 2022

@dsnopek I agree with bastiaan mainly due to the incentivization argument, followed by the other things I said.

BTW if this gets merged I'd be glad to help you transition to the EGLManager on your OpenGL VR code :)

@Riteo Riteo marked this pull request as ready for review November 4, 2022 17:58
@Riteo Riteo requested review from a team as code owners November 4, 2022 17:58
@Riteo
Copy link
Contributor Author

Riteo commented Nov 4, 2022

Marking this PR as ready for review as I tested it a bit through a modified version of Calinou's 4.0 version of the sponza demo (https://github.com/Calinou/godot-sponza/tree/4.0-dev) and it seems to work fine.

Now, if the consensus is ok with the possible OpenXR runtime issues abundantly discussed previously, I think that this can be merged. (For the reference I'd be ok, but I'm not an XR user, so I'm not sure how much my opinion is worth in this regard)

Also, as I said before, if this PR gets merged I'll have no issues trying to give my hand (sadly not in VR) at helping anyone who might be affected by this change, such as @dsnopek :)

This uncouples Godot from GLX, effectively allowing other
`DisplayServer`s like Wayland to be implemented.
@Riteo
Copy link
Contributor Author

Riteo commented Nov 7, 2022

So apparently, mostly due to issues with nvidia and OpenXR compatibility, it may still be wanted to fallback to GLX. This means that I'll have to revisit how these interfaces are loaded. Expect another PR which loads GLX dynamically.

Edit: this has been done. Once I figure out a clean way of letting the DisplayServer choose its own loading pointer it should be possible to make X11 choose either GLX or EGL.

@Riteo
Copy link
Contributor Author

Riteo commented Nov 21, 2022

Thinking about it, like @clayjohn noted, adding EGL support for X11 while still fallbacking to GLX wouldn't have that many pros compared to just GLX, so this PR will switch to just implementing an EGL driver that the Wayland PR will use.

Wayland will depend on this one, this won't be supposed to be merged as-is.

@Riteo
Copy link
Contributor Author

Riteo commented Nov 21, 2022

You know what, I'm closing this one and creating a fresh PR, it wouldn't make much sense to overwrite everything, making the discussion above disconnected from this other approach.

@Riteo Riteo closed this Nov 21, 2022
@Riteo
Copy link
Contributor Author

Riteo commented Nov 21, 2022

Superceded by #68944.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants