#8958 closed patch
Android port
| Reported by: | anguslees | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | Port: Android |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
Preliminary Android port.
- Lacks a mouse cursor and trackball support. - ports.mk additions are a bit of a mess (and configure doesn't Just Work).
Ticket imported from: #2603856. Ticket imported from: patches/1063.
Attachments (2)
Change History (25)
comment:1 by , 15 years ago
comment:2 by , 15 years ago
Yeah, the port is pretty well structured. And thank you for following our code formatting conventions.
And yeah, ports.mk should go to platform-specific directory. Our makefile system will pick that up.
comment:5 by , 15 years ago
This is the patch from the second market release that I uploaded some time ago (sorry for the delay).
Visible changes: mouse cursor and trackball support
I tried to fix the issues fingolfin raised with this patch, but there's more to be done. There's still some build mess left in ports.mk and issues with configure both related to the poor Android toolchain. I'm experimenting with alternate toolchain choices so hopefully this ugliness can go away once I find a better stage2 compiler. Ditto with my warning flags hack.
comment:6 by , 15 years ago
Ah, it seems this patch doesn't have some of the stuff fingolfin asked for (like the trivially simple revert of the "generic backend" bit from configure). Just so you don't think I'm an ass, I am working on them they're just not in this patch ;)
Oh, and until the toolchain stuff is worked out and configure/ports.mk cleaned up, I'm not making a serious request for addition to the scummvm tree. Consider this a statement of my good intent, and a poor man's svn branch.
comment:7 by , 15 years ago
Thanks for the update in the meantime. Looking forward to see further progress :-).
comment:10 by , 15 years ago
AFAIK it should be no different to the Magic, but on the Hero, I've observed a few issues which ought to be resolved: * No way to enter text - prevents skipping through MI2 copy protection screen * No way to click when using trackball - trackball clicks are seemingly ignored, only touchscreen behaves * - As part of this, consider making the search button behaviour context-sensitive - i.e. holding it is right-click when using the touchscreen, tapping it simply right clicks at the current cursor position (otherwise it makes it awkward to right-click when using the trackball)
comment:13 by , 14 years ago
Here is the latest diff.
Note in particular that I have changed from adding an android filesystem /ASSET hack to a much cleaner android asset archive (and addSysArchivesToSearchSet) - so kyra.dat, etc could be found inside the android package. Unfortunately themes were found via direct FSDirectory filesystem lookups and couldn't cope with a zip inside a zip (as the theme-inside-android-package effectively is). In this patch, I've done a reasonable first attempt at converting ThemeEngine to use archives, but it could be cleaned up further and will unfortunately break unarchved themes.
I've moved a lot of the ports.mk mess into configure where it should be. Let me see how much of ports.mk I can remove now and we might actually be in good shape to talk about merging.
by , 14 years ago
| Attachment: | scummvm-android.patch.gz added |
|---|
Diff for latest market upload ("release 6") - against branch-1-0-0 r45736
comment:14 by , 14 years ago
Good to see some progress. The patch looks a lot better now! Some questions & remarks:
* Any particular reason your patch removes -Wshadow (in the primary Makefile) ?
* It seems that in the Java files, you mix space and tab indentions (and are using 8-space tabs). While we don't explicitly cover Java in our code formatting guidelines, it would still be good to see all files adhere to the same style (i.e. exclusively use 4-space tabs for indention)
* In base/commandLine.cpp, why change the printf(USAGE_STRING, ..) to an error() ?
* The hack in common/util.cpp (to make it use LOGW instead of fprintf hopefully can soon be removed on trunk; I am working on abstracting away any console output code.
* I know it's extra work, but we need a patch against trunk -- we certainly don't want to merge this code into the 1.0.0 branch (which is bug fixes only at this point).
* Can't comment on the ThemeEngine changes right now, need to look at it closer to see what the problem is and what you are doing to overcome it.
* I don't understand why you mess with ports.mk at all -- all of this could and should be in your own backends/platform/android/android.mk file. That's how other backends do it. In trunk it's even simpler to do that, you just have to set _port_mk in configure to point to your port specific .mk file.
comment:15 by , 14 years ago
OK, here's my latest code, rebased onto ScummVM trunk. There are various internal improvements to the port since the last patch - in particular I've simplified the way the various threads interact
- -Wshadow The Android headers generate some noise with this warning enabled. I'll drop this from my patch and patch my Android headers instead.
- Java indenting. Yep, I'll fix the indenting - haven't done that yet.
- Android apps have no stdout/stderr (or rather, stdout/stderr gets lost). I wanted to see logged errors when I generated the wrong command line, so I changed the printf to an error(). I'm happy to remove this if you don't want it (now that I have this portion of the code working).
- ports.mk hacks are now moved to android.mk. configure, ports.mk, etc should be clean in this version. I also added a README.build so you can see what is required to build the port (it's pretty clean now).
Let me know what you think of the important bits (ThemeEngine patches), and I'll fix the indenting, etc shortly. See my earlier comment for the rationale behind the ThemeEngine changes - I still need to remove some of the old FSNode codepaths from there too, if you're happy with the rest.
comment:16 by , 14 years ago
Btw anguslees: I have just got a Nexus One and am interested in helping you out. I don't have much time on my hands atm, but if it helped me to get scummvm working on my phone I could probably find some ;-)
comment:17 by , 14 years ago
Great work, gus, this looks like we are close to getting this into SVN. We could do so even if we don't resolve everything all the mods outside backends, by simply putting those into a patch inside backends/platforms/android -- this would certainly not be nice, but on the short term could be an option we might want to consider.
Some remarks on your changes:
* sound/decoders/vorbis.cpp - you extend an #ifdef for GP32 to also cover android. That line is commented, and refers to the GP32 only. Please adjust the comment to also cover Android appropriately.
* base/commandLine.cpp : you changed a printf followed by exit(1) to an error() call -- is using exit(1) a no-no on Android? I believe a couple engine still call it (which is bad in any case). The problem with calling error there is that it appends "!\n" to the output string, which is not what we want there. So this is not a good solution globally. But if you just need to invoke your OSystem's quit() method to terminate, you could do the same as common/textconsole.cpp and insert this code before the exit(1) in commandline.cpp: // Finally exit. quit() will terminate the program if g_system is present if (g_system) g_system->quit(); This should work on all our ports.
* in your *.mk files you may want to insert a couple more .PHONY statements for better performance
* In your README you explain how it is necessary to create wrapper scripts in order to use the regular Google compilers etc. -- this doesn't seem necessary to me, on other ports, we simply add such flags to the CXXFLAGS *before* invoking configure. Since you already tell people to pass in CPPFLAGS, why not just add this in? I would then also change _host_alias=arm-android-eabi back to _host_alias=arm-eabi since for most people the official SDK will be more appealing, I would imagine (because they already have it, and it's easily available from Google, no?)
* gui/ThemeEngine.* : hm, it looks to me as if your changes removed the ability to load a theme from an unpacked theme .zip, a feature that's quite important to us, and which is quite helpful when developing and debugging theme files. But maybe I am mistaken, I did not yet have the time to look at the changes closely.
comment:18 by , 14 years ago
| Owner: | set to |
|---|
by , 14 years ago
| Attachment: | scummvm-android.patch.2.gz added |
|---|
Latest patch - against ScummVM trunk
comment:19 by , 14 years ago
OK, updated patch. This is a large change from the previous one, mostly with the goal of removing the use of private Android APIs. In particular, it now uses OpenGLES for graphics and calls into Java for sound and reading asset files. There are still a few graphics regressions to shake out, but this is the codebase that any future development should be based on.
Also I'm still calling EGL directly from C, which is still technically a private API, although the header file is heavily standardised. I might also change this to Java in the long run.
Fingolfin: I've split the ThemeEngine changes off into a separate embedded patch. I have not switched to using the Android NDK toolchain as-is, since when I tried I hit some (NDK) toolchain bugs and chose not to spend more than a week debugging that right now :/
Are we there yet?
comment:20 by , 14 years ago
Hi Gus, we are mostly there, in fact, I'd be willing to commit this as is and resolve the following items later. Here are some small remarks anyway:
* In configure, you explicitly specify the endianess for android: _endian=little Is this really necessary? Our configure script usually does a good job at auto-detecting endianess, even when using a cross compiler. I like to avoid hard coding assumptions like this, just in case a future Android version runs on big endian.
* You add a reference to dists/android/AndroidManifest.xml in update-version.pl, yet this file does not exist. I assume "mkmanifest.pl" is meant to generate it, or rather, a file with the same name and ".in" appended.. Yet our update-version.pl script expects the file to be always there, to be in SVN, which it isn't, so this doesn't really make sense. Moreover, the update-version.pl script replaces some fixed strings like @VERSION@, @VER_MAJOR@ etc. in the source files -- none of these strings seems to occur in your data files, so I wonder what the point is.... ? Maybe I am missing something here?
* Your Java code resides in the package org/inodes/gus/scummvm -- if we make this port official, it would seem sensible to me to change this to org/scummvm. No?
comment:21 by , 14 years ago
Yep, dists/android/AndroidManifest.xml{,in} were somehow missing from my patch. I've added into the svn repo.
_endian=little is indeed unnecessary, so I've removed that from svn now too.
I've wondered about renaming the Java package ever since I opened this bug. Changing it would affect all installed users (it's an entirely new package as far as Android market is concerned, so no clean upgrades, save games and scummrc will be lost, etc). I'll think a bit more about what the right tradeoff is.
comment:22 by , 14 years ago
| Status: | new → closed |
|---|
comment:23 by , 5 years ago
| Component: | → Port: Android |
|---|
Thanks. However, the patch doesn't apply, it fails to create backends/platform/android/android.cpp. I think you may have done an "svn cp" of the null backend driver. So, I manually copied backends/null/ to backends/android and renamed null.cpp in the copy to android.cpp, and then the patch applied.
Overall, it looks very well. A few comments * The ports.mk changes are indeed messy (you already mentioned that ;). How about resolving this by moving your changes in there to your module.mk, i.e. backends/platforms/android/module.mk?
* I wonder about the second change to configure, where you modify the *) case to add DEFINES="$DEFINES -DUSE_$(echo $_backend | tr a-z A-Z)_DRIVER" MODULES="$MODULES backends/platform/$_backend" The idea seems to be to add "generic" support for backends which are not explicitly listed -- i.e. android. So, the user could type ./configure --backend-foobar and configure would run through. I am not sure this is a good idea. AT the very least, it should check whether backends/platform/foobar *exists* in the first place. But overall, I think I'd prefer an explicit list of supported backends in case statement.
* The Makefile changes (to the warnings) of course can't stay that way. If -pedantic and -Wshadow cause errors for you, it would be nice to know which, to see if we can fix them. Failing that, we could think about only enabling those compiler switches if a certain configure option is used (e.g. --enable-extra-warnings).
All in all, good work! :)