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

Fix Missing Custom Sequences #2649

Merged

Conversation

Malkierian
Copy link
Contributor

@Malkierian Malkierian commented Mar 25, 2023

Turns out there are sounds loaded into AudioCollection that aren't sequences, and the custom sequence loading code was assuming this wasn't the case. This modifies the custom sequence loading code to bypass these known vanilla items that are in AudioCollection blocking seqNum assignment.

Fixes #2566 but will change indexes assigned with Audio Manager while this problem existed.

Build Artifacts

…in AudioCollection to fix missing custom sequences on load.
Copy link
Contributor

@briaguya-ai briaguya-ai left a comment

Choose a reason for hiding this comment

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

Overall I'm super happy to see a fix for this! Left a couple suggestions and a few questions.

soh/src/code/audio_load.c Outdated Show resolved Hide resolved
soh/src/code/audio_load.c Outdated Show resolved Hide resolved
@@ -1366,16 +1368,23 @@ void AudioLoad_Init(void* heap, size_t heapSize) {
int startingSeqNum = MAX_AUTHENTIC_SEQID; // 109 is the highest vanilla sequence
Copy link
Contributor

Choose a reason for hiding this comment

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

is MAX_AUTHENTIC_SEQID a lie? are there authentic sequences with higher ids than 109?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the sequences end there, but AudioCollection has more than just sequences in it, like the sound fonts at 130-136 which were the original issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit cumbersome to have a sequence map in audio_load.c and AudioCollection, but I'm not sure how to unify them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So yeah, as mentioned elsewhere, it's more than just the instruments in there. the sound effects are added in as well later on.

soh/src/code/audio_load.c Outdated Show resolved Hide resolved
…ction`'s sequenceMap size to account for all audio assets already loaded into that sequenceMap. This gives a non-arbitrary number in addition to the vanilla sequence count to allocate with for `audio_load`'s sequenceMap.

Added `HasSequenceNum` to `AudioCollection` as well to streamline the check against `AudioCollection`'s sequenceMap to skip the non-sequence assets in there.

Added clarification comment for seqNum and MAX_AUTHENTIC_SEQID section.
Copy link
Contributor

@briaguya-ai briaguya-ai left a comment

Choose a reason for hiding this comment

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

Beautiful! :shipit:

@briaguya-ai briaguya-ai merged commit f7703e1 into HarbourMasters:develop-khan Apr 1, 2023
@Malkierian Malkierian deleted the missing-sequences branch April 1, 2023 04:13
@PurpleHato PurpleHato mentioned this pull request Apr 2, 2023
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.

None yet

2 participants