[Freeciv-Dev] Re: [Patch] Sound8

[Jason Short <vze2zq63@xxxxxxxxxxx>]

Raimar Falke wrote:
> I have spent the previous days working on the sound patch. The newest
> version 8 is now in the incoming (too large for the list). Changes
> since version 7 include:
>  - reworked audio* to allow a real plugin mechanism without special
>  cases and #ifdefs (this also allows to change the plugin at runtime
>  just one call to audio_select_plugin())
>  - extend README.sound
>  - add sounds for all improvements (formerly only wonders)
>  - add alternative fight and move tags
>  - rename the event tags to be consistent (at least a bit)
>  - add a mechanism to map "normal" events to sound events
>  - sound is now always be enabled
>  - a lot of other small changes
>
> TODO: 
>  - adjust civ2sounds.spec to use the new names (I don't have a civ2 cd
>  handy for this)
>
> Please test and review.
In audio_esd.c: I am confused.  Why do we need to have a limited number 
of samples?  Is this just to make shutdown() easier?  Once a sample is 
started, what happens when it ends?  It looks like the sample_id value 
just sits around until the loop comes back, and then it will be manually 
"killed" (esd_sample_free), even though it has most likely already 
finished playing.  Finally, the circular buffer used for tracking sample 
numbers will only work correctly if all samples are of the same length; 
if there was one very long sample a number of shorter ones could cycle 
through while it was playing...and later we could end up killing it even 
if it was the only sample currently playing.
In audio_sdl.c: here there is a similar situation.  SDL tells us to use 
no more than MIX_CHANNELS different channels, so here it may be 
necessary to do something clever.  But it looks like things are a bit 
broken - music is supposed to go on its own channel, but this isn't 
enforced so it looks like a regular sound could overwrite it.
Neither of these are showstopper problems - with the clean interface the 
module system has, the code is very localized and easy to fix later. 
And it's unlikely that there would be problems during normal play.  I do 
think some sort of FIXME comment should be put in, though.

Calling the sound modules "plugins" is a bit vague.  I can understand 
that -P is one of the few command-line switches still available, but 
what about "--soundPlugin" instead of "--Plugin" as the full switch? 
Hmm, but I guess maybe get_option() won't work that way - so we may be 
stuck with it unless someone can think of something better (which is 
unlikely).

I see code like:

-  if(sound_bell_at_new_turn &&
-     (!game.player_ptr->ai.control || ai_manual_turn_done))
-    sound_bell();
+  if (sound_bell_at_new_turn &&
+      (!game.player_ptr->ai.control || ai_manual_turn_done)) {
+    audio_play_sound("e_turn_bell", NULL);

which is a natural progression.  As a side note, I wonder if it would 
eventually be worthwhile to have client-side options for all different 
events (that can have sounds associated with them)?  This would give the 
client a lot more control.  Are there any changes that could make this 
conversion easier later?
style: in lsend_packet_sound, why are there no braces with 
conn_list_iterate?
Finally: I cannot build FreeCiv with the patch.  It may or may not have 
something to do with the fact that I need a 'cvs up -D "Feb 1"' to apply 
it in the first place.

Aside from the build problems, everything looks acceptable.  Of course 
it is a lot of code...
jason