[Freeciv-Dev] Re: Sound patches to be added (diffs'll come later)

[David Weisgerber <tnt@xxxxxxxxx>]

Am Samstag, 18. August 2001 10:25 schrieben Sie:
> David Weisgerber wrote:
> > Here are the newest and finest goodies from me. They now support parsing
> > a soundspec file located in /usr/local/share/freeciv/ but have a look at
> > the sources yourself I think you'll understand how the whole thing works.
> > There is also a pretty option for modpackers so they don't have to worry
> > I think.
> >
> > With this sound patch there is a soundpack from me included. Note that it
> > isn't really finished yet.
>
> Some notes:
>
> In sound.h, the unit sounds should be a separate enumeration from the
> others.

OK, fixed
>
> When you "copy" the sound config file, you need to check for errors.
> Similarly, your sprintf's should be snprintf's (or
> mysnprintf/fc_snprintf?).

OK, fixed

>
> I don't know how paths work in FreeCiv, but it seems inappropriate to
> hardcode /usr/local/share/freeciv/.  Can't you just use the freeciv
> "data" directory?

I'll fix this later...

> Your function braces aren't standard FreeCiv-styple.  The opening brace
> should be on its own line.  Some of your other braces are off, too.  The
> "if (a) if (b) {" line is quite odd.

Sorry for that, fixed...

> What is the 52 magic number in sound.c?  I can't even figure out what
> that's used for.

Sorry for that, it's now fixed.

> The "global" variables declared in sound.c should be declared static.

fixed.

> I'm also not quite sure what strcoll() is supposed to be doing.  Could
> you add a comment on that one?

The strcoll() function compares the two strings s1 and s2.
       It returns an integer less than, equal to, or greater than
       zero  if  s1  is  found, respectively, to be less than, to
       match, or be greater than s2.  The comparison is based  on
       strings  interpreted as appropriate for the program's cur­
       rent locale for category LC_COLLATE.  (See  setlocale(3)).

I think it's better but don't know...

> Rather than checking if the config file has been parsed each time a
> sound is played, why not have parse_sound_config be called upon game
> initialization?

This has some reasons. First reason is that I can change soundspec files 
during game by calling set_sound_config_file and the other one is that maybe 
modpackers will call set_sound_config_file later but I think this could be 
deimplemented soon...

> Why is there a printf in parse_sound_config?  I assume you left this in
> by accident?

sure, now it's a freelog.

> You should probably check the return value of the system() call that
> plays the sound.

and what sould I do with the return value ??? That doesn't bring something as 
you can see from the discussion. Either play doesn't fail than there is a 
sound play or play fails and it is like it was until this patch came.

> Naming all of the units shouldn't be done in the sound file.  If you
> want to do it outside of the sources, it should ultimately be put into
> some other spec file.  In fact, a lot of the options you've put into the
> sound spec file are inappropriate - for instance, defining the play
> command in this file means anyone using an incompatible platform must
> use a different sound spec file!

No, I don't think so. First point: Naming the units in the sound file is good 
for mod makers.
Second point: Maybe you define a specfile which doesn't play wave. Instead it 
plays MP3s or AC3s than you have to use another program.

> Listing all of the unused sounds in the spec file as '<soundname> = ""'
> is IMO ugly and inelegant.  Can't you just assume there's no sound if
> the sound isn't specified?

Maybe I'll fix this later...

Much thanks for help!!!

CU,
David

-- 
Kunde: "Ich  benutze Windows 98!"
Techniker: "Ja."
Kunde: "Mein Rechner funktioniert nicht."
Techniker: "Das sagten Sie  bereits!"