RFC: Reworking sound sub-system

[calris]

You have to have configured with --enable-test, then

./run-tests

for birth tests,

OK - so how do I figure out why the tests failed. And did they fail before my work on the sound modules (not sure how my work could impact in birth)

[calris]

And what are the opinions regarding #include in .h files?

The Windows build error is due to win-layout.c needing ui-keymap.h which needs ang_file which is defined in z-file.h

ui-keymap.h does not have any #includes, but I notice others, like ui-prefs.h, do.

I like to include any necessary headers in headers that need them, but I've worked with people that think this is a bad idea (I think mainly for dependency tracking).

My argument (and the argument of a lot of developers) is that if you add functionality to some code that needs definitions in foo.h then you should only ever need to #include fooh. - foo.h will #include everything foo.h needs in order to compile.

[calris]

Windows only has one missing #include, by the looks:

In file included from ./ui-prefs.h:25:0,
                 from win/win-layout.c:21:
./ui-keymap.h:60:18: error: unknown type name �ang_file’
 void keymap_dump(ang_file *fff);
                  ^
Makefile.win:134: recipe for target 'win/win-layout.o' failed

Added #include "z-file.h" to ui-keymap.h to resolve this

Also fixed an oversight with the new handling of supported file type hook

Pushed to sound-devel

I think I'm getting closer to the finish line - When we are all happy, I will trash sound-devel and develop a better set of patches that apply the changes incrementally, somthing like:
- Add sound-core
- Migrate Linux sound
- Migrate Windows sound

[Nick]

OK - so how do I figure out why the tests failed. And did they fail before my work on the sound modules (not sure how my work could impact in birth)

I've been wondering the same thing recently - unit tests are in src/tests/bin/whatever and can be run with gdb, but I'm not sure how to debug an individual birth test. Maybe someone else knows.

[calris]

I've been wondering the same thing recently - unit tests are in src/tests/bin/whatever and can be run with gdb, but I'm not sure how to debug an individual birth test. Maybe someone else knows.

OK, looks like the birth test creates a new character, and tests that, at the end of the test, it is Level 1 with the same race and class specified by the test. Really not sure how it could fail TBH

[calris]

Nick, I've created a new branch for the sound sub system here

Let me know if it's to your liking and I'll generate a pull request

EDIT: Damb - just noticed there is no commit messages - I'll fix that and push them again (EDIT: Done)

[Nick]

Nick, I've created a new branch for the sound sub system here

Let me know if it's to your liking and I'll generate a pull request

EDIT: Damb - just noticed there is no commit messages - I'll fix that and push them again (EDIT: Done)

That's still failing on not declaring parse_prefs_dummy for me; if I add

#include "ui-prefs.h"

to sounds.h, then it fails on declaring register_sound_pref_parser twice.

It's probably related to what switches are set for ./configure; it needs to work with all combinations.

[calris]

That's still failing on not declaring parse_prefs_dummy for me; if I add

#include "ui-prefs.h"

to sounds.h, then it fails on declaring register_sound_pref_parser twice.

It's probably related to what switches are set for ./configure; it needs to work with all combinations.

Is that Windows build? I don't have any build tools for anything but Linux, so it's a bit hard for me to test non-Linux builds

[Nick]

Is that Windows build? I don't have any build tools for anything but Linux, so it's a bit hard for me to test non-Linux builds

No, that's linux build, with (IIRC) --enable-sdl --enable-sdl-mixer --enable-stats --enable-test.

[calris]

No, that's linux build, with (IIRC) --enable-sdl --enable-sdl-mixer --enable-stats --enable-test.

Oops - I'll check it out.

Apart from build failures, are you happy with the structure of the new sound module and the coding style?

[Nick]

Apart from build failures, are you happy with the structure of the new sound module and the coding style?

Yeah, absolutely

[calris]

Hmmm, I just did the following for each patch in order:

# make distclean
# ./autogen.sh
# ./configure --enable-sdl --enable-sdl-mixer --enable-stats --enable-test
# make

And every one compiles clean

[calris]

Ah fudgicles...

Looks like my latest system update (to bring in sqlite to enable compiling with stats) has borked SDL

Can't get sound to work on any of my (previously working) dev branches. Quiting hangs Angband and Ctrl-C causes a segfault somewhere in the SDL library.

EDIT: SDL Graphics still works

EDIT 2: A reboot solved it

[calris]

That's still failing on not declaring parse_prefs_dummy for me; if I add

#include "ui-prefs.h"

to sounds.h, then it fails on declaring register_sound_pref_parser twice.

It's probably related to what switches are set for ./configure; it needs to work with all combinations.

register_sound_pref_parser() is only defined in sound.h if SOUND is NOT defined
register_sound_pref_parser() is defined in sound-core.c, but sound-core.c should only compile if SOUND IS defined

So somehow you managed to get SOUND to be both DEFINED and UNDEFINED.

Are you compiling on a Quantum computer?

[calris]

The plot thickens...

I just switched to the Rune ID branch and cherry picked my sound code into it (applies cleanly). I did a 'make clean', not a 'make distclean' before ./configure and got this when I went to do the make:

In file included from snd-sdl.c:23:0:
sound.h: In function ‘register_sound_pref_parser’:
sound.h:71:41: error: ‘parse_prefs_dummy’ undeclared (first use in this function)
  return parser_reg(p, SOUND_PRF_FORMAT, parse_prefs_dummy);
                                         ^
sound.h:71:41: note: each undeclared identifier is reported only once for each function it appears in
sound.h:72:1: warning: control reaches end of non-void function [-Wreturn-type]
 }
 ^
Failed to compile snd-sdl.c!

If I do a 'make distclean' it compiles clean and runs with sound as expected