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

[no squash] Migrate the Android port to SDL2 #14498

Merged
merged 3 commits into from Apr 8, 2024
Merged

Conversation

grorp
Copy link
Member

@grorp grorp commented Mar 27, 2024

This PR migrates the Android build to SDL2. Some of the benefits are:

  • Less maintenance
  • Mouse support
  • Controller support (still needs an implementation on the Minetest side)

Please note that since CIrrDeviceSDL doesn't support the ogles1 driver, this PR removes ogles1 support on Android.*

*It should be possible to make CIrrDeviceSDL support ogles1 by using code from https://github.com/MoNTE48/Irrlicht.

To do

This PR is a Ready for Review.

Note that the first two commits just add/delete some boilerplate. The third commit is where it gets interesting.

How to test

Play Minetest on Android.

If you want to see something new and cool, connect a bluetooth mouse and keyboard, disable enable_touch in the settings menu and play like a desktop player.

@grorp grorp added Android Feature ✨ PRs that add or enhance a feature @ Engine Core What happens inside the very engine labels Mar 27, 2024
@rubenwardy rubenwardy added this to the 5.9.0 milestone Mar 27, 2024
@oong819
Copy link
Contributor

oong819 commented Mar 28, 2024

Tons of Java code added, is this considered bad?

@grorp
Copy link
Member Author

grorp commented Mar 28, 2024

Tons of Java code added, is this considered bad?

This isn't our own Java code, it's the Java boilerplate required by SDL (https://github.com/libsdl-org/SDL/tree/SDL2/android-project/app/src/main/java)

@lhofhansl
Copy link
Contributor

Not really important, but I will point out that the SDL backend does not support FSAA.
(hence on my desktop I build irrlicht without SDL)

@grorp
Copy link
Member Author

grorp commented Mar 28, 2024

Not really important, but I will point out that the SDL backend does not support FSAA. (hence on my desktop I build irrlicht without SDL)

I'm not sure why you're pointing this out here instead of in an issue. Here it'll be forgotten once the PR is merged. Anyway, works for me:

On desktop (Linux) with SDL, FSAA works for me (both with opengl and opengl3 video driver)

On Android with SDL, FSAA works for me too (with ogles2 video driver)

I found a problem though: Instead of handling unsupported FSAA values gracefully (by downgrading), the app crashes. This happens for FSAA values > 4 on my phone. The last message in the logs looks like this:

WARNING[Main]: Irrlicht: Could not create window...: Couldn't find matching EGL config (call to eglChooseConfig failed, reporting an error of EGL_SUCCESS)

Although this is an issue with the existing SDL Irrlicht device, I'll probably add a fix to this PR.

@lhofhansl
Copy link
Contributor

I'm not sure why you're pointing this out here instead of in an issue.

Fair enough. I did point it out in an MT and in in Irrlicht issue.

On desktop (Linux) with SDL, FSAA works for me (both with opengl and opengl3 video driver)

That is curious. Whatever I try with SDL, the FSAA is just ignored, whereas with GLX (and no other change) it works. I wonder what's missing on my side.

Probably not good to continue on this PR, as it is only (very) tangentially related.

@sfan5 sfan5 self-requested a review March 29, 2024 10:32
@okias
Copy link
Contributor

okias commented Apr 2, 2024

Best I can do is Acked-by: David Heidelberg <david@ixit.cz> for Android code, but nice!

irr/src/CIrrDeviceSDL.cpp Outdated Show resolved Hide resolved
irr/src/CIrrDeviceSDL.cpp Outdated Show resolved Hide resolved
irr/src/CMakeLists.txt Outdated Show resolved Hide resolved
irr/src/CMakeLists.txt Outdated Show resolved Hide resolved
irr/src/CMakeLists.txt Show resolved Hide resolved
if(NOT ANDROID)
find_package(SDL2 REQUIRED)
else()
# provided by MinetestAndroidLibs.cmake
Copy link
Member

Choose a reason for hiding this comment

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

I guess in the future we could package SDL2 in a way that the standard cmake way works...

irr/src/CMakeLists.txt Show resolved Hide resolved
irr/src/COGLESExtensionHandler.cpp Show resolved Hide resolved
src/CMakeLists.txt Show resolved Hide resolved
@grorp
Copy link
Member Author

grorp commented Apr 3, 2024

The CI doesn't even run, it fails with "The job could not retrieve the repository token." before starting

@sfan5
Copy link
Member

sfan5 commented Apr 4, 2024

Fails to run on my phone:

--------- beginning of main
04-04 15:34:50.727 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: Using system-wide paths (NOT RUN_IN_PLACE)
04-04 15:34:50.732 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: Detected share path: /storage/emulated/0/Android/data/net.minetest.minetest/files/Minetest
04-04 15:34:50.732 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: Detected user path: /storage/emulated/0/Android/data/net.minetest.minetest/files/Minetest
04-04 15:34:50.732 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: Detected cache path: /data/user/0/net.minetest.minetest/cache
04-04 15:34:50.732 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: Using in-place locale directory /storage/emulated/0/Android/data/net.minetest.minetest/files/Minetest/locale even though a static one was provided.
04-04 15:34:50.732 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: Minetest 5.9.0-dev-debug-cff6c807a-dirty (Android)
04-04 15:34:50.733 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: Using LuaJIT 2.1.1707061634
04-04 15:34:50.733 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: Built by Clang 17.0
04-04 15:34:50.733 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: Running on Linux/5.10.177-android12-9-27921017-abA256BXXU1AXB6 aarch64
04-04 15:34:50.733 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: BUILD_TYPE=Debug
04-04 15:34:50.733 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: RUN_IN_PLACE=0
04-04 15:34:50.733 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: USE_CURL=1
04-04 15:34:50.733 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: USE_GETTEXT=1
04-04 15:34:50.733 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: USE_SOUND=1
04-04 15:34:50.733 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: STATIC_SHAREDIR="/usr/local/share/minetest"
04-04 15:34:50.733 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: STATIC_LOCALEDIR="/usr/local/share/locale"
04-04 15:34:50.733 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: SER_FMT_VER_HIGHEST_READ=29 LATEST_PROTOCOL_VERSION=44
04-04 15:34:50.738 27840 27941 V Minetest: 2024-04-04 15:34:50: VERBOSE[Main]: httpfetch_init: parallel_limit=8
04-04 15:34:50.739 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: Gettext: domainname="minetest" path="/storage/emulated/0/Android/data/net.minetest.minetest/files/Minetest/locale"
04-04 15:34:50.739 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: Message locale is now set to: C
04-04 15:34:50.740 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: Searching worlds...
04-04 15:34:50.740 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]:   In /storage/emulated/0/Android/data/net.minetest.minetest/files/Minetest/worlds: 
04-04 15:34:50.741 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: 0 found.
04-04 15:34:50.741 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: Using default world at [/storage/emulated/0/Android/data/net.minetest.minetest/files/Minetest/worlds/world]
04-04 15:34:50.769 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: Audio: Global Initialized: OpenAL 1.1 ALSOFT 1.23.1, using OpenAL Soft
04-04 15:34:50.770 27840 27941 V Minetest: 2024-04-04 15:34:50: VERBOSE[Main]: Trying video driver OpenGL ES2
04-04 15:34:50.792 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: Irrlicht: SDL initialized
04-04 15:34:50.831 27840 27941 E Minetest: 2024-04-04 15:34:50: ERROR[Main]: Irrlicht: Could not initialize context: Could not create EGL context (call to eglCreateContext failed, reporting an error of EGL_BAD_ATTRIBUTE)
04-04 15:34:50.835 27840 27941 D Minetest: 2024-04-04 15:34:50: INFO[Main]: Irrlicht: Quit SDL

@okias
Copy link
Contributor

okias commented Apr 4, 2024

@sfan5 output of something like https://github.com/strazzere/eglinfo-android/ could be useful, if you have chance to run it

@grorp
Copy link
Member Author

grorp commented Apr 4, 2024

Rebased to include the fix you committed recently (eb8785a), maybe it works now?

@sfan5
Copy link
Member

sfan5 commented Apr 4, 2024

Rebased to include the fix you committed recently (eb8785a), maybe it works now?

I already merged master before testing, so unfortunately no.

@okias
Copy link
Contributor

okias commented Apr 4, 2024

I think this is just unsupported GLES version which is requrested. ContextAttrib contains only EGL_CONTEXT_CLIENT_VERSION.

btw. I don't see any assert after leaving OpenGLESVersion = 0; (at least in case of DriverType being set to wrong option)

You want also s/EGL_CONTEXT_CLIENT_VERSION/EGL_CONTEXT_MAJOR_VERSION/ as spec recommends. https://registry.khronos.org/EGL/sdk/docs/man/html/eglCreateContext.xhtml

My guess is the code request GLES 0 here.

@grorp
Copy link
Member Author

grorp commented Apr 4, 2024

@okias I assume you're referring to the CEGLManager.cpp code? That's not used anymore on Android, SDL is doing this stuff for us now.

@grorp
Copy link
Member Author

grorp commented Apr 4, 2024

@sfan5 please re-test.

@sfan5
Copy link
Member

sfan5 commented Apr 4, 2024

nope
and now the code no longer reports a precise error

04-04 23:07:32.322  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: Using system-wide paths (NOT RUN_IN_PLACE)
04-04 23:07:32.327  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: Detected share path: /storage/emulated/0/Android/data/net.minetest.minetest/files/Minetest
04-04 23:07:32.327  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: Detected user path: /storage/emulated/0/Android/data/net.minetest.minetest/files/Minetest
04-04 23:07:32.327  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: Detected cache path: /data/user/0/net.minetest.minetest/cache
04-04 23:07:32.328  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: Using in-place locale directory /storage/emulated/0/Android/data/net.minetest.minetest/files/Minetest/locale even though a static one was provided.
04-04 23:07:32.328  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: Minetest 5.9.0-dev-debug-16935b7d6-dirty (Android)
04-04 23:07:32.328  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: Using LuaJIT 2.1.1707061634
04-04 23:07:32.328  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: Built by Clang 17.0
04-04 23:07:32.328  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: Running on Linux/5.10.177-android12-9-27921017-abA256BXXU1AXB6 aarch64
04-04 23:07:32.328  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: BUILD_TYPE=Debug
04-04 23:07:32.328  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: RUN_IN_PLACE=0
04-04 23:07:32.328  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: USE_CURL=1
04-04 23:07:32.328  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: USE_GETTEXT=1
04-04 23:07:32.328  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: USE_SOUND=1
04-04 23:07:32.328  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: STATIC_SHAREDIR="/usr/local/share/minetest"
04-04 23:07:32.329  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: STATIC_LOCALEDIR="/usr/local/share/locale"
04-04 23:07:32.329  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: SER_FMT_VER_HIGHEST_READ=29 LATEST_PROTOCOL_VERSION=44
04-04 23:07:32.335  2843  3148 V Minetest: 2024-04-04 23:07:32: VERBOSE[Main]: httpfetch_init: parallel_limit=8
04-04 23:07:32.335  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: Gettext: domainname="minetest" path="/storage/emulated/0/Android/data/net.minetest.minetest/files/Minetest/locale"
04-04 23:07:32.335  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: Message locale is now set to: C
04-04 23:07:32.337  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: Searching worlds...
04-04 23:07:32.337  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]:   In /storage/emulated/0/Android/data/net.minetest.minetest/files/Minetest/worlds: 
04-04 23:07:32.338  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: 0 found.
04-04 23:07:32.338  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: Using default world at [/storage/emulated/0/Android/data/net.minetest.minetest/files/Minetest/worlds/world]
04-04 23:07:32.417  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: Audio: Global Initialized: OpenAL 1.1 ALSOFT 1.23.1, using OpenAL Soft
04-04 23:07:32.417  2843  3148 V Minetest: 2024-04-04 23:07:32: VERBOSE[Main]: Trying video driver OpenGL ES2
04-04 23:07:32.443  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: Irrlicht: SDL initialized
04-04 23:07:32.501  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: Irrlicht: Could not create window and context!
04-04 23:07:32.503  2843  3148 D Minetest: 2024-04-04 23:07:32: INFO[Main]: Irrlicht: Quit SDL

Edit: even more logging didn't reveal anything obvious wrong. I suspect something else is wrong that prevents this from working in general.

@grorp
Copy link
Member Author

grorp commented Apr 4, 2024

Readded proper logging, sorry.
Does it work with CIrrDeviceSDL from Multicraft? / Does Multicraft work on the same device?

Edit (in response to your edit): Hmm, it unfortunately works for me, so I can't investigate this very well.

@sfan5
Copy link
Member

sfan5 commented Apr 4, 2024

What I just discovered is that this is a debug build and the release build works on the same device.
this works:

diff --git a/irr/src/CIrrDeviceSDL.cpp b/irr/src/CIrrDeviceSDL.cpp
index fcd9b2efd..0eb26cb3c 100644
--- a/irr/src/CIrrDeviceSDL.cpp
+++ b/irr/src/CIrrDeviceSDL.cpp
@@ -529,7 +529,7 @@ bool CIrrDeviceSDL::createWindowWithContext() {
        }
 
 #ifdef _DEBUG
-       SDL_GL_SetAttribute(SDL_GL_CONTEXT_FLAGS, SDL_GL_CONTEXT_DEBUG_FLAG | SDL_GL_CONTEXT_ROBUST_ACCESS_FLAG);
+       SDL_GL_SetAttribute(SDL_GL_CONTEXT_FLAGS, /*SDL_GL_CONTEXT_DEBUG_FLAG | SDL_GL_CONTEXT_ROBUST_ACCESS_FLAG*/ 0);
 #endif
 
        if (CreationParams.Bits == 16) {

@grorp
Copy link
Member Author

grorp commented Apr 4, 2024

What I just discovered is that this is a debug build and the release build works on the same device. this works:

Gotta add a fallback for this too. Or can I just remove it?

Edit: Removed the thing now, tell me if you want it back.

@okias
Copy link
Contributor

okias commented Apr 4, 2024

both flags seems to be WGL/GLX specific, at least by this docs: https://wiki.libsdl.org/SDL2/SDL_GLcontextFlag

for both I'm seeing

This flag is currently ignored on other targets that don't support equivalent functionality.

which kinda doesn't match the Android experience

EDIT: and the docs lying: https://github.com/libsdl-org/SDL/blob/5fa87e29e7f32707e8d511bbfdede0dcea546131/src/video/SDL_egl.c#L974

@okias
Copy link
Contributor

okias commented Apr 4, 2024

The code I reference is able to drop DEBUG flag, but not the SDL_GL_CONTEXT_ROBUST_ACCESS_FLAG. That's kinda what SDL missing, so it "silently fails". The same code from the link I shared for DEBUG flag should be added to SDL to catch ROBUST too

@sfan5
Copy link
Member

sfan5 commented Apr 4, 2024

Edit: Removed the thing now, tell me if you want it back.

I think it makes sense in principle but it needs to be hooked up differently. I'd comment it out for now.

Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

Works as far as I can tell.

@grorp grorp changed the title Migrate the Android port to SDL2 [manual squash] Migrate the Android port to SDL2 Apr 5, 2024
grorp and others added 3 commits April 7, 2024 17:40
It now works even if window creation succeeds and only context creation fails.
Adapted from https://github.com/MoNTE48/Irrlicht . This should be fine
license-wise since that repo still contains the original Irrlicht license
(this is no legal advice).
Original PR: MoNTE48/Irrlicht#9

Co-authored-by: Deve <deveee@gmail.com>
@grorp grorp changed the title [manual squash] Migrate the Android port to SDL2 [no squash] Migrate the Android port to SDL2 Apr 7, 2024
@grorp grorp merged commit cc1bfc6 into minetest:master Apr 8, 2024
13 checks passed
@grorp grorp deleted the android-sdl branch April 8, 2024 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android @ Engine Core What happens inside the very engine Feature ✨ PRs that add or enhance a feature One approval ✅ ◻️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants