Opened 5 years ago

Last modified 3 years ago

#2996 new task

[PATCH] Update Android support

Reported by: BogDan Owned by:
Priority: Should Have Milestone: Backlog
Component: Core engine Keywords: patch
Cc: Patch:

Attachments (38)

android_patch_set.zip (160.9 KB) - added by BogDan 5 years ago.
android_patch_set_v2.zip (161.8 KB) - added by BogDan 5 years ago.
0001-Compile-all-Pyrogenesis-dependencies.patch (108.1 KB) - added by BogDan 5 years ago.
0002-A-small-step-for-wildfiregames-one-giant-leap-for-An.patch (182.2 KB) - added by BogDan 5 years ago.
0003-Add-scaleRatio-variable.patch (4.0 KB) - added by BogDan 5 years ago.
0004-Remove-hack.patch (803 bytes) - added by BogDan 5 years ago.
0005-Use-snprintf-insted-of-sprintf_s.patch (13.9 KB) - added by BogDan 5 years ago.
0006-Fix-crappy-vswprintf-implemenation-on-Android.patch (7.4 KB) - added by BogDan 5 years ago.
0007-On-Android-use-vswprintf_s-because-we-need-to-tweak-.patch (874 bytes) - added by BogDan 5 years ago.
0008-SDL_SetWindowGammaRamp-fails-on-Andorid-so-don-t-bot.patch (1.2 KB) - added by BogDan 5 years ago.
0009-FIX-SIGBUS-Don-t-read-unaligned-on-ARM.patch (8.0 KB) - added by BogDan 5 years ago.
0010-Fix-particle-shader.patch (1.1 KB) - added by BogDan 5 years ago.
0011-GLES-doesn-t-have-glDisable-GL_TEXTURE_2D.patch (709 bytes) - added by BogDan 5 years ago.
0002-A-small-step-for-wildfiregames-one-giant-leap-for-An-v2.patch (181.1 KB) - added by BogDan 5 years ago.
0002-1-A-small-step-for-wildfiregames-one-giant-leap-for-An.patch (178.1 KB) - added by BogDan 5 years ago.
0002-2-No-mocks-on-Andorid.patch (890 bytes) - added by BogDan 5 years ago.
0002-3-Fix-crash.patch (677 bytes) - added by BogDan 5 years ago.
0002-4-Update-main-method.patch (1.4 KB) - added by BogDan 5 years ago.
0002-5-Used-the-data-set-from-Java.patch (961 bytes) - added by BogDan 5 years ago.
0006-1-Fix-crappy-vswprintf-implemenation-on-Android.patch (1.6 KB) - added by BogDan 5 years ago.
0006-2-Use-ls-hs-everywhere.patch (6.0 KB) - added by BogDan 5 years ago.
0006-1-Fix-crappy-vswprintf-implemenation-on-Android.2.patch (1.7 KB) - added by BogDan 5 years ago.
0001-Compile-all-Pyrogenesis-dependencies.2.patch (108.1 KB) - added by BogDan 5 years ago.
0002-A-small-step-for-wildfiregames-one-giant-leap-for-An.2.patch (182.1 KB) - added by BogDan 5 years ago.
0002-A-small-step-for-wildfiregames-one-giant-leap-for-An.3.patch (182.1 KB) - added by BogDan 5 years ago.
0001-Compile-all-Pyrogenesis-dependencies.3.patch (109.2 KB) - added by BogDan 5 years ago.
0002-A-small-step-for-wildfiregames-one-giant-leap-for-An.4.patch (182.1 KB) - added by BogDan 5 years ago.
0003-Make-qmake-happy.patch (19.2 KB) - added by BogDan 5 years ago.
0004-Scale-the-GUI-on-Android.patch (771 bytes) - added by BogDan 5 years ago.
0005-update-.gitignore.patch (647 bytes) - added by BogDan 5 years ago.
0006-Make-gcc-happy.patch (11.6 KB) - added by BogDan 5 years ago.
0001-Compile-all-Pyrogenesis-dependencies.4.patch (109.2 KB) - added by BogDan 5 years ago.
0007-Say-hello-to-KTX-format.patch (143.4 KB) - added by BogDan 5 years ago.
0008-We-need-OpenGL-ES-3.0-for-ETC2.patch (1.3 KB) - added by BogDan 5 years ago.
0007-Say-hello-to-KTX-format.2.patch (145.1 KB) - added by BogDan 5 years ago.
0007-Say-hello-to-KTX-format.3.patch (145.1 KB) - added by BogDan 5 years ago.
0007-Say-hello-to-KTX-format.4.patch (138.1 KB) - added by BogDan 5 years ago.
0009-Add-externalTextureCompressTool-etct-parameter-to-ar.patch (9.2 KB) - added by BogDan 5 years ago.

Change History (89)

Changed 5 years ago by BogDan

Attachment: android_patch_set.zip added

Changed 5 years ago by BogDan

Attachment: android_patch_set_v2.zip added

comment:1 Changed 5 years ago by stanislas69

Keywords: review patch added

comment:2 Changed 5 years ago by leper

Keywords: review removed

Diffs, not zips. Also SubmittingPatches.

comment:3 Changed 5 years ago by BogDan

There are diffs zipped into a single file. They are too many (~30) to add them one by one. Do you prefer to squash them into a single patch?

comment:4 Changed 5 years ago by leper

Squash related ones into nice changes. You can keep that on github in a branch if you want, but if your patches need a zip file something somewhere went horribly wrong.

Changed 5 years ago by BogDan

Changed 5 years ago by BogDan

Attachment: 0004-Remove-hack.patch added

Changed 5 years ago by BogDan

Changed 5 years ago by BogDan

comment:5 Changed 5 years ago by BogDan

Description: modified (diff)

Well, it doesn't need a zip file because its size is big, but because they are too many :). Anyway I reduced the number to 11 patches.

comment:6 Changed 5 years ago by Itms

Keywords: review added

comment:7 Changed 5 years ago by leper

In 0002:

  • Mixing code changes with addition of project files is not really what I would consider nice related changes.
  • source/lib/sysdep/os/unix/unix_executable_pathname.cpp: Shouldn't be needed, add source/mocks/ to your include path.
  • source/lib/sysdep/os/linux/lcpu.cpp, source/scriptinterface/ScriptInterface.cpp: Not needed, add libraries/source/valgrind/include to your include path.

comment:8 Changed 5 years ago by historic_bruno

  • 0004-Remove-hack.patch​: that was purely for testing, IIRC there wasn't a good way of navigating the game setup UI or passing autostart options to the engine, has that changed?
  • 0010-Fix-particle-shader.patch: can you confirm the behavior is correct in all cases? I remember myconid changing that years ago to "fix" some obscure bug, though it wasn't clear to me then (it's certainly not now). That's something I've had to change every time I test GLES though, so it would be good to have it resolved one way or another.

comment:9 in reply to:  7 Changed 5 years ago by BogDan

Replying to leper:

In 0002:

  • Mixing code changes with addition of project files is not really what I would consider nice related changes.
  • source/lib/sysdep/os/unix/unix_executable_pathname.cpp: Shouldn't be needed, add source/mocks/ to your include path.

It doesn't work just to include source/mocks and libraries/source/cxxtest-4.4, because I get link errors (probably because I didn't compile cxxtest on Android, which IMHO is not needed at this point).

  • source/lib/sysdep/os/linux/lcpu.cpp, source/scriptinterface/ScriptInterface.cpp: Not needed, add libraries/source/valgrind/include to your include path.

Fixed.

Please let me know if you still want to split the patch in two.

comment:10 in reply to:  8 Changed 5 years ago by BogDan

Replying to historic_bruno:

  • 0004-Remove-hack.patch​: that was purely for testing, IIRC there wasn't a good way of navigating the game setup UI or passing autostart options to the engine, has that changed?

I could start the game with no problems at all.

  • 0010-Fix-particle-shader.patch: can you confirm the behavior is correct in all cases? I remember myconid changing that years ago to "fix" some obscure bug, though it wasn't clear to me then (it's certainly not now). That's something I've had to change every time I test GLES though, so it would be good to have it resolved one way or another.

It fixes some GL errors, Philip told me to revert it. But TBH I have no idea if the behavior is correct in all cases :).

Changed 5 years ago by BogDan

Changed 5 years ago by BogDan

Attachment: 0002-3-Fix-crash.patch added

Changed 5 years ago by BogDan

Changed 5 years ago by BogDan

Changed 5 years ago by BogDan

comment:12 Changed 5 years ago by historic_bruno

0006-2-Use-ls-hs-everywhere.patch​ is a good general change, and should probably be specified in the coding conventions for reference. I've had *print problems on some other platforms besides Android, maybe BSD? That's why quite a few were already changed from %s to %hs.

comment:13 in reply to:  12 Changed 5 years ago by leper

Replying to historic_bruno:

0006-2-Use-ls-hs-everywhere.patch​

r16148.

The coding conventions already specify this btw. Maybe a sentence under the logging header would help.

comment:14 Changed 5 years ago by historic_bruno

The example shows it, but it's not clear why (and I think people typically learn to use %s).

comment:15 in reply to:  8 ; Changed 5 years ago by Philip Taylor

Replying to historic_bruno:

  • 0004-Remove-hack.patch​: that was purely for testing, IIRC there wasn't a good way of navigating the game setup UI or passing autostart options to the engine, has that changed?

I think the problem was that my Android device had a 800x480 screen, and the menu UI is unusable at that size (everything overlaps everything else). Shouldn't be a problem on modern devices that are >1024x768 though. (But they have the problem that one's fingers are probably too large to press the tiny buttons on a 4.5" 300ppi display.)

  • 0010-Fix-particle-shader.patch: can you confirm the behavior is correct in all cases? I remember myconid changing that years ago to "fix" some obscure bug, though it wasn't clear to me then (it's certainly not now). That's something I've had to change every time I test GLES though, so it would be good to have it resolved one way or another.

I think it should be easyish to fix correctly (assuming the current code is functionally correct) - just make ParticleRenderer::RenderParticles pass CCamera::GetOrientation (which is the model-view matrix) as a new uniform alongside transform, and make the shader use that.

0006-1-Fix-crappy-vswprintf-implemenation-on-Android.patch: That will fail on format strings like "%%hs". Not very likely to be a problem in practice, but it should either be fixed or have a FIXME comment. Also it'll fail on "%.*hs", which matters a bit more since Preprocessor.cpp does that.

0002-1-A-small-step-for-wildfiregames-one-giant-leap-for-An.patch: Adding a new build system with its own list of source files and libraries and compiler flags etc seems a bit unfortunate (since it'll quickly get out of sync, and it loses features like tests) - we use Premake specifically to avoid that problem on all the other platforms.

comment:16 in reply to:  15 Changed 5 years ago by BogDan

Replying to Philip:

0006-1-Fix-crappy-vswprintf-implemenation-on-Android.patch: That will fail on format strings like "%%hs". Not very likely to be a problem in practice, but it should either be fixed or have a FIXME comment. Also it'll fail on "%.*hs", which matters a bit more since Preprocessor.cpp does that.

Ah, I missed "%.*[ls/hs]". I'll update that patch. Regarding "%%hs" I'll add a FIXME comment.

0002-1-A-small-step-for-wildfiregames-one-giant-leap-for-An.patch: Adding a new build system with its own list of source files and libraries and compiler flags etc seems a bit unfortunate (since it'll quickly get out of sync, and it loses features like tests) - we use Premake specifically to avoid that problem on all the other platforms.

I was forced to do it, because premake can't be used by QtCreator?, which we need it to be able to debug (easily and in a decent way) the application on Android. IMHO it's a fair tradeoff, what you gain by using QtCreator it worths the trouble to update the new build system from time to time. If the source dirs layout are not changed, then the new build system doesn't need any change. Even more, this is not a hit and run patch, I'm planning to hang around for a while and I'll update the build system if needed :). Of course, it doesn't mean that premake can't be used to build the Android application, I didn't do it because my premake skills are 0 (AD). I can help anyone who wants to add the needed support in premake to build the Android part.

comment:17 Changed 5 years ago by BogDan

0001 added "--without-lzma" to configure script.

comment:18 Changed 5 years ago by BogDan

0002 .2 removed qt5 dependency from build.gradle script

comment:19 Changed 5 years ago by BogDan

0002 .3 removed more qt5 stuff

comment:20 Changed 5 years ago by Philip Taylor

(I'm currently through the patches to tidy up some minor things and commit them. r16165 fixes particle.vs properly. r16163 fixes some other GL errors. #3011 fixes sys_vswprintf by deleting it. (Using vswprintf_s there is bad, because vswprintf_s is meant to return "" on overflow, while CLogger expects sys_vswprintf to return a truncated string; though due to a bug, vswprintf_s on Linux was truncating instead (while vsprintf_s worked correctly). It's all a bit of a mess...))

comment:21 Changed 5 years ago by philip

In 16202:

Remove Android hack to autoload a map.

Patch by BogDan?. Refs #2996.

comment:22 Changed 5 years ago by philip

In 16203:

Workaround for limitations of Android's vswprintf implementation.

Based on patch by BogDan?. Refs #2996.

comment:23 Changed 5 years ago by philip

In 16204:

SDL_SetWindowGammaRamp fails on Android, so don't bother using it.

Based on patch by BogDan?. Refs #2996.

comment:24 Changed 5 years ago by philip

In 16205:

Avoid misaligned pointer dereferences, which can fail on ARM.

Based on patch by BogDan?. Refs #2996.

comment:25 Changed 5 years ago by philip

In 16206:

GLES doesn't have glDisable(GL_TEXTURE_2D).

Patch by BogDan?. Refs #2996.

comment:26 Changed 5 years ago by Philip Taylor

Some changes from the patches here:

"Use snprintf insted of sprintf_s" - don't - Microsoft's secure CRT is a better API than the POSIXy string API (e.g. snprintf doesn't include a terminator if the string gets truncated, potentially leading to buffer overflow bugs; and its return value is different on Windows, so it's not portable). We have code to emulate the secure CRT on non-Windows platforms, so it ought to work fine everywhere (and if it doesn't it should be fixed).

XeroXMB.cpp: Renamed readUnaligned to be shorter, moved to source file so it can have a short name with conflicting with other things, made it work the same on all platforms (compilers are good enough at converting a memcpy into efficient code).

secure_crt.cpp androidFormat: Removed std::move in return, since as far as I can tell there's never any need for it.

secure_crt.cpp tvsprintf_s: Removed memset which turns out to be necessary. Removed support for %.*hs, %.*ls since the user of those was deleted.

Removed SDL_WINDOW_ALLOW_HIGHDPI since SDL2 doesn't use it on Android, so there's no point specifying it now, and it may cause compatibility problems if SDL ever does start supporting it.

sys_vswprintf: Deleted by #3011.

comment:27 Changed 5 years ago by Philip Taylor

Hmm, it looks like QtCreator doesn't attempt to build secure_crt.cpp (so it fails to find sprintf_s) - maybe that's because wsecure_crt.cpp #includes it, so it thinks it's a dependency and excludes it? It ought to build both.

comment:28 Changed 5 years ago by Philip Taylor

comment:29 Changed 5 years ago by philip

In 16209:

Add workaround for Android libc++ swprintf bug.

Based on patch by BogDan?. Refs #2996.

comment:30 Changed 5 years ago by philip

In 16210:

Make Android %ls/%hs workaround work on secure_crt as well as wsecure_crt.

Refs #2996.

comment:31 Changed 5 years ago by Philip Taylor

r16223 lets you set "gui.scale = 0.5" in a config file, and it will scale the GUI without affecting the rest of the rendering. That should make 0003-Add-scaleRatio-variable.patch not necessary for usability. (It's not very pretty scaling though, and rendering the world at full resolution is probably not fantastic for performance.)

comment:32 Changed 5 years ago by historic_bruno

Milestone: BacklogAlpha 18

Changed 5 years ago by BogDan

Attachment: 0003-Make-qmake-happy.patch added

Changed 5 years ago by BogDan

Changed 5 years ago by BogDan

Changed 5 years ago by BogDan

Attachment: 0006-Make-gcc-happy.patch added

comment:33 Changed 5 years ago by BogDan

Update patches (rebase on top of master branch).

Changed 5 years ago by BogDan

Changed 5 years ago by BogDan

comment:34 Changed 5 years ago by BogDan

Updated patch 0001 Added KTX & ETC support

Changed 5 years ago by BogDan

Changed 5 years ago by BogDan

comment:35 Changed 5 years ago by BogDan

0007-Say-hello-to-KTX-format.3.patch​ fixed all desktop problems, now KTX is dropin replacement for DDS.

Though there is still a problem using ETC2 because 0 AD expects fonts to be compresses as a GL_ALPHA format, but it seems ETC2 doesn't support such a thing. But that can be fix in another patch.

Let me know if I can do anything to help you review & upstream these patches.

Changed 5 years ago by BogDan

comment:36 Changed 5 years ago by BogDan

I splitted 0007 patch in two parts:

  • 0007 is only about KTX format
  • 0009 is adds the possibility to use an external script/application to compress the textures. Now the user can compress the textures using any format he wants. The default script texturecompress.sh, uses Mali's etcpack to compress the pyrogenesis textures as ETC2.

comment:37 Changed 5 years ago by leper

Milestone: Alpha 18Alpha 19

comment:38 Changed 5 years ago by BogDan

Most of the attached patches are outdated, please cherrypick the new ones from my github repository KTX branch (https://github.com/bog-dan-ro/0ad/commits/ktx). All my patches are rebased on top of current github master branch.

comment:39 Changed 5 years ago by fabio

Hi bog_dan_ro, I have some questions. What's the state of the patches, are all regressions fixed now? Did you look at how using the OBB? Is there some other big TODO left, do you plan to work on it? Is this in a state it can be tested? If so it would be nice if you publish a test package.

comment:40 Changed 5 years ago by BogDan

Hi Fabio,

  • the patches are (should be) regression free. I tested them on Linux and on Android. It will be great if someone else will test it on more platforms :)
  • I'm going to work on OBB after alpha 18 is shipped and after we have a consensus on the way of shipping 0 A.D. on Android markets, if my proposal, to compress textures and bundle all 0 A.D. files in one OBB, will be accepted, then I'll need a few (*free*) days to implement it ;-).
  • besides OBB I'd like to (try to) rewrite the Atlas UI on QtQuickControls? to enable it on mobile devices. Of course, as I said before, this is the very last thing on my todo list.
  • yes you can test it on Android but you'll need a mouse because, currently, is not touch friendly at all. Sadly I have no idea how make it touch friendly ... Also even now it looks stable (no more crashes) don't forget that the game runs very slow even on high end devices with GLES 3.0 or better :).

I'll publish a test package after I'll implement the OBB :). Until then if you want to test it you can follow the instructions from http://trac.wildfiregames.com/wiki/AndroidPort . The only thing that has changed is the compression step, so instead of: ./binaries/system/pyrogenesis -archivebuild=binaries/data/mods/public -archivebuild-output=temp/public.zip -archivebuild-compress you need to pass also "-etct" parameter ./binaries/system/pyrogenesis -archivebuild=binaries/data/mods/public -archivebuild-output=temp/public.zip -archivebuild-compress -etct

You need to do the same thing for mods/mod.

etct stands for External Texture Compression Tool (check https://github.com/bog-dan-ro/0ad/commit/3376d7cb28995d50e6747621b758be22d0adf4ba for more info about it).

Of course the previous step must be done using *ktx* branch of my repo. You also need to download Mali's texture compression tool (http://malideveloper.arm.com/develop-for-mali/tools/software-tools/mali-gpu-texture-compression-tool/) and make sure *etcpack* is in your $PATH, before you start to compress game data files.

If you can't build the game for Android, I just did it for you, here https://drive.google.com/open?id=0Bzzpb2GB8vXiSTdMaG1PaVRrUE0&authuser=0 you can download the APK (of course you still need to create and push the game data to your sdcard).

comment:41 Changed 5 years ago by fabio

Great work, hopefully it will get reviewed and merged soon! I am trying generating the etc2 zips now (it is a bit annoying that closed source etcpack was built against old libraries libjpeg8 and libtiff4, not available on debian 8, I installed libjpeg8 using sid package and fooled libtiff4 symlinking it from libtiff5...).

comment:42 Changed 5 years ago by BogDan

Yeah, I did the same thing :) BTW I'm on IRC, if you need my help please feel free to ping me

comment:43 Changed 5 years ago by fabio

OK, after adding the etc2 compressed zips (generated from your ktx branch) I noticed these issues:

  • the technical feedback (window at bottom-right) gives a "upload failed (<url> malformed)";
  • also opening web links doesn't work;
  • every quick tap counts as a double click, just go in the Options window and I cannot change anything (a quick tap quickly enable and disable it); it works fine if I press the touch screen for a longer time;
  • it crashes as soon as I start a match at 99% loading (also I tried starting a match on a Timy random map whith just 1 player), so I cannot test the game itself.

On the menu I run it at about 30-50 fps.

comment:44 Changed 5 years ago by BogDan

I need the logcat from your device $ adb logcat -c then start the game

comment:46 Changed 4 years ago by Itms

Milestone: Alpha 19Alpha 20
Type: defecttask

comment:47 Changed 4 years ago by fabio

Just FYI I noticed in Warsow 2.0 release notes they added KTX texture format support.

comment:48 Changed 4 years ago by BogDan

yeah, sadly most probably my KTX patch is outdated and personally I have no time to fix it anywhere soon.

comment:49 Changed 4 years ago by Itms

Milestone: Alpha 20Backlog

comment:50 Changed 4 years ago by Itms

Keywords: review removed
Note: See TracTickets for help on using tickets.