Avoid accounting for the pause pitch adjust effect when "fixing" hardware offset adjustments

[peppy]

Bit of an unfortunate one. Because we are applying the pitch adjustment to the lowest level (Track), it's hard to filter out in parent clock calculations.

Tried a few solutions but this feels the best. Note that we can't just undo the pauseFreqAdjust adjustment as it will div-by-zero.

Closes #14773.

[peppy]

I'm going to block temporarily as there is some cross-over in what this is fixing and #14361, which looks to be addressing an issue with a similar cause. Potentially a combined or similar solution can be implemented there as well.

Would still appreciate feedback on the direction of this PR

[frenzibyte]

This actually may be somewhat good as an alternative to #14361 with no complexity in solution.

I can still imagine cases like FailAnimation track adjustment affecting the current time (which #14361 can handle), but it's not relevant for now I believe.

[frenzibyte]

The only case I'm afraid of with this way is if CurrentTime gets accessed before the first call to ProcessFrame, where seeking to a certain position would apply the offset inversely in MasterGameplayClockContainer.Seek() but would still not have CurrentTime correct due to offsetAdjust still being zero.

That should theortically be the case in MultiSpectatorScreen, as it stops the clock before the first Update call to the gameplay clock container where the clock would receive the first ProcessFrame (and even if it gets it somehow, it will still be blocked by pauseRateAdjust.Value already being not one):

osu/osu.Game/Screens/OnlinePlay/Multiplayer/Spectate/MultiSpectatorScreen.cs

Lines 150 to 159 in 9bacc0d

	protected override void LoadComplete()
	{
	base.LoadComplete();
	masterClockContainer.Reset();
	masterClockContainer.Stop();
	syncManager.ReadyToStart += onReadyToStart;
	syncManager.MasterState.BindValueChanged(onMasterStateChanged, true);
	}

Changing Seek so that it also has this offsetAdjust logic might be one way to solve this, however.

[smoogipoo]

Has this issue been forgotten?

[peppy]

Not forgotten, but I'm also not sure if valid.. I'll need to read this comment multiple more times to understand the implications. Having a test scene covering the issue/regression would definitely be helpful.

[bdach]

I didn't forget about this review comment. I just don't understand what it's trying to say or what a practical scenario for reproducing such a failure would be.

[frenzibyte]

Coming back to this, it turns out the point I brought isn't actually correct. To put it in code rather than words:

Add(clock = new MasterGameplayClockContainer());

// ensure gameplay clock in stopped state so offsetAdjust doesn't get updated.
clock.Stop();
clock.Reset();

// set arbitrary offset
config.SetValue(OsuSetting.AudioOffset, 200.0);

clock.Seek(2500);
// initial thought: CurrentTime = 2300
// actual and correct: CurrentTime = 2500
clock.Seek(5000);
// initial thought: CurrentTime = 4800
// actual and correct: CurrentTime = 5000

What I was thinking at the point of #14776 (comment) is that when the GCC is stopped, the Seek calls will get incorrectly adjusted by the offset and the CurrentTime will be decreased by the offset from the requested time, as the HardwareCorrectionOffsetClock will never update offsetAdjust.

But what I didn't realize is that when offsetAdjust is never updated (i.e. offsetAdjust = 0), the offset will still be applied to the current time as if Rate is 1, rather than not applying anything (perhaps the naming is what threw me off in the first place 😛).

[bdach]

I 100% agree with this one over the other linked alternative because I could actually understand what this is doing in 15 minutes.

[peppy]

Tentatively unblocking this one as I'm not really ready to make any calls on the other PR. On a second read I don't think this PR adds too much new complexity and fixes a majority failure case, but I'm also fine with leaving it for now if anyone disagrees.

[smoogipoo]

I'm not 100% on this change, but let's get it in for the time being.