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

cmake: enable MLN_WITH_EGL and OPENGL_USE_GLES for Wayland #2022

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

JonasVautherin
Copy link
Collaborator

What it achieves

Without these changes, I need to configure with the unintuitive:

cmake -DMLN_WITH_EGL=ON -DMLN_WITH_WAYLAND=ON -DOPENGL_USE_GLES3=ON -Bbuild -S.

With these changes, I can configure successfully with:

cmake -DMLN_WITH_WAYLAND=ON -Bbuild -S.

The change

This change sets two values to TRUE in platform/linux/linux.cmake when MLN_WITH_WAYLAND is set:

  • MLN_WITH_EGL: without this set, it will try to find the GLX component of OpenGL, which is an X11 thing. Without this change I get the following error:
CMake Error at /usr/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find OpenGL (missing: GLX)
CMake Error at /usr/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find OpenGL (missing: OPENGL_opengl_LIBRARY)

Further considerations

One could ask: "is it not an issue to set OPENGL_USE_GLES2, in case someone wants OPENGL_USE_GLES3?", and my (admittedly limited) understanding is that the FindOpenGL module will still detect the right one, but we need one to be set for the check here (otherwise it will look for the GLVND library). In my case, I have glfw-3 installed on my system, and it gets detected even with OPENGL_USE_GLES2 set. Not sure what would happen if I had both installed (my distro doesn't provide both, I can't test 🙈). Maybe it would be better to set OPENGL_USE_GLES3?

Notes

This is my suggestion to improve the Wayland build, following my struggling with #2019.

@jwinarske: FYI

@acalcutt
Copy link
Collaborator

acalcutt commented Jan 16, 2024

You question about OPENGL_USE_GLES2 vs OPENGL_USE_GLES3 reminds me of #1079 where the change to GLES3 had to be reverted because the package that included it was GLES2. I wonder if it is similar here.

@jwinarske
Copy link
Contributor

It's not clear to me why there are GLESv2 and GLESv3 flags. The functions exist in the same file named libgles2.so. There is no libgles3.so. You can create an EGL config that works with both GLESv2 and GLESv3. The resolver handles the differences. Enabling config attributes should be exposed. Apache-2.0 Example:
https://github.com/toyota-connected/ivi-homescreen/blob/main/shell/constants.h#L88

@JonasVautherin
Copy link
Collaborator Author

It's not clear to me why there are GLESv2 and GLESv3 flags.

They apparently mostly help deciding whether find_package(OpenGL) will include GLX and GLVND or not, see here.

The one thing that is not entirely clear to me is that it sets OPENGL_gles2_LIBRARY or OPENGL_gles3_LIBRARY, which are later searched.

But Maplibre only requires the EGL component in the CMakeLists, not the GLES2 or GLES3 components.

If it did, then it would be easy to know which OPENGL_USE_GLESX to set. But because it doesn't, my feeling is that we just need to set one of them for the GLX/GLVND checks.

At least setting either OPENGL_USE_GLES2 or OPENGL_USE_GLES3 works for me, and I know I only have glfw-3 (which I had to install for this very configure step to pass).

@acalcutt
Copy link
Collaborator

If it doesn't make much difference I would think it would be better to set "OPENGL_USE_GLES3", just because the code is ES 3.0 now #995 . pretty much everywhere else 'GLES2' got changed to 'GLES3' (there was just the one case I mentioned where that caused an issue, but most places it was fine.)

@JonasVautherin
Copy link
Collaborator Author

Is there something more I should be doing on this PR? If you don't think it makes sense, we can just close it 👍.

@ntadej
Copy link
Collaborator

ntadej commented Jan 22, 2024

For my curiosity, where is OPENGL_USE_GLES3 used? Because I can't find any reference in the repo, but I guess it's used in one of the dependencies?

@JonasVautherin
Copy link
Collaborator Author

@ntadej: As described above, and as far as I can tell, it seems like it is mostly used in FindOpenGL.cmake. Without OPENGL_USE_GLES2 or OPENGL_USE_GLES3 set, the CMake module for OpenGL will include the GLVND library which is for X11.

@JonasVautherin
Copy link
Collaborator Author

I don't think it will go further than this 🙈.

@ntadej ntadej reopened this Feb 20, 2024
@ntadej
Copy link
Collaborator

ntadej commented Feb 20, 2024

Why close, I think at the moment we just do not have anyone who can test this.

@louwers, @acalcutt, do you think this is safe to just merge as is?

@acalcutt
Copy link
Collaborator

I personally think it is fine, since it only changes anything if that -DMLN_WITH_WAYLAND=ON option is used. Thought I am not familiar with if MLN_WITH_WAYLAND option was actually an option before.

Copy link

Bloaty Results (iOS) 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  [ = ]       0  [ = ]       0    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-2022-compared-to-main.txt

Copy link

Bloaty Results 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  [ = ]       0  [ = ]       0    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2022-compared-to-main.txt

Compared to d387090 (legacy)

    FILE SIZE        VM SIZE    
 --------------  -------------- 
   +18% +21.0Mi  +398% +23.8Mi    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2022-compared-to-legacy.txt

@louwers
Copy link
Collaborator

louwers commented Feb 20, 2024

@JonasVautherin Can you add a comment to this PR? Then let's merge. Sounds like something that will be fixed upstream eventually.

@JonasVautherin
Copy link
Collaborator Author

Can you add a comment to this PR?

Not sure what you mean 😕. Do you want me to modify the description of the PR? 👀

@louwers
Copy link
Collaborator

louwers commented Feb 20, 2024

@JonasVautherin Sorry, what I mean is, could you add a comment to the if() in the CMake file that explains why it is there with a link to this PR?

@JonasVautherin
Copy link
Collaborator Author

Oh, got it 😊. I added comments there, let me know what you think!

@louwers louwers enabled auto-merge (squash) February 21, 2024 12:05
@louwers
Copy link
Collaborator

louwers commented Feb 21, 2024

Thanks, looks good.

@ntadej
Copy link
Collaborator

ntadej commented Feb 21, 2024

It seems codecov is not reporting for some reason ...

@louwers
Copy link
Collaborator

louwers commented Feb 21, 2024

According to https://app.codecov.io/gh/maplibre/maplibre-native/commits it is not uploaded...

@louwers louwers merged commit 61fd33f into maplibre:main Feb 21, 2024
17 checks passed
@JonasVautherin JonasVautherin deleted the cmake-improve-wayland branch February 21, 2024 14:27
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.

5 participants