[Freeciv-Dev] Re: sound patch v5

["Per I. Mathisen" <Per.Inge.Mathisen@xxxxxxxxxxx>]

On Sun, 27 Jan 2002, Raimar Falke wrote:
> configure:
> > checking for SDL - version >= 1.2.0...
> > *** 'sdl-config --version' returned 1.1.8, but the minimum version

I never actually got around to check if older versions work too. I'll do
that.

> > *** of SDL required is 1.2.0. If sdl-config is correct, then it is
> > *** best to upgrade to the required version.
> > *** If sdl-config was wrong, set the environment variable SDL_CONFIG
> > *** to point to the correct copy of sdl-config, and remove the file
> > *** config.cache before re-running configure
> > no
>
> > ./configure: SDL: command not found
>
> This shouldn't happen.

What happened? What shouldn't happen? Did configure abort?

What is the context of "SDL: command not found"?

> > checking for SDL/SDL_mixer.h... yes
> > checking for Mix_OpenAudio in -lSDL_mixer... yes
>
> "./configure --enable-sound=esd" should work as expected.

Right. Disable the other sound systems when one chosen explicitly.

> Compilation:
> gcc -DHAVE_CONFIG_H -I. -I. -I.. -I./include -I../common -I../intl     -Wall 
> -Wpointer-arith -Wcast-align -Wmissing-prototypes -Wmissing-declarations 
> -Werror  -g -O2 -Wall -c audio_esd.c
> cc1: warnings being treated as errors
> audio_esd.c:48: warning: no previous prototype for `audio_init_esd'
> audio_esd.c:67: warning: no previous prototype for `audio_play_esd'
> audio_esd.c:97: warning: no previous prototype for `music_play_esd'
> audio_esd.c:120: warning: no previous prototype for `music_stop_esd'
> audio_esd.c:129: warning: no previous prototype for `audio_done_esd'

Will fix.

> gcc -DHAVE_CONFIG_H -I. -I. -I.. -I./include -I../common -I../intl       -g 
> -O2 -Wall -c audio_sdl.c
> audio_sdl.c:27:17: SDL.h: Datei oder Verzeichnis nicht gefunden
> audio_sdl.c:28:23: SDL_mixer.h: Datei oder Verzeichnis nicht gefunden

That is strange. Above I got the impression that SDL was _not_ configured
in. Then audio_sdl.c should not be compiled, either. To get it to compile,
comment out "#define SDL" in config.h and recompile.

> Maybe change these "//" to not teach people bad manners.

Ok :)

> > +char*
> > +my_strdup (char *str)
> > +{
> > +  char *new_str;
> > +
> > +  if (str)
> > +    {
> > +      new_str = (char *)malloc ((strlen (str) + 1) * sizeof(char));
> > +      strcpy (new_str, str);
> > +    }
> > +  else
> > +    new_str = NULL;
> > +
> > +  return new_str;
> > +}
>
> Auaaaa a separate strdup just for a test program.

It is part of SDL's configure macro. It doesn't become part of freeciv's
codebase. I didn't write it. I shouldn't change it just for stylistic/
aestethic reasons - it was written that way for a reason by people who
knew what they were doing. Or so I presume.

> > +/* keep it open throughout */
> > +struct section_file tagstruct, *tagfile = &tagstruct;
> > +
> > +/* dynamic plug-in control */
> > +/* esd=1, sdl=2 */
> > +int audio_plugin = -1;
>
> Shouldn't these be statics?
...
> > +int sample_id[MAX_SAMPLES];
> > +int music_id=-1;
> > +int last_sample=0;
> > +int sock = -1;
>
> Static?

Is there any point in declaring globals as static? Aren't they always?

> You don't want to start introducing inline here.

Ok...

> > +char *audio_describe_plugin(int plugin)
> > +{
> > +  /* don't show anything if sound is disabled */
>
> > +  if (audio_disable)
> > +    return NULL;
>
> Here and also in the rest of the code: please add {} around these. Use
>   grep -Irn if FILES |grep -v '{'|grep -v '#'
>   grep -Irn else FILES |grep -v '{'|grep -v '#'
>   grep -Irn for FILES |grep -v '{'|grep -v '#'
> to catch them.

Ok.

> > +  assert(plugin>0 && plugin<=2);
>                   ^ blanks missing. indent will take care of this.

Ok.

> I'm not sure but AFAIK the '#' has to be in the first column to be valid.

I don't think so, but consistency is good, I suppose.

> > +  assert(0);
> > +  return 0; /* we should never get here */
>
> NDEBUG will bring us there. You may want to use "exit(1)".

The comment is "should never", not "can never". Why exit(1) when the
program continues to be in a sane state despite the bug?

> > +      freelog(LOG_VERBOSE,"Neither of tags %s and %s found",tag,alt_tag);
>
> Isn't this error a bit "harder"?

No. It happens all the time, since most hooks are commented out and
unused in the spec-file (due to lack of sound files).

> > +#define MAX_NUM_PLUGINS 2
> > +
>
> > +int audio_mute; /* set to 1 to silence the sound system */
> > +int audio_disable; /* set to 1 to disable sound on startup */
>
> Should be prefixed with "extern".

Ok.

> You may also want to eliminate these
> globals and provide functions instead: audio_mute(), audio_unmute(),...

Ok.

> > +#include "esd.h"
>
> Should be <esd.h>

Yes.

> > +#include "SDL.h"
> > +#include "SDL_mixer.h"
>
> Should be in <>.

Yes.

> > @@ -131,7 +133,10 @@
> >      fprintf(stderr, _("  -p, --port PORT\tConnect to server port PORT\n"));
> >      fprintf(stderr, _("  -s, --server HOST\tConnect to the server at 
> > HOST\n"));
> > +    fprintf(stderr, _("  -a, --audio FILE\tRead audio tags from FILE\n"));
>
> -a is already used. Also -s[ound] is used already. Also -n[oise].

What do you suggest, then? I can't say I like the current getopt regime.

> > @@ -240,4 +241,7 @@
> >      if (show_combat) {
> >        int hp0 = packet->attacker_hp, hp1 = packet->defender_hp;
> > +      audio_play(unit_type(punit0)->sound_fight,"f_generic");
>
> > +      myusleep(100);
>
> Why?

Two sounds are played on top of each other with a 1/10 second delay
between them, so that the sound of the attack gets a slight head start
(especially for the case where the sound is the same for attacker and
defender). I'm not sure this has much effect - I'll test it more.

> > @@ -719,5 +727,5 @@
> >  {
> >    int where = MW_OUTPUT;   /* where to display the message */
> > -
> > +
> >    if (packet->event >= E_LAST)  {
> >      /* Server may have added a new event; leave as MW_OUTPUT */
> > @@ -770,5 +779,5 @@
> >  void handle_move_unit(struct packet_move_unit *packet)
> >  {
> > -  /* should there be something here? - mjd */
> > +  assert(!packet); /* this packet should never get sent to a client */
> >  }
> >
>
> Not sound related.

Right. But the latter change should still go in - it took me some time to
figure that one out, and it is a documentation bug.

> > @@ -3005,4 +3008,6 @@
> >    cptr=put_string(cptr, packet->graphic_str);
> >    cptr=put_string(cptr, packet->graphic_alt);
> > +  cptr=put_string(cptr, packet->sound_move);
> > +  cptr=put_string(cptr, packet->sound_fight);
> >    if(unit_type_flag(packet->id, F_PARATROOPERS)) {
> >      cptr=put_uint16(cptr, packet->paratroopers_range);
> > @@ -3059,4 +3064,6 @@
> >    iget_string(&iter, packet->graphic_str, sizeof(packet->graphic_str));
> >    iget_string(&iter, packet->graphic_alt, sizeof(packet->graphic_alt));
> > +  iget_string(&iter, packet->sound_move, sizeof(packet->sound_move));
> > +  iget_string(&iter, packet->sound_fight, sizeof(packet->sound_fight));
> >    if(packet->flags & (1L<<F_PARATROOPERS)) {
> >      iget_uint16(&iter, &packet->paratroopers_range);
>
> Use has_capability.

I had hoped to avoid using capabilities. Got to add that all over the
place if this is to be of any use.

But... ok.

[diff_ignore:]
> > +*.wav
...
> > +*.wav
...
> Why two times?

Because it is so important.

Really.

Ok, I'll fix it ;)

> > +  assert(sound&&*sound);
>
> You use this idiom in some places. What about
> "assert(sound && strlen(sound) > 0);"?

Isn't that saying the same in lot more words (and an additional function
call)?

> > @@ -1811,5 +1899,5 @@
> >
> >    if(!(srvarg.metaserver_no_send)) {
> > -    freelog(LOG_NORMAL, _("Sending info to metaserver [%s]"),
> > +    freelog(LOG_VERBOSE, _("Sending info to metaserver [%s]"),
> >         meta_addr_port());
> >      server_open_udp(); /* open socket for meta server */
> > @@ -1981,4 +2069,6 @@
> >      sniff_packets();
> >    }
> > +
> > +  server_close_udp();
> >
> >    close_connections_and_socket();
>
> Not sound related.

No idea how that crept into the patch. Sorry.

Thanks for the review.

Yours,
Per

"What we anticipate seldom occurs: but what we least expect generally
happens." -- Benjamin Disraeli