[Freeciv-Dev] Re: [Patch] Sound8

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Feb 12, 2002 at 06:06:15AM -0500, Jason Short wrote:
> 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.

To be honest I didn't change these ESD/SDL specific code. I have no
real idea what is does.

> 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?

With the sound patch the client can customize this through the
soundspec file. Yes this isn't as configurable as a GUI. Or did you
mean something else?

> style: in lsend_packet_sound, why are there no braces with 
> conn_list_iterate?

Because the perl script which creates packets_lsend.c doesn't have
this. There is also a void cast missing.

> 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.

aclocal; autoheader; autoconf; automake

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

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  Windows: From the people who brought you edlin...