Complete.Org: Mailing Lists: Archives: freeciv-dev: January 2002:
[Freeciv-Dev] Re: sound patch v5
Home

[Freeciv-Dev] Re: sound patch v5

[Top] [All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
To: "Per I. Mathisen" <Per.Inge.Mathisen@xxxxxxxxxxx>
Cc: freeciv-dev@xxxxxxxxxxx
Subject: [Freeciv-Dev] Re: sound patch v5
From: Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>
Date: Sun, 27 Jan 2002 13:04:47 +0100
Reply-to: rf13@xxxxxxxxxxxxxxxxxxxxxx

[ all about sound12.patch ]

On Mon, Jan 14, 2002 at 06:48:08PM +0100, Per I. Mathisen wrote:
> On Mon, 14 Jan 2002, Per I. Mathisen wrote:
> > After patching and before doing a "./configure", do this:
> >
> >     # autoheader; autoconf; automake
> >
> > Otherwise it won't work.
> 
> I forgot one. You should do this
> 
>       # aclocal; autoheader; autoconf; automake

configure:

...
> checking for IMLIB - version >= 1.9.2... yes
> checking for esd-config... /usr/bin/esd-config
> checking for ESD - version >= 0.0.20... yes
> checking building ESOUND support... YES
> checking target system type... i686-pc-linux-gnu
> checking for sdl-config... /usr/bin/sdl-config
> checking for SDL - version >= 1.2.0... 
> *** 'sdl-config --version' returned 1.1.8, but the minimum version
> *** 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.

> checking for SDL/SDL_mixer.h... yes
> checking for Mix_OpenAudio in -lSDL_mixer... yes

"./configure --enable-sound=esd" should work as expected.

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'

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

Patch itself:

> +To add support for a new system, change these files (where "whatever" 
> +is sound system):
> +     configure.in                    // add new test
> +     acconfig.h                      // add new config metavariable
> +     client/audio.c                  // link in new system
> +     client/Makefile.am              // add the files below
> +     client/audio_whatever.c         // audio plugin
> +     client/audio_whatever.h         // audio plugin's header

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

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

> +  /* HP/UX 9 (%@#!) writes to sscanf strings */
> +  tmp_version = my_strdup("$min_sdl_version");

> +/* 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?

> +/**************************************************************************
> +...
> +**************************************************************************/
> +static inline void audio_check_capabilities(struct section_file *file, const 
          ^^^^^^

You don't want to start introducing inline here.

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

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

> +     #ifdef ESD
> +     if (audio_plugin == 1) {
> +             if (format)
> +                     music_play_esd(fullpath);
> +             else
> +                     audio_play_esd(fullpath);
> +             return 1;
> +     }
> +     #endif
> +     #ifdef SDL
> +     if (audio_plugin == 2) {
> +             if (format)
> +                     music_play_sdl(fullpath);
> +             else
> +                     audio_play_sdl(fullpath);
> +             return 1;
> +     }
> +     #endif

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

> +  assert(0);
> +  return 0; /* we should never get here */

NDEBUG will bring us there. You may want to use "exit(1)".

> +void audio_play(char *tag, char *alt_tag)
> +{
> +  assert(tag);
> +
> +  /* check if we are supposed to be silent */
> +  if (audio_mute||audio_disable) return;
> +
> +  /* try playing primary tag first, if not go to alternative tag */
> +  if (!audio_play_tag(tag,0))
> +    if (!audio_play_tag(alt_tag,0))

> +      freelog(LOG_VERBOSE,"Neither of tags %s and %s found",tag,alt_tag);

Isn't this error a bit "harder"?

> +}

> +#ifndef FC__AUDIO_H
> +#define FC__AUDIO_H
> +
> +#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". You may also want to eliminate these
globals and provide functions instead: audio_mute(), audio_unmute(),...

> +#include "esd.h"

Should be <esd.h>

> +int sample_id[MAX_SAMPLES];
> +int music_id=-1;
> +int last_sample=0;
> +int sock = -1;

Static?

> +#include "SDL.h"
> +#include "SDL_mixer.h"

Should be in <>.

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

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

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

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

> diff -u2NrX freeciv-sound/diff_ignore freeciv/diff_ignore 
> freeciv-sound/diff_ignore
> --- freeciv/diff_ignore       Mon Jan 14 18:07:47 2002
> +++ freeciv-sound/diff_ignore Mon Jan 14 16:22:34 2002
> @@ -1,3 +1,4 @@
>  #*#
> +*.wav
>  *.a
>  *.bak
> @@ -9,4 +10,5 @@
>  *.rej
>  *.swp
> +*.wav
>  *~
>  .#*

Why two times?

> +  assert(sound&&*sound);

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

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

Actual test: not possible because of compile problems.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "The BeOS takes the best features from the major operating systems. 
  It's got the power and flexibility of Unix, the interface and ease 
  of use of the MacOS, and Minesweeper from Windows."


[Prev in Thread] Current Thread [Next in Thread]