EasyRPG / Player

RPG Maker 2000/2003 and EasyRPG games interpreter
https://easyrpg.org/player/
GNU General Public License v3.0
965 stars 183 forks source link

Config for Soundfont, Font and "Focus Lost" #3168

Closed Ghabry closed 3 days ago

Ghabry commented 7 months ago

Pause when focus lost is now a setting, not a compile time option:

screenshot_0 png

More Audio options

screenshot_2 png

Information about MIDI drivers

screenshot_1 png

The soundfont can be selected

screenshot_3 png

Font configuration with preview:

screenshot_4 png

screenshot_5 png

Mimigris commented 7 months ago

I know that it is still WIP but here's what I found in terms of issues while testing:

Now more on a feedback side, feel free to take that into account or not:

Also, the webplayer doesn't seem to like the entries Open Font/Sound directory and seems to randomly crash outside of that, I'm not entirely sure why for the second part.

Ghabry commented 7 months ago

Thanks for the exhaustive testing. I won't respond to all but they all sound reasonable.

The Pause when focus lost text changed since the picture that you sent and is now too long to properly fit on-screen (a custom font is used here but it's also the case with the default font)

Hm I already fixed this one locally, maybe forgot to commit it.

But yeah some messages do not fit. I think about implementing a "scrolling" feature that scrolls the text back and forth so the rest is visible.

Unlike changing MIDI Drivers, changing the soundfont on the fly with the setting menu will not change it without reloading the game, which can be annoying to properly test

Yeah I would prefer to have some kind of "hot reloading" here so the audio changes immediately. I just couldn't find a way yet that does not crash xD.

Having the possibility to maybe add an X number of pixels after a character could be useful for some fonts where all of the letters are sticked together, making them unreadable

Good idea. This is already required for StringPic so can be exposed.

Ghabry commented 6 months ago

Instead of "Open (Sound)Font" emscripten got now "Upload (Sound)Font".

Can already see certain game developers demanding that this is patched out but not my problem :P

https://easyrpg.org/play/pr3168

Note that live switching between two MIDI devices (e.g. FmMidi to Fluidsynth) is not supported. Such a change requires playing a new MIDI file.

Ghabry commented 6 months ago

Removed the "preload in a thread" commit as it appears to crash sometimes and have no time to figure out where the race condition is. Here the code for posterity:

commit ea340f22dea3571fa4988734b93ab4c4b90cc413
Date:   Thu Dec 21 15:07:15 2023 +0100

    Fluidsynth: Preinit soundfont in a thread on our major platforms.

    Reduces lag

diff --git a/src/audio_generic.cpp b/src/audio_generic.cpp
index cd45e13d..0839dcaa 100755
--- a/src/audio_generic.cpp
+++ b/src/audio_generic.cpp
@@ -218,7 +218,7 @@ bool GenericAudio::PlayOnChannel(BgmChannel& chan, Filesystem_Stream::InputStrea
        chan.decoder.reset();

        // Order is Fluidsynth, WildMidi, Native, FmMidi
-       bool fluidsynth = Audio().GetFluidsynthEnabled() && MidiDecoder::CreateFluidsynth(true);
+       bool fluidsynth = Audio().GetFluidsynthEnabled() && MidiDecoder::CreateFluidsynthWithLock(true);
        bool wildmidi = Audio().GetWildMidiEnabled() && MidiDecoder::CreateWildMidi(true);

        if (!fluidsynth && !wildmidi && Audio().GetNativeMidiEnabled()) {
diff --git a/src/audio_midi.cpp b/src/audio_midi.cpp
index 9827be97..91878f7c 100644
--- a/src/audio_midi.cpp
+++ b/src/audio_midi.cpp
@@ -26,7 +26,18 @@
 #ifdef USE_AUDIO_RESAMPLER
 #include "audio_resampler.h"
 #include "audio.h"
+#endif
+
+#if (defined(HAVE_FLUIDSYNTH) || defined(HAVE_FLUIDLITE)) && defined(SUPPORT_STD_SYNCHRONIZATION)
+#include <atomic>
+#include <mutex>
+#include <thread>

+namespace {
+   std::mutex sf_load_mutex;
+   std::thread sf_load_thread;
+   std::atomic<bool> sf_load_thread_done;
+}
 #endif

 void MidiDecoder::GetFormat(int& freq, AudioDecoderBase::Format& format, int& channels) const {
@@ -53,7 +64,7 @@ std::unique_ptr<AudioDecoderBase> MidiDecoder::Create(bool resample) {
    std::unique_ptr<AudioDecoderBase> mididec;

    if (Audio().GetFluidsynthEnabled()) {
-       mididec = CreateFluidsynth(resample);
+       mididec = CreateFluidsynthWithLock(resample);
    }

    if (!mididec && Audio().GetWildMidiEnabled()) {
@@ -90,6 +101,19 @@ std::unique_ptr<AudioDecoderBase> MidiDecoder::CreateFluidsynth(bool resample) {
    return mididec;
 }

+std::unique_ptr<AudioDecoderBase> MidiDecoder::CreateFluidsynthWithLock(bool resample) {
+#if (defined(HAVE_FLUIDSYNTH) || defined(HAVE_FLUIDLITE)) && defined(SUPPORT_STD_SYNCHRONIZATION)
+   const std::lock_guard<std::mutex> lock(sf_load_mutex);
+
+   if (sf_load_thread_done) {
+       sf_load_thread.join();
+       sf_load_thread_done.store(false);
+   }
+#endif
+
+   return CreateFluidsynth(resample);
+}
+
 std::unique_ptr<AudioDecoderBase> MidiDecoder::CreateWildMidi(bool resample) {
    std::unique_ptr<AudioDecoderBase> mididec;

@@ -133,6 +157,10 @@ std::unique_ptr<AudioDecoderBase> MidiDecoder::CreateFmMidi(bool resample) {
 }

 bool MidiDecoder::CheckFluidsynth(std::string& status_message) {
+#if (defined(HAVE_FLUIDSYNTH) || defined(HAVE_FLUIDLITE)) && defined(SUPPORT_STD_SYNCHRONIZATION)
+   const std::lock_guard<std::mutex> lock(sf_load_mutex);
+#endif
+
    if (works.fluidsynth && works.fluidsynth_status.empty()) {
        CreateFluidsynth(true);
    }
@@ -141,7 +169,22 @@ bool MidiDecoder::CheckFluidsynth(std::string& status_message) {
    return works.fluidsynth;
 }

+void MidiDecoder::PreloadFluidsynth(bool resample) {
+#if (defined(HAVE_FLUIDSYNTH) || defined(HAVE_FLUIDLITE)) && defined(SUPPORT_STD_SYNCHRONIZATION)
+   sf_load_thread_done.store(false);
+
+   sf_load_thread = std::thread([resample]() {
+       CreateFluidsynthWithLock(resample);
+       sf_load_thread_done.store(true);
+   });
+#endif
+}
+
 void MidiDecoder::ChangeFluidsynthSoundfont(StringView sf_path) {
+#if (defined(HAVE_FLUIDSYNTH) || defined(HAVE_FLUIDLITE)) && defined(SUPPORT_STD_SYNCHRONIZATION)
+   const std::lock_guard<std::mutex> lock(sf_load_mutex);
+#endif
+
    if (!works.fluidsynth || works.fluidsynth_status.empty()) {
        // Fluidsynth was not initialized yet or failed, will use the path from the config automatically
        works.fluidsynth = true;
@@ -173,6 +216,10 @@ void MidiDecoder::Reset() {
 #endif

 #if defined(HAVE_FLUIDSYNTH) || defined(HAVE_FLUIDLITE)
+#ifdef SUPPORT_STD_SYNCHRONIZATION
+   const std::lock_guard<std::mutex> lock(sf_load_mutex);
+#endif
+
    FluidSynthDecoder::ResetState();
 #endif
 }
diff --git a/src/audio_midi.h b/src/audio_midi.h
index 8882cc35..67313c7b 100644
--- a/src/audio_midi.h
+++ b/src/audio_midi.h
@@ -200,6 +200,8 @@ public:

    static std::unique_ptr<AudioDecoderBase> CreateFluidsynth(bool resample);

+   static std::unique_ptr<AudioDecoderBase> CreateFluidsynthWithLock(bool resample);
+
    static std::unique_ptr<AudioDecoderBase> CreateWildMidi(bool resample);

    static std::unique_ptr<AudioDecoderBase> CreateFmMidi(bool resample);
@@ -212,6 +214,13 @@ public:
     */
    static bool CheckFluidsynth(std::string& status_message);

+   /**
+    * Attempts to preload Fluidsynth and soundfont.
+    * Only works on our major platforms because std::mutex and std::thread are
+    * required.
+    */
+   static void PreloadFluidsynth(bool resample);
+
    static void ChangeFluidsynthSoundfont(StringView sf_path);

    /**
diff --git a/src/output.cpp b/src/output.cpp
index 62606ede..78adc9f8 100644
--- a/src/output.cpp
+++ b/src/output.cpp
@@ -51,6 +51,10 @@
 #include "font.h"
 #include "baseui.h"

+#ifdef SUPPORT_STD_SYNCHRONIZATION
+#  include <mutex>
+#endif
+
 using namespace std::chrono_literals;

 namespace {
@@ -99,6 +103,10 @@ namespace {
        std::string msg;
        LogLevel lvl = {};
    } last_message;
+
+#ifdef SUPPORT_STD_SYNCHRONIZATION
+   std::recursive_mutex output_mutex;
+#endif
 }

 LogLevel Output::GetLogLevel() {
@@ -118,6 +126,10 @@ void Output::IgnorePause(bool const val) {
 }

 static void WriteLog(LogLevel lvl, std::string const& msg, Color const& c = Color()) {
+#ifdef SUPPORT_STD_SYNCHRONIZATION
+   std::lock_guard<std::recursive_mutex> lock(output_mutex);
+#endif
+
 #ifdef EMSCRIPTEN

 // Allow pretty log output and filtering in browser console
diff --git a/src/player.cpp b/src/player.cpp
index 0107f478..492c1e4e 100644
--- a/src/player.cpp
+++ b/src/player.cpp
@@ -667,6 +667,7 @@ void Player::CreateGameObjects() {

    // Reinit MIDI
    MidiDecoder::Reset();
+   MidiDecoder::PreloadFluidsynth(true);

    // Load the meta information file.
    // Note: This should eventually be split across multiple folders as described in Issue #1210
diff --git a/src/system.h b/src/system.h
index 74b8d0b1..83850b3d 100644
--- a/src/system.h
+++ b/src/system.h
@@ -52,6 +52,7 @@
 #  define SUPPORT_JOYSTICK
 #  define SUPPORT_JOYSTICK_AXIS
 #  define SUPPORT_TOUCH
+#  define SUPPORT_STD_SYNCHRONIZATION
 #elif defined(EMSCRIPTEN)
 #  define SUPPORT_MOUSE
 #  define SUPPORT_TOUCH
@@ -77,6 +78,7 @@
 #  define SUPPORT_TOUCH
 #  define SUPPORT_JOYSTICK
 #  define SUPPORT_JOYSTICK_AXIS
+#  define SUPPORT_STD_SYNCHRONIZATION
 #elif defined(__SWITCH__)
 #  define SUPPORT_JOYSTICK
 #  define SUPPORT_JOYSTICK_AXIS
@@ -93,6 +95,7 @@
 #  define SUPPORT_TOUCH
 #  define SUPPORT_JOYSTICK
 #  define SUPPORT_JOYSTICK_AXIS
+#  define SUPPORT_STD_SYNCHRONIZATION
 #endif

 #ifdef USE_SDL
Ghabry commented 6 months ago

This is ready for testing now.

Stuff I will not address here (that is all reasonable, could be moved to another issue):

If I remember correctly, there is a thing on the RPG_RT where the two fonts used are not the same depending on the region/encoding, maybe having something to reverse the fonts used (either as a toggle directly in the menu or automatically depending on the encoding) could be used

Having the possibility to maybe add an X number of pixels after a character could be useful for some fonts where all of the letters are sticked together, making them unreadable

This requires rewriting parts of the font code because fonts are "shared" so has the possibility to cause issues when the default font is used in Show String Pic.

Mimigris commented 6 months ago

FontIssue.zip I have some fonts that cause a crash when attempted to be previewed in the font section of the settings (either a complete crash of the Player or a display of an error Cannot render character), feel free to take a look at it.

Ghabry commented 6 months ago

Rebased to have the better spacing. Good for testing.

@Mimigris I know why it crashes. Fixing this 2024, no time anymore xD

Mimigris commented 6 months ago

I had a lot of fonts that were not really readable due to spacing issues but now with this commit almost every font I have (if we exclude fonts that are not intended to really be used at this size) are readable 🎉 Since it now works as intended, there should be no need to have an option to adjust the spacing indeed.

@Mimigris I know why it crashes. Fixing this 2024, no time anymore xD

No problem, no need to rush, have a nice holiday!

Also, as a bonus, to give an example on a use of the font setting, the feature can be used to play a game in a language where the proper display of the character requires an additional font and that was not set with the game itself: here's how the Thai translation of Ib looks: EasyRPG Thai Ib fork <-> EasyRPG (use of the default font) <-> EasyRPG (Tahoma, Font Size 12) image

Ghabry commented 6 months ago

No idea what that Thai fork is but yeah, Thai is an example for a language where you need a shaping library (Harfbuzz) for the positioning of the glyphs (just moving the drawing pen to the right doesn't work as you can see in the middle, the glyphs can be also stacked etc.)

Ghabry commented 5 months ago

@Mimigris question: You reported a while ago somewhere else that the font embedded in RPG_RT gives in some cases an "Freetype: Couldn't render glyph error" and closed the Player.

The crash fix for "FontIssue.zip" included a fix for this error (well fix, it loads a fallback glyph now) in case you want to retest this :thinking:

Mimigris commented 5 months ago

@Mimigris question: You reported a while ago somewhere else that the font embedded in RPG_RT gives in some cases an "Freetype: Couldn't render glyph error" and closed the Player.

The crash fix for "FontIssue.zip" included a fix for this error (well fix, it loads a fallback glyph now) in case you want to retest this 🤔

From what I can see that seems to work as a fix for said issue (#2880), now the broken character is replaced by another, so it works as a fix to avoid the crash :+1: (technically it's not entirely accurate to the RPG_RT since it's not the same character that it is displayed between both but... we're talking about a broken font in any case, so that's not the biggest issue in this case, the font itself would need to be replaced) screenshot_0

jetrotal commented 5 months ago

Tested the soundfont settings for the first time. Triggered 2 soundfonts at the same time, don't know exactly why:

image

And would be cool to support the ".soundfont" file format with the sf2. For people, like me, who simply copied soundfont from game folder to soundfont folder.

Maybe also list ./gameFolder/easyRPG.soundfont, or lock it if the game provides a soundfont?

Ghabry commented 3 months ago

Sorry but after ignoring this for three months I lost a bit the track what was missing here :sweat:

Can you help me with telling me again which issues should be fixed before merge?

Mimigris commented 3 months ago

Sorry but after ignoring this for three months I lost a bit the track what was missing here 😓

Can you help me with telling me again which issues should be fixed before merge?

My first comment on the issue (https://github.com/EasyRPG/Player/pull/3168#issuecomment-1842975058) should still be up to date outside of what you just pushed (what is crossed is already fixed, and the other entries are normally still present)

Ghabry commented 3 months ago

The <Open (Sound)Font directory> should not be present on platforms where it is not possible to open a directory directly (e.g. Wii)

This is now added.

When Pause when focus lost is disabled, if the window of the game is put in the background, the Mouse Scroll Up/Down keys can still be used if the cursor is over the window of the game and the mouse position is still tracked

That the scrolling works when the cursor is above the window is normal. Is a feature of the operating system. This works for any application windows: Unfocus your browser window and scroll :) (Windows has this since Windows 10)

Do you consider this problematic?

Mimigris commented 3 months ago

That the scrolling works when the cursor is above the window is normal. Is a feature of the operating system. This works for any application windows: Unfocus your browser window and scroll :) (Windows has this since Windows 10)

Do you consider this problematic?

This shouldn't be problematic since you need to specifically have your mouse over the window of the game (so even if in some contexts it could be annoying, it can easily be ignored), so not really, it was more a report of what I found that could be considered as an issue.

Ghabry commented 3 months ago

Redid the font size changing: Is now shown next to the font and can be adjusted with left/right. Has the advantage that you finally get a live preview that respects the size :)

grafik

jetrotal commented 3 months ago

UI nitpick: Instead of having "Show FPS" and "Show FPS in window", we could have Show FPS: [OFF], [Overlay], [Titlebar], [Overlay + Titlebar]

keenanweaver commented 3 months ago

Super outside the scope of this PR, but could hardware MIDI support be a potential feature in the future? I have a Roland SC-55 and I think it'd be neat to use! dsda-doom uses the PortMIDI library and it works great with that.

Ghabry commented 3 months ago

@Mimigris fluidlite should work now and the error with the native midi reporting as "not working" is resolved (I hope, only tested on Linux).

I think this also fixes the "lag". Before the Native MIDI handler was launched 5 times in a row (side effect of how the settings update...). Now it is only launched once.

fdelapena commented 3 months ago

Super outside the scope of this PR, but could hardware MIDI support be a potential feature in the future? I have a Roland SC-55 and I think it'd be neat to use! dsda-doom uses the PortMIDI library and it works great with that.

@keenanweaver Native MIDI is the suitable way for hardware devices support. Instead of PortMIDI, EasyRPG Player uses its own cross platform implementation. It is supported on Linux, macOS and Windows. You likely need to make it available to operating system port then should be selectable for output.

Ghabry commented 3 months ago

Imo this is now finished.

All pending issues look like "nice to have" so they should be moved in a separate issue.

Have fun testing, hope this doesn't break too much ;)

florianessl commented 3 months ago

I'm getting a very weird crash on startup when trying to start a debug build of this branch on Windows: "Error: Couldn't create -15x-855638032 image." ('Release' seems to work fine)

Both 'width' & 'height' are pretty much corrupted garbage at this point, with negative values... image

I'm still actively working on getting some solid C++ experience but for a newbie like myself this is still a little bit too steep of an issue. But even though I can't make out the exact cause, I could isolate the source of problem to the 'font' member in the window base class. Applying this patch file fixes the crash: 0001-Remove-member-font-from-Window-base-class-revert-to-.patch

Ghabry commented 3 months ago

@florianessl unfortunately I cannot reproduce this. Did you try a clean rebuild? Does this happen with every game?

I even pointed the Player to the Windows font dir with --font-path C:\Windows\Fonts and scrolled through the list without any issues :D

screenshot_0

florianessl commented 3 months ago

@Ghabry I did some more research and found that this crash only happens, when "NewGame=1" is set... But after doing a complete Clean and Rebuilding, everything seems to work fine now. I'm sorry for the hassle!

jetrotal commented 3 months ago

@Ghabry I tried RPG Maker default fonts from https://rpgmaker.net/engines/rt2k3/utilities/1/ Would be possible to have it pixelated the same way it is in RPG_RT? they will always have the harfbuzz look?


Edit: I found a harfbuzz discussion where they discuss about a problem of font proportions related to an update on freetype that can be reversed: https://bugzilla.redhat.com/show_bug.cgi?id=1470509#c5

Ghabry commented 3 months ago

@jetrotal can you tell me font file and size and a screenshot that shows a comparison where you are unhappy with the result?

jetrotal commented 3 months ago

fonts.zip I tried both Ms Mincho and MS Gothic (Default RPG Maker fonts) in different sizes. I guess the default one is 12?

image

Ghabry commented 3 months ago

The msgothi0.ttc and msmincho.ttc from your link above look better.

jetrotal commented 3 months ago

The msgothi0.ttc and msmincho.ttc from your link above look better.

Oh, you are correct... Sorry about the non-issue I pointed then. Weird, I remember testing ttc without success before

florianessl commented 3 months ago

I should probably mention this blitting issue here, as this PR refactors some of the font handling.. (These screenshots were actually taken with this branch but the exact same issue has previously been observed on 0.8) https://github.com/EasyRPG/Player/issues/2961#issuecomment-2032839468

Ghabry commented 2 months ago

@florianessl it is not perfect yet but give it a try:

grafik

EDIT: Offset calculation for the mask is now correct

grafik


But this is now the final final contribution to this PR ;)

Ghabry commented 2 months ago

This is now ready in case somebody wants to do final testing.


The font stuff needs some Android Ui support imo but I will do this in another PR together with the LZH archive support. As you know I hate Android dev so I always do everything in one go :sweat_smile:

Ghabry commented 1 month ago

(Rebased). I give one more week for feedback. Then going to merge this.

Mimigris commented 1 month ago

Changing the volume of the MIDI currently being played in the settings while having the MIDI output set as FluidSynth or FMmidi will not work entirely, since when reloading the music it seems that the volume set is different (tested with the title screen of Wadanohara and the Great Blue Sea - I suspect that the percentage of the volume is relative compared to the percentage that was set in the options when the music started for those two, = if a music starts while your setting is at 10%, the music almost cannot be heard in the end).

Ghabry commented 1 month ago

The MIDI volume issue is fixed now. Forgot that the path where I adjusted the volume is used by all MIDI code, not just MidiOut. The volume does still not perfectly match though. Good enough imo.

Also this took me faaaar too long to figure out (2 hours -_-) but I finally solved this "pop" issue you get when only using MIDI out for BGM and a sound effect ends...

jetrotal commented 1 month ago

After reading about the stretch settings in the code, got curious about something:

image

The stretch only works horizontally? Shouldn't consider the vertical borders of the window as well?

Ghabry commented 1 month ago

This only stretches vertically because it was initially invented for Android. When you stretch in both directions it will look very interesting in Portrait orientation. :)

Ghabry commented 1 month ago

not commited yet as it depends on the other Android PR.

Had to disable the Settings Scene for Fluidsynth Soundfonts and Text Fonts on Android because the normal Player code cannot handle these URI-encoded SAF paths (because they use %2F as delimiter, not / -_-).

But here is the better workaround

grafik

Ghabry commented 1 month ago

Will merge this on Sunday when no further issues arise as this is slowly becoming a blocker for my other PRs.