Fix beatmap skin sample lookups falling back to non-custom sample banks if the custom bank sample was not found #33908
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ks if the custom sample bank sample was not found Closes ppy#33900. I think. Stable's sample lookup logic is horrible. The user in the issue claimed they were hearing `drum-hitfinish2`, but they were really hearing `drum-hitfinish`, because they're the same `.wav` file in the beatmap. Now the reason *why* they were hearind `drum-hitfinish` is that the sample control point was specifying something like: 23946,-200,4,2,4,40,0,0 To decipher, this is: - default sample bank of soft - custom sample bank of 4 Taking one of the objects affected, namely 00:23:946 (2) - that's a slider with finish addition and drum addition bank on the slider head. The slider head is thus attempting to play `soft-hitnormal4` and `drum-hitfinish4`. Neither `soft-hitnormal4` or `soft-hitnormal` exist in the beatmap, so that plays fine via falling back to user skin's `soft-hitnormal`, but `drum-hitfinish4` ends up falling back to `drum-hitfinish` which *does* exist in the beatmap skin and thus plays wrongly from the beatmap skin rather than the user skin. I have no idea how to ensure this is correct across every beatmap and skin out there so my approach is to just spray and pray (and rely on issue reports I guess). I *think* this matches the stable logic which is nestled within https://github.com/peppy/osu-stable-reference/blob/a5e5fe6ef240505d13526cf32783cad261e9bd8b/osu!/Audio/AudioEngine.cs#L1136-L1230 but honestly if you put a gun to my head I couldn't be sure if it matches completely in every possible circumstance or not.
bdach
changed the title
Collaborator
Author
|
I've got to admit that my confidence in this change is even lower given this was failing actual tests, but I'm also not sure how much the tests in question were grounded in reality rather than back-engineered from the code we had at the time they were written, so I mostly just adjusted them to pass. I also added coverage of the apparent fail case here and here. |
peppy
approved these changes
Jun 30, 2025
Member
peppy
left a comment
peppy
left a comment
There was a problem hiding this comment.
As with any other sample change:
- Deploy
- Wait for users to find a new edge case
This was referenced Sep 5, 2025
This was referenced Oct 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #33900. I think. Stable's sample lookup logic is horrible.
The user in the issue claimed they were hearing
drum-hitfinish2, but they were really hearingdrum-hitfinish, because they're the same.wavfile in the beatmap. Now the reason why they were hearingdrum-hitfinishis that the sample control point was specifying something like:To decipher, this is:
Taking one of the objects affected, namely 00:23:946 (2) - that's a slider with finish addition and drum addition bank on the slider head. The slider head is thus attempting to play
soft-hitnormal4anddrum-hitfinish4.Neither
soft-hitnormal4orsoft-hitnormalexist in the beatmap, so that plays fine via falling back to user skin'ssoft-hitnormal, butdrum-hitfinish4ends up falling back todrum-hitfinishwhich does exist in the beatmap skin and thus plays wrongly from the beatmap skin rather than the user skin.I have no idea how to ensure this is correct across every beatmap and skin out there so my approach is to just spray and pray (and rely on issue reports I guess). I think this matches the stable logic which is nestled within here but honestly even if you put a gun to my head I couldn't be sure if it matches completely in every possible circumstance or not.