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

a path towards moving from Allegro to SDL #1096

Closed
ericoporto opened this issue Jun 24, 2020 · 129 comments
Closed

a path towards moving from Allegro to SDL #1096

ericoporto opened this issue Jun 24, 2020 · 129 comments
Labels
ags 4 related to the ags4 development context: code fixing/improving existing code: refactor, optimise, tidy...

Comments

@ericoporto
Copy link
Member

ericoporto commented Jun 24, 2020

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 [email protected]: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.

@ivan-mogilko ivan-mogilko added ags 4 related to the ags4 development context: code fixing/improving existing code: refactor, optimise, tidy... labels Jun 24, 2020
@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Jun 24, 2020

I may be overreacting, but I don't like this sentence:

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

This is not the kind of task to be made by multiple contributors doing small changes, there must be a general plan and focused developer(s) to overwatch the process. It's better to have 1-2 people with good understanding of the engine code working on this to keep things organized.
At most only truly distinct feature that could be done separately is audio.

@ericoporto
Copy link
Member Author

ericoporto commented Jun 24, 2020

I think a minimum of two people is required. I expressed badly, but writing code requires, other than the code: fix vcxproj files, fix makefiles, fix cmake files, testing and fixing initially in two (Windows and Linux?) , than three (MacOS), than 4 (Android), depending how libraries are inserted building or including these libraries (which means adding in the three dockerfiles), ... Even if is one for doing everything and the other to help testing.

I think it's important to decide if we are building statically or dynamically (should game.exe accompany a SDL.dll ?) and how SDL gets in AGS project (code in repo or reference). It's important to note SDL may also go into editor due to Native.dll.

Edit: switched ordering so it starts with audio, but a note that SDL doesn't support all audio features we have on allegro and will require additional libraries if those features are desired. I tried to briefly explain it in the wiki.

Edit2: just discovered the SDL sources include vcxproj files, so maybe someone can figure out a way to include them in the Engine solution in a way that both Common.lib and Engine picks it. This way we add the SDL source directly in our repo and just link it using CMake / Makefiles on Linux and MacOS builds.

Edit3: It almost builds on Windows, it fails at link with the AGS Engine, perhaps someone knows what is wrong in this: ericoporto@bbcc61d

@JanetGilbert
Copy link
Contributor

Hi, this is interesting to me as Wadjet Eye is attempting to get our games ported to Switch. We started working with Nick Sonneveld but then the project went on hiatus for a year and now we are working with Eduardo Garcia. We are using the method of using Nick Sonneveld's SDL version of AGS combined with Ryan C. Gordon's port of SDL to Switch. I'm not sure how much we can share code as all the Switch stuff is under Nintendo's NDA so it can't be open source.

@ericoporto
Copy link
Member Author

Hey @JanetGilbert , can you take a look at the path proposition and show to Eduardo Garcia too? The way the proposal diverges from @sonneveld approach is it prioritizes cutting ties from Allegro instead of relying in it's methods and reworking them in the backend. Sonneveld also changed other things that didn't make sense for me until you mentioned the Switch xD. I ommitted them here because I wanted the minimal things to move for SDL pure as fast as possible so other more interesting problems can be dealt with.

@JanetGilbert
Copy link
Contributor

I'm not the techie on this project, AGS for SDL wouldn't compile for me! So Eduardo is handling the engine stuff, and I'm just going to be working on the scripting. I'm pretty sure it'd be easier to compile the less it depends on Allegro and associated libraries so I'm all for it. I've asked Eduardo to post here with his opinions.

@Arcnor
Copy link

Arcnor commented Jun 25, 2020

Hi there. As @JanetGilbert mentions I'm working on porting AGS to the Switch. I started using Nick's port, although I've modified it a lot, which means my changes are far away both from AGS master & Nick's original changes :).

In any case, this is what I think it's best for a future version of AGS, in somewhat random order (sorry about that). I'm also assuming backwards compatibility is not desired, and if it is, separate tools can be written to deal with the migration. What I don't think should happen is maintaining support for both versions in the same engine, as there are certain things that cannot be reconciled (just my humble opinion, like everything I'm going to add here, please keep that in mind 😄 ):

First of all, in general (and stuff I'm already doing) should be dropping support for the following:

  • Non 32 bit drawing code. If somebody wants 8 bit textures, they can use a palette they force themselves. If they want special effects, a plugin can be created that somewhat does those, maybe with shaders or maybe with some other magic, but maintaining the existing code for multiple color support is a real mess (IMHO, etc...)
  • Any script function that modifies textures, like "Dynamic Drawing" or "Raw drawing" or stuff like that. This is due to performance, and not knowing if textures you are using have been modified or not, it's a headache down the road, specially depending on the kind of rendering you end up using.
  • Any non-essential platform handling code, just use as much SDL (or similar libraries) as you can. Right now AGS deals with a million platform dependant things, and its abstracted in many ways to accommodate so. That made sense back in the day when multiplatform libraries were the exception rather than the norm, but nowadays there are plenty of options maintained by other people to help you, take advantage of that.

About the wiki entries themselves:
Path handling: Forget about using SDL or rolling your own, using dirent in Windows, etc, etc... There are many options in 2020 to deal with this as I mentioned before, and this is an excellent example of it. One of the best and most simple ones is just using PhysFS, which also happily works on consoles, and abstracts things like paths or even "containers" (you can have files inside a zip or in a directory, but in the end PhysFS abstracts all that). It's also by the main maintainer of SDL, so you know quality is at least as good.

SDL Library placement in Filesystem and CMakeLists.txt, Makefile, Vcxproj and SDL in the CI and Dockerfiles: This is a non issue, just use CMake for everything, SDL just works with it everywhere (I have it compiling for Linux, Windows, macOS and Switch on my own CI) and has its own CMakefile already. If you start juggling with different build systems you're going to have a bad day... Also, don't "ignore Android & iOS", it should work as well (I haven't added a build step for them, but I'm not expecting big issues with them, except maybe iOS for a bit, but it's a one time thing)

SDL Video and Window handling and graphics and SDL and Directx9 renderer: If you use the SDL Renderer API everything should just work everywhere (including consoles and smartphones), the only reason not to do it is in case you need special shaders or similar.
The only shader I've seen cannot be replicated directly is lighting, but that might just mean there might be another way of getting the same or a similar result using the SDL Renderer API instead of having the exact 100% result that AGS3 does today, given that AGS4 is a clean break.
It's also a good opportunity to drop the madness around transparency working in different ways depending if you have a value of 0, 1 to 250 or >250... Same for tinting, even if it doesn't match the AGS3 algorithm, just doing a simple color multiplication like the SDL Renderer API does should be enough...

All of that said, this is not the approach I'm going for, because the way in which AGS does rendering today doesn't match modern architectures (direct framebuffer access is not a thing anymore, anywhere) AND I need to keep compatibility with AGS3 because of Wadjet Eye's games, but the way I'm using is an even more extreme change so you might not want to do that for now, although I think it's the best option moving forward. As a summary, I'm not using SDL or its API for rendering, and also there is a lot of preprocessing of the data before it can be used with my fork of the engine, but it speeds up things tremendously, which is required for the Switch and maybe other platforms. I'll post more details if you're interested, although it's a work in progress and I'm a week or two away until I verify that everything works 100% like before.

But again, if you can use SDL Rendering and its limitations, it will work EVERYWHERE, which is work you don't need to do, as other people with way more experience is taking care of all the platform stuff for us.

Reduce Allegro BITMAPS part 1: Most of the mentioned libraries for antialiasing can just be dropped if the AA process is left to the driver (OpenGL, Metal, etc), but I guess you know this already, it's just not clear from the text.

For the rest I don't have any strong opinions yet, it's also a bit vague in many places like audio, so that's it for now.

@morganwillcock
Copy link
Member

@Arcnor thank you for posting this here

the way I'm using is an even more extreme change so you might not want to do that for now, although I think it's the best option moving forward. As a summary, I'm not using SDL or its API for rendering, and also there is a lot of preprocessing of the data before it can be used with my fork of the engine, but it speeds up things tremendously, which is required for the Switch and maybe other platforms. I'll post more details if you're interested

I think we also looking to change the project data formats and the whole game/media build pipeline, since what exists now is too dependent on the editor and hampers user workflow in general. It would be very interesting to learn more when it is convenient to you.

@Arcnor
Copy link

Arcnor commented Jun 26, 2020

@Arcnor thank you for posting this here

the way I'm using is an even more extreme change so you might not want to do that for now, although I think it's the best option moving forward. As a summary, I'm not using SDL or its API for rendering, and also there is a lot of preprocessing of the data before it can be used with my fork of the engine, but it speeds up things tremendously, which is required for the Switch and maybe other platforms. I'll post more details if you're interested

I think we also looking to change the project data formats and the whole game/media build pipeline, since what exists now is too dependent on the editor and hampers user workflow in general. It would be very interesting to learn more when it is convenient to you.

You're welcome @morganwillcock :). In any case, it looks like I misinterpreted what this ticket was about and there was a discussion about it here https://discordapp.com/channels/221047797292597249/221048199895449600/725838953147662358 so I'll let others comment on the best way to continue.

@morganwillcock
Copy link
Member

@Arcnor Unfortunately I missed the conversation in-person, but I did read it earlier and was a little disheartened by the direction it went in... I guess we will see how things turn out, but it would be good to stay in touch.

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Jun 26, 2020

From that discussion it appeared to me that we do not have the same understanding even of the goals, or relevant problems.

Personally I believe that the above advices are good, even if we won't use all of them. And it's definitely will be useful to exchange ideas (so long as they may be disclosed and not part of private code etc).

However, my impression was that this ticket is meant strictly for the purpose of replacing one backend library with another, which is a good first step. After that further changes could be done.

The main purpose of this replacement is to have contemporary library underneath, and really not much else.

@morganwillcock
Copy link
Member

I think the discussion was broader that what relates to this single ticket. I don't really see much logic to taking the branch that is decided to drop backwards compatibility and actually have significant modification, force yourself to port the graphics backend accommodating all the current features and issues, and then at the end of the process look at potentially removing some of those features and fixing some of those issues.

@ivan-mogilko
Copy link
Contributor

Alright, then, maybe I don't understand the goals at all. Do I understand correctly that you suggest to first rewrite the engine's object logic and change backend only after?

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Jun 27, 2020

force yourself to port the graphics backend accommodating all the current features and issues, and then at the end of the process look at potentially removing some of those features and fixing some of those issues.

Another thing, tbh that's a bit abstract statement. The question is, what is ought to be removed, and whether porting introduces extra difficulties solving which may eventually come redundant.
From what I understood (but did not check closely yet) that port keeps existing AGS renderers, only using SDL for drawing bitmaps (same way as allegro is used now).
At the same time it provides more stable contemporary system support for window creation, input etc, which may at least give certain relief in these areas.

I think any concerns about SDL port need to be clarified and discussed, to make sure everyone's see the same picture.

@ericoporto
Copy link
Member Author

ericoporto commented Jun 27, 2020

ags4...ericoporto:ags4--sdl2-sound-1

Above is a branch with Sonneveld's SDL2 sound implementation applied to AGS4 branch.

Sound turned out to be the biggest impact on the branch.

It includes the following:

  • SDL2;
  • SDL_sound + MojoAL for audio playing and decoding;
  • PhysFS (for the asset filesystem);
  • AGS Strings based on std::string, with additional nonstd::string_view added for script dicts. These are apparently needed for the multithreading (may not be if we use mutexes?);
  • C++ is kept at 11 but we can go 17 and just get the new features directly;
  • AGS sound rewritten with a audiocore thread that keeps the information of sound in ags in sync with the one in the MojoAL/SDL_sound;
  • only a single audio_clip class since everything goes through OpenAL compatible MojoAL instead of different decoders;
  • modified Apeg for video (may be broken);

As soon as threads are added for the sound to work well a lot of things will go crazy, if you run through a debugger it's pretty obvious that things are getting interrupted, you will step over/into and be met with a new context in the next step, in the case, the sound system will try to read something just before it's ready.

I imagine new strings and PhysFS was pushed into as a solution to this, the other alternative would be probably adding mutexes, but the new libraries work and reduce code.

This branch has only been built using CMake 3.16 (in CLion 2020.1) on Ubuntu 20.04. For Windows VS2019 with CMake it will clash when building allegro_main.cpp since both Allegro and SDL2 add a macro that redefines the main(argc,argv) on Windows. For this I need someone with Windows experience to help. For this reason the Editor is broken here - can't build AGS Native.dll.

Anyone with a Linux machine and a game compatible with ags4 (3.5.0.1-3.5.0.23 I think?), I highly recommend building this branch to listen to it as it sounds amazing.


Sorry for the confusion, this was a practical ticket of steps to get to SDL replacing Allegro. I didn't meant to diverge and discuss why.

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Jun 27, 2020

AGS Strings based on std::string, with additional nonstd::string_view added for script dicts. These are apparently needed for the multithreading (may not be if we use mutexes?);

I don't know how this is done in that branch, but in general sense they are not needed for multithreading, and mutexes would be required to protect std::string as well, as they are not protecting themselves.

C++ is kept at 11 but we can go 17 and just get the new features directly;

I would not do that right away unless critically needed (which I doubt it is). Every upgrade like this changes compiler requirement (and that's different on every platform) so this is something to be very careful about.

@ericoporto
Copy link
Member Author

ericoporto commented Jun 27, 2020

I don't know how this is done in that branch, but in general sense they are not needed for multithreading, and mutexes would be required to protect std::string as well, as they are not protecting themselves.

Makes sense. I was going from reading the comments, commits and discussions. Not actually the code, or documentation which I should have :/

I would not do that right away unless critically needed (which I doubt it is). Every upgrade like this changes compiler requirement (and that's different on every platform) so this is something to be very careful about.

Yes, makes sense.

Just to be clear, the branch doesn't actually upgrade the C++ to 17, I just mentioned it because I added some polyfill features from 14 and 17 to make the branch work (only string_view and make_unique), but these are coded there in C++11.


This is the same branch above (ags4 + MojoAL +SDL2 + SDL Sound), but it has the code before the std::string switch and PhysFS addition: https://github.com/ericoporto/ags/commits/ags4--sdl2-sound

The game will play in silence if you build for release, but if one build for debug it should crash when trying to access a buffer before the data is there. The thread method is on audiocore class.

Unfortunately I also only ran and built with CMake (through CLion 2020.1) and Ubuntu 20.04.

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Sep 25, 2020

Just a note to above branches, since I started investigating them, the commits there are real mess, with all due respect :-D: a single commit called "adds PhysFS support..." for example changes a whole lot of stuff at once, both related and not related to streams, moves code around etc. Need to try and split these in smaller changes to understand what's going on. Probably will try without extra libs, and without new utility classes (if possible) at first, as I want to know how all that is relevant to the multithread problems.

@ivan-mogilko
Copy link
Contributor

As for the steps, I tend to agree that sound may be easiest to replace, because it's pretty much a separate thing.

I also learnt there is a way to use SDL in your program without SDL main and loop, which maybe will help to create intermediate builds, gradually replacing Allegro to SDL in various engine parts.

@ericoporto
Copy link
Member Author

The commits are indeed a mess. Trying without extra libs is a good strategy! I am excited you are looking into this. :)

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Sep 25, 2020

I cloned the test branch and added few fixes to build on Windows: https://github.com/ivan-mogilko/ags-refactoring/tree/ags4--sdl2-sound-1

PhysFS appeared to be completely unnecessary, used in one class which may be disabled (there's few cases of unused new code overall, maybe prepared for the future).

I added SDL2 static lib from their website directly for this test. I don't know whether it's best option to have full SDL2 source in the repository, or why it was made so in the first place (for simplicity sake?).

EDIT: although it compiles, it does not really work though, when trying to play a sound it displays several assertions fail and then crashes; so I must investigate further.

@rofl0r
Copy link
Contributor

rofl0r commented Sep 25, 2020

I don't know whether it's best option to have full SDL2 source in the repository

this doesn't sound good at all, much less so "best"

@ivan-mogilko
Copy link
Contributor

@ericoporto unfortunately the code as it is does not work at least for me.

I suspect that sound playback init has some actions in a wrong order.

Without going too much into details, what happens there is that ALSource is getting polled by the internal thread (mix_context in mojoal.c) earlier than it's initialized with actual data (call to alSourceQueueBuffers in our audio_core.cpp). Because of that sound gets discarded by mojoal and never really plays.

I'll have to research this further, because I had no experience with this sound library before.

@ericoporto
Copy link
Member Author

ericoporto commented Sep 25, 2020

image
@ivan-mogilko I get the following error with VS2019's CMake when building your branch.

I haven't build my branch on Windows, as I note there, the only place I tested at the time was Ubuntu 20.04 and latest CMake there.

I suspect that sound playback init has some actions in a wrong order.

This is possible, my first approach used only the audio core and nothing else (I think basically no changes except on the media folder). At the time I got a "race condition" (not sure, the context switched and the audio data was gone in the next step) trying to access the vox package.

this is the branch: https://github.com/ericoporto/ags/commits/ags4--sdl2-sound

this is the significant commit: ericoporto@611b12c

But after I pulled more things from Sonnevelds branch (the branch you looked into) then it worked - so this is why I dismissed this at the time.

The game I used to test at the time was Tea for Two.

I'll have to research this further, because I had no experience with this sound library before.

This is valid. Overall I see Icculus libraries being used in most games I play but I find their documentation kinda... non-existent. So the source is probably where a lot of information is. But things do tend to work accross different systems.

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Sep 25, 2020

Ok, this is weird, but I think I found a bug in MojoAL source... (ivan-mogilko@c23b596#diff-baa934647843fd49d8f1863017135f30L4145)

The problem there was that alGenBuffers function that suppose to return fresh buffers to fill instead always returns first N buffer IDs regardless of whether they are currently free or not. Because of this our own list of buffers gets filled with same first 2 IDs, and engine tries to read into them while they still may be in use..... causing asserts in MojoAL in turn.

Idk, maybe I misunderstood something, but only after this change I can finally hear sound.

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Sep 25, 2020

Aha, apparently there was a bug, because newest library code has additional condition, compared to what we use:
https://hg.icculus.org/icculus/mojoAL/file/tip/mojoal.c#l4290

EDIT: applied their patch instead, looks more correct

@ericoporto
Copy link
Member Author

Weird question, but does it sound good? When listening, do you notice any stutter or something?

@ericoporto
Copy link
Member Author

ericoporto commented Nov 20, 2020

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
Copy link
Member Author

ericoporto commented Nov 20, 2020

I changed the platform detection code to be done using preprocessor definitions in the Allegro C code instead of using CMake: ericoporto@dddc4cb

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
Copy link
Contributor

ivan-mogilko commented Nov 20, 2020

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
Copy link
Contributor

ivan-mogilko commented Nov 20, 2020

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
Copy link
Member Author

ericoporto commented Nov 20, 2020

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
Copy link
Contributor

ivan-mogilko commented Nov 20, 2020

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
Copy link
Contributor

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

@ericoporto
Copy link
Member Author

Awesome! :D Great work!

@rofl0r
Copy link
Contributor

rofl0r commented Nov 20, 2020

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
Copy link
Contributor

ivan-mogilko commented Nov 21, 2020

@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
Copy link
Member Author

@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
Copy link
Contributor

rofl0r commented Nov 21, 2020

../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
Copy link
Contributor

rofl0r commented Nov 21, 2020

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
Copy link
Contributor

rofl0r commented Nov 21, 2020

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
Copy link
Contributor

ivan-mogilko commented Nov 21, 2020

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
Copy link
Contributor

rofl0r commented Nov 21, 2020

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
Copy link
Contributor

rofl0r commented Nov 21, 2020

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
Copy link
Member Author

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
Copy link
Contributor

ivan-mogilko commented Nov 23, 2020

@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
Copy link
Contributor

@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: #1121 (comment)

@ivan-mogilko
Copy link
Contributor

I opened a PR: #1136

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

@ericoporto
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

ericoporto commented Dec 9, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ags 4 related to the ags4 development context: code fixing/improving existing code: refactor, optimise, tidy...
Projects
None yet
Development

No branches or pull requests

7 participants