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

Adjust clock usage in line with framework changes #24885

Merged
merged 30 commits into from
Oct 6, 2023
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
6006517
Change `FramedBeatmapClock` to always be decoupled
peppy Sep 21, 2023
3f27be1
Replace most usages of `DecoupleableInterpolatingFramedClock`
peppy Sep 21, 2023
117cd74
Update usage of `DecoupleableInterpolatingFramedClock` in `FramedBeat…
peppy Sep 21, 2023
04e6ec8
Fix interpolation not being applied when `applyOffsets` is set
peppy Sep 21, 2023
21a2e27
Simplify some pieces of `FramedBeatmapClock`
peppy Sep 21, 2023
5f634f2
Remove unnecessary encapsulation workaround
peppy Sep 21, 2023
df08c4e
Disable decoupling for `OsuGameBase`'s beatmap implementation
peppy Sep 21, 2023
a451ab7
Remove hopefully-unnecessary workaround
peppy Sep 21, 2023
a1e2989
Remove second hopefully-unnecessary workaround
peppy Sep 21, 2023
0200b63
Add note about beatmap offset not being reapplied correctly on `Chang…
peppy Sep 21, 2023
59d6e67
Add missing `TestManualClock.Reset` implementation for safe measure
peppy Sep 21, 2023
586311d
Fix souce clock not always being transferred to `FramedBeatmapClock` …
peppy Sep 22, 2023
bf08fbe
Set source directly in `FramedBeatmapClock` ctor
peppy Sep 22, 2023
3cb928f
Add note about test not calling `ProcessFrame`
peppy Sep 22, 2023
6629a47
Fix `FramedBeatmapClock`'s source not being processed
peppy Sep 22, 2023
8367bb6
Don't apply decoupling to `SpectatorPlayerClock`s
peppy Sep 22, 2023
a3e4d19
Update tests which were not using an `IAdjustableClock` as `GameplayC…
peppy Sep 22, 2023
c40bd74
Update usages of `GameplayClockContainer` not using an adjustable source
peppy Sep 22, 2023
faa0481
Fix editor operating directly on track rather than decoupled clock
peppy Sep 22, 2023
251a85d
Fix `StopUsingBeatmapClock` not transferring time and running state
peppy Sep 22, 2023
d019cb5
Update in line with framed clock changes
peppy Sep 22, 2023
5a17a86
Merge branch 'master' into clock-fix-attempt-2
peppy Oct 4, 2023
a186d97
Update framework
peppy Oct 4, 2023
20342d4
Update framework
peppy Oct 4, 2023
f8455af
Fix double interpolation being applied when `applyOffsets=false`
peppy Oct 4, 2023
354193c
Merge branch 'master' into clock-fix-attempt-2
peppy Oct 6, 2023
1005b7a
Fix incorrect test assumption in `TestSceneEditorClock`
peppy Oct 5, 2023
f9021bf
Remove pointless test class in `TestSceneStoryboardSamples`
peppy Oct 5, 2023
10ce570
Update framework
bdach Oct 6, 2023
5ea45e3
Rename local variable
bdach Oct 6, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion osu.Android.props
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
<EmbedAssembliesIntoApk>true</EmbedAssembliesIntoApk>
</PropertyGroup>
<ItemGroup>
<PackageReference Include="ppy.osu.Framework.Android" Version="2023.922.0" />
<PackageReference Include="ppy.osu.Framework.Android" Version="2023.1006.0" />
</ItemGroup>
<PropertyGroup>
<!-- Fody does not handle Android build well, and warns when unchanged.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public partial class TestSceneDrumSampleTriggerSource : OsuTestScene
[SetUp]
public void SetUp() => Schedule(() =>
{
gameplayClock = new GameplayClockContainer(manualClock)
gameplayClock = new GameplayClockContainer(manualClock, false, false)
{
RelativeSizeAxes = Axes.Both,
Children = new Drawable[]
Expand Down
12 changes: 2 additions & 10 deletions osu.Game.Tests/Gameplay/TestSceneStoryboardSamples.cs
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ public void TestSampleHasLifetimeEndWithInitialClockTime()
public void TestSamplePlaybackWithBeatmapHitsoundsOff()
{
GameplayClockContainer gameplayContainer = null;
TestDrawableStoryboardSample sample = null;
DrawableStoryboardSample sample = null;

AddStep("disable beatmap hitsounds", () => config.SetValue(OsuSetting.BeatmapHitsounds, false));

Expand All @@ -141,7 +141,7 @@ public void TestSamplePlaybackWithBeatmapHitsoundsOff()
Child = beatmapSkinSourceContainer
});

beatmapSkinSourceContainer.Add(sample = new TestDrawableStoryboardSample(new StoryboardSampleInfo("test-sample", 1, 1))
beatmapSkinSourceContainer.Add(sample = new DrawableStoryboardSample(new StoryboardSampleInfo("test-sample", 1, 1))
{
Clock = gameplayContainer
});
Expand Down Expand Up @@ -199,14 +199,6 @@ public TestCustomSkinWorkingBeatmap(RulesetInfo ruleset, IStorageResourceProvide
protected internal override ISkin GetSkin() => new TestSkin("test-sample", resources);
}

private partial class TestDrawableStoryboardSample : DrawableStoryboardSample
{
public TestDrawableStoryboardSample(StoryboardSampleInfo sampleInfo)
: base(sampleInfo)
{
}
}

#region IResourceStorageProvider

public IRenderer Renderer => host.Renderer;
Expand Down
7 changes: 4 additions & 3 deletions osu.Game.Tests/NonVisual/GameplayClockContainerTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using NUnit.Framework;
using osu.Framework.Audio;
using osu.Framework.Audio.Track;
using osu.Framework.Bindables;
using osu.Framework.Timing;
using osu.Game.Screens.Play;
Expand All @@ -16,16 +17,16 @@ public partial class GameplayClockContainerTest
[TestCase(1)]
public void TestTrueGameplayRateWithGameplayAdjustment(double underlyingClockRate)
{
var framedClock = new FramedClock(new ManualClock { Rate = underlyingClockRate });
var framedClock = new TrackVirtual(60000) { Frequency = { Value = underlyingClockRate } };
var gameplayClock = new TestGameplayClockContainer(framedClock);

Assert.That(gameplayClock.GetTrueGameplayRate(), Is.EqualTo(2));
}

private partial class TestGameplayClockContainer : GameplayClockContainer
{
public TestGameplayClockContainer(IFrameBasedClock underlyingClock)
: base(underlyingClock)
public TestGameplayClockContainer(IClock underlyingClock)
: base(underlyingClock, false, false)
{
AdjustmentsFromMods.AddAdjustment(AdjustableProperty.Frequency, new BindableDouble(2.0));
}
Expand Down
7 changes: 6 additions & 1 deletion osu.Game.Tests/OnlinePlay/TestSceneCatchUpSyncManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@

namespace osu.Game.Tests.OnlinePlay
{
// NOTE: This test scene never calls ProcessFrame on clocks.
// The current tests are fine without this as they are testing very static scenarios, but it's worth knowing
// if adding further tests to this class.
[HeadlessTest]
public partial class TestSceneCatchUpSyncManager : OsuTestScene
{
Expand All @@ -28,7 +31,7 @@ public partial class TestSceneCatchUpSyncManager : OsuTestScene
[SetUp]
public void Setup()
{
syncManager = new SpectatorSyncManager(master = new GameplayClockContainer(new TestManualClock()));
syncManager = new SpectatorSyncManager(master = new GameplayClockContainer(new TestManualClock(), false, false));
player1 = syncManager.CreateManagedClock();
player2 = syncManager.CreateManagedClock();

Expand Down Expand Up @@ -188,6 +191,8 @@ public bool Seek(double position)

public void Reset()
{
IsRunning = false;
CurrentTime = 0;
}

public void ResetSpeedAdjustments()
Expand Down
2 changes: 1 addition & 1 deletion osu.Game.Tests/Visual/Editing/TestSceneEditorClock.cs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public void TestStopAtTrackEnd()
AddStep("seek near end", () => EditorClock.Seek(EditorClock.TrackLength - 250));
AddUntilStep("clock stops", () => !EditorClock.IsRunning);

AddUntilStep("clock stopped at end", () => EditorClock.CurrentTime - EditorClock.TotalAppliedOffset, () => Is.EqualTo(EditorClock.TrackLength));
AddUntilStep("clock stopped at end", () => EditorClock.CurrentTime, () => Is.EqualTo(EditorClock.TrackLength));

AddStep("start clock again", () => EditorClock.Start());
AddAssert("clock looped to start", () => EditorClock.IsRunning && EditorClock.CurrentTime < 500);
Expand Down
4 changes: 2 additions & 2 deletions osu.Game.Tests/Visual/Gameplay/TestSceneHUDOverlay.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@
using System.Linq;
using NUnit.Framework;
using osu.Framework.Allocation;
using osu.Framework.Audio.Track;
using osu.Framework.Extensions.ObjectExtensions;
using osu.Framework.Graphics;
using osu.Framework.Graphics.Containers;
using osu.Framework.Testing;
using osu.Framework.Timing;
using osu.Game.Configuration;
using osu.Game.Graphics.Containers;
using osu.Game.Rulesets.Mods;
Expand Down Expand Up @@ -41,7 +41,7 @@ public partial class TestSceneHUDOverlay : OsuManualInputManagerTestScene
private GameplayState gameplayState = TestGameplayState.Create(new OsuRuleset());

[Cached(typeof(IGameplayClock))]
private readonly IGameplayClock gameplayClock = new GameplayClockContainer(new FramedClock());
private readonly IGameplayClock gameplayClock = new GameplayClockContainer(new TrackVirtual(60000), false, false);

// best way to check without exposing.
private Drawable hideTarget => hudOverlay.ChildrenOfType<SkinComponentsContainer>().First();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@
#nullable disable

using osu.Framework.Allocation;
using osu.Framework.Audio.Track;
using osu.Framework.Graphics;
using osu.Framework.Graphics.Containers;
using osu.Framework.Testing;
using osu.Framework.Timing;
using osu.Game.Overlays.SkinEditor;
using osu.Game.Rulesets;
using osu.Game.Rulesets.Osu;
Expand All @@ -32,7 +32,7 @@ public partial class TestSceneSkinEditorMultipleSkins : SkinnableTestScene
private GameplayState gameplayState = TestGameplayState.Create(new OsuRuleset());

[Cached(typeof(IGameplayClock))]
private readonly IGameplayClock gameplayClock = new GameplayClockContainer(new FramedClock());
private readonly IGameplayClock gameplayClock = new GameplayClockContainer(new TrackVirtual(60000), false, false);

[Cached]
public readonly EditorClipboard Clipboard = new EditorClipboard();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@
using System.Linq;
using NUnit.Framework;
using osu.Framework.Allocation;
using osu.Framework.Audio.Track;
using osu.Framework.Extensions.IEnumerableExtensions;
using osu.Framework.Graphics;
using osu.Framework.Graphics.Containers;
using osu.Framework.Testing;
using osu.Framework.Timing;
using osu.Game.Rulesets;
using osu.Game.Rulesets.Mods;
using osu.Game.Rulesets.Osu;
Expand All @@ -39,7 +39,7 @@ public partial class TestSceneSkinnableHUDOverlay : SkinnableTestScene
private GameplayState gameplayState = TestGameplayState.Create(new OsuRuleset());

[Cached(typeof(IGameplayClock))]
private readonly IGameplayClock gameplayClock = new GameplayClockContainer(new FramedClock());
private readonly IGameplayClock gameplayClock = new GameplayClockContainer(new TrackVirtual(60000), false, false);

private IEnumerable<HUDOverlay> hudOverlays => CreatedDrawables.OfType<HUDOverlay>();

Expand Down
4 changes: 1 addition & 3 deletions osu.Game.Tests/Visual/Gameplay/TestSceneStoryboard.cs
Original file line number Diff line number Diff line change
Expand Up @@ -106,14 +106,12 @@ private void loadStoryboard(Storyboard toLoad)
if (storyboard != null)
storyboardContainer.Remove(storyboard, true);

var decoupledClock = new DecoupleableInterpolatingFramedClock { IsCoupled = true };
storyboardContainer.Clock = decoupledClock;
storyboardContainer.Clock = new FramedClock(Beatmap.Value.Track);

storyboard = toLoad.CreateDrawable(SelectedMods.Value);
storyboard.Passing = false;

storyboardContainer.Add(storyboard);
decoupledClock.ChangeSource(Beatmap.Value.Track);
}

private void loadStoryboard(string filename, Action<Storyboard>? setUpStoryboard = null)
Expand Down
66 changes: 19 additions & 47 deletions osu.Game/Beatmaps/FramedBeatmapClock.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
using System.Diagnostics;
using osu.Framework;
using osu.Framework.Allocation;
using osu.Framework.Audio.Track;
using osu.Framework.Bindables;
using osu.Framework.Graphics;
using osu.Framework.Timing;
Expand All @@ -28,16 +27,6 @@ public partial class FramedBeatmapClock : Component, IFrameBasedClock, IAdjustab
{
private readonly bool applyOffsets;

/// <summary>
/// The length of the underlying beatmap track. Will default to 60 seconds if unavailable.
/// </summary>
public double TrackLength => Track.Length;

/// <summary>
/// The underlying beatmap track, if available.
/// </summary>
public Track Track { get; private set; } = new TrackVirtual(60000);

/// <summary>
/// The total frequency adjustment from pause transforms. Should eventually be handled in a better way.
/// </summary>
Expand All @@ -53,7 +42,7 @@ public partial class FramedBeatmapClock : Component, IFrameBasedClock, IAdjustab

private IDisposable? beatmapOffsetSubscription;

private readonly DecoupleableInterpolatingFramedClock decoupledClock;
private readonly DecouplingFramedClock decoupledTrack;

[Resolved]
private OsuConfigManager config { get; set; } = null!;
Expand All @@ -66,25 +55,21 @@ public partial class FramedBeatmapClock : Component, IFrameBasedClock, IAdjustab

public bool IsRewinding { get; private set; }

public bool IsCoupled
{
get => decoupledClock.IsCoupled;
set => decoupledClock.IsCoupled = value;
}

public FramedBeatmapClock(bool applyOffsets = false)
public FramedBeatmapClock(bool applyOffsets, bool requireDecoupling, IClock? source = null)
{
this.applyOffsets = applyOffsets;

// A decoupled clock is used to ensure precise time values even when the host audio subsystem is not reporting
decoupledTrack = new DecouplingFramedClock(source) { AllowDecoupling = requireDecoupling };

// An interpolating clock is used to ensure precise time values even when the host audio subsystem is not reporting
// high precision times (on windows there's generally only 5-10ms reporting intervals, as an example).
decoupledClock = new DecoupleableInterpolatingFramedClock { IsCoupled = true };
var interpolatedTrack = new InterpolatingFramedClock(decoupledTrack);

if (applyOffsets)
{
// Audio timings in general with newer BASS versions don't match stable.
// This only seems to be required on windows. We need to eventually figure out why, with a bit of luck.
platformOffsetClock = new OffsetCorrectionClock(decoupledClock, ExternalPauseFrequencyAdjust) { Offset = RuntimeInfo.OS == RuntimeInfo.Platform.Windows ? 15 : 0 };
platformOffsetClock = new OffsetCorrectionClock(interpolatedTrack, ExternalPauseFrequencyAdjust) { Offset = RuntimeInfo.OS == RuntimeInfo.Platform.Windows ? 15 : 0 };

// User global offset (set in settings) should also be applied.
userGlobalOffsetClock = new OffsetCorrectionClock(platformOffsetClock, ExternalPauseFrequencyAdjust);
Expand All @@ -94,7 +79,7 @@ public FramedBeatmapClock(bool applyOffsets = false)
}
else
{
finalClockSource = decoupledClock;
finalClockSource = interpolatedTrack;
}
}

Expand All @@ -110,6 +95,7 @@ protected override void LoadComplete()
userAudioOffset = config.GetBindable<double>(OsuSetting.AudioOffset);
userAudioOffset.BindValueChanged(offset => userGlobalOffsetClock.Offset = offset.NewValue, true);

// TODO: this doesn't update when using ChangeSource() to change beatmap.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be an observation about the current state of affairs made in passing, rather than a TODO added after realising something in this pull broke this, so I'm ignoring it.

beatmapOffsetSubscription = realm.SubscribeToPropertyChanged(
r => r.Find<BeatmapInfo>(beatmap.Value.BeatmapInfo.ID)?.UserSettings,
settings => settings.Offset,
Expand All @@ -124,17 +110,7 @@ protected override void Update()
{
base.Update();

if (Source != null && Source is not IAdjustableClock && Source.CurrentTime < decoupledClock.CurrentTime - 100)
{
// InterpolatingFramedClock won't interpolate backwards unless its source has an ElapsedFrameTime.
// See https://github.com/ppy/osu-framework/blob/ba1385330cc501f34937e08257e586c84e35d772/osu.Framework/Timing/InterpolatingFramedClock.cs#L91-L93
// This is not always the case here when doing large seeks.
// (Of note, this is not an issue if the source is adjustable, as the source is seeked to be in time by DecoupleableInterpolatingFramedClock).
// Rather than trying to get around this by fixing the framework clock stack, let's work around it for now.
Seek(Source.CurrentTime);
}
else
finalClockSource.ProcessFrame();
finalClockSource.ProcessFrame();

if (Clock.ElapsedFrameTime != 0)
IsRewinding = Clock.ElapsedFrameTime < 0;
Expand All @@ -157,46 +133,42 @@ public double TotalAppliedOffset

#region Delegation of IAdjustableClock / ISourceChangeableClock to decoupled clock.

public void ChangeSource(IClock? source)
{
Track = source as Track ?? new TrackVirtual(60000);
decoupledClock.ChangeSource(source);
}
public void ChangeSource(IClock? source) => decoupledTrack.ChangeSource(source);

public IClock? Source => decoupledClock.Source;
public IClock Source => decoupledTrack.Source;

public void Reset()
{
decoupledClock.Reset();
decoupledTrack.Reset();
finalClockSource.ProcessFrame();
}

public void Start()
{
decoupledClock.Start();
decoupledTrack.Start();
finalClockSource.ProcessFrame();
}

public void Stop()
{
decoupledClock.Stop();
decoupledTrack.Stop();
finalClockSource.ProcessFrame();
}

public bool Seek(double position)
{
bool success = decoupledClock.Seek(position - TotalAppliedOffset);
bool success = decoupledTrack.Seek(position - TotalAppliedOffset);
finalClockSource.ProcessFrame();

return success;
}

public void ResetSpeedAdjustments() => decoupledClock.ResetSpeedAdjustments();
public void ResetSpeedAdjustments() => decoupledTrack.ResetSpeedAdjustments();

public double Rate
{
get => decoupledClock.Rate;
set => decoupledClock.Rate = value;
get => decoupledTrack.Rate;
set => decoupledTrack.Rate = value;
}

#endregion
Expand Down
13 changes: 2 additions & 11 deletions osu.Game/OsuGameBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ public virtual string Version
/// For now, this is used as a source specifically for beat synced components.
/// Going forward, it could potentially be used as the single source-of-truth for beatmap timing.
/// </summary>
private readonly FramedBeatmapClock beatmapClock = new FramedBeatmapClock(true);
private readonly FramedBeatmapClock beatmapClock = new FramedBeatmapClock(applyOffsets: true, requireDecoupling: false);

protected override Container<Drawable> Content => content;

Expand Down Expand Up @@ -441,16 +441,7 @@ private void addFilesWarning()
}
}

private void onTrackChanged(WorkingBeatmap beatmap, TrackChangeDirection direction)
{
// FramedBeatmapClock uses a decoupled clock internally which will mutate the source if it is an `IAdjustableClock`.
// We don't want this for now, as the intention of beatmapClock is to be a read-only source for beat sync components.
//
// Encapsulating in a FramedClock will avoid any mutations.
var framedClock = new FramedClock(beatmap.Track);

beatmapClock.ChangeSource(framedClock);
}
private void onTrackChanged(WorkingBeatmap beatmap, TrackChangeDirection direction) => beatmapClock.ChangeSource(beatmap.Track);

protected virtual void InitialiseFonts()
{
Expand Down
2 changes: 1 addition & 1 deletion osu.Game/Screens/Edit/Editor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -862,7 +862,7 @@ protected void SetPreviewPointToCurrentTime()

private void resetTrack(bool seekToStart = false)
{
Beatmap.Value.Track.Stop();
clock.Stop();

if (seekToStart)
{
Expand Down
Loading