Add new music backend #613
Conversation
game/gamemusic.cpp
Outdated
|
|
||
| void renderSound(int16_t *out, size_t n) override { | ||
| if (!enable.load()) { | ||
| memset(out, 0, n * 4); |
game/ui/gamemenu.cpp
Outdated
| @@ -1171,6 +1171,7 @@ void GameMenu::updateValues() { | |||
|
|
|||
| set("MENU_ITEM_PLAYERGUILD","Debugger"); | |||
| set("MENU_ITEM_LEVEL","0"); | |||
| set("MENUITEM_AUDIO_PROVIDER_CHOICE", "OpenGothic (Legacy)|GothicKit (Experimental)"); | |||
| Tempest::Device& dev; | ||
| Tempest::SoundDevice sound; | ||
|
|
||
| std::recursive_mutex sync; | ||
| std::unique_ptr<Dx8::DirectMusic> dxMusic; | ||
| DmLoader* dmLoader; |
There was a problem hiding this comment.
generally raw-pointers (with ownership) are not welcome. Should be unique_ptr
There was a problem hiding this comment.
Since it's a C library, this is sort of required. The DmLoader is reference-counted under the hood so a unique_ptr would be semantically incorrect. Even if we ignored that, to make it a unique_ptr would require adding a custom de-allocation function which makes passing it around a pain. I'll do that though if you want me to :)
There was a problem hiding this comment.
Introducing a .hpp header, similarly to how it done in vulkan can be a good option.
In this hpp, you can place tiny C++ wrappers around C-api
game/resources.h
Outdated
| @@ -210,11 +214,13 @@ class Resources final { | |||
| } | |||
| }; | |||
|
|
|||
|
|
|||
game/resources.h
Outdated
| @@ -15,6 +15,8 @@ | |||
| #include "graphics/material.h" | |||
| #include "sound/soundfx.h" | |||
|
|
|||
| #include "dmusic.h" | |||
There was a problem hiding this comment.
Can DmSegment and DmSegment be forward declarated?
game/gamemusic.cpp
Outdated
|
|
||
| setEnabled(musicEnabled!=0); | ||
| impl->setVolume(musicVolume); | ||
| Log::e("provider:", providerIndex); |
game/gamemusic.cpp
Outdated
| Log::e("provider:", providerIndex); | ||
|
|
||
| if (providerIndex != provider) { | ||
| Log::i("Switching music provider"); |
There was a problem hiding this comment.
can keep provider id here (or even human-readable name)
|
|
||
| virtual void stopTheme() = 0; | ||
|
|
||
| virtual void setEnabled(bool enable) = 0; |
There was a problem hiding this comment.
virtual void setEnabled(bool enable) const = 0;
game/gamemusic.cpp
Outdated
|
|
||
| virtual bool isEnabled() = 0; | ||
|
|
||
| virtual const std::optional<zenkit::IMusicTheme> getPlayingTheme() = 0; |
There was a problem hiding this comment.
generally in project I would like to move away from getPlayingTheme kind of call to simple playingTheme
There was a problem hiding this comment.
I was thinking about MusicProvider being an interface (the whole "polymorphism bad" shtick) so I was trying to avoid adding fields to it. Sure can do though if want me to.
What do you mean by "semi-predictably"? Do you have a steps to reproduce? |
Nope, I don't. It just starts happening randomly in the codebase and then persists at the same that until I debug it enough, then it starts working again. For example, the game plays the gamestart sound bite which requires it to load a WAV from the resources. Doing that, you lock a This happened in different places too but when and when it happens is a mystery to me. |
|
Hi @lmichaelis, I'm having issues trying to compile your branch, could I do something better? Checked out lmichaelis/feat/new-music-system + This is on Fedora 40, gcc (GCC) 14.0.1 20240411 |
Heyo, looks like your compiler is trying to compile for C++14, rather than >= C++17. Have you tried resetting your CMake cache and recompiling? |
|
I've disabled Are these missing fuctions specifically C++17 additions? From my log it may be misleading but I think there's no mention of the C++ standard used, it's just by chance that the current compiler major version is 14, this GCC implements C++20 except for modules and almost all of C++23, should be new enough: |
|
Maybe it's this? https://gcc.gnu.org/gcc-14/porting_to.html:
|
|
Indeed, sticking |
|
Sorry for posting too much, one last post. Going to Valley of Mines and switching the sound provider results in this: |
Thanks! I'll fix it asap.
Could you turn on trace logging for dmusic in |
Sure, here it is: |
Thanks @Nindaleth, but I am unable to replicate the issue. Which distribution of Gothic are you using and how exactly do you trigger the crash? A short video might be helpful for illustration. |
It's the GOG's Gothic 2 Gold in English, installed from
I have MARVIN'd my way into the very beginning of Valley of Mines (it's very easy for me to hear the difference between vanilla and OG), saved there. Then I just load the game and try to switch, nothing really special that could be caught on video. I start the game like this (then run and no debuginfod) I could upload some of the Gothic's data files into a temporary online storage if that'd help? |
|
Thanks for the explanation. I still can't get this crash to happen though. I do have the GOG version but it does not yield that particular crash. I do still have issues with futexes (at least in debug and debug-release modes) which cause the kernel to kill the OpenGothic process.
Not really, but you could post the sha256sum of |
|
FYI: I've bumped ZenKit on master, so you can rebase |
Files on my install are matching your sha256sums completely. My issue was elsewhere and it was self-inflicted. Since the original issues compiling I had tried setting a newer C++ standard in OpenAL vendored lib in case it would force the rest of the codebase onto that older standard - which turned out to be bogus because it was caused by a change in behaviour of GCC 14. That was the sole source of trouble; after reverting this change and recompiling I don't have get any more crashes on switching the audio provider. There's still a few mentions of missing instrument patches in the log just like before, but with the tracing on I'm now also getting lots of
and the music plays, audibly different compared to the OG sound provider. Hooray! |
lmichaelis
force-pushed
the
feat/new-music-system
branch
from
fedc0f8 to
81312a1
Compare
|
Hello there! Finally got to code-review stuff :) What is the current status on this PR? Is it still work-in-progress or issues are non-blocking and we can proceed to merge? |
|
The issues should be non-blocking from my POV. Feel free to change things directly in this branch if you want/need to ;) |
|
While testing today run into a crash: Setup: Other issues:
|
|
Hm I've failed to reproduce the issue on Linux. I will try again on Windows (hopefully) this weekend. As for the playback issues: That's why we've got the new system as an option, don't we :) I've written them down and I'll look into them soon. |
Correct, just need to sort out crash issue, rest is FYI :) |
Debug crash today a bit. Issue here is a call to // My quick fix
diff --git a/src/Synth.c b/src/Synth.c
index a1bde22..8905fc8 100644
--- a/src/Synth.c
+++ b/src/Synth.c
@@ -86,7 +87,19 @@ static DmResult DmSynth_updateFonts(DmSynth* slf, DmBand* band) {
tsf_set_output(new_fnt.syn, TSF_STEREO_INTERLEAVED, (int) slf->rate, 0);
tsf_set_volume(new_fnt.syn, slf->volume);
- rv = DmSynthFontArray_add(&slf->fonts, new_fnt);
+ if (slf->channels_len>0) {
+ DmSynthFont* prev = slf->fonts.data;
+ rv = DmSynthFontArray_add(&slf->fonts, new_fnt);
+ if (rv != DmResult_SUCCESS)
+ return rv;
+ ptrdiff_t offset = (slf->fonts.data - prev);
+ for(size_t r=0; r<slf->channels_len; ++r) {
+ slf->channels[r].font += offset; // HACK with pointer offsets
+ }
+ Dm_report(DmLogLevel_INFO, "font = %p", slf->fonts);
+ } else {
+ rv = DmSynthFontArray_add(&slf->fonts, new_fnt);
+ }
if (rv != DmResult_SUCCESS) {
return rv;
} |
Found by @Try in Try/OpenGothic#613. Thanks for finding the fix!
|
Good catch! I've added the fix 👍 |
|
Seem to work well now - merged! |
This PR adds a new, experimental music backend recently created at GothicKit/dmusic. It can be enabled by changing the "Soundprovider" option in the sound settings to "GothicKit (Experimental)".
Note @Try: I am having really weird multithreading issues on Fedora. It's likely they are not caused by OpenGothic (since
std::recursive_mutexshould be fine) but because of them I am having a hard time properly testing everything. It should be mostly okay now though. (specifically: the Kernel keeps semi-predictably sending SIGKILL to the process making the cause impossible to easily debug)