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

Fixes for AGS3 SDL2 port #1148

Closed
33 of 37 tasks
ericoporto opened this issue Dec 9, 2020 · 80 comments
Closed
33 of 37 tasks

Fixes for AGS3 SDL2 port #1148

ericoporto opened this issue Dec 9, 2020 · 80 comments
Labels
ags3 related to ags3 (version with full backward compatibility) backend: sdl2 related to sdl2 library
Milestone

Comments

@ericoporto
Copy link
Member

ericoporto commented Dec 9, 2020

Issue to aggregate remaining fixes for AGS3 SDL2 port (now merged into master).

All Ports


Windows


Linux


MacOS

  • Not exactly a task, but the port needs more testing.
  • Update build instructions. (cmake generates xcode project or cmake builds directly, mention SDL2 framework)

Android

  • Touch to mouse translation auto translated from SDL2 is not similar to old Android backend behavior. On a plus it allows drag and drop module to work, finger hold on screen is readable by mouse.IsButtonDown(): https://youtu.be/lbdth4SHbSE.

iOS

iOS support is a bit tricky (for me), SDL2 supports but I lack the device to test it, so for now I think the main task is finding someone who is interested in picking it up. If no one picks it up, I can do it on an emulator.

https://hg.libsdl.org/SDL/file/bc96d65e47da/docs/README-cmake.md
https://hg.libsdl.org/SDL/file/bc96d65e47da/docs/README-ios.md

mapped on #1505


Completed Tasks

Click to Expand - Finished Tasks hidden for readability

I marked with one asterisk * what is reasonably done but hasn't been merged yet, and with two asterisk ** what the fix is made but isn't completely working yet and hasn't been merged. Probably not relevant anymore, but kept for history.

All Ports

Windows

Linux

MacOS

Android

  • Java completely rewritten to use SDL2 class instead of AGS Glue. *
  • Launcher (Java code) has to be redone.
  • Optionally, add single game launcher (Java code) in the AGS repository.
  • Use of CMake builds for Android to easily adapt to Android build system changes, it also reduces code. *
  • Restore or not usage of android.cfg, some options relied on Allegro4 android backend.drag and drop module to work, finger hold on screen is readable by mouse.IsButtonDown(): https://youtu.be/lbdth4SHbSE.
  • Add OpenGL ES2 support, wasn't available on previous port in the ags3 branch, but I have the code already written and it works best with newer android devices. *
  • Delete unused code. Some old specific Allegro4 Android code can be deleted.
  • Update build instructions. (./gradlew build or load the project in Android Studio, mention SDK and NDK versions)

https://hg.libsdl.org/SDL/file/default/docs/README-android.md


If more issues are found or something is missing, please write about it below.

@ivan-mogilko
Copy link
Contributor

By the way, SDL2 devs fixed my reported bug :)
https://bugzilla.libsdl.org/show_bug.cgi?id=5329

I think they target 2.0.14 release, so we may update when it's out.

@ericoporto
Copy link
Member Author

Ha, very cool! 😃

One thing I remembered is there were some TO-DOs on the sound part but I don't remember if they are supposed to be in the list above. 😬

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Dec 10, 2020

Something that might be added:

  • sound config, affecting selected driver (right now it's a simple on-off switch)
  • config option for software renderer that defines preferred output type (used when creating SDL_Renderer)

There were some problems which I met while testing random games, but I ignored them temporarily. Primarily in 8-bit games, iirc colors could have been shifted again, but not sure. I'd need to repeat tests and document these.

@ivan-mogilko
Copy link
Contributor

Oh, also support resizing window on desktop oses; I skipped that for simplicity but it was supported in sonneveld's branch.

@ericoporto
Copy link
Member Author

Added above in the list :)

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Jan 8, 2021

I was a bit distracted for a while, is my understanding correct that macos builds and works with SDL2 now?
Desktop OSes are my primary concern before merging this into ags3 (master) and ags4.
Applying Android changes is my first next todo. iOS may wait, because it's more niche and usually commercial gamedevs do their own fixes for it anyway.


On another topic, I was supplying prebuilt libraries for Windows in my dropbox. Is there any option to do so on github? I know one could attach these to one of the releases, but this seem an obscure place, as the links should stay permanent for a long while.

@ericoporto
Copy link
Member Author

ericoporto commented Jan 8, 2021

It worked on my Mojave box and High Sierra, yes. I haven't tried on the more recent MacOSes - but I believe it should. I can make a Catalina test but I don't have access to the latest Big Sur yet.

I am working on Android here: ericoporto/ags/ags3--sdl2-android and picking up the latest Android project from my own main branch for merging - I will write better commit messages, for now I am just figuring the commits since in some parts is a lot of things. Just haven't being able to finish yet.


About prebuilt Windows libraries, it could be a repository just for them in the organization.

As an example, I made a NuGet package for the xiph libraries but just them here: https://github.com/ericoporto/xiph-for-ags

I made it build and upload using only Azure to avoid manual steps, I could make something like this. I actually used it on the Windows Docker to avoid a vcpkg bug on our CI, but maybe a proper repo for Windows prebuilt binaries could exist in ags org, with it's own releases. I didn't went through, but by shipping it's own .prop file in it, the NuGet library can even be part of the Windows VS Solution itself if desired. Anyway, in this case, the libraries are both available in it's own GitHub releases and the NuGet package repository.

Other than making a separate repository for ags dependencies on Windows, GitHub has a packages feature that I haven't explored but it may be able to serve just that.

@rofl0r
Copy link
Contributor

rofl0r commented Jan 9, 2021

Other than making a separate repository for ags dependencies on Windows

this seems to be a good choice. i've seen various orgas on github that put their releases/binaries into dedicated repos. the files will stay available using a link with commit hash even if they are replaced in subsequent commits, unless the commits adding them become orphans (e.g. with a force-push removing them). an alternative could be sourceforge or osdn.net (located in japan).

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Jan 9, 2021

  • TODO: Update license files in the win installer folder

@ericoporto
Copy link
Member Author

Added to the list!

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Jan 9, 2021

Something I forgot to test earlier, but I suspect that we may use slightly less old DirectX SDK. There was a need to use particular "August 2007" version because it contained headers necessary for Allegro 4 graphic and input drivers.

As a part of commit ae8b736 I've removed following linked libs: ddraw.lib, dinput.lib, dsound.lib, dxguid.lib. The only lib still required is d3d9.lib (for Direct3D renderer). This means we may try to use latest SDK that still supports Direct3D 9 (iirc from 2008 or 2009, but I don't remember exactly).

UPDATE: turned out we do not require DirectX SDK at all anymore, as engine only needs d3d9.lib which is a part of Windows SDK.

@ivan-mogilko
Copy link
Contributor

Found bug in new sound backend: it does not have a proper cancellation in case it failed to decode sound data.
If the clip is told to play repeatedly it gets into infinite loop trying to fill the buffer over and over again.

@ivan-mogilko
Copy link
Contributor

MIDI playback is broken currently. I don't remember if it happens for any midi files, but in this game for instance midi music sounds like 5-10 times slower when run on Windows.

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Jan 16, 2021

(+) removed vorbisfile lib link from Windows version, need to double check that it still works if we remove it from other ports (linux etc); simply did not have time to test it myself yet.

@ivan-mogilko
Copy link
Contributor

Merged latest ags3 branch, DrawSprite fix should be there now, and maybe something else.

@rofl0r
Copy link
Contributor

rofl0r commented Jan 16, 2021

but in this game for instance midi music sounds like 5-10 times slower when run on Windows.

this sounds like the bug i reported to SDL bugtracker which is the result of libmodplug not finding midi instruments (mentioned in #367)

@ericoporto
Copy link
Member Author

Just heads-up, SDL is migrating to GitHub: https://github.com/libsdl-org/SDL

@rofl0r
Copy link
Contributor

rofl0r commented Jan 21, 2021

Just heads-up, SDL is migrating to GitHub: https://github.com/libsdl-org/SDL

that's great. though i can't find any official announcement of the move, did you see one?
the sdl hp also still links to their hg repo which has seen recent commits as of 20 hrs ago.

@ericoporto
Copy link
Member Author

ericoporto commented Jan 22, 2021

Merged latest ags3 branch, DrawSprite fix should be there now, and maybe something else

Sorry, I have rebased and am still hitting DrawSprite error. I confirmed it also happens on Linux build. I think it's from agsblend, but I have the plugins built-in, plus I really do remember this specific error being fixed but I couldn't find the commit. Problem is I don't remember how this was fixed and searching issues here I came up empty.

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Jan 22, 2021

I confused this with DrawImage bug. DrawSprite is probably from agsblend plugin?

So, you are saying this happens only in ags3--sdl branch? and not in current master?

@ericoporto
Copy link
Member Author

ericoporto commented Jan 22, 2021

You are right, I confused one error with the other too! Turns out I thought I had built the Engine with built-in plugins but I didn't. Sorry for the confusion. (It's alright in both the master and ags3--sdl2, I tested!)

@ericoporto
Copy link
Member Author

ericoporto commented Jan 24, 2021

Hey, I am trying to make the new SDL "Software" Renderer to work on Android and I am trying to figure out how to stretch it through the screen. I looked at source code and the new SDL Software Renderer is different from the one that was before (that one had other bugs though), anyway any guesses on where to look for why it's not stretching?

I tried using AGS Script API to alternate between windowed mode and fullscreen, and while windowed makes the game correctly stretch in the non-fullscreen area, going back to fullscreen after still makes the game tiny.

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Jan 24, 2021

I could look into this if I find spare time, but if I remember correctly, although the old pre-SDL renderer worked in similar principle, the essential difference was that the conversion into opengl texture and stretching was done inside patched allegro library. The engine's software renderer never really bothered what the final resolution is in case of mobile device.

So my guess would be that android backend simply does not pass final screen resolution to the engine, because it was not done before. Maybe it should work just how our OpenGL renderer works now - there is some functions or variables that retrieve device resolution.
android_screen_physical_width and android_screen_physical_height? Unless they were renamed or replaced.

@rofl0r
Copy link
Contributor

rofl0r commented Jan 24, 2021

i've found this guide quite good: https://wiki.libsdl.org/MigrationGuide#Moving_from_SDL_1.2_to_2.0

if i remember correctly, one basically just has to fill a surface with pixel data in the original format and depending on fullscreen settings the chosen renderer will stretch it to screen size either in sw or hw.

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Jan 24, 2021

if i remember correctly, one basically just has to fill a surface with pixel data in the original format and depending on fullscreen settings the chosen renderer will stretch it to screen size either in sw or hw.

This is similar to what we do now:
https://github.com/adventuregamestudio/ags/blob/ags3--sdl2/Engine/gfx/ali3dsw.cpp#L501

The point is that on desktop systems engine calculates final resolution from config, but on mobile it has to rely on corresponding backend or "glue" passing these parameters.

@ericoporto
Copy link
Member Author

ericoporto commented Jan 24, 2021

Thanks dudes! I will eat lunch and rellax a little and take a look later today. I have followed things with a debugger and haven't found anything yet but I haven't dived into SDL itself. One thing weird is I found a specific game (Suli) that is working correctly with it on Android! I also advanced a bit on CI and organizing commits and am thinking to at least have a working WIP PR just to see how it goes.

Edit: I actually removed a lot of Android specific variables, including that device size, theoretically it's not needed anymore - the OGL renderer is working fine without them. :)

@ivan-mogilko
Copy link
Contributor

Well, sounds like it already changed beyond my knowledge.

@ya-isakov
Copy link

So, are there plans to allow Cmake build with system SDL2, and static sdl2-sound? I can try to do a MR...

@rofl0r
Copy link
Contributor

rofl0r commented Apr 11, 2021

are there plans to allow Cmake build with system SDL2, and static sdl2-sound?

doesn't cmake allow you configurable options, like configure-style --with-system-sdl2 ?
i think i've seen something like that when configuring cmake stuff with ccmake.

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Apr 11, 2021

So, are there plans to allow Cmake build with system SDL2, and static sdl2-sound? I can try to do a MR...

I cannot tell about the plans in regards to that, but if something is beneficial, then it may be added as an option.

sdl2-sound will be released eventually and then it may possibly be taken from the system too.

@ericoporto
Copy link
Member Author

hey, with the Software driver, when I try to alternate between Windowed and Fullscreen (doesn't matter which one started), the virtualScreen from the SDLRendererGraphicsDriver is becoming null and crashing for me (I tested on Ubuntu 20.04). This doesn't happen with other gfx drivers.

Is this happening with any of you?

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Apr 26, 2021

@ericoporto

hey, with the Software driver, when I try to alternate between Windowed and Fullscreen (doesn't matter which one started), the virtualScreen from the SDLRendererGraphicsDriver is becoming null and crashing for me (I tested on Ubuntu 20.04). This doesn't happen with other gfx drivers.

Is this happening with any of you?

Yes, some logic is broken there compared to 3.5.*, it deletes virtual screen when display mode is changed but does not recreate it. I need to check this out. Not sure it should destroy it on mode switch anymore with the new software renderer.

@ivan-mogilko
Copy link
Contributor

Pushed couple of fixes 42f6167 and 8c1ed36

@ericoporto
Copy link
Member Author

ericoporto commented Apr 26, 2021

Thanks, I will test it. I wasn't sure if it was it or my machine - had some video driver problems on this weekend.

Edit: it fixed! Software driver now works alternating between Fullscreen and windowed!

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Apr 30, 2021

I created "release-3.5.1" branch for finalizing and future patching 3.5.1, and merged ags3--sdl2 branch into master.

So master now has sdl2 backend and from now on the development of 3.6.0 continues there.

@ivan-mogilko ivan-mogilko added the ags3 related to ags3 (version with full backward compatibility) label Apr 30, 2021
@ericoporto
Copy link
Member Author

I remembered something reggarding MIDI, the most recent version of SDL Sound here (at this time) has brought back actual timidity for MIDI playback, which fixes distortions.

Independently of this change, Windows users using the SDL2 port are required (in comparison to allegro4 branch) to do something like: http://dusted.dk/pages/playingMidiOnWindows/

I also updated the list at the top so it's a bit more readable.

@ivan-mogilko
Copy link
Contributor

In regards to Linux, there's still a section in README that mentions midi patches from allegro site. I put a "fixme" comment there because I don't know how SDL midi works and whether anything similar is required.

@rofl0r
Copy link
Contributor

rofl0r commented May 1, 2021

the most recent version of SDL Sound here (at this time) has brought back actual timidity for MIDI playback

actually they've thrown out modplug midi (after i fixed their code) and replaced it with timidity. whether that was a good move remains to be seen.

I put a "fixme" comment there because I don't know how SDL midi works and whether anything similar is required.

a timidity.cfg is required that has a correct setup pointing to a set of GUS-style midi patterns or a more modern soundfont. i guess it can't hurt to link to the patchset you previously recommended, unless it's incompatible (but iirc it isnt).

@ivan-mogilko
Copy link
Contributor

Found and fixed a major memory leak in the new audio system: f14e178

Following game script helps to observe the issue:

function room_RepExec() {
	if (IsKeyPressed(eKeySpace)) {
		atest.Play();
	}
}

function on_key_press(eKeyCode key) {
	atest.Play();
}

Simply holding a space key would crash a game in few seconds. After the fix the memory seem to no longer grow when the audio gets restarted.

@ericoporto
Copy link
Member Author

ericoporto commented Jul 5, 2021

@ivan-mogilko can you revise the list on top post and see if there's anything that has already been fixed that I didn't pickup?

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Jul 5, 2021

@ivan-mogilko can you revise the list on top post and see if there's anything that has already been fixed that I didn't pickup?

Right, I will do tomorrow.

There's a bug report recently that 3.6.0 always starts in a minimal-sized window on mac. Not sure if that's a renderer issue or a config parsing issue.
But maybe at this point it's better to begin opening separate tickets for each problem.

@ivan-mogilko
Copy link
Contributor

@ericoporto may we try enabling an OpenGL renderer for mac port? If it uses GLAD to load, like on Windows and Linux, then supposedly the engine will not fail completely if OpenGL is not supported on the machine and will be able to fallback to SDL software renderer.

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Jul 6, 2021

In regards to things fixed, this was fixed some time ago:

Editor doesn't put SDL2.dll together in the directory

but this will have to be changed again if we statically link

and I dont know about this one:

Sound subsystem and thread has to be explicitly stopped on exit before shutting down backend and deleting data, otherwise there may be errors.

Probably missed this, or dont remember. Is this still an issue?

@ericoporto
Copy link
Member Author

ericoporto commented Jul 6, 2021

may we try enabling an OpenGL renderer for mac port

Sure, I will look into it, I will get a PR for this tomorrow.

Sound subsystem and thread has to be explicitly stopped on exit before shutting down backend and deleting data, otherwise there may be errors.

Probably missed this, or dont remember. Is this still an issue?

I don't know about this one, you asked me at time to add this in the list, so I added. 😬

About the SDL2.dll, if you want to go with it dynamically linked I can add it to the installer, CI, ... I personally find neater when the ags game is just a magical exe file, but it's alright having dlls too.

@ericoporto
Copy link
Member Author

ericoporto commented Jul 8, 2021

@ivan-mogilko I did a version here that has OpenGL enabled for MacOS and can safely fail and use Software renderer if needed. I opened a PR in #1353 . I am not sure if the code changes make sense.

@ivan-mogilko
Copy link
Contributor

Okay, this one looks like complete now:

Sound subsystem and thread has to be explicitly stopped on exit before shutting down backend and deleting data, otherwise there may be errors.

@ericoporto
Copy link
Member Author

ericoporto commented Jul 20, 2021

Found bug in new sound backend: it does not have a proper cancellation in case it failed to decode sound data.
If the clip is told to play repeatedly it gets into infinite loop trying to fill the buffer over and over again.

from #1148 (comment)

@ivan-mogilko do you remember what that was? Does this bug still exists?

@ivan-mogilko
Copy link
Contributor

do you remember what that was? Does this bug still exists?

It should still be there I think, I'll try to address this in following days.

@ivan-mogilko
Copy link
Contributor

Found bug in new sound backend: it does not have a proper cancellation in case it failed to decode sound data.
If the clip is told to play repeatedly it gets into infinite loop trying to fill the buffer over and over again.

Ok I think this one is covered now.

Also made decoder pre-init sound sample and return error on failure, this restores original AudioClip.Play behavior in script too.

@ericoporto
Copy link
Member Author

ericoporto commented Jan 17, 2022

I think this issue has been running for a long time, we can split things that are needed to it's own issues and look into closing this one. When I wrote it, I meant it to just get the ball running for CI, build instructions and dependencies on the different platforms, and things that could be minor regressions. I think this is done.

Edit: I also wonder if we need two milestones for SDL2, one for what is needed for a beta release and other for things we want to have before the stable release.

@ericoporto
Copy link
Member Author

ericoporto commented Mar 7, 2022

I pulled some remaining items on the list to their own issues.

Regarding the remaining ones:

I think any additional discussion could have it's own issue if needed.

It's important to remember the ags allegro4 version also had it's bugs, it's possible we don't fix all new bugs and regressions and it's fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ags3 related to ags3 (version with full backward compatibility) backend: sdl2 related to sdl2 library
Projects
None yet
Development

No branches or pull requests

5 participants