RFC: Reworking sound sub-system

Collapse
X
 
  • Time
  • Show
Clear All
new posts
  • calris
    Adept
    • Mar 2016
    • 194

    RFC: Reworking sound sub-system

    Now that my character is dead, I can get back to coding

    I'm not sure if anyone is the designated 'maintainer' of the messaging sub-systems, so I thought I should run this be the dev team to make sure I'm not treading on any toes...

    I've been looking at the interaction between the 'sound' and 'message' code and I think I can see a way to clean up the code somewhat - in particular the use of #include "list-message.h" into a static array no less than three times in message.c

    I think there is some scope to make 'sound' a much cleaner hook into the 'message' sub-system

    Let's look first at the message queue:

    Code:
    typedef struct _msgqueue_t
    {
    	message_t *head;
    	message_t *tail;
    	msgcolor_t *colors;
    	u32b count;
    	u32b max;
    } msgqueue_t;
    We have linked list of message texts and linked list of message colours...

    I propose:

    1. Moving the colours out of here and create a new array (of size MSG_MAX) of structures that contain the colour and sound definitions
    2. Reformat sound.cfg to be like messages.prf - something like:
    Code:
    sound:DISSARM:plm_bang_ceramic plm_chest_latch plm_click_switch3
    3. Ditch the second column in list-message.h (it's only used to map the names used in sound.cfg anyway)
    4. Get rid of message_lookup_by_sound_name() and message_sound_name() (no longer needed as they are used to map the current sound.cfg names to MSG_ numbers)
    5. Replace calls to sound() with calls to msgt(), so
    Code:
    sound(MSG_MULTIPLY);
    becomes
    Code:
    msgt(MSG_MULTIPLY, "");
    6. Convert many (if not all) calls to msg() to msgt() - Take, for example:
    Code:
    msg("You feel stronger!");
    If we want to assign a sound to this one day, we'll need to either convert it then, or add the horribly hacky call to sound(). Why not just have
    Code:
    msgt(MSG_STAT_GAIN, "You feel stronger!");
    Now to the discussions about mp3, ogg, wav, etc...

    Notice that I removed the file extension from sound.cfg (I think we can safely assume a rename to sound.cfg is in order btw). We can leave it up to each individual platform to support whatever format they can (possibly even multiple formats if we like).

    This brings me to the final part of the cleanup (and I've mentioned this in a previous post) - moving the processing of sound.prf into a single location with hooks that each platform sets for the initialisation, loading, playing, and unloading of sounds

    EDIT: We could just move the sound file names into message.prf

    Code:
    message:<message-type>:<color>:<sound file> <sound file> ... <sound file>
  • Nick
    Vanilla maintainer
    • Apr 2007
    • 9634

    #2
    I've done very little with the sound system, but what you're suggesting sounds sensible.

    One thing I have thought is that the text (with formatting where necessary) of every game message should be in a text file. I don't think that really affects what you're doing, but maybe...

    ...actually, maybe it does. I don't think you want sound file names in a message text file, but you could have messages indexed by labels, like
    Code:
    label:MSG_STAT_GAIN
    text:You feel %s
    color:r
    or whatever.

    Just thinking aloud, really, but keep the messages->text file idea in mind.
    One for the Dark Lord on his dark throne
    In the Land of Mordor where the Shadows lie.

    Comment

    • calris
      Adept
      • Mar 2016
      • 194

      #3
      Good thinking 99

      I'll work on the message/sound hooks first, then look at extracting message text into a pref file

      Comment

      • takkaria
        Veteran
        • Apr 2007
        • 1951

        #4
        Originally posted by Nick
        I've done very little with the sound system, but what you're suggesting sounds sensible.

        One thing I have thought is that the text (with formatting where necessary) of every game message should be in a text file. I don't think that really affects what you're doing, but maybe...

        ...actually, maybe it does. I don't think you want sound file names in a message text file, but you could have messages indexed by labels, like
        Code:
        label:MSG_STAT_GAIN
        text:You feel %s
        color:r
        or whatever.

        Just thinking aloud, really, but keep the messages->text file idea in mind.
        I think it's a bad idea to use format strings like this in external files because they're really brittle. I'd suggest something like "You feel {adjective}" instead, or something like gettext using $1 to mean 'first argument' etc.
        takkaria whispers something about options. -more-

        Comment

        • takkaria
          Veteran
          • Apr 2007
          • 1951

          #5
          Hey, I'm familiar with the code and I think your plans are great and it's been needing doing for some time, especially standardising the file format and stopping every single different port reimplementing it. Here are the two comments I had that weren't "hurrah!":

          5. Replace calls to sound() with calls to msgt(), so
          Code:
          sound(MSG_MULTIPLY);
          becomes
          Code:
          msgt(MSG_MULTIPLY, "");
          I think in cases where there is no message but there is a sound hook, it's cleaner to keep a call to sound() rather than passing an empty string to msgt(), both because it requires no change to the API and semantically because the intended outcome is not to print a message but to make a sound.

          EDIT: We could just move the sound file names into message.prf

          Code:
          message:<message-type>:<color>:<sound file> <sound file> ... <sound file>
          I think it's cleaner to keep sound file and message colours in separate places as it makes it easier to introduce new sound sets without touching stuff that isn't related to them.
          takkaria whispers something about options. -more-

          Comment

          • Nick
            Vanilla maintainer
            • Apr 2007
            • 9634

            #6
            Originally posted by takkaria
            I think it's a bad idea to use format strings like this in external files because they're really brittle. I'd suggest something like "You feel {adjective}" instead, or something like gettext using $1 to mean 'first argument' etc.
            Indeed. If you keep telling me this enough times, I may remember
            One for the Dark Lord on his dark throne
            In the Land of Mordor where the Shadows lie.

            Comment

            • calris
              Adept
              • Mar 2016
              • 194

              #7
              Here's a first pass at reworking the guts of the sound sub-system

              Pastebin.com is the number one paste tool since 2002. Pastebin is a website where you can store text online for a set period of time.


              It compiles, but it doesn't work (crashes) - job for tommorow (technically later today - it's nearly 4am)

              You should get the general jist of it

              Comment

              • Nick
                Vanilla maintainer
                • Apr 2007
                • 9634

                #8
                Originally posted by calris
                Here's a first pass at reworking the guts of the sound sub-system
                I didn't read the full detail, but this looks great. When you're looking for errors, I did notice a number of the lines in sound.prf didn't start with sound:.

                One thing you will definitely have to do is add yourself to the copyright statement at the top of the relevant files.
                One for the Dark Lord on his dark throne
                In the Land of Mordor where the Shadows lie.

                Comment

                • calris
                  Adept
                  • Mar 2016
                  • 194

                  #9
                  Thanks for the review Nick

                  There were only a couple of minor problems with the code (useing mem_alloc() instead of mem_zalloc() was the main one)

                  So SDL sound works with the new code. Happy days

                  Comment

                  • calris
                    Adept
                    • Mar 2016
                    • 194

                    #10
                    Ironed out all the bugs and tidied up the code

                    The latest version of the new sound sub-system is here:

                    Pastebin.com is the number one paste tool since 2002. Pastebin is a website where you can store text online for a set period of time.


                    Looking forward to a in-depth review of this. A few 'nice' features:
                    - I'm using an AVL (balanced binary) tree to store sound and mapping information so there should be a lot less memory used by the sound sub system (EDIT: The old system loaded sounds multiple times - once for EVERY instance in the sound config)
                    - sounds are now defined in a .prf file chainloaded by pref.prf so now it's trivial to re-assign sounds (and even have sounds assigned per-class and per-character) in user preference files
                    - file extensions are not used in the sound.prf file - it is up to the platforms sound module to search add supported extensions
                    - SDL handles mp3 and ogg now. Adding new extensions is trivial (see supported_file_types[] in snd_sdl.c)
                    - Did I mention balanced trees? I'm sure there are lots of other places we can use them
                    - The only thing main.c now know about sound is to call init_sound()

                    Comment

                    • calris
                      Adept
                      • Mar 2016
                      • 194

                      #11
                      Originally posted by takkaria
                      I think it's cleaner to keep sound file and message colours in separate places as it makes it easier to introduce new sound sets without touching stuff that isn't related to them.
                      Indeed it was

                      Now we can define custom message/sound maps in the players pref files - it was just a happy side-effect of the implementation I had not even considered. But seriously, how neat is that

                      Comment

                      • Nick
                        Vanilla maintainer
                        • Apr 2007
                        • 9634

                        #12
                        Originally posted by calris
                        Looking forward to a in-depth review of this. A few 'nice' features:
                        - I'm using an AVL (balanced binary) tree to store sound and mapping information so there should be a lot less memory used by the sound sub system (EDIT: The old system loaded sounds multiple times - once for EVERY instance in the sound config)
                        - sounds are now defined in a .prf file chainloaded by pref.prf so now it's trivial to re-assign sounds (and even have sounds assigned per-class and per-character) in user preference files
                        - file extensions are not used in the sound.prf file - it is up to the platforms sound module to search add supported extensions
                        - SDL handles mp3 and ogg now. Adding new extensions is trivial (see supported_file_types[] in snd_sdl.c)
                        - Did I mention balanced trees? I'm sure there are lots of other places we can use them
                        - The only thing main.c now know about sound is to call init_sound()
                        Excellent - these are massive improvements. My questions for you are:
                        • I want to have a good look at this - could you put this in a git branch and put it on github?
                        • I assume this is Linux only at this point - will it break the other ports as is?
                        • What assumptions does it make about what sound files are available?
                        • What effect if any does this have on messages so far?
                        • Why should I work out the answers to these questions myself when I can get you to tell me?
                        One for the Dark Lord on his dark throne
                        In the Land of Mordor where the Shadows lie.

                        Comment

                        • calris
                          Adept
                          • Mar 2016
                          • 194

                          #13
                          Originally posted by Nick
                          Excellent - these are massive improvements. My questions for you are:
                          • I want to have a good look at this - could you put this in a git branch and put it on github?
                          • I assume this is Linux only at this point - will it break the other ports as is?
                          • What assumptions does it make about what sound files are available?
                          • What effect if any does this have on messages so far?
                          • Why should I work out the answers to these questions myself when I can get you to tell me?
                          Commit can be found here:



                          EDIT: There are no assumptions - if the platform sound module can't load a sound file, it simply won't be played. It is up to the platform module to resolve sound names to files (typically by iterating through the sound name with known file extensions)

                          It's linux only - all other ports (AKAICT) define their own handling of sound.cfg in their respective main-xxx.c, so other ports SHOULDN'T be impacted at all. Creating architecture sound modules to hook into this new code is next on my hit list (should be trivial)

                          There is no effect on the messaging system at all

                          Comment

                          • calris
                            Adept
                            • Mar 2016
                            • 194

                            #14
                            I've just pushed another patch into sound-devel

                            I realised that initialising sound _after_ loading pref files was going to prevent user prefs loaded afterward to not update the message<->sound map (and not load any new sounds)

                            So now, you can modify the sounds for messages on the fly.

                            I also introduced a pre-load sounds flag (which the old SDL module had, but could never be used due the the current parsing of module sub-options from main). I haven't hooked it into a command line option yet - I would like to resolve parsing sound options from main().

                            I think the best option is to make snd-sdl a module in it's own right (i.e. not a sub-module of sound-core) and ditch the '-s' switch (instead use -msnd-sdl) so then we could have something like:

                            angband -mx11 -- -n6 -msnd-sdl -- -pn -fogg

                            to run a 6 window x11 display, SDL sounds, no preloading of sounds (-pn), forcing loading of ogg files

                            Comment

                            • takkaria
                              Veteran
                              • Apr 2007
                              • 1951

                              #15
                              Hey, this is looking great.

                              Originally posted by calris
                              I also introduced a pre-load sounds flag (which the old SDL module had, but could never be used due the the current parsing of module sub-options from main). I haven't hooked it into a command line option yet - I would like to resolve parsing sound options from main().

                              I think the best option is to make snd-sdl a module in it's own right (i.e. not a sub-module of sound-core) and ditch the '-s' switch (instead use -msnd-sdl) so then we could have something like:

                              angband -mx11 -- -n6 -msnd-sdl -- -pn -fogg

                              to run a 6 window x11 display, SDL sounds, no preloading of sounds (-pn), forcing loading of ogg files
                              I'm not sure if it makes sense to make the sound module a main module because that would mean having multiple main modules. Surely you can get the same sound option behaviour using -s as you can using -m?
                              takkaria whispers something about options. -more-

                              Comment

                              Working...
                              😀
                              😂
                              🥰
                              😘
                              🤢
                              😎
                              😞
                              😡
                              👍
                              👎