[Axem]

While FreeSpace's state machine can be robust at times, there's still some weird unexpected behavior.

For example in this little project clicking the scripted mainhall button in the command briefing will go to the mainhall, but it won't stop the briefing music and the mainhall music will start and play over top of it. But I don't want the briefing music to always stop, it should continue to play if I go to the Journal (from within the Command Briefing) and back.

So this code patch is just adding an optional argument to the ad.stopMusic() function that will stop the briefing music audio stream and reset the handle when required by a scripter.

http://lazymodders.fsmods.net/files/patches/briefingmusic.patch

[niffiwan]

I have a couple of comments:

Code: [Select]

if(!ade_get_args(L, "i|b", &ah, &fade, &briefing))

I think you need "i|bb" if you want to have two optional boolean parameters?

Code: [Select]

+ if (briefing == true) {
+ audiostream_close_file(Briefing_music_handle, fade);
+ Briefing_music_handle = -1;
+ }
+ else
+ audiostream_close_file(ah, fade);
+

Stylisticly, if braces are used in part of an if-else I prefer to use them in both parts. I think it makes the code a bit easier to read, and less likely for mistakes to be introduced in future.  (but it's a nitpick really...)

Apart from that, looks good

[Axem]

1.) I originally had i|b|b there and m|m said I didn't need that extra | part so I guess I misunderstood him. I confess not actually testing this version of the patch.

2.) m|m also mentioned that, if you want to add the extra brackets I don't see the harm.

[niffiwan]

I was going to commit this, but then I had a thought (uh-oh!).  What about making this more generic, so that (at a later date) other types of music can be stopped in addition to the briefing music without having to either add a new LUA function, break the existing function, or have a more awkward argument list (like a series of booleans, one per music type)?

i.e.
make the arguments to ad.stopMusic()

audiohandle (int)
fade (optional boolean)
"music type" (optional string)

And to implement the original "stop briefing music" feature you'd set "music type" to "briefing". Other types of music to stop could be added later as required.

How does that sound?  If it's OK I'm happy to make that change if you'd prefer not to.

Also, while I was looking at the music code I found the function "void briefing_stop_music()".  I thought it would also be a good idea to modify this function at accept the fade parameter and reuse it in ad.stopMusic().  (this is something else I'm happy to do with the patch).

[pecenipicek]

While FreeSpace's state machine can be robust at times, there's still some weird unexpected behavior.

For example in this little project clicking the scripted mainhall button in the command briefing will go to the mainhall, but it won't stop the briefing music and the mainhall music will start and play over top of it. But I don't want the briefing music to always stop, it should continue to play if I go to the Journal (from within the Command Briefing) and back.

So this code patch is just adding an optional argument to the ad.stopMusic() function that will stop the briefing music audio stream and reset the handle when required by a scripter.

http://lazymodders.fsmods.net/files/patches/briefingmusic.patch

i imagine that getting the handle for currently playing music and then using ad.stopMusic on that handle in the script is not possible, hence the patch?

[Axem]

niffiwan: That sounds good. I was sort of afraid of the same thing. "Boy this is amazingly specific."

pecenipicek: Kind of. There's no real way to identify which audio stream is music or just effects (I think anyway). I was able to make a lua function that only returned the briefing music handle, but that again seemed amazingly specific.

[pecenipicek]

niffiwan: That sounds good. I was sort of afraid of the same thing. "Boy this is amazingly specific."

pecenipicek: Kind of. There's no real way to identify which audio stream is music or just effects (I think anyway). I was able to make a lua function that only returned the briefing music handle, but that again seemed amazingly specific.

i'd personally go with either that from lua or generalise it in the form of get_currently_active_music, which'd give you an array or whatever of currently playing audio handles in the music channel, rather than throwing another bool at the problem

[m!m]

It's sadly not that easy. Simply stopping a sound handle does not work correctly as the briefing code assumes that the handle is set to -1 if the music is stopped which is not possible if we just return that handle from a scripting function.

[niffiwan]

i'd personally go with either that from lua or generalise it in the form of get_currently_active_music, which'd give you an array or whatever of currently playing audio handles in the music channel, rather than throwing another bool at the problem

I can see your point... but I think Axem is correct, briefing music is started as type ASF_MENUMUSIC, which is also used for:

1) autopilot messages
2) sexp_play_sound_from_file() (maybe, depending on what's passed as the 1st param)
3) credits music
4) mainhall music
5) briefing music
6) LUA playMusic()

So you can't return all the instances of ASF_MENUMUSIC in AudioStream Audio_streams[MAX_AUDIO_STREAMS]; because you won't know which stream is the briefing music. 

(just saw m!m's post)

It's sadly not that easy. Simply stopping a sound handle does not work correctly as the briefing code assumes that the handle is set to -1 if the music is stopped which is not possible if we just return that handle from a scripting function.

Right, because audiostream_close_file() called from LUA won't alter Briefing_music_handle. You'd have to add a special case to ad.stopMusic() anyway.

Anyway, so I'll do something like this:

i.e.
make the arguments to ad.stopMusic()

audiohandle (int)
fade (optional boolean)
"music type" (optional string)

...

Also, while I was looking at the music code I found the function "void briefing_stop_music()".  I thought it would also be a good idea to modify this function at accept the fade parameter and reuse it in ad.stopMusic().  (this is something else I'm happy to do with the patch).

And since I've found them already, I might also add options to stop all these from ad.stopMusic():

1) "credits" Credits_music_handle/void credits_stop_music()
2) "main_hall" Main_hall_music_handle/void main_hall_stop_music()
3) "briefing" Briefing_music_handle/void briefing_stop_music()]

[niffiwan]

I've finished the proposed changes:
https://github.com/niffiwan/fs2open.github.com/commit/ede3aa0e44507df8a70ddc4a50dc76dfaa727975

And SVN patch attached.

Axem - could you please give this a test and see if it works as expected?  Thanks

(and here's an executable: http://www.mediafire.com/download/m1wvii14nv639l6/fs2_open_3_7_1_SSE2_briefingmusic.7z)

[attachment deleted by an evil time traveler]

[Axem]

It sounds like it works as expected.

[m!m]

Might be a good idea to skip the string comparisons in the lua function if there is no extra argument. It would also be nice to add additional validation so a typo will result in a nice error message instead of stopping an invalid sound handle.

[niffiwan]

good points - I'll make those changes & post another build.

edit: and done - here's a new executable: http://www.mediafire.com/download/k4dhcdobptc8vi4/fs2_open_3_7_1_SSE2_briefingmusic2.7z

(git branch: https://github.com/niffiwan/fs2open.github.com/commits/axem-brief-music)

I used an FSO Warning for an invalid music name as using ade_set_error requires the LUA script to handle the error. The warning should be more obvious.

Axem - could you please give it another test? Thanks

[m!m]

A LuaError would provide much better debugging information like a lua stacktrace so the modder known where the error occured instead of having to check every call of the function.

[niffiwan]

Thank you again! I've made that change, retested and pushed to the branch.

Hopefully that should be all, I'll commit it in a few days if there are no further comments.

[m!m]

Looks good to me.

[niffiwan]

comitted