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

HL:Invasion Add MiniAudio Music implementation #273

Merged
merged 4 commits into from
Jul 18, 2022

Conversation

RoyShapiro
Copy link

Hello again!

I've added Miniaudio music implementation to Invasion mod, as promised.
It should now be much more portable.

The CMakeList.txt still uses null music player by default, so the music implementation is selectable at compile time.
I'm thinking of dropping both the GStreamer ("bloat" (c) @nekonomicon ) and the original FMod (proprietary and incredibly outdated) implementations.

What do you think?

@nekonomicon
Copy link
Member

I don't see any reason to use FMod or GStreamer anyway.
May be will be good to use miniaudio by default.

@a1batross
Copy link
Member

a1batross commented Jul 11, 2022 via email

@nekonomicon
Copy link
Member

Unfortunately, original invasion uses fmod on server side.
I think it must be disabled for multiplayer.

@RoyShapiro
Copy link
Author

@nekonomicon
Can do. I'd still leave the "null" option included for systems that may not support miniaudio (VGUI still remains a portability issue).
Invasion is a purely singleplayer mod, if you don't count the photoshopped-out button in btns_main.bmp :D

@a1batross
This particular code is non-generic, it comes with an Invasion-specific implementation of "trigger_music" that uses playlists. I'm not aware if this was ever used word for word in any other mods. That said, it seems like Poke646 and Vendetta, off the top of my head, also use server-side FMod implementation. So, this code may be useful there as well, but will likely need adaptation. I don't know the details, though.

@nekonomicon
Copy link
Member

Poke646 reads playlist and uses FMod on client side.

@RoyShapiro
Copy link
Author

@nekonomicon
Okay, good to know.

I've removed gstreamer / fmod / null implementations and options in a new commit, RoyShapiro@418ae2d.
I've also added the option -DDISABLE_MINIAUDIO instead to disable the current one if necessary. It uses defines within music.h and music.cpp themselves to disable miniaudio code, this is much neater.

I've also tried to find a way to disable this in multiplayer,
see music.cpp in commit RoyShapiro@418ae2d lines 413 and 441 respectively (I may be doing something wrong there, but I saw other triggers using the same code for that purpose), but when I tried to test that, by forcing the game into multiplayer, I got Error: No spawn function for trigger_music and it didn't spawn anyway. IDK why, it most definitely has a spawn function and work fine in singleplayer. Weird O.o.

@nekonomicon
Copy link
Member

Try to use UTIL_Remove.
It will prevent spawn.

@RoyShapiro
Copy link
Author

RoyShapiro commented Jul 12, 2022

@nekonomicon
Okay, it's probably going to be a really stupid question, but where would I use UTIL_Remove? AFAIK the first method called would be "Spawn", but in multiplayer, it says that it doesn't have spawn, even though it does. O.o

Anyway, I tried moving the music player code to the client instead as suggested, and was successful.
Haven't pushed anything, yet. Should I? Or will we leave it on the server?

The code turned out to be a little less... Compact.
There's a small class in hud.h \ hud.cpp now, that reads messages coming from server.
Also, there is an int variable on the server side, that's used for REG_USER_MSG.
Most of those are defined in player.cpp. I temporarily moved it to my own music.cpp.

I guess it stands to reason to move it to player.cpp where the rest of them reside, does it?

@a1batross
Copy link
Member

There is multiplayer part of Invasion? I never played that mod to be honest. :)

@RoyShapiro
Copy link
Author

@a1batross
No, there is not :D
But since it feels like we want this code to be more generic and applicable to other mods, potentially, before merging, I figured I had to test it that way. So I kinda forced it upon the mod by temporarily editing gameinfo.txt and liblist.gam files.

@a1batross
Copy link
Member

Under generic support, I've meant #270

@RoyShapiro
Copy link
Author

@a1batross Oh, I see, so @FreeSlave wanted to tackle the issue as well. I didn't know that, so my attempts to kinda do the same are unrelated and I wasn't initially planning to do a generic implementation. I just wanted to "debloat" my previous code mess :D

Anyway, Invasion has some peculiarities, like the playlist format.

I've just pushed the client side version here, 3e62e6b
it's still pretty Invasion-specific, but the same idea, sans the playlist format, can be relatively easily applicable elsewhere.

@nekonomicon
Copy link
Member

LGTM.
We can try to fix windows build later.

@nekonomicon nekonomicon merged commit 710fb2b into FWGS:invasion Jul 18, 2022
@RoyShapiro
Copy link
Author

@nekonomicon Thanks. According to logs it gives no errors on Windows anymore, but gets "cancelled" due to unspecified reasons.

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.

3 participants