Skip to content

Remove double Enabled bindable from ReadyButton #37117

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.

Sign up for GitHub

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

Open
LiquidPL wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove double Enabled bindable from ReadyButton #37117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
81 changes: 25 additions & 56 deletions osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerSpectateButton.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,48 +2,42 @@
// See the LICENCE file in the repository root for full licence text.

using System.Linq;
using Moq;
using NUnit.Framework;
using osu.Framework.Allocation;
using osu.Framework.Audio;
using osu.Framework.Extensions;
using osu.Framework.Extensions.ObjectExtensions;
using osu.Framework.Bindables;
using osu.Framework.Graphics;
using osu.Framework.Graphics.Containers;
using osu.Framework.Graphics.Cursor;
using osu.Framework.Platform;
using osu.Framework.Testing;
using osu.Game.Beatmaps;
using osu.Game.Graphics.UserInterface;
using osu.Game.Online.API.Requests.Responses;
using osu.Game.Online.Multiplayer;
using osu.Game.Online.Rooms;
using osu.Game.Rulesets;
using osu.Game.Screens.OnlinePlay;
using osu.Game.Screens.OnlinePlay.Multiplayer;
using osu.Game.Screens.OnlinePlay.Multiplayer.Match;
using osu.Game.Tests.Resources;
using osuTK;

namespace osu.Game.Tests.Visual.Multiplayer
{
public partial class TestSceneMultiplayerSpectateButton : MultiplayerTestScene
{
private readonly Bindable<BeatmapAvailability> availability = new Bindable<BeatmapAvailability>();
private readonly Mock<MultiplayerBeatmapAvailabilityTracker> availabilityTracker = new Mock<MultiplayerBeatmapAvailabilityTracker>();

private MultiplayerSpectateButton spectateButton = null!;
private MatchStartControl startControl = null!;
private Room room = null!;

private BeatmapSetInfo importedSet = null!;
private RulesetStore rulesets = null!;
private BeatmapManager beatmaps = null!;

[BackgroundDependencyLoader]
private void load(GameHost host, AudioManager audio)
{
Dependencies.Cache(rulesets = new RealmRulesetStore(Realm));
Dependencies.Cache(beatmaps = new BeatmapManager(LocalStorage, Realm, null, audio, Resources, host, Beatmap.Default));
Dependencies.Cache(Realm);

beatmaps.Import(TestResources.GetQuickTestBeatmapForImport()).WaitSafely();
availability.Value = BeatmapAvailability.LocallyAvailable();
availabilityTracker.SetupGet(a => a.Availability).Returns(availability);
Dependencies.CacheAs<OnlinePlayBeatmapAvailabilityTracker>(availabilityTracker.Object);
}

public override void SetUpSteps()
Expand All @@ -56,46 +50,29 @@ public override void SetUpSteps()

AddStep("create button", () =>
{
importedSet = beatmaps.GetAllUsableBeatmapSets().First();
Beatmap.Value = beatmaps.GetWorkingBeatmap(importedSet.Beatmaps.First());

MultiplayerBeatmapAvailabilityTracker tracker = new MultiplayerBeatmapAvailabilityTracker();

Child = new DependencyProvidingContainer
Child = new PopoverContainer
{
RelativeSizeAxes = Axes.Both,
CachedDependencies =
[
(typeof(OnlinePlayBeatmapAvailabilityTracker), tracker)
],
Children =
[
tracker,
new PopoverContainer
Child = new FillFlowContainer
{
AutoSizeAxes = Axes.Both,
Direction = FillDirection.Vertical,
Children = new Drawable[]
{
RelativeSizeAxes = Axes.Both,
Child = new FillFlowContainer
spectateButton = new MultiplayerSpectateButton
{
AutoSizeAxes = Axes.Both,
Direction = FillDirection.Vertical,
Children = new Drawable[]
{
spectateButton = new MultiplayerSpectateButton
{
Anchor = Anchor.Centre,
Origin = Anchor.Centre,
Size = new Vector2(200, 50)
},
startControl = new MatchStartControl
{
Anchor = Anchor.Centre,
Origin = Anchor.Centre,
Size = new Vector2(200, 50)
}
}
Anchor = Anchor.Centre,
Origin = Anchor.Centre,
Size = new Vector2(200, 50)
},
startControl = new MatchStartControl
{
Anchor = Anchor.Centre,
Origin = Anchor.Centre,
Size = new Vector2(200, 50)
}
}
]
}
};
});
}
Expand Down Expand Up @@ -164,13 +141,5 @@ private void assertSpectateButtonEnablement(bool shouldBeEnabled)

private void assertReadyButtonEnablement(bool shouldBeEnabled)
=> AddUntilStep($"ready button {(shouldBeEnabled ? "is" : "is not")} enabled", () => startControl.ChildrenOfType<MultiplayerReadyButton>().Single().Enabled.Value == shouldBeEnabled);

protected override void Dispose(bool isDisposing)
{
base.Dispose(isDisposing);

if (rulesets.IsNotNull())
rulesets.Dispose();
}
}
}
12 changes: 2 additions & 10 deletions osu.Game/Screens/OnlinePlay/Components/ReadyButton.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,29 +12,21 @@ namespace osu.Game.Screens.OnlinePlay.Components
{
public abstract partial class ReadyButton : RoundedButton
{
public new readonly BindableBool Enabled = new BindableBool();

private readonly IBindable<BeatmapAvailability> availability = new Bindable<BeatmapAvailability>();

[BackgroundDependencyLoader]
private void load(OnlinePlayBeatmapAvailabilityTracker beatmapTracker)
{
availability.BindTo(beatmapTracker.Availability);
availability.BindValueChanged(_ => updateState());

Enabled.BindValueChanged(_ => updateState(), true);
availability.BindValueChanged(_ => UpdateEnabledState());
}

private void updateState() =>
base.Enabled.Value = availability.Value.State == DownloadState.LocallyAvailable && Enabled.Value;
protected virtual void UpdateEnabledState() => Enabled.Value = availability.Value.State == DownloadState.LocallyAvailable;

public override LocalisableString TooltipText
{
get
{
if (base.Enabled.Value)
return string.Empty;

if (availability.Value.State != DownloadState.LocallyAvailable)
return "Beatmap not downloaded";

Expand Down
14 changes: 2 additions & 12 deletions osu.Game/Screens/OnlinePlay/Multiplayer/Match/MatchStartControl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ public partial class MatchStartControl : CompositeDrawable
[Resolved]
private MultiplayerClient client { get; set; } = null!;

private readonly MultiplayerReadyButton readyButton;
private readonly MultiplayerCountdownButton countdownButton;

private IBindable<bool> operationInProgress = null!;
Expand All @@ -58,7 +57,7 @@ public MatchStartControl()
{
new Drawable?[]
{
readyButton = new MultiplayerReadyButton
new MultiplayerReadyButton
{
RelativeSizeAxes = Axes.Both,
Size = Vector2.One,
Expand Down Expand Up @@ -179,7 +178,6 @@ private void updateState()
{
if (client.Room == null)
{
readyButton.Enabled.Value = false;
countdownButton.Enabled.Value = false;
return;
}
Expand Down Expand Up @@ -207,19 +205,11 @@ private void updateState()
}
}

readyButton.Enabled.Value = countdownButton.Enabled.Value =
countdownButton.Enabled.Value =
Copy link
Copy Markdown
Contributor Author

@LiquidPL LiquidPL Mar 26, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

If we were to go with this, then I suppose that MultiplayerCountdownButton should have its own Enabled updating logic moved inside its class as well.

client.Room.State != MultiplayerRoomState.Closed
&& !client.Room.CurrentPlaylistItem.Expired
&& !operationInProgress.Value;

// When the local user is the host and spectating the match, the ready button should be enabled only if any users are ready.
if (localUser?.State == MultiplayerUserState.Spectating)
readyButton.Enabled.Value &= client.IsHost && newCountReady > 0 && !client.Room.ActiveCountdowns.Any(c => c is MatchStartCountdown);

// When the local user is not the host, the button should only be enabled when no match is in progress.
if (!client.IsHost)
readyButton.Enabled.Value &= client.Room.State == MultiplayerRoomState.Open;

// At all times, the countdown button should only be enabled when no match is in progress.
countdownButton.Enabled.Value &= client.Room.State == MultiplayerRoomState.Open;

Expand Down
37 changes: 37 additions & 0 deletions osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using osu.Framework.Allocation;
using osu.Framework.Audio;
using osu.Framework.Audio.Sample;
using osu.Framework.Bindables;
using osu.Framework.Extensions.ObjectExtensions;
using osu.Framework.Localisation;
using osu.Framework.Threading;
Expand All @@ -18,12 +19,17 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match
{
public partial class MultiplayerReadyButton : ReadyButton
{
[Resolved]
private OngoingOperationTracker ongoingOperationTracker { get; set; } = null!;

[Resolved]
private MultiplayerClient multiplayerClient { get; set; } = null!;

[Resolved]
private OsuColour colours { get; set; } = null!;

private IBindable<bool> operationInProgress = null!;

private MultiplayerRoom? room => multiplayerClient.Room;

private Sample? countdownTickSample;
Expand All @@ -33,6 +39,9 @@ public partial class MultiplayerReadyButton : ReadyButton
[BackgroundDependencyLoader]
private void load(AudioManager audio)
{
operationInProgress = ongoingOperationTracker.InProgress.GetBoundCopy();
operationInProgress.BindValueChanged(_ => UpdateEnabledState());

countdownTickSample = audio.Samples.Get(@"Multiplayer/countdown-tick");
countdownWarnSample = audio.Samples.Get(@"Multiplayer/countdown-warn");
countdownWarnFinalSample = audio.Samples.Get(@"Multiplayer/countdown-warn-final");
Expand Down Expand Up @@ -64,6 +73,7 @@ private void onRoomUpdated() => Scheduler.AddOnce(() =>

updateButtonText();
updateButtonColour();
UpdateEnabledState();
});

private void scheduleNextCountdownUpdate()
Expand Down Expand Up @@ -113,6 +123,33 @@ private void playTickSound(int secondsRemaining)
}
}

protected override void UpdateEnabledState()
{
base.UpdateEnabledState();

if (room == null)
{
Enabled.Value = false;
return;
}

var localUser = multiplayerClient.LocalUser;

int newCountReady = room.Users.Count(u => u.State == MultiplayerUserState.Ready);

Enabled.Value &= room.State != MultiplayerRoomState.Closed
&& !room.CurrentPlaylistItem.Expired
&& !operationInProgress.Value;

// When the local user is the host and spectating the match, the ready button should be enabled only if any users are ready.
if (localUser?.State == MultiplayerUserState.Spectating)
Enabled.Value &= multiplayerClient.IsHost && newCountReady > 0 && !room.ActiveCountdowns.Any(c => c is MatchStartCountdown);

// When the local user is not the host, the button should only be enabled when no match is in progress.
if (!multiplayerClient.IsHost)
Enabled.Value &= room.State == MultiplayerRoomState.Open;
}

private void updateButtonText()
{
if (room == null)
Expand Down
11 changes: 8 additions & 3 deletions osu.Game/Screens/OnlinePlay/Playlists/PlaylistsReadyButton.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ protected override void LoadComplete()

room.PropertyChanged += onRoomPropertyChanged;
updateRoomUserScore();

Scheduler.AddDelayed(UpdateEnabledState, 1000, true);
UpdateEnabledState();
Comment on lines +51 to +52
Copy link
Copy Markdown
Contributor Author

@LiquidPL LiquidPL Mar 26, 2026

Choose a reason for hiding this comment

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

Changed this to update the state every second instead of frame while cleaning this up. Might be leaning too much towards premature optimization but I dunno.

}

private void onRoomPropertyChanged(object? sender, PropertyChangedEventArgs e)
Expand All @@ -63,13 +66,15 @@ private void updateRoomUserScore()
int remaining = room.MaxAttempts.Value - room.UserScore.PlaylistItemAttempts.Sum(a => a.Attempts);

hasRemainingAttempts = remaining > 0;

UpdateEnabledState();
}

protected override void Update()
protected override void UpdateEnabledState()
{
base.Update();
base.UpdateEnabledState();

Enabled.Value = hasRemainingAttempts && enoughTimeLeft();
Enabled.Value &= hasRemainingAttempts && enoughTimeLeft();
}

public override LocalisableString TooltipText
Expand Down
Loading