From 41826d0606d37e8d1b46bcad8e774d0b69af9521 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 23 Sep 2024 13:17:46 +0200 Subject: [PATCH 1/2] Add failing test case to demonstrate failure --- .../VolumeAwareHitSampleInfoTest.cs | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 osu.Game.Rulesets.Taiko.Tests/VolumeAwareHitSampleInfoTest.cs diff --git a/osu.Game.Rulesets.Taiko.Tests/VolumeAwareHitSampleInfoTest.cs b/osu.Game.Rulesets.Taiko.Tests/VolumeAwareHitSampleInfoTest.cs new file mode 100644 index 0000000000..2b3a922067 --- /dev/null +++ b/osu.Game.Rulesets.Taiko.Tests/VolumeAwareHitSampleInfoTest.cs @@ -0,0 +1,27 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using NUnit.Framework; +using osu.Game.Audio; +using osu.Game.Rulesets.Taiko.Skinning.Argon; + +namespace osu.Game.Rulesets.Taiko.Tests +{ + [TestFixture] + public class VolumeAwareHitSampleInfoTest + { + [Test] + public void TestVolumeAwareHitSampleInfoIsNotEqualToItsUnderlyingSample( + [Values(HitSampleInfo.HIT_NORMAL, HitSampleInfo.HIT_CLAP)] + string sample, + [Values(HitSampleInfo.BANK_NORMAL, HitSampleInfo.BANK_SOFT)] + string bank, + [Values(30, 70, 100)] int volume) + { + var underlyingSample = new HitSampleInfo(sample, bank, volume: volume); + var volumeAwareSample = new VolumeAwareHitSampleInfo(underlyingSample); + + Assert.That(underlyingSample, Is.Not.EqualTo(volumeAwareSample)); + } + } +} From e8a394f89485e61b37c61f095c7c9ed1c5c3b121 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 23 Sep 2024 13:27:36 +0200 Subject: [PATCH 2/2] Fix argon volume-aware hitsounds not correctly playing immediately after object placement Closes https://github.com/ppy/osu/issues/29832. The underlying reason for the incorrect sample playback was an equality comparer failure. Samples are contained in several pools which are managed by the playfield. In particular, the pools are keyed by `ISampleInfo` instances. This means that for correct operation, `ISampleInfo` has to implement `IEquatable` and also provide an appropriately correct `GetHashCode()` implementation. Different audible samples must not compare equal to each other when represented by `ISampleInfo`. As it turns out, `VolumeAwareHitSampleInfo` failed on this, due to not overriding equality members. Therefore, a `new HitSampleInfo(HitSampleInfo.HIT_NORMAL, HitSampleInfo.BANK_NORMAL, volume: 70)` was allowed to compare equal to a `VolumeAwareHitSampleInfo` wrapping it, *even though they correspond to completely different sounds and go through entirely different lookup path sequences*. Therefore, to fix, provide more proper equality implementations for `VolumeAwareHitSampleInfo`. When testing note that this issue *only occurs immediately after placing an object*. Saving and re-entering editor makes this issue go away. I haven't looked too long into why, but the general gist of it is ordering; it appears that a `normal-hitnormal` pool exists at point of query of a new object placement, but does not seem to exist when entering editor afresh. That said I'm not sure that ordering aspect of this bug matters much if at all, since the two `IHitSampleInfo`s should never be allowed to alias with each other at all wrt equality. --- .../Argon/VolumeAwareHitSampleInfo.cs | 20 +++++++++++++++++++ osu.Game/Audio/HitSampleInfo.cs | 2 +- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/osu.Game.Rulesets.Taiko/Skinning/Argon/VolumeAwareHitSampleInfo.cs b/osu.Game.Rulesets.Taiko/Skinning/Argon/VolumeAwareHitSampleInfo.cs index 3ca4b5a3c7..288ffde052 100644 --- a/osu.Game.Rulesets.Taiko/Skinning/Argon/VolumeAwareHitSampleInfo.cs +++ b/osu.Game.Rulesets.Taiko/Skinning/Argon/VolumeAwareHitSampleInfo.cs @@ -1,6 +1,7 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. +using System; using System.Collections.Generic; using osu.Game.Audio; @@ -48,5 +49,24 @@ namespace osu.Game.Rulesets.Taiko.Skinning.Argon return originalBank; } } + + public override bool Equals(HitSampleInfo? other) => other is VolumeAwareHitSampleInfo && base.Equals(other); + + /// + /// + /// This override attempts to match the override above, but in theory it is not strictly necessary. + /// Recall that must meet the following requirements: + /// + /// + /// "If two objects compare as equal, the method for each object must return the same value. + /// However, if two objects do not compare as equal, methods for the two objects do not have to return different values." + /// + /// + /// Making this override combine the value generated by the base implementation with a constant means + /// that and instances which have the same values of their members + /// will not have equal hash codes, which is slightly more efficient when these objects are used as dictionary keys. + /// + /// + public override int GetHashCode() => HashCode.Combine(base.GetHashCode(), 1); } } diff --git a/osu.Game/Audio/HitSampleInfo.cs b/osu.Game/Audio/HitSampleInfo.cs index f9c93d72ff..ce5e217532 100644 --- a/osu.Game/Audio/HitSampleInfo.cs +++ b/osu.Game/Audio/HitSampleInfo.cs @@ -96,7 +96,7 @@ namespace osu.Game.Audio public virtual HitSampleInfo With(Optional newName = default, Optional newBank = default, Optional newSuffix = default, Optional newVolume = default) => new HitSampleInfo(newName.GetOr(Name), newBank.GetOr(Bank), newSuffix.GetOr(Suffix), newVolume.GetOr(Volume)); - public bool Equals(HitSampleInfo? other) + public virtual bool Equals(HitSampleInfo? other) => other != null && Name == other.Name && Bank == other.Bank && Suffix == other.Suffix; public override bool Equals(object? obj)