Default to normal bank if invalid sample bank is specified

[sw1tchbl4d3r]

This is a small fix for maps like https://osu.ppy.sh/beatmapsets/1401944#taiko/2892286, which specify an invalid sample bank.
One of the offending lines in this map is 256,192,4447,1,8,41:0:0:0: at 00:04:447.
This specifies sample bank 41, which is not part of the enum, and as such leads to no hitsound when played.
Stable defaults it to the none sample bank so we might as well do the same.
Let me know if there's a test needed for this, I just thought this isn't worth one.

[bdach]

Let me know if there's a test needed for this, I just thought this isn't worth one.

I disagree, weird edge cases like this are what tests are made for. Please add one.

[bdach]

Okay, well, I investigated with stable, and some of the assertions in the OP do not hold against that.

First of all, there is no explicit IsDefined() check anywhere in the beatmap parsing code:

                                        if (split.Length > 5 && split[5].Length > 0)
                                        {
                                            string[] ss = split[5].Split(':');
                                            sampleSet = (SampleSet)Convert.ToInt32(ss[0]);
                                            addSampleSet = (SampleSet)Convert.ToInt32(ss[1]);
                                            customSample = ss.Length > 2 ? (CustomSampleSet)Convert.ToInt32(ss[2]) : CustomSampleSet.Default;
                                            volume = ss.Length > 3 ? Int32.Parse(ss[3]) : 0;
                                            sampleFile = ss.Length > 4 ? ss[4] : string.Empty;
                                        }

                                        HitCircle h = hitFactory.CreateHitCircle(pos, time, forceNewCombo | new_combo, soundType, new_combo ? combo_offset : 0, sampleSet, addSampleSet, customSample, volume, sampleFile);
                                        AddCircle(h);

(source)

The undefined SampleSet enum is allowed to freely propagate around the codebase from that point on. What happens later appears to be highly dependent on the ruleset. I've actually ran into this before.

For instance, /b/1803763 (a standard map) uses sample bank 43 on the first object. This sample bank is allowed to live without a care in the world, until it meets AudioEngine, at which point this happens:

            switch (sampleSet)
            {
                case SampleSet.Normal:
                default:
                    sample = 0;
                    break;
                case SampleSet.None:
                case SampleSet.Soft:
                    sample = 1;
                    break;
                case SampleSet.Drum:
                    sample = 2;
                    break;
            }

(source).

Note that the "undefined" enum case is covered by the default case label here, which means that an undefined sample set is coerced to normal, not to none.

The taiko case mentioned in the OP is more complicated, because taiko's sample handling is the worst more complicated. It appears that taiko on stable doesn't... seem to care? which sample set the object has defined, and just uses the ambient CurrentSampleSet:

            if (InputManager.rightButton1i || InputManager.rightButton2i)
            {
                ControlPoint p = hitObjectManager.Beatmap.ControlPointAt(AudioEngine.Time + 2);
                HitSoundInfo hitSoundInfo = new HitSoundInfo(HitObjectSoundType.Clap, AudioEngine.CurrentSampleSet, AudioEngine.CustomSamples, p.Volume, AudioEngine.CurrentSampleSet);
                AudioEngine.PlayHitSamples(hitSoundInfo, InputManager.rightButton1i ? -0.2f : 0.2f, true);

(source)

which means that the 41 thing actually never got read by anything that would go near sample playback in taiko.

All in all, I'm gonna need to hear an explanation or some support for the assertions in the OP of this pull, because they appear to be incorrect.

[bdach]

as above

[sw1tchbl4d3r]

The assertions in OP aren't really assertions, rather behavior I've discovered in the editor and an educated guess, since I don't have access to the stable source code, I will admit the wording is subpar. The editor just set the Sampleset to Normal and Default, which made me think that there is a default: case as mentioned above for all invalid values and the 0 case, and I assumed this was fixed in parsing because the beatmap editor picks this case up as well.

[sw1tchbl4d3r]

Even though it may not match with stable, I still feel like this is probably the way to go, maybe with a change to a default of normal instead of none.
It's a simple and sane-looking fix, which doesn't even let the broken value propagate in the first place, and the effort of mimicking stable when this one already fixes the tiny subset of maps with this issue is probably not worth it, especially in the weird taiko case which works differently in lazer and doesn't play the sound at all. It'll break encode-decode stability, sure, but since the source map is a "broken" map in the first place I don't see an issue with this.

[bdach]

Not sure what the point of the comment above is. The review process will determine as to whether this is correct and whether we want it.

I'll need to run checks against beatmaps that use these pathological sample bank types and determine whether applying the fix at decoder level is correct. I'm not fundamentally against it but if we're putting in effort to fix a stable discrepancy, we should fix it 100% and not go back to it ever again.

[bdach]

So I made a better test beatmap (sample test - sample test (spaceman_atlas) [sample test].zip) and basically experimentally confirmed the findings above, and applied a better fix for this.

Beatmap in question has a single timing point with drum bank set and 10 objects:

#	standard bank	additions bank	additions
1	auto (drum)	auto (drum)	none
2	normal	auto (drum)	none
3	soft	auto (drum)	none
4	drum	auto (drum)	none
5	undefined	auto (drum)	none
6	auto (drum)	auto (drum)	finish
7	auto (drum)	normal	finish
8	auto (drum)	soft	finish
9	auto (drum)	drum	finish
10	auto (drum)	undefined	finish

stable

2023-09-19.14-04-01.mp4

lazer, master

2023-09-19.14-06-15.mp4

lazer, this PR at 8f9cde0

2023-09-19.14-07-46.mp4

lazer, this PR at c4a0ca3

2023-09-19.14-09-32.mp4

Gonna request a second review since I essentially rewrote this entire pull.