Do not use beatmap hitsounds on timing points without custom sample bank

[Fire937]

Resolves #8528

It seems the problem mentioned in this issue stems from the .osu file not being interpreted correctly. The author of this specific beatmap wanted the default hitsound to be played but the custom hitsound is played instead. That explains why we can only hear it through the left speaker. The sound files hitnormal-normal.wav and hitnormal-normal2.wav are both successfully played at certain points in the map where they are meant to be played.

This PR adds an attribute to HitSampleInfo in order to be aware when the sample to be played should be osu!'s default hitsound (as opposed to the beatmap's sound files).

[bdach]

Thanks for the contribution, but I'm reasonably certain that this PR is incorrect in its direction. Checking on the map linked in the issue this PR is supposed to resolve, after applying this change normal-hitnormal.wav from the beatmap skin does not play as it is supposed to, and the lazer default skin hitsound plays instead.

As described in this comment in the issue, the desired resolution is to add support for hitnormal2, which that beatmap skin also provides, but which isn't currently played back on lazer.

(As an aside, the "fallback to default hitsound" logic is largely there as far as I can see - GetSample will already return null if the desired sample is not found in the files, so the IsCustom addition seems to be unnecessary.)

The xmldocs seem to be slightly wrong on master, though. Feel free to open that as a separate code quality pull request.

[Fire937]

Actually the files hitnormal and hitnormal2 are both played only for 2 circles in the beatmap in hard difficulty (they are 2m18s after the beginning of the song). All the other circles were supposed to play the default hitsounds. The hitsounds you hear in osu-stable aren't actually the 2 files in the beatmap (they do not sound like that) and the .osu goes in this direction.

I believe the bug was not happening in the other maps because all their files probably had a suffix (something like normal-hitnormal1 instead of normal-hitnormal), however i did not look into this further so those are just suppositions.

I agree though that adding this IsCustom attribute seems a bit awkward. I think a better solution would be to add the suffix 1 to sound files that don't have one, so that unsuffixed becomes reserved to osu default sound. I don't know if some other components would be affected by such a change though.

[bdach]

Thanks for the above explanation - I guess I stand corrected. Personally I'd have probably included it in the PR description to begin with to minimise confusion.

[Fire937]

You're totally right, i definitely wasn't detailed enough in my PR description. I edited it accordingly and will remember this for later PRs.

Now i am not sure how i can interpret this code inspect error.
Invoke as extension method in ..\Skinning\SkinnableSound.cs on line 30

[LittleEndu]

You can Google all the inspection errors, you'll get a result from jetbrains with more details. You should also run the inspection yourself. There's a script in the root of the repo

[bdach]

Now that I can see what the issue is exactly, I think there's still some work to be done here. To begin with, the tuple should not exist; the easiest way to deal with that would be to include IsCustom to ISampleInfo, but I'm not sure it makes sense from an interface standpoint. I'd say ideally this case should be handled somewhere near or inside LegacySkin.

There's also another matter - there is an existing test that seems to test exactly the behaviour that this PR intends to avoid:

osu/osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapDecoderTest.cs

Lines 420 to 436 in 19ce3eb

	[Test]
	public void TestDecodeHitObjectCustomSampleBank()
	{
	var decoder = new LegacyBeatmapDecoder { ApplyOffsets = false };
	using (var resStream = TestResources.OpenResource("hitobject-custom-samplebank.osu"))
	using (var stream = new LineBufferedReader(resStream))
	{
	var hitObjects = decoder.Decode(stream).HitObjects;
	Assert.AreEqual("normal-hitnormal", getTestableSampleInfo(hitObjects[0]).LookupNames.First());
	Assert.AreEqual("normal-hitnormal2", getTestableSampleInfo(hitObjects[1]).LookupNames.First());
	Assert.AreEqual("normal-hitnormal3", getTestableSampleInfo(hitObjects[2]).LookupNames.First());
	}
	static HitSampleInfo getTestableSampleInfo(HitObject hitObject) => hitObject.SampleControlPoint.ApplyTo(hitObject.Samples[0]);
	}

The first hitobject with a custom sample bank of 0 is expected to be associated with normal-hitnormal, which is exactly the cause of the problem here. 100% parity with stable would probably have to be verified by looking at its source and checking what the final behaviour should be, which I'm not in a position to do.

[Fire937]

Thank you for you advice, as you say handling the case in LegacySkin makes much more sense.

About the test you mentioned, this PR should not affect its result as the return value of LookupNames remains unchanged.

[bdach]

While you're correct that the fix won't affect that particular test, the reason it won't is because this particular fix is applied after decoding. The thing that has me wondering is that this assertion was written like this in the first place, and I'm wondering whether it was an oversight and cases such as this one were not known / missed at the time of writing it, or that behaviour also is relied on somewhere and we have a contradiction.

[Fire937]

My understanding is that a sample index of 1 is always associated with a file that has no suffix, the suffixes are applied only for indexes strictly greater than 1. So normal-hitnormal1.wav and normal-hitnormal0.wav do not exist.

I think the reason why this issue was never noticed before is probably because it is most of the time hard to notice the difference between the custom sounds and the default sounds if we don't pay attention to it. The fact this one particular beatmap had the sound panned far in a single direction made it easy to understand something was wrong.

[bdach]

As I said in the other PR: I'd like to see tests for the behaviour introduced here too, to make sure it doesn't regress with other unrelated changes in the future.

[Fire937]

@bdach I added tests for beatmap decoding. They make sure the IsCustom flag is correctly set according to the timing point's sample index option. I also included tests for GetSample in the other PR (#8683) to make sure the samples correctly fallback to default when the flag is not set.