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

Improve multiplayer room status handling #30838

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
20 changes: 9 additions & 11 deletions osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoom.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
using osu.Game.Online.API.Requests.Responses;
using osu.Game.Online.Multiplayer;
using osu.Game.Online.Rooms;
using osu.Game.Online.Rooms.RoomStatuses;
using osu.Game.Overlays;
using osu.Game.Rulesets.Osu;
using osu.Game.Screens.OnlinePlay.Lounge;
Expand Down Expand Up @@ -76,7 +75,6 @@ public void TestMultipleStatuses()
createLoungeRoom(new Room
{
Name = "Multiplayer room",
Status = new RoomStatusOpen(),
EndDate = DateTimeOffset.Now.AddDays(1),
Type = MatchType.HeadToHead,
Playlist = [item1],
Expand All @@ -85,7 +83,6 @@ public void TestMultipleStatuses()
createLoungeRoom(new Room
{
Name = "Private room",
Status = new RoomStatusOpenPrivate(),
Password = "*",
EndDate = DateTimeOffset.Now.AddDays(1),
Type = MatchType.HeadToHead,
Expand All @@ -95,36 +92,38 @@ public void TestMultipleStatuses()
createLoungeRoom(new Room
{
Name = "Playlist room with multiple beatmaps",
Status = new RoomStatusPlaying(),
Status = RoomStatus.Playing,
EndDate = DateTimeOffset.Now.AddDays(1),
Playlist = [item1, item2],
CurrentPlaylistItem = item1
}),
createLoungeRoom(new Room
{
Name = "Finished room",
Status = new RoomStatusEnded(),
Name = "Closing soon",
EndDate = DateTimeOffset.Now.AddSeconds(5),
}),
createLoungeRoom(new Room
{
Name = "Closed room",
EndDate = DateTimeOffset.Now,
}),
createLoungeRoom(new Room
{
Name = "Spotlight room",
Status = new RoomStatusOpen(),
Category = RoomCategory.Spotlight,
}),
createLoungeRoom(new Room
{
Name = "Featured artist room",
Status = new RoomStatusOpen(),
Category = RoomCategory.FeaturedArtist,
}),
}
};
});

AddUntilStep("wait for panel load", () => rooms.Count == 6);
AddUntilStep("wait for panel load", () => rooms.Count == 7);
AddUntilStep("correct status text", () => rooms.ChildrenOfType<OsuSpriteText>().Count(s => s.Text.ToString().StartsWith("Currently playing", StringComparison.Ordinal)) == 2);
AddUntilStep("correct status text", () => rooms.ChildrenOfType<OsuSpriteText>().Count(s => s.Text.ToString().StartsWith("Ready to play", StringComparison.Ordinal)) == 4);
AddUntilStep("correct status text", () => rooms.ChildrenOfType<OsuSpriteText>().Count(s => s.Text.ToString().StartsWith("Ready to play", StringComparison.Ordinal)) == 5);
}

[Test]
Expand All @@ -136,7 +135,6 @@ public void TestEnableAndDisablePassword()
AddStep("create room", () => Child = drawableRoom = createLoungeRoom(room = new Room
{
Name = "Room with password",
Status = new RoomStatusOpen(),
Type = MatchType.HeadToHead,
}));

Expand Down
41 changes: 0 additions & 41 deletions osu.Game.Tests/Visual/Playlists/TestScenePlaylistsRoomSubScreen.cs

This file was deleted.

21 changes: 21 additions & 0 deletions osu.Game/Graphics/OsuColour.cs
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,27 @@ public Color4 ForModType(ModType modType)
}
}

/// <summary>
/// Retrieves the accent colour representing a <see cref="Room"/>'s current status.
/// </summary>
public Color4 ForRoomStatus(Room room)
{
if (DateTimeOffset.Now >= room.EndDate)
Copy link
Member

Choose a reason for hiding this comment

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

Having this logic in OsuColour also weirds me out a bit. Maybe it's fine, but it's not really where I'd expect to find a date check.

Maybe it'll feel better if moved to a property on Room?

if (room.HasEnded)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit of a weird one because it needs to be queried periodically. But okay I'll do this for now as a pure code quality improvement.

return YellowDarker;

switch (room.Status)
{
case RoomStatus.Playing:
return Purple;

default:
if (room.HasPassword)
return GreenDark;

return GreenLight;
}
}

/// <summary>
/// Retrieves colour for a <see cref="RankingTier"/>.
/// See https://www.figma.com/file/YHWhp9wZ089YXgB7pe6L1k/Tier-Colours
Expand Down
34 changes: 34 additions & 0 deletions osu.Game/Localisation/RoomStatusPillStrings.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

using osu.Framework.Localisation;

namespace osu.Game.Localisation
{
public static class RoomStatusPillStrings
{
private const string prefix = @"osu.Game.Resources.Localisation.RoomStatusPill";

/// <summary>
/// "Ended"
/// </summary>
public static LocalisableString Ended => new TranslatableString(getKey(@"ended"), @"Ended");

/// <summary>
/// "Playing"
/// </summary>
public static LocalisableString Playing => new TranslatableString(getKey(@"playing"), @"Playing");

/// <summary>
/// "Open (Private)"
/// </summary>
public static LocalisableString OpenPrivate => new TranslatableString(getKey(@"open_private"), @"Open (Private)");

/// <summary>
/// "Open"
/// </summary>
public static LocalisableString Open => new TranslatableString(getKey(@"open"), @"Open");

private static string getKey(string key) => $@"{prefix}:{key}";
}
}
10 changes: 5 additions & 5 deletions osu.Game/Online/Multiplayer/MultiplayerClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
using osu.Game.Online.API.Requests.Responses;
using osu.Game.Online.Multiplayer.Countdown;
using osu.Game.Online.Rooms;
using osu.Game.Online.Rooms.RoomStatuses;
using osu.Game.Overlays.Notifications;
using osu.Game.Rulesets;
using osu.Game.Rulesets.Mods;
Expand Down Expand Up @@ -395,15 +394,17 @@ Task IMultiplayerClient.RoomStateChanged(MultiplayerRoomState state)
switch (state)
{
case MultiplayerRoomState.Open:
APIRoom.Status = APIRoom.HasPassword ? new RoomStatusOpenPrivate() : new RoomStatusOpen();
APIRoom.Status = RoomStatus.Idle;
break;

case MultiplayerRoomState.WaitingForLoad:
case MultiplayerRoomState.Playing:
APIRoom.Status = new RoomStatusPlaying();
APIRoom.Status = RoomStatus.Playing;
break;

case MultiplayerRoomState.Closed:
APIRoom.Status = new RoomStatusEnded();
APIRoom.EndDate = DateTimeOffset.Now;
APIRoom.Status = RoomStatus.Idle;
break;
}

Expand Down Expand Up @@ -821,7 +822,6 @@ private void updateLocalRoomSettings(MultiplayerRoomSettings settings)
Room.Settings = settings;
APIRoom.Name = Room.Settings.Name;
APIRoom.Password = Room.Settings.Password;
APIRoom.Status = string.IsNullOrEmpty(Room.Settings.Password) ? new RoomStatusOpen() : new RoomStatusOpenPrivate();
APIRoom.Type = Room.Settings.MatchType;
APIRoom.QueueMode = Room.Settings.QueueMode;
APIRoom.AutoStartDuration = Room.Settings.AutoStartDuration;
Expand Down
21 changes: 4 additions & 17 deletions osu.Game/Online/Rooms/Room.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,10 @@
using System.ComponentModel;
using System.Linq;
using System.Runtime.CompilerServices;
using System.Runtime.Serialization;
using Newtonsoft.Json;
using osu.Game.IO.Serialization.Converters;
using osu.Game.Online.API.Requests.Responses;
using osu.Game.Online.Multiplayer;
using osu.Game.Online.Rooms.RoomStatuses;

namespace osu.Game.Online.Rooms
{
Expand Down Expand Up @@ -248,7 +246,7 @@ public int ChannelId
}

/// <summary>
/// The current room status.
/// The current status of the room.
/// </summary>
public RoomStatus Status
{
Expand All @@ -265,18 +263,6 @@ public RoomAvailability Availability
set => SetField(ref availability, value);
}

[OnDeserialized]
private void onDeserialised(StreamingContext context)
{
// API doesn't populate status so let's do it here.
if (EndDate != null && DateTimeOffset.Now >= EndDate)
Status = new RoomStatusEnded();
else if (HasPassword)
Status = new RoomStatusOpenPrivate();
else
Status = new RoomStatusOpen();
}

[JsonProperty("id")]
private long? roomId;

Expand Down Expand Up @@ -349,8 +335,9 @@ private void onDeserialised(StreamingContext context)
[JsonProperty("channel_id")]
private int channelId;

// Not serialised (see: GetRoomsRequest).
private RoomStatus status = new RoomStatusOpen();
[JsonProperty("status")]
[JsonConverter(typeof(SnakeCaseStringEnumConverter))]
private RoomStatus status;

// Not yet serialised (not implemented).
private RoomAvailability availability;
Expand Down
14 changes: 3 additions & 11 deletions osu.Game/Online/Rooms/RoomStatus.cs
Original file line number Diff line number Diff line change
@@ -1,19 +1,11 @@
// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

#nullable disable

using osu.Game.Graphics;
using osuTK.Graphics;

namespace osu.Game.Online.Rooms
{
public abstract class RoomStatus
public enum RoomStatus
{
public abstract string Message { get; }
public abstract Color4 GetAppropriateColour(OsuColour colours);

public override int GetHashCode() => GetType().GetHashCode();
public override bool Equals(object obj) => GetType() == obj?.GetType();
Idle,
Playing,
}
}
14 changes: 0 additions & 14 deletions osu.Game/Online/Rooms/RoomStatuses/RoomStatusEnded.cs

This file was deleted.

14 changes: 0 additions & 14 deletions osu.Game/Online/Rooms/RoomStatuses/RoomStatusOpen.cs

This file was deleted.

14 changes: 0 additions & 14 deletions osu.Game/Online/Rooms/RoomStatuses/RoomStatusOpenPrivate.cs

This file was deleted.

14 changes: 0 additions & 14 deletions osu.Game/Online/Rooms/RoomStatuses/RoomStatusPlaying.cs

This file was deleted.

15 changes: 12 additions & 3 deletions osu.Game/Screens/OnlinePlay/Components/StatusColouredContainer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,27 @@ protected override void LoadComplete()
base.LoadComplete();

room.PropertyChanged += onRoomPropertyChanged;

Scheduler.AddDelayed(updateRoomStatus, 5000, true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to confirm intent - are these schedules supposed to handle the passage of time in relation to EndDate in order to correctly transition to ended state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. I thought of doing something similar to DrawableDate with a dynamic reschedule time but I don't think this needs to be accurate. Maybe in the future this will be messaged from the multiplayer server anyway...

Copy link
Member

Choose a reason for hiding this comment

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

I think this is a bit weird.

Depending on the load time of individual components, the 5 second poll may occur at different times, causing some elements to update at different times, even on the same room display.

I don't have an immediate better solution in mind though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general I expect these schedules to be triggered on the same frame for the same DrawableRoom. For different DrawableRooms - sure they can trigger at different times, but they're created at different times anyway so I don't see that as an issue.

As I said the future solution here is realtime updates from the multiplayer server.

Copy link
Member

Choose a reason for hiding this comment

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

Could we use this for now?

diff --git a/osu.Game/Screens/OnlinePlay/Components/StatusColouredContainer.cs b/osu.Game/Screens/OnlinePlay/Components/StatusColouredContainer.cs
index a811ee3371..379dc65bcd 100644
--- a/osu.Game/Screens/OnlinePlay/Components/StatusColouredContainer.cs
+++ b/osu.Game/Screens/OnlinePlay/Components/StatusColouredContainer.cs
@@ -1,6 +1,7 @@
 // Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
+using System;
 using System.ComponentModel;
 using osu.Framework.Allocation;
 using osu.Framework.Graphics;
@@ -30,7 +31,8 @@ protected override void LoadComplete()
 
             room.PropertyChanged += onRoomPropertyChanged;
 
-            Scheduler.AddDelayed(updateRoomStatus, 5000, true);
+            if (room.EndDate > DateTimeOffset.Now)
+                Scheduler.AddDelayed(updateRoomStatus, (room.EndDate.Value - DateTimeOffset.Now).TotalMilliseconds + 500, true);
             updateRoomStatus();
         }
 
diff --git a/osu.Game/Screens/OnlinePlay/Lounge/Components/RoomStatusPill.cs b/osu.Game/Screens/OnlinePlay/Lounge/Components/RoomStatusPill.cs
index 6da8f3ecbd..7007e6c8b8 100644
--- a/osu.Game/Screens/OnlinePlay/Lounge/Components/RoomStatusPill.cs
+++ b/osu.Game/Screens/OnlinePlay/Lounge/Components/RoomStatusPill.cs
@@ -1,6 +1,7 @@
 // Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
+using System;
 using System.ComponentModel;
 using osu.Framework.Allocation;
 using osu.Framework.Graphics;
@@ -37,7 +38,8 @@ protected override void LoadComplete()
 
             room.PropertyChanged += onRoomPropertyChanged;
 
-            Scheduler.AddDelayed(updateDisplay, 5000, true);
+            if (room.EndDate > DateTimeOffset.Now)
+                Scheduler.AddDelayed(updateDisplay, (room.EndDate.Value - DateTimeOffset.Now).TotalMilliseconds + 500, true);
             updateDisplay();
             FinishTransforms(true);
         }

It would at least mean that various elements would have their state changes synced up closer than rand(0,5000).

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I guess not since RoomStatusPill is also showing things like "is playing"?

updateRoomStatus();
}

private void onRoomPropertyChanged(object? sender, PropertyChangedEventArgs e)
{
if (e.PropertyName == nameof(Room.Status))
updateRoomStatus();
switch (e.PropertyName)
{
case nameof(Room.Category):
case nameof(Room.Status):
case nameof(Room.EndDate):
case nameof(Room.HasPassword):
updateRoomStatus();
break;
}
}

private void updateRoomStatus()
{
this.FadeColour(colours.ForRoomCategory(room.Category) ?? room.Status.GetAppropriateColour(colours), transitionDuration);
this.FadeColour(colours.ForRoomCategory(room.Category) ?? colours.ForRoomStatus(room), transitionDuration);
}

protected override void Dispose(bool isDisposing)
Expand Down
Loading
Loading