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

Conversation

smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented Nov 22, 2024

Prereqs:

Old Server New Server
Old Client 🟢 🟢
New Client 🟢 🟢

Aside from refactoring the weirdness that is the existing RoomStatus abstract class and deserialisation process, it also makes the Playing state work in the lounge which is a missing feature from osu!stable.

2024-11-22.20-58-07.mp4

@@ -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"?

@peppy peppy self-requested a review November 26, 2024 09:02
@@ -29,18 +29,27 @@ protected override void LoadComplete()
base.LoadComplete();

room.PropertyChanged += onRoomPropertyChanged;

Scheduler.AddDelayed(updateRoomStatus, 5000, true);
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.

/// </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.

@peppy peppy self-requested a review November 27, 2024 04:17
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.

3 participants