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

Use direct SDL2 calls instead of relying on an extra process #15

Merged
merged 5 commits into from
Oct 21, 2024

Conversation

clementgallet
Copy link

@clementgallet clementgallet commented Oct 3, 2024

Here is a change for the SDL2 build, to call directly the SDL2 functions instead of relying on a second executable to receive pixels and send events.

To build this, three .jar libraries are needed to be placed inside a created lib sub-directory:

I'm not an expert on Java, so these libraries may be installed automatically with a good config?

You also need the SDL2 library on your system. The libsdl4j file must have a lower version than the SDL2 library installed on your system, otherwise the wrapper will fail initializing. You should be able to safely pick an old SDL2 library because all the SDL2 functions called are pretty standard.

What is supported right now in this initial commit is game window, keyboard events, joystick events (buttons and hats only), mouse events. What is not supported is changing screen resolution, flipping, fullscreen, as well as joystick axes. I've kept the corresponding code commented so that this can be completed eventually.

This was tested on Linux only.

EDIT: I forgot to give context on this. This is needed to give support to freej2me in libTAS. The original sdl2 build uses two processes, which is not supported at all. With this PR, the emulator launches fine in libTAS with inputs working correctly.

@vadosnaprimer
Copy link

A little note: the goal of this change is to help with libTAS support, see clementgallet/libTAS#156 (comment)

@AShiningRay
Copy link
Collaborator

Huh... the usefulness of this actually extends far beyond libTAS, it's very portable as it doesn't rely on arch-specific code (at least not to the extent of being more limiting than JVM ports themselves, from what i've seen so far), simplifies the whole SDL stack to the point i'd turn it into the actual FreeJ2ME jar, and to top it all off, it appears to be rather easy to integrate this with the AWTGUI, so it's the best of both worlds in this emulator as far as i'm concerned. The only downside i can think of is that it won't be able to share code with the libretro interface as i was intending to do... but then again, we might as well drop the current FreeJ2ME jar and go with this one instead so it still makes things easier to maintain either way.

Bear in mind i didn't test this at the moment, might be able to this weekend... but from what i've seen, it's almost good to go. Seriously considering packaging these dependencies in this repo and crediting them separately, since it'd make the whole build process easier for users... and i'd also like to keep Ant as our builder instead of Maven.

All in all, i'm heavily in favor of this change... gonna follow it up in a few days.

@AShiningRay
Copy link
Collaborator

Currently i get a blank canvas when running this... not sure if that's to be expected at this point or not (might be my hybrid graphics setup for all i know). Couldn't find anything immediately noticeable that might explain why it's behaving like this either.

What i do know at the moment is that the issue certainly lies in the 'setByte()' for loop, as throwing solid colors into it does make a difference and said solid colors are displayed as expected... which is strange, because the color values being passed to it are also correct based on the prior SDL implementation.

@AShiningRay
Copy link
Collaborator

Okay, so right now, at least it's functional.
image

Performance is pretty terrible compared to the current FreeJ2ME standalone and libretro, performing pretty similar to the web preview that runs on a JavaScript translation layer. BUT, it works, and it's a good framework for us to build from, as it integrates SDL and the java code (mostly) seamlessly.

There are also a few bugs of note, with the most notorious being the SDL window getting stuck at a blank canvas at the start, requiring the renderer and texture to be re-created a few ms after booting in order to start capturing frame data, but other than that, it's solid...

@clementgallet Here's a patch that will get your source on par with mine:
0001-Anbu-Improve-on-the-new-SDL-interface.zip

It introduces some of the missing functionality, and fixes other smaller issues on the SDL side of things too. Looking forward to getting this one ready for review, then moving on to the UI overhaul.

AShiningRay and others added 2 commits October 6, 2024 23:44
This should not only help it render normally with a workaround (entire
window was a black canvas on my platform before), but also fix
some other issues it had, like having input logic tied to render
logic, and not supporting window scaling + fullscreen (this one still
needs work).

It now has support for Dumping Midi Streams and Custom SoundFonts,
although the user can't set it at runtime due to the lack of a UI,
which hopefully won't be much of a problem if AWT's integration
ends up being easy enough.

Another "issue" for now is performance, with this interface performing
FAR worse than the Standalone AWT jar, and Libretro's interface, but
it's also something that can be worked on later.

Dependencies are also added to the repo with this one. They won't need
to be updated any time soon, and might even be downgraded to make
sure we support older versions of SDL2.
SDL_DestroyRenderer(renderer);
renderer = SDL_CreateRenderer(window, -1, 0);
texture = SDL_CreateTexture(renderer, SDL_PIXELFORMAT_RGB24, SDL_TEXTUREACCESS_STREAMING, lcdWidth, lcdHeight);
sdl.pixels = new Memory(lcdWidth * lcdHeight * 3);
Copy link
Author

@clementgallet clementgallet Oct 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed or part of the workaround? Only SDL_SetWindowSize() should be required, and the game I'm testing is crash on SDL_DestroyRenderer() here after a few seconds, possibly caused by a resolution change?

Copy link
Collaborator

@AShiningRay AShiningRay Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately DestroyRenderer is required, at least here, in order to even show something on the screen after booting. Still, for now that should only trigger right at boot unless the user is changing the screen res on the fly, which shouldn't be possible yet (scaling works fine from what i've tested, though it did also crash previously before i put that block inside 'paint()' to make sure it only got called before an actual render). Which game is that? I'll take a look at it and see what it might be trying to do.

Edit: I also definitely don't want that as an actual solution, it's just a hack that allows me to actually run this interface on my end and begin fixing other stuff... ideally we should find out why this interface refuses to run properly without this (and that awful updateRenderer mess right above), then clean it all up, i just couldn't find anything conclusive yet, unfortunately.

@clementgallet
Copy link
Author

My crash was just due to the fact that we forgot to set resolutionChanged to false after recreating the renderer :D
I guess the texture limit was reached or something like that.

@AShiningRay
Copy link
Collaborator

My crash was just due to the fact that we forgot to set resolutionChanged to false after recreating the renderer :D I guess the texture limit was reached or something like that.

Nice, but I still need to find a way to remove the need for that entirely... the old interface didn't need this, and it still breaks in JBenchmark 2 once i start the tests if resolutionChanged is set to false after booting. Though i don't know why exactly that's happening only here and nowhere else yet. My only hunch so far is that MobilePlatform isn't setting up the canvas just in time for SDL to get data from it, but it still needs a deeper look into.

At the very least, this is moving along nicely.

@AShiningRay
Copy link
Collaborator

AShiningRay commented Oct 12, 2024

@clementgallet It appears that SDL doesn't like it when the Display class acquires a lock in order to set a new target Displayable to be rendered. (I really don't know why i overcomplicated this part so much at first, lol)

Notifying SDL about those situations and only resetting the renderer then (and also when resizing the screen to not break rendering) seems to fix the issue, now it not only renders fine after boot on all jars i tested, it also fixes JBenchmark2, which calls upon setCurrent only after the user starts the test (which meant it was instantly frozen from there on, as the bootup hack wouldn't cover this unless the renderer was constantly recreated)

As a bonus, not needing to re-create the renderer except in cases where it's required to do so makes SDL WAY faster, to the point it almost matches the libretro interface. Sending the patch right now. (Edit: Now rebased to fit on top of the current tree, and libsdl4j has also been trimmed down, now i think we'd be good for a final review pass):
fixup-patches.zip

This would leave only the SDL UI and input mapping to be done, but it's not something that this PR should tackle, and i can handle that later as well, the focus here is in making this work better with TAS... which reminds me, would hooking up an AWT GUI break anything even if SDL is already the main process? Currently considering it because to me that one seems way easier to get input stuff sorted out, since mouse will work there out of the box. If it would, then there wouldn't be much to do other than rewrite the whole config UI, which is doable, but nowhere near as good looking or user-friendly.

@vadosnaprimer
Copy link

There's currently a conflict but I don't understand how to resolve it.

@AShiningRay
Copy link
Collaborator

There's currently a conflict but I don't understand how to resolve it.

Big audio rewrite is the issue. Thankfully it's pretty small stuff, as only Anbu got hit, and it does very little with audio in there.

@AShiningRay AShiningRay linked an issue Oct 21, 2024 that may be closed by this pull request
@AShiningRay AShiningRay merged commit 5d2b56d into TASEmulators:devel Oct 21, 2024
1 check failed
@AShiningRay
Copy link
Collaborator

Wow, i'm not using Github's web thingy for this ever again. Sorry for that... will fire up my desktop to redo this merge manually.

@AShiningRay
Copy link
Collaborator

Alright, now the main tree at least builds fine and publishes artifacts... man, what a mess that was. Sorry for that @clementgallet , your branch is completely nonsensical now because of this.

@clementgallet
Copy link
Author

Do you expect me to do something about the last patch you sent?

@AShiningRay
Copy link
Collaborator

Do you expect me to do something about the last patch you sent?

No need, it's all in this repo's main branch now, i'm just sorry for messing up your own sdl-native branch along the way, that's all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI artifacts from pull requests fail to get uploaded SDL Interface should behave like the Libretro core
3 participants