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 argon volume-aware hitsounds not correctly playing immediately after object placement #29970

Merged
merged 2 commits into from
Sep 27, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Sep 23, 2024

Closes #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:

private DrawablePool<PoolableSkinnableSample> prepareSamplePool(ISampleInfo sampleInfo)
{
if (samplePools.TryGetValue(sampleInfo, out var pool)) return pool;
AddInternal(samplePools[sampleInfo] = pool = new DrawableSamplePool(sampleInfo, 1));
return pool;
}

This means that for correct operation, ISampleInfo has to implement IEquatable<ISampleInfo> 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 IHitSampleInfos should never be allowed to alias with each other at all wrt equality.

…ter object placement

Closes ppy#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<ISampleInfo>` 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.
@peppy peppy merged commit 9a39b80 into ppy:master Sep 27, 2024
11 of 13 checks passed
@bdach bdach deleted the volume-aware-sample-equality-pitfall branch September 27, 2024 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

osu!std hitsounds playing in osu!taiko editor when setting don object volume anywhere from 60 to 85
2 participants