Altsounds not behaving as expected

[droscoe]

I have been struggling with trying to balance altsound volume, and I am now thinking there is something wrong with the software.

I am running: VPX 10.7.3 VPinMame 3.5

For my tests, I am using my laptop, which has 2-channel audio with the Realtek drivers I used Audacity to normalize loudness on all sound files to -14 LUFS

I have several sounds assigned to channel 0 (background music)

• Gain is set at 100

I have several sounds that are not assigned to a channel (SFX)

• Gain is set to 100
• Duck value set to 5

These values are assigned merely for testing.

What is happening:

When I start a game, the background music file is playing fine. As soon as one of the SFX sounds on the unassigned channels triggers, the duck value kicks in and lowers the background music volume From there, background music volume never returns to the previous level until the ball drains.. When the ball drains, the levels return to normal until the first SFX sound is hit and the problem repeats.

As far as I can tell, this problem does not occur with sounds assigned to channel 1 (overlay). For these files, it appears the level for the background music returns when the sound is finished playing.

It is possible I am simply not doing this right, but the documentation I could find on this suggests I am not, and that this is not expected behavior. I appreciate any help!

UPDATE: Similar issue; If I leave the duck values for SFX blank, the background music plays fine at first, but then shuts off entirely when some amount of SFX sounds play. So far, I can’t tell if it’s random or not.

-Dave

[droscoe]

UPDATE 05/01/23 I've started looking through the code and I think I've solved at least one issue. Previously I indicated that if I leave the ducking values for SFX blank in the CSV file, it seems to randomly stop the background music at some point. Here's what I think is going on.

When the .CSV is first parsed, the ducking value read from the file is parsed on this line:

csv_get_int_field(c, colDUCK, &val);

We don't really know what value is here since it was blank in the sheet. But, we know it isn't zero. Then memory is allocated to hold information for each file in the altsound package:

psd.ducking = (signed char*)malloc(psd.num_files*sizeof(signed char));

NOTE: The malloc call here is not initializing the allocated memory to zero

Them, the value is stored in the allocated memory: val = 0; csv_get_int_field(c, colDUCK, &val); psd.ducking[i] = min(val, (int)100); The value above is protected against going higher than 100, but it is not protected against going lower than zero. If val ends up being negative because the malloc call just happened to allocate memory with junk bytes that just happen to equate to a negative integer, it will be happily stored in the array.

When the SFX sound represented by that duck value is played, the following code will get called:

if (psd.channel[idx] == 1)
{
    channel_1_ducking = 100;

     if (channel_0 != 0) 
    {
        if (psd.stop[idx] == 0)
        {
            if (psd.ducking[idx] < 0)
                BASS_ChannelPause(channel_0);
            else
                channel_1_ducking = psd.ducking[idx];
    }

In the code above, if there is a sound playing on channel 0, and we're not being told to STOP it, we then check if the duck value specified by the SFX file is less than zero. If it is, we pause channel 0. Otherwise, its simply ducked

It seems that this is intentional behavior, but I would expect the paused sound to resume at some point? In any case, we got here completely by accident because either the memory is not initialized after malloc, and/or we aren't protecting adequately against values less than zero.

[droscoe]

Saw something else that seems a bit off. The variable "min_ducking" is set to 100 initially, and then it's set to another value in the ducking_callback() function:

min_ducking = 100;
if (channel_1 != 0 && channel_1_ducking < min_ducking)
    min_ducking = channel_1_ducking;

for (i = 0; i < ALT_MAX_CHANNELS; ++i) {
    if (channel_x[i] != 0 && channel_x_ducking[i] < min_ducking)
    min_ducking = channel_x_ducking[i];
}

It's not reset before this function exits. If another sound plays, then the comparisons against "min_ducking" is going to be against the modified value, which I'm guessing is not what was intended. For example, assuming the ducking_callback() function was called as a result of this code:

if (psd.ducking[idx] > 0 && psd.ducking[idx] < min_ducking) {
    float new_val;
    min_ducking = psd.ducking[idx];
    new_val = channel_0_vol*(float)((double)psd.ducking[idx] / 100.);
    if (channel_0_vol != new_val)
        BASS_ChannelSetAttribute(channel_0, BASS_ATTRIB_VOL, new_val);
}

The resulting call to ducking_callback() will reset min_ducking to 100, modify it, and then the code above can be hit again. In this case, the min_duck value will still be whatever was set in the ducking_callback() function.

It's not entirely clear how the "min_ducking" variable was meant to be used. Its a static global, but not sure why, other than to make it available to the callback function. If that's the case, it's safer to just define it within scoping limits of where it is used, and declare it multiple times. This way, its value can be controlled better.

[toxieainc]

It would be ace if you could please test/verify your findings and do a pull request. :)

These code snippets were added during a time as that format was still evolving (i think only 2 such packages were available or so), so thats why a lot of the code could not be tested with real life data.

[droscoe]

As soon as I can get the compilation working, I'd be happy to.

[droscoe]

@toxieainc I now have a fork up and running. If you want to assign this to me, I'll take the lead on sorting it out

[toxieainc]

Thanks, then please go ahead. We usually don't assign things explicitly (as simply too few devs around here ;)).

[droscoe]

@toxieainc I've resumed work on this. The main problem I am tracking down is why the sound volume behavior seems so erratic. I see code like this:

if ((core_gameData->gen == GEN_WPCDCS) ||
                (core_gameData->gen == GEN_WPCSECURITY) ||
                (core_gameData->gen == GEN_WPC95DCS) ||
                (core_gameData->gen == GEN_WPC95))

In these blocks, there is code that will limit the master volume. I was curious why? It was my understanding that altsound was meant to replace the ROM sounds, and the code intercepts the ROM commands and plays a local sound file. Why would it still matter what the hardware platform of the original ROM was? Wouldn't only the local sound hardware be relevant?

Also, I've enabled logging and added some debug output. I see lines like this: 0083 unknown 0 Assuming these were ROM events without an associated sound file in the altsound, I created a temporary file matching the hex ID, and that got rid of the error, and played the temp sound file. When using altsounds, can ANY of the ROM sounds play, if there isn't a sound file provided? I think I've found code that mutes all rom sound when the altsound handler is called.

[toxieainc]

In general this is true, BUT in addition to the altsound files and respective parameters, also the pinball machine code itself sets parameters, and these have to be respected, too. Otherwise one does not get some effects, like fading in/out of music/sfx. Cause altsound in itself just maps the sound commands.

[toxieainc]

As for using ROM sounds where no altsound exists: Could be done i think, but not trivial for the general usage case. There could be all kinds of side effects, if one does not just mute the sound emulation but instead only sends a fraction of the sound commands to the soundboard emulation.

[droscoe]

@toxieainc

As for using ROM sounds where no altsound exists: Could be done i think, but not trivial for the general usage case. There could be all kinds of side effects, if one does not just mute the sound emulation but instead only sends a fraction of the sound commands to the soundboard emulation.

No, I wasn't suggesting adding that feature. I was just curious about the current behavior. It's actually better if the altsound completely replaces the ROM sounds (IMO).

UPDATE on my debugging. I have confirmed that if music is playing on channel 0 and a SFX plays on CHX with a ducking value that lowers the gain of CH0, it will stay that way unless another sound plays on CH0 or if the ducking value of a sound on CHX raises it. It does not remember the gain of the music at the time it was ducked.

I tested this by setting alternating ducking values for sounds on CHX to 80 and 0 respectively. If a sound was playing on CH0 and a sound with a ducking value of 0 plays on CHX, the music on CH0 goes to 0 gain. It will stay that way until a sound on CHX plays with a ducking value of 80.

[droscoe]

As a first pass, I cleaned up the snd_alt.h file, breaking up some of the monolithic code into discrete functions. I know what the bug is with the ducking approach, and have an idea to fix it.

[toxieainc]

Sounds good, and thanks!

That code evolved pretty bad. It started as a prototype just to compliment the closed source variant, and then pretty much always stayed at that stage with more stuff just being thrown on top. ;)

[droscoe]

Sounds good, and thanks!

I am only too happy to contribute to this community. I’ve been wanting to get involved on the code side for quite a while.

That code evolved pretty bad. It started as a prototype just to compliment the closed source variant, and then pretty much always stayed at that stage with more stuff just being thrown on top. ;)

Amen, brother! Story of my life. I don’t know if you’re old, like me, but I refer to a term from the old show “M.A.S.H.” with stuff like this… “meatball surgery”, it’s like performing surgery on a meatball! 🤣

[droscoe]

@toxieainc Does the BASS sync callback run on a separate thread than the alsound code? Look at this:

BEGIN DUCKING_CALBACK: CHANNEL: 2147483712  DATA: 0  USER: 0
CHANNEL X SOUND FINISHED
**BEGIN ALT_SOUND_HANDLE**
CHANNEL 0 IS PLAYING
MIN_DUCKING: 100
CHECKING CHANNEL X SOUNDS
**Attenuation: 0.000000**
 CHANNEL X DUCKING: 100
**Master Volume (Pre Attenuation): 1.000000**
 MIN_DUCKING: 100
**Master Volume (Post Attenuation): 1.000000**
 CHECKING CHANNEL X SOUNDS
**MAME_GEN: 2**
CHANNEL X DUCKING: 100
**MUTING INTERNAL PINMAME MIXER**
MIN_DUCKING: 100
CHECKING CHANNEL X SOUNDS
**Hardware Generation: GEN_WPCALPHA_2, GEN_WPCDMD, GEN_WPCFLIPTRON**
CHANNEL X DUCKING: 100
MIN_DUCKING: 100
**007F unknown 0**
CHECKING CHANNEL X SOUNDS
**END ALT_SOUND_HANDLE**
CHANNEL X DUCKING: 100
MIN_DUCKING: 100
CHECKING CHANNEL X SOUNDS
CHANNEL X DUCKING: 100
MIN_DUCKING: 100
CHECKING CHANNEL X SOUNDS
CHANNEL X DUCKING: 100
MIN_DUCKING: 100

I was outputting data from the ducking_callback and noticed that the alt_sound_handle() function was called (output in bracketed by **). This should only happen if the callback is on a different thread. Do you know?

BTW, the ducking_callback() was previously never getting called, the SyncProc being passed to the function was not being passed by pointer, so it was never getting hit.

[toxieainc]

Threading: I don't remember, sorry. :/

Ducking: Ouch, good catch, thanks!!

[droscoe]

Threading: I don't remember, sorry. :/

No worries, brother. I did some digging and I found this in the documentation at SYNCPROC:

BASS creates a single thread dedicated to executing sync callback functions, so a callback function should be quick as other syncs cannot be processed until it has finished. Attribute slides (initiated with [BASS_ChannelSlideAttribute](https://www.un4seen.com/doc/bass/BASS_ChannelSlideAttribute.html)) are also performed by the sync thread, so are also affected if a sync callback takes a long time.

Now that I know this is happening, I might have to protect the main thread since they are both working with some of the same variables. That mixed output is scary

[droscoe]

@toxieainc Is there any significance to “channel 1” “channel 0” other than the behavior? I mean, if they were assigned arbitrary channels internally, but behaved the same way, would that be OK? BTW, I’m very close to a PR on this.

[toxieainc]

I think so. Most likely i just 'fixed' them/made these separate because they have other logic compared to the others.

[droscoe]

While testing the folder parsing code, VPinMAME was crashing. I instrumented the code to see what was going on. There are hidden files being picked up by the directory scan named “desktop.ini”. I don’t see them in my folders, even with “show hidden” selected. The code checks for “.” In the first byte of the name which catches “.” and “..” and excludes them. It also checks for “.txt” somewhere in the name string, and exclude that too. It does not check for “.ini” files, so those were included, and the parsing code crashes. There are unprotected NULLs that result from trying to open those files. I’m running on Windows 11. I’ve fixed the parsing to exclude “.ini” files as well as adding protection for NULL dereferencing

[droscoe]

@toxieainc I have a few people testing now. Fix is good! Coordinating with altsound authors before releasing. Existing altsounds have probably been compensating (trying to) for the original problem. After this fix, these altsound packages won’t sound right. I’m trying to avoid perception of a degraded VPinMAME.

I pretty much completely rewrote this code. I left the csv and folder parsing code largely intact, but the rest is all new.

[droscoe]

Found a really good example of the thread safety issue that exists with the altsound processing:

    BEGIN: PROCESS_CHANNELX
        BEGIN: FIND_FREE_CHANNEL
        END: FIND_FREE_CHANNEL
        ASSIGNED CHANNEL: 3
        BEGIN: DUCKING_CALLBACK
            HANDLE: 2147483662  CHANNEL: 2147483661  DATA: 0  USER: 2
            STREAM FINISHED ON CHANNEL 2
            CHANNEL_STREAM[0] ACTIVE
            CHANNEL_DUCKING[0]: 100
            NUM STREAMS ACTIVE: 1
            MIN DUCKING OF ALL ACTIVE CHANNELS: 100
            NEW CHANNEL 0 VOLUME: 0.500000
        END: DUCKING_CALBACK
        SFX_STREAM_HANDLE: 2147483663
        SYNCH_HANDLE: 2147483664
        SFX_VOL: 0.700000  GAIN: 0.700000  GLOBAL_VOL: 1.000000  MASTER_VOL 1.000000
        SFX DUCKING: 60
    END: PROCESS_CHANNELX

Notice how the DUCKING_CALLBACK processing begins in the middle of PROCESS_CHANNELX. It's not causing a crash, but it could mess up internal bookkeeping, contributing to volume level control problems, perhaps even interrupted streams. There's no easy way to fix this that would be platform independent at the moment.

[toxieainc]

Why would it be platform dependent? What would be your suggestion how to fix it?

[droscoe]

Why would it be platform dependent? What would be your suggestion how to fix it?

Windows doesn’t support POSIX threads. Would need to put code to support both platforms. I don’t want to drag the code further down the Windows path, and I haven’t fully worked out how to define the critical section. I also don’t want to create a performance hit on the callback. Just needs a bit more focused attention. Since it’s a pre-existing problem and doesn’t appear to be causing a noticeable issue, I’m going to defer that for a later issue. For now, I’ve organized the code to try and minimize the touch points with common memory.

[toxieainc]

Wouldn't something like https://en.cppreference.com/w/cpp/thread/lock etc work?

[droscoe]

Wouldn't something like https://en.cppreference.com/w/cpp/thread/lock etc work?

The altsound code is C.

[mjrgh]

The altsound code is C.

C and C++ play pretty well together via extern "C". If all it needs is some simple thread serialization, it's probably doable with the C++ thread library.

[droscoe]

@toxieainc As I clean up, I came across this in my debugging notes:

          MIXER_NAME (Channel 0) IS NOT NULL
          MIXER_NAME (Channel 1) IS NOT NULL
      MIXER_NAME (Channel 2) IS NOT NULL
      MIXER_NAME (Channel 3) IS NOT NULL
      MIXER_NAME (Channel 4) IS NOT NULL
      MIXER_NAME (Channel 5) IS NOT NULL
      MIXER_NAME (Channel 6) IS NOT NULL
      MIXER_NAME (Channel 7) IS NOT NULL
      MIXER_NAME (Channel 8) IS NOT NULL
      MIXER_NAME (Channel 9) IS NOT NULL
      MIXER_NAME (Channel 10) IS NULL
      MIXER_NAME (Channel 11) IS NULL
      MIXER_NAME (Channel 12) IS NULL
      MIXER_NAME (Channel 13) IS NULL
      MIXER_NAME (Channel 14) IS NULL
      MIXER_NAME (Channel 15) IS NULL
      MIXER_NAME (Channel 16) IS NULL
      MIXER_NAME (Channel 17) IS NULL
      MIXER_NAME (Channel 18) IS NULL
      MIXER_NAME (Channel 19) IS NULL
      MIXER_NAME (Channel 20) IS NULL
      MIXER_NAME (Channel 21) IS NULL
      MIXER_NAME (Channel 22) IS NULL
      MIXER_NAME (Channel 23) IS NULL
      MIXER_NAME (Channel 24) IS NULL

ALT_MAX_CHANNELS is 16 in the code. Does the output above suggest there are only actually 10 channels available?

[droscoe]

@toxieainc here's another one: -saw:

BEGIN SFX/VOICE
      CH_X_STREAM_HANDLE: 2147483654
      SFX_VOL: 0.244094  GAIN: 1.000000  GLOBAL_VOL: 0.244094  MASTER_VOL 1.000000
      SYNCH_HANDLE: 2147483655
      playing CHX: cmd 00E0 gain 1.00 duck 0 C:\vPinball\VisualPinball\VPinMAME\altsound\che_cho\0x00e0-plonk.ogg
END SFX/VOICE

and...

BEGIN PLAY MUSIC
      CH_0_STREAM_HANDLE: 2147483653
      MUSIC_VOL: 0.244094  GAIN: 1.000000  GLOBAL_VOL: 0.244094  MASTER_VOL 1.000000
      playing CH0: cmd 0002 gain 1.00 duck 0 C:\vPinball\VisualPinball\VPinMAME\altsound\che_cho\0x0002-main_theme_mounties.ogg
END PLAY MUSIC

Everything appears to be fine, yet no audio playback is heard. The above are only examples. I see similar behavior on almost all tables. I suspect these sounds are being triggered before the ROM is fully initialized. Once the game loads, the sounds work fine.

• Is my assumption correct?
• Is there any way to check this and ignore these commands until the ROM is ready?

The ducking callback for channel1, X sounds is not being hit until later as well, which bolsters the idea that the system just isn't ready yet

[droscoe]

In general this is true, BUT in addition to the altsound files and respective parameters, also the pinball machine code itself sets parameters, and these have to be respected, too. Otherwise one does not get some effects, like fading in/out of music/sfx. Cause altsound in itself just maps the sound commands.

@toxieainc I understand this completely. However, the #1 complaint from people using Altsounds is "the altsounds are too low". Here is a prime example. I play Fish Tales all the time using the standard ROM. When I play the altsound, the volume after the attract music drops WAAAAAAY low, and stays there. Here is the output from the code:

BEGIN: ALT_SOUND_HANDLE
    Master Volume (Post Attenuation): 1.000000
    BEGIN: PREPROCESS_COMMANDS
        MAME_GEN: 8
        Hardware Generation: GEN_WPCALPHA_2, GEN_WPCDMD, GEN_WPCFLIPTRON
        Change volume 0.09
    END: PREPROCESS_COMMANDS
    COMMAND FILTERED: 00F3
END: ALT_SOUND_HANDLE

That GLOBAL volume change to 0.09 is the culprit. All of the volumes computed from that are so low, you can barely hear them. The real ROM does not do that. There's something wrong with the command parsing code. I intend to look into this. In the meantime, would it be terrible if I just nerfed those volume changes until it can be fixed?

[droscoe]

I just confirmed that the thread safety issue is resolved. Thanks for the suggestion on std::mutex. I had to convert snd_alt to C++, but that was fairly simple.

BEGIN: ALT_SOUND_HANDLE
    ACQUIRING MUTEX
    MUTEX ACQUIRED
    Master Volume (Post Attenuation): 1.000000
    BEGIN: PREPROCESS_COMMANDS
        MAME_GEN: 1024
        Hardware Generation: GEN_WPCALPHA_1, GEN_S11, GEN_S11X, GEN_S11B2, GEN_S11C
    END: PREPROCESS_COMMANDS
    BEGIN: PROCESS_CHANNELX
        BEGIN: FIND_FREE_CHANNEL
            FOUND FREE CHANNEL: 2
        END: FIND_FREE_CHANNEL
        SUCCESS: FIND_FREE_CHANNEL(): 2
        BEGIN: CREATE STREAM
-->         BEGIN: DUCKING_CALLBACK
                ACQUIRING MUTEX
Callback entered, but does not continue

-->             SUCCESS: BASS_StreamCreateFile(bbnny_l2\0x0030-gunfire.ogg): 2147483661
alt_sound_handle continues...

                BEGIN: CREATE_SYNC
                    SUCCESS: BASS_ChannelSetSync(2147483662)
                END: CREATE_SYNC
                SUCCESS: create_sync(2147483661)
            END: CREATE STREAM
            SUCCESS: create_stream(bbnny_l2\0x0030-gunfire.ogg): 2147483661
            BEGIN: SET_VOLUME
                VOL: 0.70 (GAIN: 0.70  GLOBAL_VOL: 1.00  MASTER_VOL 1.00)
                SUCCESS: BASS_ChannelSetAttribute(2147483661, BASS_ATTRIB_VOL, 0.70)
            END: SET_VOLUME
            SFX DUCKING: 0.60
        END: PROCESS_CHANNELX
        BEGIN: GET_MIN_DUCKING
            CHANNEL_STREAM[0]: ACTIVE
            CHANNEL_DUCKING[0]: 0.60
            CHANNEL_STREAM[1]: ACTIVE
            CHANNEL_DUCKING[1]: 1.00
            CHANNEL_STREAM[2]: ACTIVE
            CHANNEL_DUCKING[2]: 0.60
            NUM STREAMS ACTIVE: 3
            MIN DUCKING OF ALL ACTIVE CHANNELS: 0.60
        END: GET_MIN_DUCKING
        MIN DUCKING VALUE: 0.60
        BEGIN: SET_VOLUME
            VOL: 0.30 (GAIN: 0.30  GLOBAL_VOL: 1.00  MASTER_VOL 1.00)
            SUCCESS: BASS_ChannelSetAttribute(2147483657, BASS_ATTRIB_VOL, 0.30)
        END: SET_VOLUME
        SUCCESS: BASS_ChannelPlay(2147483661): CH(2) CMD(0030) GAIN(0.70) DUCK(0.60) SAMPLE(bbnny_l2\0x0030-gunfire.ogg)
        BEGIN: POSTPROCESS_COMMANDS
            MAME_GEN: 1024
        END: POSTPROCESS_COMMANDS
--> END: ALT_SOUND_HANDLE
alt_sound_handle completes

--> MUTEX ACQUIRED
ducking_callback gets mutex and continues...

    HANDLE: 2147483660  CHANNEL: 2147483659  DATA: 0  USER: 0
    STREAM(2147483659) FINISHED ON CH(0)
Warning: sound latch 2 written before being read. Previous: 42, new: 30
    BEGIN: FREE_STREAM
        SUCCESS: BASS_StreamFree(2147483659)
    END: FREE_STREAM
    SUCCESS: free_stream(2147483659)
    BEGIN: GET_MIN_DUCKING
        CHANNEL_STREAM[1]: ACTIVE
        CHANNEL_DUCKING[1]: 1.00
        CHANNEL_STREAM[2]: ACTIVE
        CHANNEL_DUCKING[2]: 0.60
        NUM STREAMS ACTIVE: 2
        MIN DUCKING OF ALL ACTIVE CHANNELS: 0.60
    END: GET_MIN_DUCKING
    MIN DUCKING VALUE: 0.60
    BEGIN: SET_VOLUME
        VOL: 0.30 (GAIN: 0.30  GLOBAL_VOL: 1.00  MASTER_VOL 1.00)
        SUCCESS: BASS_ChannelSetAttribute(2147483657, BASS_ATTRIB_VOL, 0.30)
    END: SET_VOLUME
END: DUCKING_CALLBACK

[droscoe]

@toxieainc in sndbrd.c

void sndbrd_data_w(int board, int data) {
  const struct sndbrdIntf *b = intf[board].brdIntf;
  if(b && (b->flags & SNDBRD_NOTSOUND)==0) {
    snd_cmd_log(board, data);

in snd_cmd.c

void snd_cmd_log(int boardNo, int cmd) {
#ifdef VPINMAME_ALTSOUND
  if (options.samplerate != 0 && pmoptions.sound_mode == 1)
    alt_sound_handle(boardNo, cmd);
#endif

I am confused by the name "snd_cmd_log". This implies it is a logging function and doesn't really make sense as the entry point for altsound/pinsound processing. This also requires the alt_sound_handle() function to manage initialization as part of its main handling function. Ideally, this would be integrated at a lower level, with the initialization happening prior to calling the main handler.

What is the legacy of this? I'd like to explore the the possibility of evolving this to have a cleaner integration, but don't want to disturb legacy.

[toxieainc]

If you look at the very beginning of the file you'll roughly see the explanation. (or the readmes, 'sound commander')

The altsound functionality just abuses all of the sound commander functionality to redirect the sound board commands to the altsound parser/player.

[toxieainc]

@toxieainc I understand this completely. However, the #1 complaint from people using Altsounds is "the altsounds are too low". Here is a prime example. I play Fish Tales all the time using the standard ROM. When I play the altsound, the volume after the attract music drops WAAAAAAY low, and stays there. Here is the output from the code: That GLOBAL volume change to 0.09 is the culprit. All of the volumes computed from that are so low, you can barely hear them. The real ROM does not do that. There's something wrong with the command parsing code. I intend to look into this. In the meantime, would it be terrible if I just nerfed those volume changes until it can be fixed?

Would be nice if you could only limit this to machines/generations that are really affected by this parsing bug. Cause mostly this really works as intended.

[droscoe]

@toxieainc Would you be ok with me adding source groups to the main vpinmame CMake file? It won't affect anything other than the way the source files look in Visual Studio. Instead of a huge list, there will be what appear to be separate projects matching the directory structure in the repo

[mjrgh]

Would you be ok with me adding source groups to the main vpinmame CMake file?

I'd actually prefer to keep it flat. I know you're thinking that this would make it easier to navigate the giant source list, but it's just the opposite in every experience I've ever had with this code. I find that when I'm trying to cross-reference something in an unfamiliar part of the code, it's a huge pain if I have to guess at which folder a file is in, because whoever came up with the folder hierarchy had their own idea of how things are related. I don't always have a search key that I can use to find it that way, so it's often easier to find a file by name, which is always easier if you can see them all at once without having to open all of the little drop arrows one by one manually.

[droscoe]

@mjrgh I can appreciate that. I'm fine with a flat source list, when the list is short, but this is not. As a relative newcomer to this codebase, I found it quite daunting to navigate in the IDE, which is why I've been using the proposed project structure on my own branch for quite some time. It's been a great help, and I was thinking others might find it helpful as well, especially new developers coming into the community.

Re: imposing a vision of relationships. Those relationships are already there. The structure of the code within the filesystem already reflects what I am proposing. The real imposition is the current flat structure. It does not reflect the structure of the code in the repo. Directory structure within a filesystem is meant to suggest relationships among the files contained within. Why shouldn't the project files reflect that as well?

With many things, a little is good, and too much can be bad. An overly nested source tree and/or filesystem takes away from the maintainability of the code base. I am not a fan of that, either. The structure I'm proposing more closely matches the filesystem. At a glance, I know roughly where in the filesystem a given compilation unit is, just by its place within the project file. Additionally, I also have some context on what those files mean. Doesn't this exactly address one of your objections? With a flat structure, all you can do is guess where things are. Files in the CPU source folder are probably related to CPU functionality, and can be found in the src/cpu folder. Files in the SOUND source folder are probably related to audio processing and can be found in the src/sound folder., etc..

I have included a few screen captures of what the project structure would look like, if this change is accepted.

[droscoe]

One final note: Rather than rejecting the idea because there is a perception that finding files would be more difficult (which I don't see being the case), we could double down and "fix" another issue with the project definition that would make finding files referenced in source files even easier. For example:

target_include_directories(vpinmame PUBLIC
   src
   src/windows
  [...]

Now, let's say you have a source file that is including a file from src/windows. Based on the above, you could simply add #include "window.c". The compiler will be happy with this. The developer, however has no information by looking at that include, where that file actually resides. It could be even worse. If there is a files with the same name in another location. that is also in your include path, you could be including the wrong file, opening yourself up to some frustrating debugging time. If, on the other hand, the CMake file did this:

target_include_directories(vpinmame PUBLIC
   src
   [...]
)

Then all files that need src/windows/window.c would have to add #include "windows/window.c" This tells the developer that the file resides in the "windows" folder, which is a lot more information than you had before. Adding that additional path segment also decreases the likelihood of unknowingly including the wrong file. Like anything, this can be taken too far, so it does take some finesse to keep it sane.

[mjrgh]

I think you're working with the code base a lot more than I am lately, so I don't want to express any sort of strong an opinion here - it won't affect me too much for the foreseeable future one way or the other. The only reason I weighed in is that, funnily enough, what you're proposing sounds like a return to the old project structure from before cmake came along and flattened it all out, and I always found the old project tree cumbersome. I know, it's perverse to prefer a flat list of 200 files, but I nonetheless found the flat layout delightfully streamlined when I discovered the change. I'm probably the only one, though.

Regards, Mike

---

From: Dave Roscoe @.> Sent: Friday, July 14, 2023 10:05 PM To: vpinball/pinmame @.> Cc: mjrgh @.>; Mention @.> Subject: Re: [vpinball/pinmame] Altounds not behaving as expected (Issue #113)

@mjrghhttps://github.com/mjrgh I can appreciate that. I'm fine with a flat source list, when the list is short, but this is not. As a relative newcomer to this codebase, I found it quite daunting to navigate in the IDE, which is why I've been using the proposed project structure on my own branch for quite some time. It's been a great help, and I was thinking others might find it helpful as well, especially new developers coming into the community.

Re: imposing a vision of relationships. Those relationships are already there. The structure of the code within the filesystem already reflects what I am proposing. The real imposition is the current flat structure. It does not reflect the structure of the code in the repo. Directory structure within a filesystem is meant to suggest relationships among the files contained within. Why shouldn't the project files reflect that as well?

With many things, a little is good, and too much can be bad. An overly nested source tree and/or filesystem takes away from the maintainability of the code base. I am not a fan of that, either. The structure I'm proposing more closely matches the filesystem. At a glance, I know roughly where in the filesystem a given compilation unit is, just by its place within the project file. Additionally, I also have some context on what those files mean. Doesn't this exactly address one of your objections? With a flat structure, all you can do is guess where things are. Files in the CPU source folder are probably related to CPU functionality, and can be found in the src/cpu folder. Files in the SOUND source folder are probably related to audio processing and can be found in the src/sound folder., etc..

I have included a few screen captures of what the project structure would look like, if this change is accepted.

[cpu]https://user-images.githubusercontent.com/10767925/253717061-e9cee5f5-11cc-4a4e-a38f-7c34ddcca1a2.png [src]https://user-images.githubusercontent.com/10767925/253717065-b4cdb875-586d-43ab-9011-11d92cf8242b.png [wpc]https://user-images.githubusercontent.com/10767925/253717067-06aa83b0-9e4f-438e-997e-84c09f613a44.png

— Reply to this email directly, view it on GitHubhttps://github.com/vpinball/pinmame/issues/113#issuecomment-1636666575, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AB7DK3JXOP2B3TNPAHCT4UDXQIQIBANCNFSM6AAAAAAXQ5CFSM. You are receiving this because you were mentioned.Message ID: @.***>

[droscoe]

Mike, are you using the win_build.bat file I created to generate your projects? If so, I could add an option to flatten the structure. It would be pretty easy to do.

[jsm174]

For some history, when I initially added the CMakeLists files, the effort was entirely to support libpinmame and building it in GitHub actions. At the time I started by grepping through the Visual Studio project files just to see what the minimum set of files that were actually needed. (Minus platform specific code, you can't just compile every source file in a cpu folder). Anyway, this is basically how it got flattened out -- I never put any effort into the structure.

After we saw the CI working well with libpinmame, we decided to do it for all the variations of PinMAME. It was never the intention to replace any of the create*.bat scripts or *.vcproj files.

My only comments on this would be:

• I realize CMake can do so much more than we have it doing. Technically we don't need CMakeLists_win-x64.txt and CMakeLists_win-x86.txt, etc. However, having them split up this way has been extremely helpful. We can easily do a diff between any CMake file and immediately see what is different. We don't have to look at passed in options and search through hundreds of If statements.

• Whatever changes you make please make them consistent across all the variations, ie: libpinmame, pinmame, pinmame32, ppuc, vpinmame, xpinmame. This will continue making it helpful with diffs, etc.

• I've seen the flattening in the XCode projects (cmake -G Xcode -B build) as well, so hopefully your changes work there too.

• I forced myself to use VS Code. I have the same UI and debugging experience in Linux, Mac, and Windows (which is actually pretty crazy awesome). Since VS Code is generic, it just presents the entire folder as is, so this hasn't ever been an issue:

[mjrgh]

Mike, are you using the win_build.bat file I created to generate your projects? If so, I could add an option to flatten the structure. It would be pretty easy to do.

Great idea - that seems like it would cover all the bases.

Mike

---

From: Dave Roscoe @.> Sent: Friday, July 14, 2023 10:59 PM To: vpinball/pinmame @.> Cc: mjrgh @.>; Mention @.> Subject: Re: [vpinball/pinmame] Altounds not behaving as expected (Issue #113)

Mike, are you using the win_build.bat file I created to generate your projects? If so, I could add an option to flatten the structure. It would be pretty easy to do.

— Reply to this email directly, view it on GitHubhttps://github.com/vpinball/pinmame/issues/113#issuecomment-1636677485, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AB7DK3KUJSRKLIEM2RC5AMTXQIWVLANCNFSM6AAAAAAXQ5CFSM. You are receiving this because you were mentioned.Message ID: @.***>

[toxieainc]

Just my 2 cents, but i also have no strong opinion on that: I prefer the 'old' vcproj structure, and not the flattened variant.

[droscoe]

I have a separate branch for the structure changes, and I'll follow-up with that at a later date. An update on the AltSound fix:

This started as a simple bugfix, but that involved digging into unfamiliar code to learn how it worked. As a result of that investigation, I have rewritten the code entirely. I have separated the C interface and created an all C++ implementation. The CSV and file parsers are all re-written. I have moved the traditional altsound and legacy (pinsound-like) command processing to its own class, and created an all new format: G-Sound. G-Sound combines elements of both the current CSV and the Legacy formats, and adds new features designed to make it easier for authors to create AltSound mixes. To support all of the new features, the altsound code will use a .ini file to configure aspects of traditional, legacy and G-Sound operation. This file will be transparent to most users, and is created automatically for packages that do not have one.

In addition, I have removed VPInMAME dependencies within the AltSound code. This will ultimately allow AltSound to be supported by non-Windows platforms. As a bonus, all formats now support command recording and playback. This saves authors and devs from having to create/recreate potentially complex command sequences on the virtual table. Instead, it can be created once and iterated on as many times as needed. This is implemented by a special standalone AltSound build, which is selectable from the win_build.bat script. The standalone is a simple executable that creates the appropriate AltSound processor, reads in the command file, and injects them into the processor, preserving the relative times that the commands occurred during the recording.

I've also added an independent logger that runs when any altsound table is played. The logging level is configurable depending on what the user/author/dev wants to see.

The code is being rigorously tested, and is almost ready for merging into the main baseline, pending review and approval.

Next steps, I will be writing documentation of how the different altsound formats work, as well as how to use the recording and playback feature. I will also be writing a GUI frontend for the standalone tool, and a GUI tool to convert traditional altsound to g-sound format. The features of that tool are evolving and will be a separate effort.

[toxieainc]

Sounds great! Thanks.. :)

[droscoe]

@toxieainc this can be closed with PR 142 merged

[droscoe]

@toxieainc any chance you can push a new pre-release? We have a big release ready to go, which uses the new G-Sound format. It will be easier for users if they can download it via pre-release without GitHub account. Thanks!

[toxieainc]

Can do.. Please check the commit i just did, there is one if/else case that features the same code https://github.com/vpinball/pinmame/commit/cc4986a97d92d80ab492b2bd3d921e3bf5bb35f5#diff-70b7a0b9450d9afb5406ee2c96e9b03821533b08d12fb574d90a019b9d1e6f57R246 , maybe thats worth double checking before?

[droscoe]

Can do.. Please check the commit i just did, there is one if/else case that features the same code cc4986a#diff-70b7a0b9450d9afb5406ee2c96e9b03821533b08d12fb574d90a019b9d1e6f57R246 , maybe thats worth double checking before?

Good catch! Right now the only difference between the true/false boolean is whether or not the value is for gain or ducking.. I had plans of handling the ducking slightly differently, but then never implemented it, which is why both branches ended up being identical. Since both are handled the same, it doesn't BREAK anything, but isn't good code. I'll capture it in my notes and fix it in a follow up PR. There's some commented out code I also want to clean up.

[toxieainc]

Should i wait with the pre-release then, or go for it now?

[toxieainc]

Also one more thing: In the whitestar global volume handling you have processor->setGlobalVol(std::min((float)cmd_buffer[1] / 127.f, 1.0f)); My code had (float)(0x2F-cmd_in) / 31.f for the volume. Copy paste error or intended?