a path towards moving from Allegro to SDL

ericoporto commented 4 years ago

ags/wiki/SDL-Migration-proposition.md

The document is in the wiki so people can write in and clarify! If you are worried about losing your edit, do it using git, the document is SDL-Migration-proposition.md on the root directory.

https: git clone https://github.com/adventuregamestudio/ags.wiki.git
ssh: git clone git@github.com:adventuregamestudio/ags.wiki.git

I wrote above a document proposing a path toward migrating from Allegro, meaning, removing Allegro dependency completely, and using instead mostly SDL. It may turn out we need something else too.

Since last ags3-sdl2 port did not get merged, this tries to detail a plan, with ags4 in mind and focus on complete removal of Allegro.

Please comment ~~, also accepting people to write the code~~. My central idea was to have code flowing towards this goal into AGS repo.

ericoporto commented 3 years ago

@ivan-mogilko my code should fail at building SDL_sound, the error should be it complaining that the target uninstall is a duplicate target when building it. The easy solution is simply provide an alternative CMakeLists.txt for it where this target is either removed, has a different name or it checks and only adds such target if it doesn't exist (like here). I haven't done that yet because I thought I could get it fixed on upstream. I will add a commit giving a solution to this either tonight or tomorrow morning. (If you edit the fetched CMakeLists.txt for SDL_Sound and remove this target everything will build once CMake refreshes)

rofl0r commented 3 years ago

Many of these definitions are no longer necessary for a stripped library, but some still do, like ones that tell endianess

i happen to have produced a header that should be able to tell endianness at compile time on almost all systems in the universe https://github.com/rofl0r/endianness.h

ericoporto commented 3 years ago

@ivan-mogilko Added the changes to build on top of your test linux branch here : https://github.com/ericoporto/ags/commit/94d94aee9ea7fb30e9bd8e91e5accc410a4a97ff

My full branch with this is here: ericoporto/ags/tree/ags3--sdl2-port-full--testlinux

Can you test it and see if it works for you? You may need to delete or refresh your CMake cache.

EDIT: also, judging by the console output. SDL_sound could not find OpenAL for some reason ("missing: OPENAL_LIBRARY OPENAL_INCLUDE_DIR ")

That should be normal, if no OPENAL is found it will use MojoAL in place. It should be AGS itself that didn't found it.

ivan-mogilko commented 3 years ago

Couple of more commits, I removed alunixac.h reference from alucfg.h and instead put some defines that are still in use in the stripped code there (https://github.com/ivan-mogilko/ags-refactoring/commits/ags3--sdl2-port-full--testlinux).

@ericoporto also applied your cmake update with some exceptions. Now it passes configuration step successfully, but every time I try doing cmake build something wrong happens during building SDL2, which crashes my linux vm (resets the guest os). Not sure if it's cmake, or gcc, or just something in vm itself.

Could you clarify, why including extra was necessary? I can build with existing Makefile well, in both debug and ndebug.

BTW, judging by cmake output, it still creates few allegro macros which we may deprecate, but I guess this is a minor issue.

ericoporto commented 3 years ago

unistd.h was added for the posix chdir and getuid, that header is available in anything not Windows as far as I know, so it should not be a problem. I believe it used to be automatically included in gcc but since gcc 5 it has to be done explicitly (this link says 4.7), a similar case is reported here.

The reported crash is very weird and I have no idea what it could be right now, can you give me some information about your VM so I can try to reproduce (specially how much RAM it has)? It doesn't happen here but Linux is my primary OS on my devices :/ Can you tell me which GCC you are running in your Ubuntu 16.04? If your VM is updated it should be at least 5.

Have you tried to clone and build my branch directly? Can you tell me how you are building? Here is some extra info on building with CMake: https://cliutils.gitlab.io/modern-cmake/chapters/intro/running.html

My guess though would be that what's crashing is gcc, so what I can do on CMake side is use system SDL2 if it's available instead of pulling the sources and building it yourself. Can your VM build SDL2 by itself?

ericoporto commented 3 years ago

I changed the platform detection code to be done using preprocessor definitions in the Allegro C code instead of using CMake: https://github.com/ericoporto/ags/commit/dddc4cbe7b8c9059fc0aedaa87851ee89d25950a

This is the branch I made for this: ericoporto/ags/tree/ags3--sdl2-port-full--testlinux2

(I stole from AGS Common/core/platform.h and as far as I can tell it works.)

ivan-mogilko commented 3 years ago

Could you explain why is this alplatdetect is necessary, cannot we just pass necessary flag from makefile? Also, strictly speaking MSVC != Windows, as there may be other compilers used on Windows. EDIT: the correct detection for MSVC is _MSC_VER (https://docs.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-160)

ivan-mogilko commented 3 years ago

Regarding the previously reported crash,

VM is Ubuntu 16.04 LTS
VM RAM is 2 GB
GCC is 5.4.0

I was building according to CMAKE.md in our repository.

I managed to build SDL 2.0.8 on my own previously, except I suspect missing some dependencies, as I was able to get video working, but sound still does not work. Should maybe try 2.0.12, as this is what cmake script is doing?

ericoporto commented 3 years ago

Could you explain why is this alplatdetect is necessary, cannot we just pass necessary flag from makefile?

I decided to skip passing anything because I thought it was more acceptable since I got from previous messages the CMake generated specific files weren't desirable. In this way it's just preprocessor code so it should work anywhere with any build system. My code is just suggestions, don't treat as written in stone, there are way too many ways of doing the same things. I don't have experience in maintenance so I don't know what is best at long term.

At the specific spot I included alplatdetect you can see a ton of preprocessor flags are being used to decide things, so it didn't felt wrong to go the same way.

Note: despite the name, CMake isn't just generating makefiles, it generates Xcode projects on MacOS, ninja for Android, and SLN/Vcxproj on Windows.

I will focus on figuring out the crash for now.

ivan-mogilko commented 3 years ago

I think I found what's causing vm to crash, it's "--parallel" option in "cmake --build . --parallel". It passes successfully without it... ~but then I got unexpected compiling errors in ags engine, so there's still something to fix...~ ah, it's likely because platform flag is not defined for allegro.

ivan-mogilko commented 3 years ago

@ericoporto ok, I added platform auto-detection, hopefully it'll make it simplier. I can build through cmake now too.

ericoporto commented 3 years ago

Awesome! :D Great work!

rofl0r commented 3 years ago

Couple of more commits, I removed alunixac.h reference from alucfg.h and instead put some defines that are still in use in the stripped code there (https://github.com/ivan-mogilko/ags-refactoring/commits/ags3--sdl2-port-full--testlinux).

thanks, it seems to build so far, apart from

../Engine/media/audio/openaldecoder.h:22:23: fatal error: SDL_sound.h: No such file or directory

this is really annoying, SDL_sound is a SDL1 library (latest release targets SDL1), then some work was done to make it compatible with SDL2, so there exist some versions (in git only) that are compatible to both SDL1 and SDL2, but one of the latest commits removed SDL1 support. so distros that want to update this package need 1) get some untagged beta-quality git checkout, and 2) either drop support for SDL1, or somehow stuff SDL_sound master into a SDL2_sound package and hack library/include paths into the Makefiles of projects using it. what would be needed here is that the authors finally make a release and rename it to SDL2_sound, just like all other 3rd party SDL2 libs did.

ivan-mogilko commented 3 years ago

@rofl0r I built and installed SDL_Sound myself from the latest mercurial commit using their cmake scripts, which ended SDL_sound.h in /user/local/include and libSDL2_sound.so (and accompanying files, .a, .so.1 etc) in /usr/local/lib. Did not know any better so I set -I /usr/local/include as a "sdl_sound cflag" in our makefile for now (and corresponding for lib).

@ericoporto what is your experience of running sdl port from this branch? I think I've broken something in package manager in my os during experiments (was trying to manually install newer SDL), some optional dependencies are missing, and now SDL is acting up, only managed to run it in windowed mode and w/o any sound. Need to cleanup os first before continuing tests.

ericoporto commented 3 years ago

@ivan-mogilko I played some Wadjet Eye Windows titles for fun using it and they seem to work. Sound is fine, including the voice acting! I am using my Ubuntu 16.04 which is ran on the machine directly to verify this. I played some other games and did not noticed anything out of ordinary in them. I had two problems with my own game (Future Flashback), but they are solvable:

My mouse wheel middle click doesn't seem to register.
I knew that my Joystick SDL2 plugin could potentially crash when the Engine was using SDL2 too (it crashes with 3.5.0 and SDL2 DIGI) but this doesn't happen with your branch, instead it's conflict result makes it not able to run in fullscreen but it runs fine when windowed! Super weird, but is a fix that should happen on my end.

rofl0r commented 3 years ago

../Engine/media/audio/openaldecoder.h:22:23: fatal error: SDL_sound.h: No such file or directory this is really annoying

i meant that it's hard to reason how to properly install SDL(2)_sound systemwide in the described situation, not that it's failing to find the header when installed (it isn't). i will ask other distro maintainers how they deal with this specific oddball package.

Did not know any better so I set -I /usr/local/include as a "sdl_sound cflag" in our makefile for now (and corresponding for lib).

no, that's a bogus change. one should never hardcode non-standard local include directories but add them to CFLAGS manually during the build, if so required, like CPPFLAGS=-I/usr/local make.

rofl0r commented 3 years ago

i will ask other distro maintainers how they deal with this specific oddball package.

due to the quirkyness of this package, it seems almost no distros provide it so far. its author should really be contacted to get this fixed. additionally the make install step is broken in hg tip for the header SDL_sound.h (fails with error). i have found only one distro who ships it, archlinux: https://aur.archlinux.org/packages/sdl2_sound-hg/ where one commenter proposed a very wise fix for the header install:

sed 's|FILES SDL_sound.h DESTINATION include|FILES src/SDL_sound.h DESTINATION include/SDL2|' -i CMakeLists.txt

this makes it so SDL_sound.h is installed into $prefix/include/SDL2/SDL_sound.h, which fixes the conflict with SDL1-sound (it would overwrite its header, making SDL-sound for SDL1 unusable), additionally it makes it automatically work because -I$prefix/include/SDL2 is already added as a CFLAG when running sdl2-config --cflags. i would recommend other distros to adopt this approach. the libraries are fortunately already in the SDL2* namespace.

the commit message of when i added the package to my distro has more details: https://codeberg.org/sabotage-linux/sabotage/commit/6645182d209373b0d8a87769d77ce10816576812

rofl0r commented 3 years ago

Couple of more commits, I removed alunixac.h reference from alucfg.h and instead put some defines that are still in use in the stripped code there (https://github.com/ivan-mogilko/ags-refactoring/commits/ags3--sdl2-port-full--testlinux).

having fixed the SDL2-sound issue, i tried to build again, but the following happens during linking:

libsrc/apeg-1.2.1/ogg.o: In function `alvorbis_close':
/tmp/ags-refactoring/Engine/libsrc/apeg-1.2.1/ogg.c:55: undefined reference to `vorbis_block_clear'
...
libsrc/apeg-1.2.1/ogg.o: In function `altheora_close':
/tmp/ags-refactoring/Engine/libsrc/apeg-1.2.1/ogg.c:70: undefined reference to `theora_clear'
...
libsrc/apeg-1.2.1/ogg.o: In function `altheora_get_frame':
/tmp/ags-refactoring/Engine/libsrc/apeg-1.2.1/ogg.c:410: undefined reference to `ogg_stream_packetout'
...

it seems the "apeg-1.2.1" code (is it still needed ? supposedly sdl2-sound can play ogg etc) has a hard requirement on libogg, libtheora and libvorbis, but the required libraries are not added to LDFLAGS when linking the engine. (after installing those 3 libs, link succeeds after manually appending -ltheora -logg -lvorbis to the linker command line extracted from make V=1).

ivan-mogilko commented 3 years ago

apeg is (and was afaik) only used to play theora videos. The error is strange, maybe I missed something, but not sure how these were linked before? and I don't get same error.

rofl0r commented 3 years ago

i suspect the cause is in Makefile-defs.linux

LIBS += $(shell pkg-config --libs ogg)

when ogg is not installed, this command will fail (interesting that it didnt cause build error) and not return the library names.

btw i suspect the x11 pkg-config line can be removed, as sdl2-config should automatically add all required libs.

rofl0r commented 3 years ago

btw i did some performance tests using --fps and starship quasar (old freeware version) and the old engine seems to be a lot faster. it runs usually at over 60fps with software and GL driver, while the SDL2 branch crawls at around 10 fps (with PC speed throttled to 800MHz via powersave CPU governor). could it be due to liballegro being built with more optimized CFLAGS when it is built as a whole? my package recipe for allegro uses these CFLAGS:

-fno-unwind-tables -fno-asynchronous-unwind-tables 
-Wa,--noexecstack -g 
-O3 -fstrength-reduce -fthread-jumps -fcse-follow-jumps -fcse-skip-blocks
-frerun-cse-after-loop -fexpensive-optimizations
-fforce-addr -fomit-frame-pointer -mtune=generic

edit: good news, the game eureka 2: hidden plains (which uses 32bit) runs equally fast in the new SDL2 version. so maybe there's just something suboptimal in the 16bit code.

edit: re-checked quasar, turned out that the old engine version used ./acsetup.cfg whereas the new one used ~/.local/share/ags/... instead. after putting driver=ogl there too, the game runs with 60 fps as well. i was confused because the engine outputs INFO: OpenGL shaders: ENABLED INFO: Created renderer: opengl even when software driver is active. interestingly though, the old version runs at 60 fps even with software renderer.

ericoporto commented 3 years ago

I have an initial branch here for Android: github.com/ericoporto/ags/tree/ags3--sdl2-port-full--android

I am not sure if this is the next step or if it's getting the CI working in a branch in the repo/PR - to trigger Cirrus builds, so I am leaving at this.

ivan-mogilko commented 3 years ago

@rofl0r

i was confused because the engine outputs INFO: OpenGL shaders: ENABLED INFO: Created renderer: opengl even when software driver is active. interestingly though, the old version runs at 60 fps even with software renderer.

That's a curious find. With the SDL port "software renderer" is software only until it finishes virtual screen, but final present is done using SDL_Renderer, which could be anything (like OpenGL on Linux, Direct3D on Windows). Virtual screen (allegro bitmap) is painted onto SDL_Texture pixels for this. Maybe the conversion or present code is suboptimal and could be improved somehow.

Also, maybe SDL can use other renderer types too (non accelerated ones), we'd only need to code a configuration for this.

ivan-mogilko commented 3 years ago

@ericoporto

I am not sure if this is the next step or if it's getting the CI working in a branch in the repo/PR - to trigger Cirrus builds

I'd like to have a branch inside main repository with the complete part of the work, to not let it dissapear if I am suddenly distracted. But before that I'd probably have to do what I mentioned in this comment: https://github.com/adventuregamestudio/ags/issues/1121#issuecomment-726453148

ivan-mogilko commented 3 years ago

I opened a PR: #1136

it's mostly same code, only reverted some unnecessary makefile changes.

ericoporto commented 3 years ago

Awesome, after that is merged we close this and we can create separate smaller issues for the future TO-DOs, the CI fix (I have started this one, but I need a bit more time to figure out some details), build instructions update and the new ports.

The AGS3 Android SDL2 port may have a few gotchas due to previous reliance on configurations in the custom Allegro4 Android backend. So at least for this one a separate issue is useful. AGS4 Android can have cool nice to have new things. :D

ericoporto commented 3 years ago

Hey, I am still working on CI fixes. I was having trouble with MSVC but latest SDL_Sound from 2 days ago has fixed all woes I had with it, including my need to rewrite it's CMakeLists.txt.

I installed Windows for the first time in years to be able to work on the Dockerfile more quickly here now and plan to have at least Windows and MacOS working in the CI this weekend and submit a PR.

ivan-mogilko commented 3 years ago

I was having trouble with MSVC but latest SDL_Sound from 2 days ago has fixed all woes I had with it, including my need to rewrite it's CMakeLists.txt.

Ah, darn, sorry, I completely forgot about that, I had to fix SDL_Sound code in two places to build it in MSVS, but did not tell about this.

ericoporto commented 3 years ago

Since #1148 has been opened for the remainder fixes, I will close this issue so details can be discussed there.

Port is happening in https://github.com/adventuregamestudio/ags/tree/ags3--sdl2

adventuregamestudio / ags

a path towards moving from Allegro to SDL #1096