Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Original music #393

Merged
merged 7 commits into from
Jul 16, 2024
Merged

Original music #393

merged 7 commits into from
Jul 16, 2024

Conversation

dethrace-labs
Copy link
Owner

@dethrace-labs dethrace-labs commented Jul 12, 2024

  • Adds support for music as in the GOG release. That is, MUSIC/Track0[1-8].ogg files. If they exist, the original CD audio functions are redirected to play the ogg files.
  • Adds stb_vorbis to enable vorbis support in miniaudio
  • Switches miniaudio to use the default header-only implementation (to make it work with vorbis)
  • Fixes bug in sound options where the dials are both initially set to the music volume

Fixes #278

@dethrace-labs dethrace-labs changed the title adds support for GOG-format music ogg files Original music Jul 12, 2024
}
if (pCDA_id == 9999) {
do {
pCDA_id = gRandom_CDA_tunes[IRandomBetween(0, 7)];
Copy link
Collaborator

@madebr madebr Jul 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dethrace's IRandomBetween(0, 7) returns variable in the range [0, 7] (inclusive 0 and 7).
However, gRandom_CDA_tunes has length 7, with maximum index 6.

The MUSIC folder of my gog copy has 8 tracks (including a 4s silence track) so I think changing gRandom_CDA_tunes to int[8] makes sense.
In the Splat Pack, there are 3 tracks and 5 dummy (4s of silence) ones.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 7th item on my system is currently 0, which causes a stack overflow in S3GetDescriptorByID(0) -> S3GetDescriptorByID(0) -> S3GetDescriptorByID(0) -> ...

Copy link
Owner Author

@dethrace-labs dethrace-labs Jul 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate your second pair of eyes on this one! I originally made the change to [8], based on there being 8 tracks in SOUND.TXT, and seeing 9607 in IDA at the end of the array. When I saw the last track was silence, I figured it was deliberately left off and switched back to [7].

I didn't think about the IRandomBetween(0, 7) though! :)

I wonder if the original code was declared as 7 but initialized with 8 items like int gRandom_CDA_tunes[7] = { 9600, 9601, 9602, 9603, 9604, 9605, 9606, 9607 }; maybe

src/DETHRACE/common/sound.c Outdated Show resolved Hide resolved
dethrace-labs and others added 3 commits July 15, 2024 09:25
Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>
Copy link
Collaborator

@madebr madebr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure assert is the right approach in S3GetDescriptorByID because it is removed when building a Release build.
But LGTM!

@dethrace-labs
Copy link
Owner Author

I'm not sure assert is the right approach in S3GetDescriptorByID because it is removed when building a Release build. But LGTM!

That was my sort of my intention. The check is not present in the original code, but the assert could be helpful if we are working on the audio code in the future and break something

@dethrace-labs dethrace-labs merged commit 6e6ce12 into main Jul 16, 2024
6 checks passed
@dethrace-labs dethrace-labs deleted the cd_audio branch July 16, 2024 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No Music
2 participants