a - #177
Merged
Merged
Conversation
Annoying my rider.
Displays the stage name and details like currently picking player and damage multiplayer where applicable. Currently only shown on the discard and pick stages. # Move user retrieval to `RankedPlayScreen` This is done because I need the relevant `APIUser` instances in order to pass them to the overlay component. `RankedPlayScreen` seems like the appropriate place to manage the overlays since it manages the stage subscreens, hence the need to access the `APIUser`s in here. # Add current stage overlay to ranked play The actual change of this PR. Very much a dev design. https://github.com/user-attachments/assets/2388e934-2fc7-4e15-9947-9f98412765d2 --------- Co-authored-by: Dean Herbert <pe@ppy.sh>
## [Rewrite `BackgroundMusicManager` to not run into framework breakage](622216d) The attempted proper fix to this was ppy/osu-framework#6727. Unfortunately when presented with [the framework bump](#37217) with that change, CI says "you're stupid" and fails on some disposal idiocy that of course is undebuggable and irreproducible: The active test run was aborted. Reason: Test host process crashed : Unhandled exception. System.AggregateException: One or more errors occurred. (Object reference not set to an instance of an object.) ---> System.NullReferenceException: Object reference not set to an instance of an object. at osu.Framework.Audio.Sample.SampleChannelBass.Dispose(Boolean disposing) at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state) --- End of stack trace from previous location --- at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state) at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot, Thread threadPoolThread) --- End of inner exception stack trace --- at osu.Framework.Audio.AudioCollectionManager`1.UpdateChildren() at osu.Framework.Audio.AudioCollectionManager`1.UpdateChildren() at osu.Framework.Audio.AudioCollectionManager`1.UpdateChildren() at osu.Framework.Audio.AudioCollectionManager`1.UpdateChildren() at osu.Framework.Threading.AudioThread.OnExit() at osu.Framework.Threading.GameThread.setExitState(GameThreadState exitState) at osu.Framework.Threading.GameThread.RunSingleFrame() at osu.Framework.Threading.GameThread.<createThread>g__runWork|70_0() at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state) --- End of stack trace from previous location --- at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state) (https://github.com/ppy/osu/actions/runs/24019928154/job/70046733058?pr=37217#step:5:119) I no longer have the energy for any of this shit. @nekodex would appreciate if you could check that I actually haven't broken anything with the bgm here. Seems okay to me in test scenes at least. ## [Apply lowest-effort maybe-fixing changes to a bunch of flaking tests](7bd3ca4) None of the failures are reproducible locally, of course. I'm tired of this. If anyone else wants to subject themselves to actually investigating any of these, by all means, godspeed and good luck.
…dinate space (#37219) Closes #37185. The checkboxes in the context menu's ternary states were supposed to always show the origin "in local space", even if anchor is set to "closest". The issue here was reusing a method that only really made sense with closest anchor active for explicit application of "local-space" origin. To recap: - Below I will use concepts of "local-space origin" and "screen-space origin". To understand the difference, let's use an example: Say there's a drawable with 180 degree rotation. Suppose it has the "local origin" of `TopCentre`. The "local origin" is just the `Drawable` notion of origin; you'd literally set `d.Origin = Anchor.TopCentre`. The "screen-space origin" of this drawable is `BottomCentre`, because due to the rotation, that's how the component will visually behave when its position is altered. The same sort of distinction applies forth to things like flips / negative scale and such. - When you have closest anchor selected, you can only choose the anchor to snap to. The drawable will snap to that anchor, and choose an origin closest to it *in screen space* such that the "closest" in "closest anchor" works as users would expect it to. In this state, if you open the context menu for origin, all items will be disabled, but the ternary menu items will show the origin state *as translated back to local space*. - When you have an explicit anchor selected, you can choose both the anchor and origin. In that case, the origin picked is always picked in local space. In the end, this is all consistent with how the `Origin` property on `Drawable` works, and also with what is serialised to skin jsons.
`RankedPlaySubScreen.CenterColumn` had a padding which moved the card hand up slightly, causing it to not fully dissapear when contracting. The padding doesn't serve any purpose anymore (remnant of the very early versions of the screens), so I just removed it. I checked against `DiscardScreen`, `PickScreen`, `OpponentPickScreen` & `EndedScreen` to make sure this doesn't cause any layout breakage. Also removed the `ButtonsContainer` since it isn't being used anywhere anymore. https://github.com/user-attachments/assets/2fd32407-fbf7-45a3-b92a-0730a0f8a3fd
…pletion (#37227) This was used in one place, but I foresee this being a more common scenario. This also fixes an edge case where the dismiss process would fail if completion happened on an async thread before the `NotificationOverlay`'s scheduler could handle the initial ingress.
This was a private request for roundtable event usage. It’s also a common feature request, so I decided to spend a bit of time getting this working well-enough. https://github.com/user-attachments/assets/acceb57f-2979-43d0-9fc2-33e977bd2dd5 --- ### Delay loading spinner / loading layer initial load briefly to avoid flickering There's cases in this overlay where loading takes a few milliseconds. The loading spinner gets annoying. This also happens elsewhere, so this could be considered a global fix. Separate PR? probably... ### Ingest loading state of dashboard child content to show more correct loading layer Each display had their own loading layer implementation, but this is already too deep (inside the scroll content) and doesn't display great when for instance, results don't take up the full screen height. --------- Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>
- No longer holds realm write transaction open while performing sqlite lookups. - No longer attempts a write transaction when it will be a noop. I'll admit that this is maybe working around the actual realm write part being slow, but as I can't profile the issue locally, the sluggishness may actually be in sqlite for those users affected (since it's only been reported for tag population and not difficulty calculation?). Regardless, this should fix the issue this iteration. I also adjusted the user messaging to let them know why tag population is happening, since we've had some questions as to why it's running in the first place (it only happens once a month, so that's understandable). - [x] Depends on #37227. - Closes #34699. - Closes #37210. Note that #36128 also exists and has valid improvements which can be addressed separately. This is intended to be something we can act on immediately. --------- Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>
…rily hidden by overlay content (#37231) In human words: I read [this forum thread](https://osu.ppy.sh/community/forums/topics/2195138?n=1) today and was horrified to see the user there opening the F3 options menu in song select *when the F1 mod overlay was pulled up*, which is (a) not intended UX, (b) looks terrible, and (c) just wrecks the game behaviourally wholesale from start to end. So with this change you don't get to open options via F3 while inside mod overlay at all. The `Action` shadowing is pretty ugly but I don't have better ideas. Initially I tried to mess with `Enabled` (as I did once previously, see 36628e2), but it's much more complicated in this case because the enabled state needs to be restored when the buttons reappear, or it could change independently while the buttons are temporarily hidden, etc. So I'd rather just not deal with all that and invent a parallel scheme.
1. Gives `MatchmakingJoinLobby` parameters. 2. Adds additional data to lobby status update models. A further PR will build upon (2) to add more data to the queue screen.
…nvocation failed (#37159) RFC Until now, if the initial `BeginPlaySession()` call failed, the client would continue operating as if it didn't - it would still continue to send frames and call `EndPlaySession()` at the end of a session. Server-side, two things generally can happen after this: - The sent frames and the `EndPlaySession()` call are [completely](https://github.com/ppy/osu-server-spectator/blob/7bab117e9d161455485368f63a0607a9e53f9f8a/osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs#L122-L125) [ignored](https://github.com/ppy/osu-server-spectator/blob/7bab117e9d161455485368f63a0607a9e53f9f8a/osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs#L153-L157) as no-ops, or - A hub filter (like `ClientVersionChecker`) that failed the initial `BeginPlaySession()` call continues to fail the calls to `SendFrameData()` and `EndPlaySession()`, all the while creating a storm in logs, because it needs to throw `HubException`s to communicate to users that they need to update their game, and the exceptions can't be silenced from logs because they look like every other failure. To that end, this has two goals: reduce useless network traffic, and reduce noise in spectator server logs after the client version checks were recently reactivated. Probably needs tests, but unsure if everyone's going to be on board with this to begin with to be quite frank, so I'm leaving tests for when I'm told this needs tests.
Using the same styling as osu!web + daily challenge. <img width="1920" height="1034" alt="Screenshot_20260409-164600" src="https://github.com/user-attachments/assets/97e2270e-af9f-478d-b2d6-c9fb8be16720" /> --------- Co-authored-by: Dean Herbert <pe@ppy.sh>
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds user-search API, matchmaking join request/response flow, matchmaking and ranked-play UI components/overlays, spectator/multiplayer API signature updates, many test adjustments (flaky/ignore/polling), notification/loading refactors, background/audio and layout changes, and large Mania test resources and tooling icon updates. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI (ScreenQueue / UserSearch)
participant Client as MultiplayerClient
participant Hub as SignalR Hub
participant Server as IMatchmakingServer
UI->>Client: MatchmakingJoinLobbyWithParams(request)
Client->>Hub: invoke MatchmakingJoinLobbyWithParams(request)
Hub->>Server: IMatchmakingServer.MatchmakingJoinLobbyWithParams(request)
Server-->>Hub: MatchmakingJoinLobbyResponse
Hub-->>Client: MatchmakingJoinLobbyResponse
Client-->>UI: update lobby state / bindings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Pull request overview
This PR introduces new matchmaking- and ranked-play-related UI/behaviour updates, including a ranked-play stage overlay/background refresh, a new dashboard “User search” tab, profile “Quick Play” matchmaking stats display, and protocol changes for joining matchmaking lobbies / starting spectator play sessions.
Changes:
- Add ranked play stage overlay + background colour transitions, and refactor ranked play UI components to use
APIUser/colour scheme updates. - Add dashboard “User search” tab and user search API request/response plumbing.
- Update multiplayer matchmaking lobby join flow to accept parameters (pool ID) and make spectator “begin play” return a success/failure flag.
Reviewed changes
Copilot reviewed 87 out of 87 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| osu.Game/Tests/Visual/Spectator/TestSpectatorClient.cs | Update test client for BeginPlayingInternal() bool return. |
| osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs | Update matchmaking join stub to return response. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/ResultsScreen.ScoreBar.cs | Score bar styling adjustments. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/ResultsScreen.PanelScaffold.cs | Update ranked play colour scheme usage. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/ResultsScreen.cs | Stage heading access + colour/user display updates. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlaySubScreen.cs | Add ShowStageOverlay, expose StageHeading, layout cleanup. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayStageOverlay.cs | New stage overlay drawable. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayScreen.cs | Stage overlay orchestration + user lookup changes. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayMatchInfo.cs | Add IsOpponentTurn helper. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayColourScheme.cs | Switch to static readonly schemes (BLUE/RED). |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayBackgroundScreen.cs | Add background colour scheme setter + refactor background field. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayBackground.cs | Replace shader background with layered/triangle background + fade API. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/PickScreen.cs | Enable stage overlay during pick. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/OpponentPickScreen.cs | Enable stage overlay + colour scheme update. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Intro/VsSequence.cs | Update colour scheme references. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Intro/StarRatingSequence.cs | Visual/wording changes + star colour improvements. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Intro/IntroScreen.cs | Stage heading access update. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Hand/HandOfCards.cs | Switch cached layout backing to LayoutValue. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/GameplayWarmupScreen.cs | Stage heading access update. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/GameplayScreen.cs | Stage heading access update. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/EndedScreen.cs | Stage heading access update. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Components/RankedPlayUserDisplay.cs | Accept APIUser directly; remove internal user lookup. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Components/RankedPlayChatDisplay.cs | Update colour scheme references. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Components/BackgroundMusicManager.cs | Refactor BGM play/stop state handling. |
| osu.Game/Screens/OnlinePlay/Matchmaking/Queue/ScreenQueue.cs | Join lobby with params on pool select; audio loader move. |
| osu.Game/Screens/Footer/ScreenFooterButton.cs | Add visibility state + action gating to prevent triggering hidden buttons. |
| osu.Game/Screens/Footer/ScreenFooter.cs | Use button State to hide/show temporarily hidden buttons. |
| osu.Game/Overlays/SkinEditor/SkinSelectionHandler.cs | Split origin application into local vs screen space. |
| osu.Game/Overlays/Settings/Sections/General/UpdateSettings.cs | Use CompleteSilently() helper. |
| osu.Game/Overlays/Profile/Header/Components/MatchmakingStatsTooltip.cs | New tooltip to display matchmaking stats table. |
| osu.Game/Overlays/Profile/Header/Components/MatchmakingStatsDisplay.cs | New “Quick Play” matchmaking stats badge + tooltip hook. |
| osu.Game/Overlays/Profile/Header/Components/MainDetails.cs | Add matchmaking stats display alongside daily challenge stats. |
| osu.Game/Overlays/Notifications/ProgressNotification.cs | Add CompleteSilently() helper. |
| osu.Game/Overlays/MusicController.cs | Simplify track switching; assert update thread. |
| osu.Game/Overlays/DashboardOverlay.cs | Add loading bindable + user search tab integration. |
| osu.Game/Overlays/Dashboard/UserSearch/UserSearchDisplay.cs | New user search display with API-backed querying. |
| osu.Game/Overlays/Dashboard/UserSearch/UserPanelList.cs | New reusable user panel list for search results. |
| osu.Game/Overlays/Dashboard/Friends/UserListToolbar.cs | Add supportsSort option. |
| osu.Game/Overlays/Dashboard/Friends/FriendDisplay.cs | Replace internal loading layer with external loading bindable. |
| osu.Game/Overlays/Dashboard/DashboardOverlayHeader.cs | Localise tab labels + add UserSearch tab. |
| osu.Game/Overlays/Dashboard/CurrentlyOnline/CurrentlyOnlineDisplay.cs | Replace internal loading layer with external loading bindable. |
| osu.Game/Online/Spectator/SpectatorClient.cs | Make BeginPlayingInternal() return success; clear state on failure. |
| osu.Game/Online/Spectator/OnlineSpectatorClient.cs | Return bool from begin play and add logging on failure. |
| osu.Game/Online/Multiplayer/OnlineMultiplayerClient.cs | Join lobby with params + response type. |
| osu.Game/Online/Multiplayer/MultiplayerClient.cs | Update abstract matchmaking join signature. |
| osu.Game/Online/Multiplayer/MatchTypes/RankedPlay/RankedPlayUserInfo.cs | Add RoundsWon field. |
| osu.Game/Online/Matchmaking/Responses/MatchmakingJoinLobbyResponse.cs | New join lobby response model. |
| osu.Game/Online/Matchmaking/Requests/MatchmakingJoinLobbyRequest.cs | New join lobby request (pool ID). |
| osu.Game/Online/Matchmaking/MatchmakingLobbyStatus.cs | Extend lobby status payload (distribution, rating, recent matches). |
| osu.Game/Online/Matchmaking/IMatchmakingServer.cs | Update server interface to new join signature. |
| osu.Game/Online/API/Requests/SearchUsersResponse.cs | New search users response model. |
| osu.Game/Online/API/Requests/SearchUsersRequest.cs | New user search request. |
| osu.Game/Online/API/Requests/Responses/APIUserMatchmakingStatistics.cs | New matchmaking stats API model. |
| osu.Game/Online/API/Requests/Responses/APIUser.cs | Add matchmaking stats to API user model. |
| osu.Game/Online/API/Requests/Responses/APIMatchmakingPool.cs | New matchmaking pool API model. |
| osu.Game/Localisation/UserInterfaceStrings.cs | Add localisable strings for dashboard tabs. |
| osu.Game/Graphics/UserInterface/LoadingSpinner.cs | Add slight delayed fade-in to reduce flicker. |
| osu.Game/Graphics/Backgrounds/Triangles.cs | Make SpawnRatio configurable via public settable property. |
| osu.Game/Database/BackgroundDataStoreProcessor.cs | Adjust user tag backpopulation logging + notification completion. |
| osu.Game.Tests/Visual/UserInterface/TestSceneUserListToolbar.cs | Update toolbar ctor usage. |
| osu.Game.Tests/Visual/UserInterface/TestSceneScreenFooter.cs | Add test ensuring hidden footer buttons can’t be triggered. |
| osu.Game.Tests/Visual/UserInterface/TestSceneNotificationOverlay.cs | Add tests for silent progress completion. |
| osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapRecommendations.cs | Mark test as flaky. |
| osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselSetsSplitApart.cs | Mark test as flaky. |
| osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselFiltering.cs | Make assertions resilient via AddUntilStep. |
| osu.Game.Tests/Visual/Ranking/TestSceneSoloResultsScreen.cs | Mark test as flaky. |
| osu.Game.Tests/Visual/Ranking/TestSceneResultsScreen.cs | Make panel retrieval resilient via AddUntilStep. |
| osu.Game.Tests/Visual/RankedPlay/TestSceneRankedPlayUserDisplay.cs | Update user display ctor usage. |
| osu.Game.Tests/Visual/RankedPlay/TestSceneRankedPlayStageOverlay.cs | New visual test for stage overlay. |
| osu.Game.Tests/Visual/RankedPlay/TestSceneRankedPlayCornerPiece.cs | Update colour scheme + user display ctor usage. |
| osu.Game.Tests/Visual/RankedPlay/TestSceneRankedPlayBackground.cs | Update background API usage. |
| osu.Game.Tests/Visual/RankedPlay/TestScenePickScreen.cs | Extend pick screen test interactions. |
| osu.Game.Tests/Visual/RankedPlay/TestSceneOpponentPickScreen.cs | Extend opponent pick screen test flow. |
| osu.Game.Tests/Visual/Online/TestSceneUserSearchDisplay.cs | New visual test for user search display. |
| osu.Game.Tests/Visual/Online/TestSceneUserProfileMatchmakingStatsDisplay.cs | New visual test for profile matchmaking stats. |
| osu.Game.Tests/Visual/Online/TestSceneFriendDisplay.cs | Make asserts resilient to async load timing. |
| osu.Game.Tests/Visual/Navigation/TestSceneScreenNavigation.cs | Mark test as flaky. |
| osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs | Avoid Single() when multiple overlays may exist. |
| osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySamplePlayback.cs | Ignore persistently failing test. |
| osu.Game.Tests/Visual/Editing/TestSceneOpenEditorTimestamp.cs | Make beatmap retrieval resilient and remove non-null extension use. |
| osu.Game.Tests/Visual/Editing/TestSceneLocallyModifyingOnlineBeatmaps.cs | Mark test as flaky. |
| osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs | Mark test as flaky. |
| osu.Game.Tests/Database/BackgroundDataStoreProcessorTests.cs | Mark test as flaky. |
| osu.Game.Tests/Beatmaps/TestSceneBeatmapDifficultyCache.cs | Mark test as flaky (with note). |
| osu.Game.Rulesets.Osu/Skinning/Default/TrianglesPiece.cs | Adjust to new SpawnRatio API. |
| osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs | Add suppression comment for Rider inspection. |
Comments suppressed due to low confidence (1)
osu.Game/Overlays/Dashboard/Friends/UserListToolbar.cs:46
supportsSortcurrently just setssortControl.Alpha = 0, but the control will still take up layout space (and may still receive input) because it remains in theFillFlowContainer. If the intent is to remove sorting UI entirely, conditionally omitsortControlfromChildren(or setsortControl.AlwaysPresent = falseand remove it from layout when disabled).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+161
to
+176
| private void performSearch() | ||
| { | ||
| loading.Value = true; | ||
| var getUsersRequest = new SearchUsersRequest(searchTextBox.Current.Value); | ||
| getUsersRequest.Success += showResults; | ||
| api.Queue(getUsersRequest); | ||
| } | ||
|
|
||
| private void showResults(SearchUsersResponse response) | ||
| { | ||
| clearPreviousResults(); | ||
| var friendsList = new UserPanelList(userListToolbar.DisplayStyle.Value, response.Users.ToArray()); | ||
| listContainer.Add(friendsList); | ||
|
|
||
| friendsList.FadeInFromZero(500, Easing.OutQuint); | ||
| } |
There was a problem hiding this comment.
performSearch() sets loading = true and queues the request, but there is no failure / aborted handler to reset loading, and responses can arrive out-of-order (stale results can overwrite newer queries). Consider tracking the query string (or an incrementing request token) and ignoring results that don't match the latest query, and ensure loading is set to false in both success and failure paths.
Comment on lines
+200
to
+212
| private void onSelectedPoolChanged(ValueChangedEvent<MatchmakingPool?> e) | ||
| { | ||
| if (e.NewValue == null) | ||
| { | ||
| client.MatchmakingLeaveLobby(); | ||
| return; | ||
| } | ||
|
|
||
| client.MatchmakingJoinLobbyWithParams(new MatchmakingJoinLobbyRequest | ||
| { | ||
| PoolId = e.NewValue.Id | ||
| }); | ||
| } |
There was a problem hiding this comment.
onSelectedPoolChanged() calls MatchmakingLeaveLobby() / MatchmakingJoinLobbyWithParams() without awaiting or FireAndForget(). If these tasks fault, exceptions may go unobserved and joining/leaving can race unexpectedly. Please await them (make the handler async) or explicitly FireAndForget() with logging.
Comment on lines
742
to
+745
|
|
||
| completeNotification(notification, processedCount, beatmapIds.Count, failedCount); | ||
| // Report the updated item count rather than the total processed. Users don't really care about noops here. | ||
| completeNotification(notification, updatedCount, updatedCount, failedCount); | ||
|
|
There was a problem hiding this comment.
completeNotification(notification, updatedCount, updatedCount, failedCount) will mark the notification as Completed even if failedCount > 0 (because processedCount == totalCount), and the progress text during processing is still based on processedCount/beatmapIds.Count. This can incorrectly report success in the presence of failures and makes progress/completion counts inconsistent. Consider keeping totalCount = beatmapIds.Count for progress/failure handling and separately reporting updatedCount in CompletionText (or extend completeNotification() to accept both processed/updated counts).
Comment on lines
+66
to
+78
| public void FadeColours(Color4 top, Color4 bottom) | ||
| { | ||
| TextureShader = shaders.Load(VertexShaderDescriptor.TEXTURE_2, @"RankedPlayBackground"); | ||
| this.TransformTo(nameof(GradientTop), top, 1500, Easing.OutQuint); | ||
| this.TransformTo(nameof(GradientBottom), bottom, 1500, Easing.OutQuint); | ||
| } | ||
|
|
||
| private readonly BufferedDrawNodeSharedData sharedData = new BufferedDrawNodeSharedData(); | ||
|
|
||
| protected override void Update() | ||
| { | ||
| base.Update(); | ||
|
|
||
| Invalidate(Invalidation.DrawNode); | ||
| } | ||
|
|
||
| protected override DrawNode CreateDrawNode() => new RankedPlayBackgroundDrawNode(this, sharedData); | ||
|
|
||
| protected override void Dispose(bool isDisposing) | ||
| { | ||
| sharedData.Dispose(); | ||
|
|
||
| base.Dispose(isDisposing); | ||
| } | ||
|
|
||
| private class RankedPlayBackgroundDrawNode : BufferedDrawNode, ICompositeDrawNode | ||
| { | ||
| protected new RankedPlayBackground Source => (RankedPlayBackground)base.Source; | ||
|
|
||
| protected new CompositeDrawableDrawNode Child => (CompositeDrawableDrawNode)base.Child; | ||
|
|
||
| public RankedPlayBackgroundDrawNode(RankedPlayBackground source, BufferedDrawNodeSharedData sharedData) | ||
| : base(source, new CompositeDrawableDrawNode(source), sharedData) | ||
| { | ||
| } | ||
|
|
||
| private Vector2 drawSize; | ||
| private float time; | ||
| private float gridSize; | ||
| private Color4 gradientOutside; | ||
| private Color4 gradientInside; | ||
| private Color4 dotsColour; | ||
|
|
||
| private IUniformBuffer<RankedPlayBackgroundParameters>? shaderParameterBuffer; | ||
|
|
||
| public override void ApplyState() | ||
| { | ||
| base.ApplyState(); | ||
|
|
||
| time = (float)(Source.Time.Current / 1000); | ||
| drawSize = Source.DrawSize; | ||
| gridSize = Source.GridSize; | ||
| gradientOutside = Source.GradientOutside; | ||
| gradientInside = Source.GradientInside; | ||
| dotsColour = Source.DotsColour; | ||
| } | ||
|
|
||
| protected override void BindUniformResources(IShader shader, IRenderer renderer) | ||
| { | ||
| shaderParameterBuffer ??= renderer.CreateUniformBuffer<RankedPlayBackgroundParameters>(); | ||
|
|
||
| shaderParameterBuffer.Data = new RankedPlayBackgroundParameters | ||
| { | ||
| DrawSize = drawSize, | ||
| Time = time, | ||
| GridSize = gridSize, | ||
| GradientOutside = new Vector4(gradientOutside.R, gradientOutside.G, gradientOutside.B, gradientOutside.A), | ||
| GradientInside = new Vector4(gradientInside.R, gradientInside.G, gradientInside.B, gradientInside.A), | ||
| DotsColour = new Vector4(dotsColour.R, dotsColour.G, dotsColour.B, dotsColour.A), | ||
| }; | ||
|
|
||
| shader.BindUniformBlock("m_RankedPlayBackgroundParameters", shaderParameterBuffer); | ||
| } | ||
|
|
||
| public List<DrawNode>? Children | ||
| { | ||
| get => Child.Children; | ||
| set => Child.Children = value; | ||
| } | ||
|
|
||
| public bool AddChildDrawNodes => RequiresRedraw; | ||
|
|
||
| [StructLayout(LayoutKind.Sequential, Pack = 1)] | ||
| private record struct RankedPlayBackgroundParameters | ||
| { | ||
| public UniformVector2 DrawSize; | ||
| public UniformFloat Time; | ||
| public UniformFloat GridSize; | ||
| public UniformVector4 GradientOutside; | ||
| public UniformVector4 GradientInside; | ||
| public UniformVector4 DotsColour; | ||
| } | ||
| } | ||
|
|
||
| public partial class Triangles : CompositeDrawable | ||
| { | ||
| private Texture triangleTexture = null!; | ||
|
|
||
| [BackgroundDependencyLoader] | ||
| private void load(TextureStore textures) | ||
| { | ||
| triangleTexture = textures.Get("Online/RankedPlay/triangle"); | ||
| } | ||
|
|
||
| protected override void LoadComplete() | ||
| { | ||
| base.LoadComplete(); | ||
|
|
||
| for (int i = 0; i < 20; i++) | ||
| { | ||
| AddInternal(new Triangle | ||
| { | ||
| Texture = triangleTexture, | ||
| RelativePositionAxes = Axes.Both, | ||
| X = RNG.NextSingle(), | ||
| Y = -0.2f + RNG.NextSingle() * 1.4f, | ||
| Origin = Anchor.Centre, | ||
| Rotation = RNG.NextSingle() * 360, | ||
| AngularVelocity = RNG.NextSingle() - 0.75f, | ||
| Size = new Vector2(100 + RNG.NextSingle() * 1000), | ||
| MovementSpeed = 0.25f + RNG.NextSingle() * 0.75f, | ||
| Alpha = 0.5f + RNG.NextSingle() * 0.5f, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| public float ParticleVelocity = 1; | ||
|
|
||
| protected override void Update() | ||
| { | ||
| base.Update(); | ||
|
|
||
| if (DrawHeight <= 0) | ||
| return; | ||
|
|
||
| float baseVelocity = 0.03f * ParticleVelocity / DrawHeight; | ||
| float elapsed = (float)Time.Elapsed; | ||
|
|
||
| foreach (var c in InternalChildren) | ||
| { | ||
| var triangle = (Triangle)c; | ||
|
|
||
| triangle.Y -= baseVelocity * elapsed * triangle.MovementSpeed; | ||
|
|
||
| triangle.Rotation += triangle.AngularVelocity * elapsed * 0.02f; | ||
|
|
||
| // wrap vertically | ||
| if (triangle.Y < -0.2f) | ||
| { | ||
| triangle.X = RNG.NextSingle(); | ||
| triangle.Y = 1.2f; | ||
| triangle.Alpha = 0.5f + RNG.NextSingle() * 0.5f; | ||
| } | ||
| else if (triangle.Y > 1.2f) | ||
| { | ||
| triangle.X = RNG.NextSingle(); | ||
| triangle.Y = -0.2f; | ||
| triangle.Alpha = 0.5f + RNG.NextSingle() * 0.5f; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private partial class Triangle : Sprite | ||
| { | ||
| public float MovementSpeed = 1; | ||
| public float AngularVelocity; | ||
| } | ||
| bgBox.Colour = ColourInfo.GradientVertical(GradientTop, GradientBottom); | ||
| triangles1Buffered.Colour = ColourInfo.GradientVertical(GradientTop.Lighten(0.2f), GradientBottom.Lighten(0.2f)); | ||
| triangles2.Colour = GradientTop.Lighten(0.5f); |
There was a problem hiding this comment.
Update() recomputes gradient ColourInfo and calls Lighten() every frame, which will allocate and can cause avoidable GC pressure. Since these colours only change when GradientTop/GradientBottom change (during FadeColours), consider updating the drawable colours only when those properties change (eg via Invalidate(Invalidation.Colour) + UpdateAfterChildren, bindables, or a cached ColourInfo recomputed on invalidation).
Comment on lines
+229
to
+247
| BeginPlayingInternal(currentScoreToken, currentState).ContinueWith(t => | ||
| { | ||
| bool success = t.GetResultSafely(); | ||
|
|
||
| BeginPlayingInternal(currentScoreToken, currentState); | ||
| if (!success) | ||
| { | ||
| Logger.Log($"Clearing {nameof(SpectatorClient)} state due to failed {nameof(BeginPlayingInternal)} call."); | ||
| Schedule(() => | ||
| { | ||
| clearScoreState(); | ||
|
|
||
| currentState.BeatmapID = null; | ||
| currentState.RulesetID = null; | ||
| currentState.Mods = []; | ||
| currentState.State = SpectatedUserState.Idle; | ||
| currentState.MaximumStatistics = []; | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
BeginPlayingInternal() now returns Task<bool>, but there are call sites that ignore the return value (eg in the reconnect handler in load(): BeginPlayingInternal(currentScoreToken, currentState);). That means failures won’t clear isPlaying / state, and any exception could go unobserved. Consider funnelling all invocations through a single helper that awaits the result (or FireAndForget() with a continuation) and applies the same failure handling used here.
Comment on lines
+8
to
+12
| /// <summary> | ||
| /// Lookup up users with the given <see cref="Query"/>. | ||
| /// </summary> | ||
| public class SearchUsersRequest : APIRequest<SearchUsersResponse> | ||
| { |
There was a problem hiding this comment.
Typo in XML doc summary: "Lookup up users" should be "Look up users".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
osu.Game/Overlays/SkinEditor/SkinSelectionHandler.cs (1)
231-240:⚠️ Potential issue | 🟡 MinorPreserve closest-anchor semantics for mixed selections.
Line 239 now applies the requested origin to items with
UsesFixedAnchor == falseas well. In a mixed selection, that leaves closest-anchor items in a transient manual-origin state until the next move re-runsApplyClosestAnchorOrigin(), so the origin change can appear to work and then snap back on drag.Please either re-normalize non-fixed-anchor items here or skip manual-origin updates for them.
💡 One possible fix
private void applyOrigins(Anchor origin) { OnOperationBegan(); foreach (var item in SelectedItems) { var drawable = (Drawable)item; applyLocalSpaceOrigin(drawable, origin); + + if (!item.UsesFixedAnchor) + ApplyClosestAnchorOrigin(drawable); } OnOperationEnded(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Overlays/SkinEditor/SkinSelectionHandler.cs` around lines 231 - 240, The applyOrigins method is improperly applying manual origins to items that should keep closest-anchor behavior; update applyOrigins (iterating SelectedItems and calling applyLocalSpaceOrigin) to skip or re-normalize items where the drawable's UsesFixedAnchor is false — i.e., only call applyLocalSpaceOrigin for drawables with UsesFixedAnchor == true, and for non-fixed-anchor drawables call ApplyClosestAnchorOrigin() (or otherwise clear manual-origin state) so mixed selections preserve closest-anchor semantics and don't transiently snap back on drag.osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayColourScheme.cs (1)
17-33:⚠️ Potential issue | 🟠 MajorAvoid shared mutable colour scheme singletons.
The
static readonlyfields prevent rebinding, but the scheme fields remain publicly mutable. While no direct mutations were found in the codebase, this pattern permits global shared state mutations that could silently affect all consumers.💡 Safer fix (create instances per access)
- public static readonly RankedPlayColourScheme BLUE = new RankedPlayColourScheme + public static RankedPlayColourScheme BLUE => new RankedPlayColourScheme { Primary = Color4Extensions.FromHex("5EBFFF"), PrimaryDarker = Color4Extensions.FromHex("4382FF"), PrimaryDarkest = Color4Extensions.FromHex("5C55FF"), Surface = Color4Extensions.FromHex("33303D"), SurfaceBorder = Color4Extensions.FromHex("514c5e"), }; - public static readonly RankedPlayColourScheme RED = new RankedPlayColourScheme + public static RankedPlayColourScheme RED => new RankedPlayColourScheme { Primary = Color4Extensions.FromHex("FF8198"), PrimaryDarker = Color4Extensions.FromHex("F94D92"), PrimaryDarkest = Color4Extensions.FromHex("B6104D"), Surface = Color4Extensions.FromHex("242023"), SurfaceBorder = Color4Extensions.FromHex("403b3f"), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayColourScheme.cs` around lines 17 - 33, The BLUE and RED static readonly RankedPlayColourScheme singletons expose publicly mutable fields, risking shared mutable state; change them to factory-style accessors that return new instances (e.g., replace static readonly BLUE/RED with static properties or methods that construct and return new RankedPlayColourScheme objects), or make RankedPlayColourScheme immutable by converting its public mutable color fields to read-only properties set only via constructor; update references to use the new factory/property or constructor pattern and ensure symbols BLUE, RED and the RankedPlayColourScheme type are the focal points for the change.osu.Game/Overlays/Dashboard/Friends/FriendDisplay.cs (1)
172-202:⚠️ Potential issue | 🟡 MinorSame cancellation concern as
CurrentlyOnlineDisplay.The loading state management has the same potential issue: if
listLoadCancellation.Cancel()aborts an in-flight load,loading.Valueremainstrueuntil the nextreloadList()call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Overlays/Dashboard/Friends/FriendDisplay.cs` around lines 172 - 202, The reloadList method can leave loading.Value stuck true if listLoadCancellation.Cancel() aborts an in-flight LoadComponentAsync; update reloadList to ensure loading.Value is cleared when a load is cancelled by wiring the cancellation token (listLoadCancellation) or cancellationSource to reset loading.Value (either by registering a cancellation callback on listLoadCancellation that sets loading.Value = false, or by checking cancellation in finishLoad and early-return while clearing loading.Value), referencing reloadList, listLoadCancellation / cancellationSource, LoadComponentAsync and the local finishLoad callback so cancelled loads cannot leave loading.Value true.osu.Game/Overlays/Dashboard/CurrentlyOnline/CurrentlyOnlineDisplay.cs (1)
141-168:⚠️ Potential issue | 🟡 MinorLoading state may remain
trueif async load is cancelled.When
listLoadCancellation.Cancel()is called at Line 143, any in-flightLoadComponentAsyncwill not invokefinishLoad, leavingloading.Value = true. Subsequent calls toreloadList()will reset it, but there's a window where the loading state is stale.Consider resetting
loading.Value = falseat the start ofreloadList()before setting it totrue, or handle it in the cancellation path.Proposed fix
private void reloadList() { listLoadCancellation?.Cancel(); var cancellationSource = listLoadCancellation = new CancellationTokenSource(); RealtimeUserList? currentList = listContainer.SingleOrDefault(); RealtimeUserList newList = new RealtimeUserList(userListToolbar.DisplayStyle.Value) { SortCriteria = { BindTarget = userListToolbar.SortCriteria }, SearchText = { BindTarget = searchTextBox.Current } }; + // Reset loading state in case previous load was cancelled loading.Value = true; LoadComponentAsync(newList, finishLoad, cancellationSource.Token);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Overlays/Dashboard/CurrentlyOnline/CurrentlyOnlineDisplay.cs` around lines 141 - 168, The loading flag can remain true if a prior LoadComponentAsync is cancelled and its finishLoad never runs; inside reloadList() after calling listLoadCancellation?.Cancel() and before creating the new CancellationTokenSource, reset loading.Value = false (and/or register cancellationSource.Token.Register(() => loading.Value = false)) so any cancellation clears the loading state; ensure this uses the existing listLoadCancellation/CancellationTokenSource and that the token registration or immediate reset is added alongside the LoadComponentAsync call that uses finishLoad.
🧹 Nitpick comments (13)
osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselFiltering.cs (1)
37-37: Consider unique step names for easier failure triage.All three steps currently use
"invocation count correct". Including the expected count in each description would make test logs easier to read when one fails.Also applies to: 41-41, 56-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselFiltering.cs` at line 37, The three AddUntilStep calls that assert NewItemsPresentedInvocationCount all share the identical step title "invocation count correct", which makes log triage hard; update each AddUntilStep invocation to use a unique, descriptive name that includes the expected count (e.g. "invocation count correct (expected 1)" or "invocation count == 2") so failures show which expectation failed—locate the AddUntilStep calls that reference NewItemsPresentedInvocationCount and change their first argument to include the expected value for that assertion.osu.Game/Graphics/Backgrounds/Triangles.cs (1)
69-69: MakeSpawnRatiosetter enforce valid values and refresh state.At Line 69,
SpawnRatiois now mutable, but changes won’t affect rendered density untilReset()runs (Line 187), and negative values can produce invalid target counts. Consider clamping to>= 0and triggeringReset()when the value changes.♻️ Proposed change
- public float SpawnRatio { get; set; } = 1; + private float spawnRatio = 1f; + public float SpawnRatio + { + get => spawnRatio; + set + { + float clamped = Math.Max(0, value); + if (spawnRatio == clamped) return; + + spawnRatio = clamped; + Reset(); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Graphics/Backgrounds/Triangles.cs` at line 69, Make the SpawnRatio setter validate and apply changes immediately: clamp incoming value to a minimum of 0, check if the clamped value differs from the current SpawnRatio, assign it, and call Reset() to refresh internal state/target counts; update the auto-property at line with a full property implementation that uses the backed value, performs the clamp (>= 0), compares old vs new, and invokes Reset() when changed.osu.Game.Tests/Visual/Editing/TestSceneLocallyModifyingOnlineBeatmaps.cs (1)
27-27: Add a short tracking reference for this flaky marker.Line 27 marks the test flaky, but there’s no linked issue/PR context here. Adding one-line traceability helps ensure it gets revisited.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game.Tests/Visual/Editing/TestSceneLocallyModifyingOnlineBeatmaps.cs` at line 27, The [FlakyTest] marker on the TestSceneLocallyModifyingOnlineBeatmaps test lacks traceability; update the attribute or add an inline comment above the TestSceneLocallyModifyingOnlineBeatmaps class to include a one-line tracking reference (e.g. issue or PR id and short reason) so future maintainers can find the root cause; ensure you modify the [FlakyTest] usage (or the single-line comment immediately above it) to contain that reference and a brief note about the observed flakiness.osu.Game/Screens/Footer/ScreenFooterButton.cs (1)
71-86: Prefer guarding invocation withVisibility.Visibleexplicitly.
Current logic blocks onlyHidden; using explicitVisibleis safer against future state expansions.Proposed tweak
- if (State.Value != Visibility.Hidden) + if (State.Value == Visibility.Visible) value.Invoke();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Screens/Footer/ScreenFooterButton.cs` around lines 71 - 86, The Action setter currently wraps the provided delegate in base.Action but allows invocation for any state other than Visibility.Hidden; change the guard to only invoke when State.Value == Visibility.Visible to be explicit and future-proof. Locate the Action property setter in ScreenFooterButton (the new Action property) and replace the conditional inside base.Action's lambda so it checks State.Value == Visibility.Visible before calling value.Invoke(); keep null handling the same.osu.Game/Online/Matchmaking/IMatchmakingServer.cs (1)
21-21: Consider documenting the new request parameter.Please add a
<param name="request">XML doc so the selected-pool semantics are explicit at the interface boundary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Online/Matchmaking/IMatchmakingServer.cs` at line 21, Add XML documentation to the MatchmakingJoinLobbyWithParams method to document the request parameter's semantics (especially the selected-pool behavior). Update the IMatchmakingServer interface's MatchmakingJoinLobbyWithParams(MatchmakingJoinLobbyRequest request) declaration to include a <param name="request"> entry that explains what fields of MatchmakingJoinLobbyRequest are used and how the selected-pool value is interpreted at the interface boundary.osu.Game/Overlays/Dashboard/UserSearch/UserPanelList.cs (1)
14-16: Acknowledged tech debt - consider future extraction.The candid comment about duplication is appreciated. If this pattern appears in multiple places (FriendsList, UserPanelList, etc.), consider extracting a shared
UserPanelFactoryor similar helper in a future refactor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Overlays/Dashboard/UserSearch/UserPanelList.cs` around lines 14 - 16, The comment in UserPanelList.cs notes heavy copy-paste from FriendList; to address this tech debt later, extract the duplicated creation logic into a shared factory/helper (e.g., a new UserPanelFactory or UserPanelBuilder) that encapsulates common construction/configuration used by FriendList and UserPanelList; locate the duplicated code in the UserPanelList and FriendList classes (methods that assemble/return UserPanel instances), move that logic into the new factory with clear methods (e.g., CreateUserPanel(user, isFriend, callbacks)), and update both UserPanelList and FriendList to use the factory so future updates live in one place.osu.Game.Tests/Visual/RankedPlay/TestSceneRankedPlayBackground.cs (1)
51-52: Consider renaming the bindables to match the target properties.
gradientOuter/gradientInnernow driveGradientBottom/GradientTop, which is easy to misread later. Renaming togradientBottom/gradientTopwould improve maintainability in this test scene.♻️ Suggested cleanup
- private readonly Bindable<Colour4> gradientOuter = new Bindable<Colour4>(Color4Extensions.FromHex("AC6D97")); - private readonly Bindable<Colour4> gradientInner = new Bindable<Colour4>(Color4Extensions.FromHex("544483")); + private readonly Bindable<Colour4> gradientBottom = new Bindable<Colour4>(Color4Extensions.FromHex("AC6D97")); + private readonly Bindable<Colour4> gradientTop = new Bindable<Colour4>(Color4Extensions.FromHex("544483")); ... - Current = gradientOuter, + Current = gradientBottom, ... - Current = gradientInner, + Current = gradientTop, ... - gradientOuter.BindValueChanged(e => background.GradientBottom = e.NewValue, true); - gradientInner.BindValueChanged(e => background.GradientTop = e.NewValue, true); + gradientBottom.BindValueChanged(e => background.GradientBottom = e.NewValue, true); + gradientTop.BindValueChanged(e => background.GradientTop = e.NewValue, true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game.Tests/Visual/RankedPlay/TestSceneRankedPlayBackground.cs` around lines 51 - 52, The bindable variables gradientOuter and gradientInner are confusing because they drive background.GradientBottom and background.GradientTop; rename gradientOuter -> gradientBottom and gradientInner -> gradientTop (and update all references) so the bindable names match the target properties used in TestSceneRankedPlayBackground, then update the BindValueChanged calls to use the new names (keeping the same lambda assignments to background.GradientBottom and background.GradientTop).osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlaySubScreen.cs (1)
54-56: Minor XML-doc grammar tweak.“content don't overlap” reads better as “content doesn't overlap”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlaySubScreen.cs` around lines 54 - 56, Update the XML documentation on the RankedPlaySubScreen class: change the phrase "content don't overlap" to grammatically correct "content doesn't overlap" in the summary for the central column (the comment referencing RankedPlayCornerPiece) so the doc reads correctly.osu.Game/Online/API/Requests/Responses/APIUserMatchmakingStatistics.cs (1)
10-14: Use a property forUserIdto keep DTO members consistent.Line 11 is a field while the rest are properties. Prefer uniform auto-properties for API models.
♻️ Suggested fix
[JsonProperty("user_id")] -public int UserId; +public int UserId { get; set; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Online/API/Requests/Responses/APIUserMatchmakingStatistics.cs` around lines 10 - 14, In APIUserMatchmakingStatistics change the public field UserId into an auto-property to match the rest of the DTO: replace the field declaration for UserId with a public int UserId { get; set; } and keep the existing [JsonProperty("user_id")] attribute intact so serialization behavior remains the same (leave PoolId as-is).osu.Game.Tests/Visual/RankedPlay/TestSceneRankedPlayStageOverlay.cs (1)
43-88: Mount overlays inContentinstead of replacing the scene root.Lines 43/57/71/80 set
Child = ..., which bypasses the hierarchy created in Line 23. PreferContent.Child = ...(orAdd(...)) to keep setup consistent.♻️ Suggested change
-AddStep("create", () => Child = new RankedPlayStageOverlay("Pick Phase", RankedPlayColourScheme.BLUE) +AddStep("create", () => Content.Child = new RankedPlayStageOverlay("Pick Phase", RankedPlayColourScheme.BLUE) ... -AddStep("create", () => Child = new RankedPlayStageOverlay("Pick Phase", RankedPlayColourScheme.BLUE) +AddStep("create", () => Content.Child = new RankedPlayStageOverlay("Pick Phase", RankedPlayColourScheme.BLUE) ... -AddStep("create blue", () => Child = new RankedPlayStageOverlay("Pick Phase", RankedPlayColourScheme.BLUE) +AddStep("create blue", () => Content.Child = new RankedPlayStageOverlay("Pick Phase", RankedPlayColourScheme.BLUE) ... -AddStep("create red", () => Child = new RankedPlayStageOverlay("Pick Phase", RankedPlayColourScheme.RED) +AddStep("create red", () => Content.Child = new RankedPlayStageOverlay("Pick Phase", RankedPlayColourScheme.RED)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game.Tests/Visual/RankedPlay/TestSceneRankedPlayStageOverlay.cs` around lines 43 - 88, The tests set the scene root directly by assigning Child = new RankedPlayStageOverlay(...) in AddStep (e.g., in TestLongUsername, TestColourScheme and the earlier create step), which bypasses the pre-built hierarchy; change these to mount the overlay into the existing scene root by using Content.Child = ... or Add(...) instead of assigning Child so the overlay is placed inside the existing container created in the test setup (look for usages of RankedPlayStageOverlay constructed inside AddStep where PickingUser/Multiplier are set and replace Child = with Content.Child = or Add(...)).osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayStageOverlay.cs (1)
138-153: Localise stage-detail strings.Lines 138 and 153 hardcode text (
"'s pick"/"x damage"). These should use localisation resources.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayStageOverlay.cs` around lines 138 - 153, The hardcoded stage-detail strings should be replaced with localized resource lookups: change the OsuSpriteText.Text that currently uses $"{PickingUser.Username}'s pick" to a localized formatted string (e.g. a resource key like "ranked_stage_pick") and pass PickingUser.Username as the format argument, and likewise replace $"{Multiplier:N0}x damage" with a localized formatted string (e.g. "ranked_damage_multiplier") passing Multiplier formatted as N0; update the resources/strings file with those keys and use the project's localization API (the same approach used elsewhere in RankedPlayStageOverlay.cs when setting text) so the OsuSpriteText.Text values are obtained from localization rather than hardcoded literals.osu.Game/Overlays/Profile/Header/Components/MatchmakingStatsDisplay.cs (1)
61-61: Localise the visible label instead of hardcoding text.Line 61 uses
"Quick Play"directly. This should come from a localisation resource key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Overlays/Profile/Header/Components/MatchmakingStatsDisplay.cs` at line 61, In MatchmakingStatsDisplay replace the hardcoded label Text = "Quick Play" with a lookup to a localisation resource (e.g. use the framework/localisation API to return a LocalisableString) so the visible label comes from a resource key like "Matchmaking.QuickPlay"; add the "Matchmaking.QuickPlay" entry to the app's resource file(s), update the Text assignment in the MatchmakingStatsDisplay class to use the localisation getter (and import the localisation namespace or change the property type to LocalisableString if required).osu.Game/Overlays/Profile/Header/Components/MatchmakingStatsTooltip.cs (1)
71-74: Tooltip column headers should be localisable.Lines 71-74 hardcode
"Wins","Plays","Points", and"Rating". Please move these to localisation resources.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Overlays/Profile/Header/Components/MatchmakingStatsTooltip.cs` around lines 71 - 74, In MatchmakingStatsTooltip replace the hardcoded TableColumn headers ("Wins", "Plays", "Points", "Rating") with localized strings: add localisation entries (e.g. keys like "tooltip.matchmaking.wins", "tooltip.matchmaking.plays", "tooltip.matchmaking.points", "tooltip.matchmaking.rating") in the project's resources and use the localisation API (or LocalisableString properties) when constructing the TableColumn instances instead of the literal strings (i.e., change new TableColumn("Wins", ...) to new TableColumn(YourLocalisation.GetString("tooltip.matchmaking.wins"), ...), similarly for the other three headers).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySamplePlayback.cs`:
- Line 23: The test is being fully disabled via [Ignore] which removes coverage
for the regression in TestSceneGameplaySamplePlayback; replace the blanket
ignore with a quarantined/flaky marker (e.g., use [FlakyTest] or your project's
quarantine attribute) instead of [Ignore], add a short TODO comment above the
TestSceneGameplaySamplePlayback class/method referencing a concrete tracking
issue ID and clear exit criteria (what must be fixed to re-enable), and ensure
the test remains discoverable in CI reports so it can be retried/monitored
rather than permanently skipped.
In `@osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs`:
- Line 665: The current assertion uses All(...) which vacuously passes for an
empty sequence; update the AddAssert call that checks
MultiplayerUserModSelectOverlay to require that at least one overlay exists and
that every overlay's State.Value == Visibility.Hidden (e.g. combine Any(...) or
Count>0 with the existing All(...) check) so the assertion only passes when
overlays are present and hidden; locate the AddAssert invocation referencing
MultiplayerUserModSelectOverlay in TestSceneMultiplayer and adjust its predicate
accordingly.
In `@osu.Game.Tests/Visual/Online/TestSceneUserProfileMatchmakingStatsDisplay.cs`:
- Around line 57-82: The test fixture sets only Pool.Name which doesn't exercise
pool-priority selection; update the three APIUserMatchmakingStatistics instances
in TestSceneUserProfileMatchmakingStatsDisplay (the objects with properties
Plays, FirstPlacements, Rank, Rating, TotalPoints, IsRatingProvisional) to
explicitly set Pool.Active and Pool.Id (use distinct ids that reflect intended
priority ordering, e.g., active pool Active=true Id=1, inactive pool
Active=false Id=2, provisional pool Active=true/false with its own Id) so the
relevance sorting logic depends on explicit Active/Id values rather than
insertion order or defaults.
In `@osu.Game.Tests/Visual/RankedPlay/TestScenePickScreen.cs`:
- Around line 55-77: The test uses fixed waits and directly calls ElementAt(i2)
and First(...) on the drawable tree which can be null/unready; replace the fixed
AddWaitStep and direct indexed/First access with deterministic readiness checks
(e.g., AddUntilStep/AddWaitUntil or WaitForCondition) that wait until
screen.ChildrenOfType<PlayerHandOfCards.PlayerHandCard>().Count() > i2 before
clicking in the loop, and wait until
screen.ChildrenOfType<PlayerHandOfCards.PlayerHandCard>().Any(it => it.Selected)
and that the selected card has a child ShearedButton available before calling
First()/clicking; keep using InputManager.MoveMouseTo/Click but only after those
conditions are satisfied.
In `@osu.Game/Database/BackgroundDataStoreProcessor.cs`:
- Around line 743-744: The completion notification currently passes updatedCount
as both processedCount and totalCount to completeNotification, causing the
processedCount==totalCount check in the completion logic to always evaluate true
and mask any failures; update the call to completeNotification to pass the
original total (e.g. beatmapIds.Count) as totalCount so failures surface
correctly, or alternatively modify the completion logic (the branch that checks
processedCount == totalCount) to also consider failedCount (e.g. treat
failedCount > 0 as a failure) before choosing the success vs failure
notification path—refer to completeNotification, updatedCount, failedCount and
beatmapIds.Count to locate and apply the fix.
In `@osu.Game/Online/API/Requests/SearchUsersRequest.cs`:
- Line 9: Correct the XML summary comment for the SearchUsersRequest to fix the
typo "Lookup up users" by changing it to a clear phrase such as "Look up users
with the given <see cref=\"Query\"/>." Update the XML comment on the
SearchUsersRequest class (or its associated summary) so the wording is
grammatically correct and references the Query property as shown.
In `@osu.Game/Online/API/Requests/SearchUsersResponse.cs`:
- Around line 16-19: The Users getter currently dereferences private field
UserData data in "Users => data.Users" which can throw if the "user" JSON is
missing; fix by initializing data defensively or guarding the getter: either
give the field a safe default (e.g. new UserData() or equivalent) on declaration
or change the Users property to return data?.Users ?? new List<APIUser>() so
Users never throws on null. Update the declaration of "private UserData data" or
the "Users" property in SearchUsersResponse to apply this null-safe behavior.
In `@osu.Game/Online/Spectator/OnlineSpectatorClient.cs`:
- Around line 74-75: The current recursive retry path that calls
BeginPlayingInternal(scoreToken, state) can loop indefinitely on repeated
shutdown responses; replace the recursion with a bounded retry loop (e.g.,
for/while with a configurable maxAttempts) inside the method that currently
returns await BeginPlayingInternal(scoreToken, state) so that you attempt
BeginPlayingInternal up to maxAttempts, break and return success on a successful
call, and throw/fail fast (propagate a specific exception or return a failed
Task) when attempts are exhausted; ensure you preserve awaiting
ConfigureAwait(false) and keep using the same scoreToken and state parameters
and add a short backoff between retries if desired.
In `@osu.Game/Online/Spectator/SpectatorClient.cs`:
- Around line 229-245: The ContinueWith callback for BeginPlayingInternal
unconditionally clears shared state on failure, allowing a stale async failure
to wipe a newer session; capture the session token used when calling
BeginPlayingInternal (the currentScoreToken passed into BeginPlayingInternal)
and inside the failure branch verify that the captured token still equals the
currentScoreToken before calling clearScoreState() and mutating currentState
(i.e., add a session guard using the token variable), so only the session that
started the failing BeginPlayingInternal is allowed to clear its own state; keep
all existing Schedule(...) logic but wrap it in this token equality check.
In `@osu.Game/Overlays/Dashboard/UserSearch/UserSearchDisplay.cs`:
- Around line 152-156: When searchTextBox.Current.Value is empty you currently
set loading.Value = false but do not clear the displayed results; update the
early-return branch to also clear the results container (listContainer) and
reset any related state (e.g., Selected/active item or paging) so stale results
disappear. Locate the check that uses searchTextBox.Current.Value and add a call
to clear listContainer (and reset any result-related fields/state) before
returning.
- Around line 136-137: The DisplayStyle change currently calls performSearch()
directly (in userListToolbar.DisplayStyle.BindValueChanged), bypassing the
guarded/debounced path; change that binding to call queueUpdateSearch() instead
(i.e., replace the performSearch() invocation in the
DisplayStyle.BindValueChanged handler with queueUpdateSearch()) so style toggles
go through the same guarded/debounce logic used by
searchTextBox.Current.BindValueChanged.
- Around line 161-176: The search results can be clobbered by out-of-order
responses; add a request-version guard by tracking a monotonic "currentSearchId"
field incremented in performSearch() and captured by the SearchUsersRequest (or
its Success handler) so showResults only updates UI when the response's captured
id equals currentSearchId; specifically, in performSearch() increment and store
currentSearchId, capture that id when wiring getUsersRequest.Success to
showResults (or use a small wrapper lambda that passes the id along), and inside
showResults ignore the response if its id doesn't match currentSearchId before
calling clearPreviousResults()/listContainer.Add()/FadeInFromZero and setting
loading.Value.
- Around line 161-167: In performSearch(), a SearchUsersRequest sets
loading.Value = true but only wires getUsersRequest.Success to showResults, so
on failure loading stays true; add a Failure handler on the same
SearchUsersRequest (getUsersRequest.Failure) that either calls
clearPreviousResults() or sets loading.Value = false (and optionally logs the
error) to ensure the loading indicator is reset when the request fails.
In `@osu.Game/Screens/OnlinePlay/Matchmaking/Queue/ScreenQueue.cs`:
- Around line 200-212: The onSelectedPoolChanged handler currently discards the
Tasks returned by client.MatchmakingLeaveLobby() and
client.MatchmakingJoinLobbyWithParams(...); update the method to call
FireAndForget() on those Tasks instead so async failures are surfaced
consistently (i.e. replace direct calls to MatchmakingLeaveLobby() and
MatchmakingJoinLobbyWithParams(new MatchmakingJoinLobbyRequest { PoolId =
e.NewValue.Id }) with client.MatchmakingLeaveLobby().FireAndForget(...) and
client.MatchmakingJoinLobbyWithParams(...).FireAndForget(...), preserving any
existing logging/context usage pattern used elsewhere in this file.
In
`@osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Components/BackgroundMusicManager.cs`:
- Around line 81-88: The scheduled delegate captures globalTrack but later
unconditionally calls musicController.Stop(), risking stopping a different
track; change the delayed callback to operate safely on the captured track:
capture the local ITrack (globalTrack), null-check it, call captured.VolumeTo(1)
to restore its volume, and only call musicController.Stop() if
musicController.CurrentTrack == captured (or skip Stop entirely if it differs)
to avoid affecting a new current track; also ensure globalTrackFadeDelegate is
replaced/cancelled appropriately and handle possible null CurrentTrack when
scheduling in Scheduler.AddDelayed.
In `@osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayScreen.cs`:
- Around line 223-246: stageOverlayContainer is never cleared causing stale
RankedPlayStageOverlay instances to accumulate; before adding a new
RankedPlayStageOverlay in the ShowStageOverlay branch, clear existing children
from stageOverlayContainer (e.g. call its Clear/RemoveAll method), and also
clear stageOverlayContainer when ShowStageOverlay is false so no overlays remain
visible; update the logic around ShowStageOverlay, RankedPlayStageOverlay, and
stageOverlayContainer to ensure at most one current overlay is present.
- Around line 174-175: The code currently uses null-forgiving operators on the
results of users.GetUserAsync(...).GetResultSafely() for localUser and
opponentUser which can crash later if the lookups returned null; change to
capture the nullable results into variables (e.g., var localUser =
users.GetUserAsync(localUserId).GetResultSafely(); var opponentUser =
users.GetUserAsync(opponentUserId).GetResultSafely();) and then check for null
before proceeding: if either is null, log a clear error via the same logger,
show an appropriate UI/error state or abort constructing the match screen (e.g.,
return/Close() or display a placeholder) so downstream code that expects
non-null users never receives a forced-null value. Ensure references to
GetUserAsync/GetResultSafely/localUser/opponentUser are updated accordingly.
In
`@osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayStageOverlay.cs`:
- Line 153: In RankedPlayStageOverlay replace the integer N0 formatting used for
the Multiplier display with the same formatting used in ResultsScreen so
fractional multipliers show (use
Multiplier?.ToStandardFormattedString(maxDecimalDigits: 1) instead of
$"{Multiplier:N0}"); update the Text assignment in the RankedPlayStageOverlay
display (the Multiplier/text generation site) to use that method and preserve
the trailing "x damage" suffix.
---
Outside diff comments:
In `@osu.Game/Overlays/Dashboard/CurrentlyOnline/CurrentlyOnlineDisplay.cs`:
- Around line 141-168: The loading flag can remain true if a prior
LoadComponentAsync is cancelled and its finishLoad never runs; inside
reloadList() after calling listLoadCancellation?.Cancel() and before creating
the new CancellationTokenSource, reset loading.Value = false (and/or register
cancellationSource.Token.Register(() => loading.Value = false)) so any
cancellation clears the loading state; ensure this uses the existing
listLoadCancellation/CancellationTokenSource and that the token registration or
immediate reset is added alongside the LoadComponentAsync call that uses
finishLoad.
In `@osu.Game/Overlays/Dashboard/Friends/FriendDisplay.cs`:
- Around line 172-202: The reloadList method can leave loading.Value stuck true
if listLoadCancellation.Cancel() aborts an in-flight LoadComponentAsync; update
reloadList to ensure loading.Value is cleared when a load is cancelled by wiring
the cancellation token (listLoadCancellation) or cancellationSource to reset
loading.Value (either by registering a cancellation callback on
listLoadCancellation that sets loading.Value = false, or by checking
cancellation in finishLoad and early-return while clearing loading.Value),
referencing reloadList, listLoadCancellation / cancellationSource,
LoadComponentAsync and the local finishLoad callback so cancelled loads cannot
leave loading.Value true.
In `@osu.Game/Overlays/SkinEditor/SkinSelectionHandler.cs`:
- Around line 231-240: The applyOrigins method is improperly applying manual
origins to items that should keep closest-anchor behavior; update applyOrigins
(iterating SelectedItems and calling applyLocalSpaceOrigin) to skip or
re-normalize items where the drawable's UsesFixedAnchor is false — i.e., only
call applyLocalSpaceOrigin for drawables with UsesFixedAnchor == true, and for
non-fixed-anchor drawables call ApplyClosestAnchorOrigin() (or otherwise clear
manual-origin state) so mixed selections preserve closest-anchor semantics and
don't transiently snap back on drag.
In
`@osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayColourScheme.cs`:
- Around line 17-33: The BLUE and RED static readonly RankedPlayColourScheme
singletons expose publicly mutable fields, risking shared mutable state; change
them to factory-style accessors that return new instances (e.g., replace static
readonly BLUE/RED with static properties or methods that construct and return
new RankedPlayColourScheme objects), or make RankedPlayColourScheme immutable by
converting its public mutable color fields to read-only properties set only via
constructor; update references to use the new factory/property or constructor
pattern and ensure symbols BLUE, RED and the RankedPlayColourScheme type are the
focal points for the change.
---
Nitpick comments:
In `@osu.Game.Tests/Visual/Editing/TestSceneLocallyModifyingOnlineBeatmaps.cs`:
- Line 27: The [FlakyTest] marker on the TestSceneLocallyModifyingOnlineBeatmaps
test lacks traceability; update the attribute or add an inline comment above the
TestSceneLocallyModifyingOnlineBeatmaps class to include a one-line tracking
reference (e.g. issue or PR id and short reason) so future maintainers can find
the root cause; ensure you modify the [FlakyTest] usage (or the single-line
comment immediately above it) to contain that reference and a brief note about
the observed flakiness.
In `@osu.Game.Tests/Visual/RankedPlay/TestSceneRankedPlayBackground.cs`:
- Around line 51-52: The bindable variables gradientOuter and gradientInner are
confusing because they drive background.GradientBottom and
background.GradientTop; rename gradientOuter -> gradientBottom and gradientInner
-> gradientTop (and update all references) so the bindable names match the
target properties used in TestSceneRankedPlayBackground, then update the
BindValueChanged calls to use the new names (keeping the same lambda assignments
to background.GradientBottom and background.GradientTop).
In `@osu.Game.Tests/Visual/RankedPlay/TestSceneRankedPlayStageOverlay.cs`:
- Around line 43-88: The tests set the scene root directly by assigning Child =
new RankedPlayStageOverlay(...) in AddStep (e.g., in TestLongUsername,
TestColourScheme and the earlier create step), which bypasses the pre-built
hierarchy; change these to mount the overlay into the existing scene root by
using Content.Child = ... or Add(...) instead of assigning Child so the overlay
is placed inside the existing container created in the test setup (look for
usages of RankedPlayStageOverlay constructed inside AddStep where
PickingUser/Multiplier are set and replace Child = with Content.Child = or
Add(...)).
In `@osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselFiltering.cs`:
- Line 37: The three AddUntilStep calls that assert
NewItemsPresentedInvocationCount all share the identical step title "invocation
count correct", which makes log triage hard; update each AddUntilStep invocation
to use a unique, descriptive name that includes the expected count (e.g.
"invocation count correct (expected 1)" or "invocation count == 2") so failures
show which expectation failed—locate the AddUntilStep calls that reference
NewItemsPresentedInvocationCount and change their first argument to include the
expected value for that assertion.
In `@osu.Game/Graphics/Backgrounds/Triangles.cs`:
- Line 69: Make the SpawnRatio setter validate and apply changes immediately:
clamp incoming value to a minimum of 0, check if the clamped value differs from
the current SpawnRatio, assign it, and call Reset() to refresh internal
state/target counts; update the auto-property at line with a full property
implementation that uses the backed value, performs the clamp (>= 0), compares
old vs new, and invokes Reset() when changed.
In `@osu.Game/Online/API/Requests/Responses/APIUserMatchmakingStatistics.cs`:
- Around line 10-14: In APIUserMatchmakingStatistics change the public field
UserId into an auto-property to match the rest of the DTO: replace the field
declaration for UserId with a public int UserId { get; set; } and keep the
existing [JsonProperty("user_id")] attribute intact so serialization behavior
remains the same (leave PoolId as-is).
In `@osu.Game/Online/Matchmaking/IMatchmakingServer.cs`:
- Line 21: Add XML documentation to the MatchmakingJoinLobbyWithParams method to
document the request parameter's semantics (especially the selected-pool
behavior). Update the IMatchmakingServer interface's
MatchmakingJoinLobbyWithParams(MatchmakingJoinLobbyRequest request) declaration
to include a <param name="request"> entry that explains what fields of
MatchmakingJoinLobbyRequest are used and how the selected-pool value is
interpreted at the interface boundary.
In `@osu.Game/Overlays/Dashboard/UserSearch/UserPanelList.cs`:
- Around line 14-16: The comment in UserPanelList.cs notes heavy copy-paste from
FriendList; to address this tech debt later, extract the duplicated creation
logic into a shared factory/helper (e.g., a new UserPanelFactory or
UserPanelBuilder) that encapsulates common construction/configuration used by
FriendList and UserPanelList; locate the duplicated code in the UserPanelList
and FriendList classes (methods that assemble/return UserPanel instances), move
that logic into the new factory with clear methods (e.g., CreateUserPanel(user,
isFriend, callbacks)), and update both UserPanelList and FriendList to use the
factory so future updates live in one place.
In `@osu.Game/Overlays/Profile/Header/Components/MatchmakingStatsDisplay.cs`:
- Line 61: In MatchmakingStatsDisplay replace the hardcoded label Text = "Quick
Play" with a lookup to a localisation resource (e.g. use the
framework/localisation API to return a LocalisableString) so the visible label
comes from a resource key like "Matchmaking.QuickPlay"; add the
"Matchmaking.QuickPlay" entry to the app's resource file(s), update the Text
assignment in the MatchmakingStatsDisplay class to use the localisation getter
(and import the localisation namespace or change the property type to
LocalisableString if required).
In `@osu.Game/Overlays/Profile/Header/Components/MatchmakingStatsTooltip.cs`:
- Around line 71-74: In MatchmakingStatsTooltip replace the hardcoded
TableColumn headers ("Wins", "Plays", "Points", "Rating") with localized
strings: add localisation entries (e.g. keys like "tooltip.matchmaking.wins",
"tooltip.matchmaking.plays", "tooltip.matchmaking.points",
"tooltip.matchmaking.rating") in the project's resources and use the
localisation API (or LocalisableString properties) when constructing the
TableColumn instances instead of the literal strings (i.e., change new
TableColumn("Wins", ...) to new
TableColumn(YourLocalisation.GetString("tooltip.matchmaking.wins"), ...),
similarly for the other three headers).
In `@osu.Game/Screens/Footer/ScreenFooterButton.cs`:
- Around line 71-86: The Action setter currently wraps the provided delegate in
base.Action but allows invocation for any state other than Visibility.Hidden;
change the guard to only invoke when State.Value == Visibility.Visible to be
explicit and future-proof. Locate the Action property setter in
ScreenFooterButton (the new Action property) and replace the conditional inside
base.Action's lambda so it checks State.Value == Visibility.Visible before
calling value.Invoke(); keep null handling the same.
In
`@osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayStageOverlay.cs`:
- Around line 138-153: The hardcoded stage-detail strings should be replaced
with localized resource lookups: change the OsuSpriteText.Text that currently
uses $"{PickingUser.Username}'s pick" to a localized formatted string (e.g. a
resource key like "ranked_stage_pick") and pass PickingUser.Username as the
format argument, and likewise replace $"{Multiplier:N0}x damage" with a
localized formatted string (e.g. "ranked_damage_multiplier") passing Multiplier
formatted as N0; update the resources/strings file with those keys and use the
project's localization API (the same approach used elsewhere in
RankedPlayStageOverlay.cs when setting text) so the OsuSpriteText.Text values
are obtained from localization rather than hardcoded literals.
In `@osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlaySubScreen.cs`:
- Around line 54-56: Update the XML documentation on the RankedPlaySubScreen
class: change the phrase "content don't overlap" to grammatically correct
"content doesn't overlap" in the summary for the central column (the comment
referencing RankedPlayCornerPiece) so the doc reads correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc6c73df-1f0b-47d2-ba76-0cb11c30d346
📒 Files selected for processing (87)
osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.csosu.Game.Rulesets.Osu/Skinning/Default/TrianglesPiece.csosu.Game.Tests/Beatmaps/TestSceneBeatmapDifficultyCache.csosu.Game.Tests/Database/BackgroundDataStoreProcessorTests.csosu.Game.Tests/Visual/Editing/TestSceneEditorSaving.csosu.Game.Tests/Visual/Editing/TestSceneLocallyModifyingOnlineBeatmaps.csosu.Game.Tests/Visual/Editing/TestSceneOpenEditorTimestamp.csosu.Game.Tests/Visual/Gameplay/TestSceneGameplaySamplePlayback.csosu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.csosu.Game.Tests/Visual/Navigation/TestSceneScreenNavigation.csosu.Game.Tests/Visual/Online/TestSceneFriendDisplay.csosu.Game.Tests/Visual/Online/TestSceneUserProfileMatchmakingStatsDisplay.csosu.Game.Tests/Visual/Online/TestSceneUserSearchDisplay.csosu.Game.Tests/Visual/RankedPlay/TestSceneOpponentPickScreen.csosu.Game.Tests/Visual/RankedPlay/TestScenePickScreen.csosu.Game.Tests/Visual/RankedPlay/TestSceneRankedPlayBackground.csosu.Game.Tests/Visual/RankedPlay/TestSceneRankedPlayCornerPiece.csosu.Game.Tests/Visual/RankedPlay/TestSceneRankedPlayStageOverlay.csosu.Game.Tests/Visual/RankedPlay/TestSceneRankedPlayUserDisplay.csosu.Game.Tests/Visual/Ranking/TestSceneResultsScreen.csosu.Game.Tests/Visual/Ranking/TestSceneSoloResultsScreen.csosu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselFiltering.csosu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselSetsSplitApart.csosu.Game.Tests/Visual/SongSelect/TestSceneBeatmapRecommendations.csosu.Game.Tests/Visual/UserInterface/TestSceneNotificationOverlay.csosu.Game.Tests/Visual/UserInterface/TestSceneScreenFooter.csosu.Game.Tests/Visual/UserInterface/TestSceneUserListToolbar.csosu.Game/Database/BackgroundDataStoreProcessor.csosu.Game/Graphics/Backgrounds/Triangles.csosu.Game/Graphics/UserInterface/LoadingSpinner.csosu.Game/Localisation/UserInterfaceStrings.csosu.Game/Online/API/Requests/Responses/APIMatchmakingPool.csosu.Game/Online/API/Requests/Responses/APIUser.csosu.Game/Online/API/Requests/Responses/APIUserMatchmakingStatistics.csosu.Game/Online/API/Requests/SearchUsersRequest.csosu.Game/Online/API/Requests/SearchUsersResponse.csosu.Game/Online/Matchmaking/IMatchmakingServer.csosu.Game/Online/Matchmaking/MatchmakingLobbyStatus.csosu.Game/Online/Matchmaking/Requests/MatchmakingJoinLobbyRequest.csosu.Game/Online/Matchmaking/Responses/MatchmakingJoinLobbyResponse.csosu.Game/Online/Multiplayer/MatchTypes/RankedPlay/RankedPlayUserInfo.csosu.Game/Online/Multiplayer/MultiplayerClient.csosu.Game/Online/Multiplayer/OnlineMultiplayerClient.csosu.Game/Online/Spectator/OnlineSpectatorClient.csosu.Game/Online/Spectator/SpectatorClient.csosu.Game/Overlays/Dashboard/CurrentlyOnline/CurrentlyOnlineDisplay.csosu.Game/Overlays/Dashboard/DashboardOverlayHeader.csosu.Game/Overlays/Dashboard/Friends/FriendDisplay.csosu.Game/Overlays/Dashboard/Friends/UserListToolbar.csosu.Game/Overlays/Dashboard/UserSearch/UserPanelList.csosu.Game/Overlays/Dashboard/UserSearch/UserSearchDisplay.csosu.Game/Overlays/DashboardOverlay.csosu.Game/Overlays/MusicController.csosu.Game/Overlays/Notifications/ProgressNotification.csosu.Game/Overlays/Profile/Header/Components/MainDetails.csosu.Game/Overlays/Profile/Header/Components/MatchmakingStatsDisplay.csosu.Game/Overlays/Profile/Header/Components/MatchmakingStatsTooltip.csosu.Game/Overlays/Settings/Sections/General/UpdateSettings.csosu.Game/Overlays/SkinEditor/SkinSelectionHandler.csosu.Game/Screens/Footer/ScreenFooter.csosu.Game/Screens/Footer/ScreenFooterButton.csosu.Game/Screens/OnlinePlay/Matchmaking/Queue/ScreenQueue.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Components/BackgroundMusicManager.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Components/RankedPlayChatDisplay.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Components/RankedPlayUserDisplay.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/DiscardScreen.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/EndedScreen.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/GameplayScreen.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/GameplayWarmupScreen.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Hand/HandOfCards.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Intro/IntroScreen.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Intro/StarRatingSequence.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Intro/VsSequence.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/OpponentPickScreen.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/PickScreen.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayBackground.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayBackgroundScreen.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayColourScheme.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayMatchInfo.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayScreen.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayStageOverlay.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlaySubScreen.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/ResultsScreen.PanelScaffold.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/ResultsScreen.ScoreBar.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/ResultsScreen.csosu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.csosu.Game/Tests/Visual/Spectator/TestSpectatorClient.cs
|
|
||
| [Test] | ||
| [FlakyTest] | ||
| [Ignore("Still failing even with [FlakyTest] applied.")] |
There was a problem hiding this comment.
Avoid indefinitely disabling this regression test.
Line 23 fully skips this test, which drops coverage for a known seek/audio edge case. Please attach a concrete tracking issue + exit criteria (or keep it flaky/quarantined instead of ignored).
Suggested direction
-[Ignore("Still failing even with [FlakyTest] applied.")]
+[FlakyTest] // TODO: replace with issue link and remove once fixed📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [Ignore("Still failing even with [FlakyTest] applied.")] | |
| [FlakyTest] // TODO: replace with issue link and remove once fixed |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySamplePlayback.cs` at line
23, The test is being fully disabled via [Ignore] which removes coverage for the
regression in TestSceneGameplaySamplePlayback; replace the blanket ignore with a
quarantined/flaky marker (e.g., use [FlakyTest] or your project's quarantine
attribute) instead of [Ignore], add a short TODO comment above the
TestSceneGameplaySamplePlayback class/method referencing a concrete tracking
issue ID and clear exit criteria (what must be fixed to re-enable), and ensure
the test remains discoverable in CI reports so it can be retried/monitored
rather than permanently skipped.
| AddStep("invoke on back button", () => multiplayerComponents.OnBackButton()); | ||
|
|
||
| AddAssert("mod overlay is hidden", () => this.ChildrenOfType<MultiplayerUserModSelectOverlay>().Single().State.Value == Visibility.Hidden); | ||
| AddAssert("mod overlay is hidden", () => this.ChildrenOfType<MultiplayerUserModSelectOverlay>().All(o => o.State.Value == Visibility.Hidden)); |
There was a problem hiding this comment.
Prevent vacuous pass in the mod overlay assertion.
On Line 665, All(...) passes for an empty sequence, so this test can succeed even when no MultiplayerUserModSelectOverlay exists. Require presence + hidden state to avoid false positives.
Suggested fix
- AddAssert("mod overlay is hidden", () => this.ChildrenOfType<MultiplayerUserModSelectOverlay>().All(o => o.State.Value == Visibility.Hidden));
+ AddAssert("mod overlay is hidden", () =>
+ {
+ var overlays = this.ChildrenOfType<MultiplayerUserModSelectOverlay>().ToList();
+ return overlays.Count > 0 && overlays.All(o => o.State.Value == Visibility.Hidden);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| AddAssert("mod overlay is hidden", () => this.ChildrenOfType<MultiplayerUserModSelectOverlay>().All(o => o.State.Value == Visibility.Hidden)); | |
| AddAssert("mod overlay is hidden", () => | |
| { | |
| var overlays = this.ChildrenOfType<MultiplayerUserModSelectOverlay>().ToList(); | |
| return overlays.Count > 0 && overlays.All(o => o.State.Value == Visibility.Hidden); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs` at line 665, The
current assertion uses All(...) which vacuously passes for an empty sequence;
update the AddAssert call that checks MultiplayerUserModSelectOverlay to require
that at least one overlay exists and that every overlay's State.Value ==
Visibility.Hidden (e.g. combine Any(...) or Count>0 with the existing All(...)
check) so the assertion only passes when overlays are present and hidden; locate
the AddAssert invocation referencing MultiplayerUserModSelectOverlay in
TestSceneMultiplayer and adjust its predicate accordingly.
Comment on lines
+57
to
+82
| Pool = | ||
| { | ||
| Name = "Active Pool" | ||
| } | ||
| }, | ||
| new APIUserMatchmakingStatistics | ||
| { | ||
| Plays = 5, | ||
| FirstPlacements = 4, | ||
| Rank = 500, | ||
| Rating = 1000, | ||
| TotalPoints = 250, | ||
| Pool = | ||
| { | ||
| Name = "Inactive Pool" | ||
| } | ||
| }, | ||
| new APIUserMatchmakingStatistics | ||
| { | ||
| Rating = 1500, | ||
| IsRatingProvisional = true, | ||
| Pool = | ||
| { | ||
| Name = "Provisional" | ||
| } | ||
| } |
There was a problem hiding this comment.
Test fixture doesn’t actually exercise pool-priority selection.
Lines 57-82 only set Pool.Name, but relevance sorting depends on Pool.Active and Pool.Id. This makes the test rely on defaults/insertion order and can miss ordering regressions.
🧪 Suggested fixture hardening
new APIUserMatchmakingStatistics
{
Plays = 10,
FirstPlacements = 8,
Rank = 1000,
Rating = 2000,
TotalPoints = 500,
Pool =
{
+ Id = 10,
+ Active = true,
Name = "Active Pool"
}
},
new APIUserMatchmakingStatistics
{
Plays = 5,
FirstPlacements = 4,
Rank = 500,
Rating = 1000,
TotalPoints = 250,
Pool =
{
+ Id = 1,
+ Active = false,
Name = "Inactive Pool"
}
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game.Tests/Visual/Online/TestSceneUserProfileMatchmakingStatsDisplay.cs`
around lines 57 - 82, The test fixture sets only Pool.Name which doesn't
exercise pool-priority selection; update the three APIUserMatchmakingStatistics
instances in TestSceneUserProfileMatchmakingStatsDisplay (the objects with
properties Plays, FirstPlacements, Rank, Rating, TotalPoints,
IsRatingProvisional) to explicitly set Pool.Active and Pool.Id (use distinct ids
that reflect intended priority ordering, e.g., active pool Active=true Id=1,
inactive pool Active=false Id=2, provisional pool Active=true/false with its own
Id) so the relevance sorting logic depends on explicit Active/Id values rather
than insertion order or defaults.
Comment on lines
+55
to
+77
| for (int i = 0; i < 3; i++) | ||
| { | ||
| int i2 = i; | ||
| AddStep($"click card {i2}", () => | ||
| { | ||
| InputManager.MoveMouseTo(this.ChildrenOfType<PlayerHandOfCards.PlayerHandCard>().ElementAt(i2)); | ||
| InputManager.Click(MouseButton.Left); | ||
| }); | ||
| } | ||
|
|
||
| AddWaitStep("wait", 3); | ||
|
|
||
| AddStep("click play button", () => | ||
| { | ||
| var button = screen | ||
| .ChildrenOfType<PlayerHandOfCards.PlayerHandCard>() | ||
| .First(it => it.Selected) | ||
| .ChildrenOfType<ShearedButton>() | ||
| .First(); | ||
|
|
||
| InputManager.MoveMouseTo(button); | ||
| InputManager.Click(MouseButton.Left); | ||
| }); |
There was a problem hiding this comment.
Add deterministic readiness checks before indexed/First() UI access.
Line 60 (ElementAt(i2)) and Lines 71-73 (First(...)/First()) can intermittently throw when the drawable tree is not ready yet; Line 65’s fixed wait does not guarantee readiness and makes the test flaky on CI.
🔧 Proposed stabilization
+ AddUntilStep("at least 3 cards visible",
+ () => this.ChildrenOfType<PlayerHandOfCards.PlayerHandCard>().Count() >= 3);
+
for (int i = 0; i < 3; i++)
{
int i2 = i;
AddStep($"click card {i2}", () =>
{
- InputManager.MoveMouseTo(this.ChildrenOfType<PlayerHandOfCards.PlayerHandCard>().ElementAt(i2));
+ var card = this.ChildrenOfType<PlayerHandOfCards.PlayerHandCard>().ElementAt(i2);
+ InputManager.MoveMouseTo(card);
InputManager.Click(MouseButton.Left);
});
}
- AddWaitStep("wait", 3);
+ AddUntilStep("selected card has play button", () =>
+ screen.ChildrenOfType<PlayerHandOfCards.PlayerHandCard>()
+ .Any(c => c.Selected && c.ChildrenOfType<ShearedButton>().Any()));
AddStep("click play button", () =>
{
- var button = screen
- .ChildrenOfType<PlayerHandOfCards.PlayerHandCard>()
- .First(it => it.Selected)
- .ChildrenOfType<ShearedButton>()
- .First();
+ var selectedCard = screen.ChildrenOfType<PlayerHandOfCards.PlayerHandCard>().First(c => c.Selected);
+ var button = selectedCard.ChildrenOfType<ShearedButton>().First();
InputManager.MoveMouseTo(button);
InputManager.Click(MouseButton.Left);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (int i = 0; i < 3; i++) | |
| { | |
| int i2 = i; | |
| AddStep($"click card {i2}", () => | |
| { | |
| InputManager.MoveMouseTo(this.ChildrenOfType<PlayerHandOfCards.PlayerHandCard>().ElementAt(i2)); | |
| InputManager.Click(MouseButton.Left); | |
| }); | |
| } | |
| AddWaitStep("wait", 3); | |
| AddStep("click play button", () => | |
| { | |
| var button = screen | |
| .ChildrenOfType<PlayerHandOfCards.PlayerHandCard>() | |
| .First(it => it.Selected) | |
| .ChildrenOfType<ShearedButton>() | |
| .First(); | |
| InputManager.MoveMouseTo(button); | |
| InputManager.Click(MouseButton.Left); | |
| }); | |
| AddUntilStep("at least 3 cards visible", | |
| () => this.ChildrenOfType<PlayerHandOfCards.PlayerHandCard>().Count() >= 3); | |
| for (int i = 0; i < 3; i++) | |
| { | |
| int i2 = i; | |
| AddStep($"click card {i2}", () => | |
| { | |
| var card = this.ChildrenOfType<PlayerHandOfCards.PlayerHandCard>().ElementAt(i2); | |
| InputManager.MoveMouseTo(card); | |
| InputManager.Click(MouseButton.Left); | |
| }); | |
| } | |
| AddUntilStep("selected card has play button", () => | |
| screen.ChildrenOfType<PlayerHandOfCards.PlayerHandCard>() | |
| .Any(c => c.Selected && c.ChildrenOfType<ShearedButton>().Any())); | |
| AddStep("click play button", () => | |
| { | |
| var selectedCard = screen.ChildrenOfType<PlayerHandOfCards.PlayerHandCard>().First(c => c.Selected); | |
| var button = selectedCard.ChildrenOfType<ShearedButton>().First(); | |
| InputManager.MoveMouseTo(button); | |
| InputManager.Click(MouseButton.Left); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game.Tests/Visual/RankedPlay/TestScenePickScreen.cs` around lines 55 -
77, The test uses fixed waits and directly calls ElementAt(i2) and First(...) on
the drawable tree which can be null/unready; replace the fixed AddWaitStep and
direct indexed/First access with deterministic readiness checks (e.g.,
AddUntilStep/AddWaitUntil or WaitForCondition) that wait until
screen.ChildrenOfType<PlayerHandOfCards.PlayerHandCard>().Count() > i2 before
clicking in the loop, and wait until
screen.ChildrenOfType<PlayerHandOfCards.PlayerHandCard>().Any(it => it.Selected)
and that the selected card has a child ShearedButton available before calling
First()/clicking; keep using InputManager.MoveMouseTo/Click but only after those
conditions are satisfied.
Comment on lines
+743
to
+744
| // Report the updated item count rather than the total processed. Users don't really care about noops here. | ||
| completeNotification(notification, updatedCount, updatedCount, failedCount); |
There was a problem hiding this comment.
Failures may be silently ignored in the completion notification.
By passing updatedCount as both processedCount and totalCount, the condition processedCount == totalCount at line 770 always evaluates to true, which means the success branch is always taken. If any items failed (e.g., failedCount > 0), users won't see the failure message since it's only shown in the else branch at lines 776-784.
Consider either:
- Passing the original
beatmapIds.CountastotalCountso failures surface properly, or - Explicitly checking
failedCountbefore deciding which completion path to take.
Proposed fix option 1: use original total count
- // Report the updated item count rather than the total processed. Users don't really care about noops here.
- completeNotification(notification, updatedCount, updatedCount, failedCount);
+ // Report the updated item count. Use processedCount for completion check so failures surface.
+ completeNotification(notification, updatedCount, processedCount + failedCount, failedCount);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Report the updated item count rather than the total processed. Users don't really care about noops here. | |
| completeNotification(notification, updatedCount, updatedCount, failedCount); | |
| // Report the updated item count. Use processedCount for completion check so failures surface. | |
| completeNotification(notification, updatedCount, processedCount + failedCount, failedCount); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game/Database/BackgroundDataStoreProcessor.cs` around lines 743 - 744,
The completion notification currently passes updatedCount as both processedCount
and totalCount to completeNotification, causing the processedCount==totalCount
check in the completion logic to always evaluate true and mask any failures;
update the call to completeNotification to pass the original total (e.g.
beatmapIds.Count) as totalCount so failures surface correctly, or alternatively
modify the completion logic (the branch that checks processedCount ==
totalCount) to also consider failedCount (e.g. treat failedCount > 0 as a
failure) before choosing the success vs failure notification path—refer to
completeNotification, updatedCount, failedCount and beatmapIds.Count to locate
and apply the fix.
Comment on lines
+200
to
+212
| private void onSelectedPoolChanged(ValueChangedEvent<MatchmakingPool?> e) | ||
| { | ||
| if (e.NewValue == null) | ||
| { | ||
| client.MatchmakingLeaveLobby(); | ||
| return; | ||
| } | ||
|
|
||
| client.MatchmakingJoinLobbyWithParams(new MatchmakingJoinLobbyRequest | ||
| { | ||
| PoolId = e.NewValue.Id | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd -a 'ScreenQueue.cs' | head -n1)"
echo "Inspecting: $file"
# Verify direct matchmaking task invocations in ScreenQueue and check whether they are fire-and-forget.
rg -nP -C2 'client\.Matchmaking(JoinLobbyWithParams|LeaveLobby)\(' "$file"Repository: winnerspiros/osu
Length of output: 760
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="osu.Game/Screens/OnlinePlay/Matchmaking/Queue/ScreenQueue.cs"
echo "=== Full context of lines 200-212 ==="
sed -n '200,212p' "$file" | cat -n
echo ""
echo "=== Method signatures for the called methods ==="
# Search for method definitions in the codebase to verify return types
rg -B2 'MatchmakingLeaveLobby|MatchmakingJoinLobbyWithParams' --type cs -g '*.cs' | grep -E '(Task|MatchmakingLeave|MatchmakingJoin)' | head -20Repository: winnerspiros/osu
Length of output: 2761
Use FireAndForget() for matchmaking task calls in pool-change handler.
Lines 204 and 208 discard Tasks returned by MatchmakingLeaveLobby() and MatchmakingJoinLobbyWithParams(), which can hide async failures. Use FireAndForget() (as used elsewhere in this file) to handle these operations explicitly.
Suggested fix
private void onSelectedPoolChanged(ValueChangedEvent<MatchmakingPool?> e)
{
if (e.NewValue == null)
{
- client.MatchmakingLeaveLobby();
+ client.MatchmakingLeaveLobby().FireAndForget();
return;
}
- client.MatchmakingJoinLobbyWithParams(new MatchmakingJoinLobbyRequest
- {
- PoolId = e.NewValue.Id
- });
+ client.MatchmakingJoinLobbyWithParams(new MatchmakingJoinLobbyRequest
+ {
+ PoolId = e.NewValue.Id
+ }).FireAndForget();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private void onSelectedPoolChanged(ValueChangedEvent<MatchmakingPool?> e) | |
| { | |
| if (e.NewValue == null) | |
| { | |
| client.MatchmakingLeaveLobby(); | |
| return; | |
| } | |
| client.MatchmakingJoinLobbyWithParams(new MatchmakingJoinLobbyRequest | |
| { | |
| PoolId = e.NewValue.Id | |
| }); | |
| } | |
| private void onSelectedPoolChanged(ValueChangedEvent<MatchmakingPool?> e) | |
| { | |
| if (e.NewValue == null) | |
| { | |
| client.MatchmakingLeaveLobby().FireAndForget(); | |
| return; | |
| } | |
| client.MatchmakingJoinLobbyWithParams(new MatchmakingJoinLobbyRequest | |
| { | |
| PoolId = e.NewValue.Id | |
| }).FireAndForget(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game/Screens/OnlinePlay/Matchmaking/Queue/ScreenQueue.cs` around lines
200 - 212, The onSelectedPoolChanged handler currently discards the Tasks
returned by client.MatchmakingLeaveLobby() and
client.MatchmakingJoinLobbyWithParams(...); update the method to call
FireAndForget() on those Tasks instead so async failures are surfaced
consistently (i.e. replace direct calls to MatchmakingLeaveLobby() and
MatchmakingJoinLobbyWithParams(new MatchmakingJoinLobbyRequest { PoolId =
e.NewValue.Id }) with client.MatchmakingLeaveLobby().FireAndForget(...) and
client.MatchmakingJoinLobbyWithParams(...).FireAndForget(...), preserving any
existing logging/context usage pattern used elsewhere in this file.
Comment on lines
+81
to
+88
| var globalTrack = musicController.CurrentTrack; | ||
|
|
||
| bgm.VolumeTo(0) | ||
| .VolumeTo(1, track_fade_duration, Easing.InCubic); | ||
| globalTrack.VolumeTo(0, track_fade_duration, Easing.OutCubic); | ||
| globalTrackFadeDelegate = Scheduler.AddDelayed(() => | ||
| { | ||
| musicController.Stop(); | ||
| globalTrack.VolumeTo(1); | ||
| }, track_fade_duration); |
There was a problem hiding this comment.
Captured track reference may mismatch musicController state.
globalTrack is captured at the time the cross-fade starts, but the scheduled delegate runs 3 seconds later. If the music controller's current track changes during that window (e.g., playlist advancement, user navigation), musicController.Stop() will affect the new track while globalTrack.VolumeTo(1) resets the volume on the old track—potentially leaving stale volume state or unexpectedly stopping a different track.
Consider storing a reference to verify track identity before acting, or operating solely on the captured track without calling musicController.Stop().
Suggested approach
globalTrackFadeDelegate = Scheduler.AddDelayed(() =>
{
- musicController.Stop();
+ // Only stop if the captured track is still the current one
+ if (musicController.CurrentTrack == globalTrack)
+ musicController.Stop();
globalTrack.VolumeTo(1);
}, track_fade_duration);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var globalTrack = musicController.CurrentTrack; | |
| bgm.VolumeTo(0) | |
| .VolumeTo(1, track_fade_duration, Easing.InCubic); | |
| globalTrack.VolumeTo(0, track_fade_duration, Easing.OutCubic); | |
| globalTrackFadeDelegate = Scheduler.AddDelayed(() => | |
| { | |
| musicController.Stop(); | |
| globalTrack.VolumeTo(1); | |
| }, track_fade_duration); | |
| var globalTrack = musicController.CurrentTrack; | |
| globalTrack.VolumeTo(0, track_fade_duration, Easing.OutCubic); | |
| globalTrackFadeDelegate = Scheduler.AddDelayed(() => | |
| { | |
| // Only stop if the captured track is still the current one | |
| if (musicController.CurrentTrack == globalTrack) | |
| musicController.Stop(); | |
| globalTrack.VolumeTo(1); | |
| }, track_fade_duration); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Components/BackgroundMusicManager.cs`
around lines 81 - 88, The scheduled delegate captures globalTrack but later
unconditionally calls musicController.Stop(), risking stopping a different
track; change the delayed callback to operate safely on the captured track:
capture the local ITrack (globalTrack), null-check it, call captured.VolumeTo(1)
to restore its volume, and only call musicController.Stop() if
musicController.CurrentTrack == captured (or skip Stop entirely if it differs)
to avoid affecting a new current track; also ensure globalTrackFadeDelegate is
replaced/cancelled appropriately and handle possible null CurrentTrack when
scheduling in Scheduler.AddDelayed.
Comment on lines
+174
to
+175
| localUser = users.GetUserAsync(localUserId).GetResultSafely()!; | ||
| opponentUser = users.GetUserAsync(opponentUserId).GetResultSafely()!; |
There was a problem hiding this comment.
Avoid null-forgiving user lookups.
Line 174 and Line 175 force non-null lookup results (!). If either lookup misses, downstream UI construction can throw.
Suggested fix
- localUser = users.GetUserAsync(localUserId).GetResultSafely()!;
- opponentUser = users.GetUserAsync(opponentUserId).GetResultSafely()!;
+ localUser = users.GetUserAsync(localUserId).GetResultSafely()
+ ?? new APIUser { Id = localUserId, Username = api.LocalUser.Value.Username };
+ opponentUser = users.GetUserAsync(opponentUserId).GetResultSafely()
+ ?? new APIUser { Id = opponentUserId, Username = $"User {opponentUserId}" };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayScreen.cs`
around lines 174 - 175, The code currently uses null-forgiving operators on the
results of users.GetUserAsync(...).GetResultSafely() for localUser and
opponentUser which can crash later if the lookups returned null; change to
capture the nullable results into variables (e.g., var localUser =
users.GetUserAsync(localUserId).GetResultSafely(); var opponentUser =
users.GetUserAsync(opponentUserId).GetResultSafely();) and then check for null
before proceeding: if either is null, log a clear error via the same logger,
show an appropriate UI/error state or abort constructing the match screen (e.g.,
return/Close() or display a placeholder) so downstream code that expects
non-null users never receives a forced-null value. Ensure references to
GetUserAsync/GetResultSafely/localUser/opponentUser are updated accordingly.
Comment on lines
+223
to
+246
| if (screen.ShowStageOverlay) | ||
| { | ||
| APIUser? pickingUser = null; | ||
| double? multiplier = matchInfo.Stage.Value < RankedPlayStage.CardPlay ? null : matchInfo.RoomState.DamageMultiplier; | ||
| RankedPlayColourScheme colourScheme = RankedPlayColourScheme.BLUE; | ||
|
|
||
| if (matchInfo.Stage.Value == RankedPlayStage.CardPlay && matchInfo.RoomState.ActiveUser != null) | ||
| { | ||
| pickingUser = matchInfo.IsOwnTurn ? localUser : opponentUser; | ||
| colourScheme = matchInfo.IsOwnTurn ? RankedPlayColourScheme.BLUE : RankedPlayColourScheme.RED; | ||
| } | ||
|
|
||
| stageOverlayContainer.Add(new RankedPlayStageOverlay(screen.StageHeading, colourScheme) | ||
| { | ||
| PickingUser = pickingUser, | ||
| Multiplier = multiplier, | ||
| }); | ||
|
|
||
| rankedPlayBackground.ColourScheme = colourScheme; | ||
| } | ||
| else | ||
| { | ||
| rankedPlayBackground.ColourScheme = null; | ||
| } |
There was a problem hiding this comment.
Clear stale overlays when switching sub-screens.
stageOverlayContainer is never cleared. This can stack multiple RankedPlayStageOverlay instances and leave stale overlay UI visible on screens where overlay should be hidden.
Suggested fix
+ stageOverlayContainer.Clear();
+
if (screen.ShowStageOverlay)
{
APIUser? pickingUser = null;
double? multiplier = matchInfo.Stage.Value < RankedPlayStage.CardPlay ? null : matchInfo.RoomState.DamageMultiplier;
RankedPlayColourScheme colourScheme = RankedPlayColourScheme.BLUE;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (screen.ShowStageOverlay) | |
| { | |
| APIUser? pickingUser = null; | |
| double? multiplier = matchInfo.Stage.Value < RankedPlayStage.CardPlay ? null : matchInfo.RoomState.DamageMultiplier; | |
| RankedPlayColourScheme colourScheme = RankedPlayColourScheme.BLUE; | |
| if (matchInfo.Stage.Value == RankedPlayStage.CardPlay && matchInfo.RoomState.ActiveUser != null) | |
| { | |
| pickingUser = matchInfo.IsOwnTurn ? localUser : opponentUser; | |
| colourScheme = matchInfo.IsOwnTurn ? RankedPlayColourScheme.BLUE : RankedPlayColourScheme.RED; | |
| } | |
| stageOverlayContainer.Add(new RankedPlayStageOverlay(screen.StageHeading, colourScheme) | |
| { | |
| PickingUser = pickingUser, | |
| Multiplier = multiplier, | |
| }); | |
| rankedPlayBackground.ColourScheme = colourScheme; | |
| } | |
| else | |
| { | |
| rankedPlayBackground.ColourScheme = null; | |
| } | |
| stageOverlayContainer.Clear(); | |
| if (screen.ShowStageOverlay) | |
| { | |
| APIUser? pickingUser = null; | |
| double? multiplier = matchInfo.Stage.Value < RankedPlayStage.CardPlay ? null : matchInfo.RoomState.DamageMultiplier; | |
| RankedPlayColourScheme colourScheme = RankedPlayColourScheme.BLUE; | |
| if (matchInfo.Stage.Value == RankedPlayStage.CardPlay && matchInfo.RoomState.ActiveUser != null) | |
| { | |
| pickingUser = matchInfo.IsOwnTurn ? localUser : opponentUser; | |
| colourScheme = matchInfo.IsOwnTurn ? RankedPlayColourScheme.BLUE : RankedPlayColourScheme.RED; | |
| } | |
| stageOverlayContainer.Add(new RankedPlayStageOverlay(screen.StageHeading, colourScheme) | |
| { | |
| PickingUser = pickingUser, | |
| Multiplier = multiplier, | |
| }); | |
| rankedPlayBackground.ColourScheme = colourScheme; | |
| } | |
| else | |
| { | |
| rankedPlayBackground.ColourScheme = null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayScreen.cs`
around lines 223 - 246, stageOverlayContainer is never cleared causing stale
RankedPlayStageOverlay instances to accumulate; before adding a new
RankedPlayStageOverlay in the ShowStageOverlay branch, clear existing children
from stageOverlayContainer (e.g. call its Clear/RemoveAll method), and also
clear stageOverlayContainer when ShowStageOverlay is false so no overlays remain
visible; update the logic around ShowStageOverlay, RankedPlayStageOverlay, and
stageOverlayContainer to ensure at most one current overlay is present.
| Origin = Anchor.CentreLeft, | ||
| UseFullGlyphHeight = false, | ||
| Font = OsuFont.Torus.With(size: 32), | ||
| Text = $"{Multiplier:N0}x damage", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether multiplier source values are integral or fractional in ranked-play flow.
rg -n -C3 --type=cs '\bDamageMultiplier\b|\bMultiplier\s*='Repository: winnerspiros/osu
Length of output: 10420
🏁 Script executed:
sed -n '145,165p' osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayStageOverlay.csRepository: winnerspiros/osu
Length of output: 768
🏁 Script executed:
# Check if there are any constraints on DamageMultiplier values
rg -n -C5 'DamageMultiplier' osu.Game/Online/Multiplayer/MatchTypes/RankedPlay/RankedPlayRoomState.csRepository: winnerspiros/osu
Length of output: 400
🏁 Script executed:
# Check the ToStandardFormattedString method to understand the difference
rg -n 'ToStandardFormattedString' --type=csRepository: winnerspiros/osu
Length of output: 3807
Align formatting with ResultsScreen for consistency.
Line 153 uses N0 format which rounds to an integer, but DamageMultiplier is a double type. This is inconsistent with ResultsScreen.cs (lines 416, 430), which formats the same field as .ToStandardFormattedString(maxDecimalDigits: 1) to preserve fractional values. Use the same formatting to ensure multipliers like 1.5 display correctly:
Text = $"{Multiplier?.ToStandardFormattedString(maxDecimalDigits: 1)}x damage",
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayStageOverlay.cs`
at line 153, In RankedPlayStageOverlay replace the integer N0 formatting used
for the Multiplier display with the same formatting used in ResultsScreen so
fractional multipliers show (use
Multiplier?.ToStandardFormattedString(maxDecimalDigits: 1) instead of
$"{Multiplier:N0}"); update the Text assignment in the RankedPlayStageOverlay
display (the Multiplier/text generation site) to use that method and preserve
the trailing "x damage" suffix.
Just an initial grab bag to keep these PRs small. ### Avoid showing countdown update when at discard screen This is needless. We already have the `DiscardFinish` stage which has a short countdown. Playing this change to the user creates unnecessary confusion. ### Allow stage caption text to be changed at any point Also remove custom colour support. We'll handle this internally in a better way in the future. ### Better explain why we're waiting after discarding our own cards
Just the bare minimum code quality so I can start working on these classes.. Please push back if this doesn't seem better than what was already there. This is mostly autopilot fixing for me based on how I've been writing code for osu! to date. There are changes to the load process but nothing which should cause issues, I hope.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/ResultScreen.MainPanel.cs (1)
564-564: Consider makinggravitya static readonly field to avoid per-frame allocations.The
gravityproperty creates a newVector2on each access inUpdate(). While the impact is minimal for short-lived particles, a static field is more efficient.Proposed fix
private partial class DamageParticle : Triangle { private Vector2 velocity = new Vector2(RNG.NextSingle(-0.3f, 0.3f), RNG.NextSingle(-0.3f, 0.3f)); - private Vector2 gravity => new Vector2(0, 0.0002f); + private static readonly Vector2 gravity = new Vector2(0, 0.0002f);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/ResultScreen.MainPanel.cs` at line 564, The gravity property currently returns a new Vector2 on each access (private Vector2 gravity => new Vector2(0, 0.0002f)), causing per-frame allocations when used in Update() or particle logic; change it to a static readonly field (e.g. private static readonly Vector2 gravity = new Vector2(0, 0.0002f)) and update any references to the property to use that field so the Vector2 is allocated once and reused across frames and instances.osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/DiscardScreen.cs (1)
240-242: Avoid baking the discard grace period into3200.Line 242 is implicitly coupled to the discard-stage transition delay. If that timing changes elsewhere, this caption will start appearing too early or too late. Please reuse a shared discard-delay constant here, or at least extract a named constant so the two stay aligned.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/DiscardScreen.cs` around lines 240 - 242, The hardcoded 3200ms delay in the waitingOpponentTextUpdate Scheduler.AddDelayed call should be replaced with a shared discard grace constant (e.g., DISCARD_GRACE_PERIOD_MS) or an existing constant used by DiscardStage countdown reset to keep timings aligned; update the call that sets StageCaption = "Waiting for your opponent..." in DiscardScreen (the waitingOpponentTextUpdate initialization using Scheduler.AddDelayed) to use that constant and ensure the constant is defined/used in both DiscardStage and DiscardScreen so changes to the discard-stage transition delay stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Components/RankedPlayStageDisplay.cs`:
- Around line 245-273: Card-discard stop events leave a stale countdownEndTime
so the overlay continues showing the old timeout; in onCountdownStopped when
countdown is a RankedPlayStageCountdown and stageCountdown.Stage ==
RankedPlayStage.CardDiscard, clear/suppress the visible timer by resetting
countdownEndTime (e.g. set countdownEndTime = DateTimeOffset.Now or otherwise
mark it inactive) and ensure activeStage is appropriately updated if you want
the stage to change; this prevents reuse of the previous countdown.TimeRemaining
and stops the stale timer from displaying.
In `@osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/ResultsScreen.cs`:
- Around line 124-125: The null-forgiving usage when assigning PlayerDamageInfo
and OpponentDamageInfo can throw if RankedPlayUserInfo.DamageInfo is null;
update the assignments in ResultsScreen (where you set PlayerDamageInfo =
matchInfo.RoomState.Users[localUserId].DamageInfo! and OpponentDamageInfo =
matchInfo.RoomState.Users[opponentId].DamageInfo!) to validate the nullable
DamageInfo first (e.g., check Users[localUserId].DamageInfo and
Users[opponentId].DamageInfo for null), and handle the null case by supplying a
safe default/empty DamageInfo, asserting/logging a descriptive warning, or
altering the downstream UI to accept null so no NullReferenceException occurs;
reference the RankedPlayUserInfo.DamageInfo property and the ResultsScreen
assignment locations when making this change.
- Around line 92-94: Capture the BeatmapInfo from working.Value into a local
variable on the UI thread before awaiting scoreLookup.Task, then use that local
variable when constructing ScoreInfo objects; specifically, before awaiting
scoreLookup.Task assign var beatmapInfo = working.Value.BeatmapInfo and after
awaiting use beatmapInfo in the Select call (instead of accessing
working.Value.BeatmapInfo) so you don't read the bindable on a background thread
in ResultsScreen when building scores with scoreManager and rulesets.
---
Nitpick comments:
In `@osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/DiscardScreen.cs`:
- Around line 240-242: The hardcoded 3200ms delay in the
waitingOpponentTextUpdate Scheduler.AddDelayed call should be replaced with a
shared discard grace constant (e.g., DISCARD_GRACE_PERIOD_MS) or an existing
constant used by DiscardStage countdown reset to keep timings aligned; update
the call that sets StageCaption = "Waiting for your opponent..." in
DiscardScreen (the waitingOpponentTextUpdate initialization using
Scheduler.AddDelayed) to use that constant and ensure the constant is
defined/used in both DiscardStage and DiscardScreen so changes to the
discard-stage transition delay stay in sync.
In
`@osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/ResultScreen.MainPanel.cs`:
- Line 564: The gravity property currently returns a new Vector2 on each access
(private Vector2 gravity => new Vector2(0, 0.0002f)), causing per-frame
allocations when used in Update() or particle logic; change it to a static
readonly field (e.g. private static readonly Vector2 gravity = new Vector2(0,
0.0002f)) and update any references to the property to use that field so the
Vector2 is allocated once and reused across frames and instances.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b142b68b-c5f2-4eae-ac7f-fe136a2a797f
📒 Files selected for processing (11)
osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Components/RankedPlayStageDisplay.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/DiscardScreen.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/EndedScreen.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/GameplayScreen.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/GameplayWarmupScreen.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Intro/IntroScreen.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/OpponentPickScreen.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/PickScreen.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlaySubScreen.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/ResultScreen.MainPanel.csosu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/ResultsScreen.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Intro/IntroScreen.cs
- osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/EndedScreen.cs
- osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/OpponentPickScreen.cs
- osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlaySubScreen.cs
Comment on lines
+245
to
273
| switch (stageCountdown.Stage) | ||
| { | ||
| case RankedPlayStage.CardDiscard: | ||
| // Discard stage ends when both players have discarded, but adds a 3 second delay before completing. | ||
| // Showing this in the countdown just creates visual noise, so let's handle internally. | ||
| if (activeStage == stageCountdown.Stage) | ||
| return; | ||
|
|
||
| break; | ||
| } | ||
|
|
||
| activeStage = stageCountdown.Stage; | ||
| countdownStartTime = DateTimeOffset.Now; | ||
| countdownEndTime = DateTimeOffset.Now + countdown.TimeRemaining; | ||
| }); | ||
|
|
||
| private void onCountdownStopped(MultiplayerCountdown countdown) => Scheduler.Add(() => | ||
| { | ||
| if (countdown is not RankedPlayStageCountdown) | ||
| if (countdown is not RankedPlayStageCountdown stageCountdown) | ||
| return; | ||
|
|
||
| switch (stageCountdown.Stage) | ||
| { | ||
| // See above special case handling. | ||
| case RankedPlayStage.CardDiscard: | ||
| return; | ||
| } | ||
|
|
||
| countdownEndTime = DateTimeOffset.Now; |
There was a problem hiding this comment.
Card-discard can keep showing a stale timer.
Lines 245-273 ignore the follow-up RankedPlayStageCountdown/stop events for RankedPlayStage.CardDiscard, but they also keep the original countdownEndTime. If the server shortens discard to the post-discard grace period, the overlay keeps counting down the old, longer timeout until the next stage starts. Please either suppress the timer for CardDiscard entirely or drive it from the latest discard countdown instead of reusing stale timing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Components/RankedPlayStageDisplay.cs`
around lines 245 - 273, Card-discard stop events leave a stale countdownEndTime
so the overlay continues showing the old timeout; in onCountdownStopped when
countdown is a RankedPlayStageCountdown and stageCountdown.Stage ==
RankedPlayStage.CardDiscard, clear/suppress the visible timer by resetting
countdownEndTime (e.g. set countdownEndTime = DateTimeOffset.Now or otherwise
mark it inactive) and ensure activeStage is appropriately updated if you want
the stage to change; this prevents reuse of the previous countdown.TimeRemaining
and stops the stale timer from displaying.
Comment on lines
+92
to
+94
| List<MultiplayerScore> apiScores = await scoreLookup.Task.ConfigureAwait(false); | ||
|
|
||
| Schedule(() => | ||
| { | ||
| if (playerScoreTickChannel != null && playerScoreBar.Height >= playerScorePercent) | ||
| playerScoreTickChannel.Looping = false; | ||
| ScoreInfo[] scores = apiScores.Select(s => s.CreateScoreInfo(scoreManager, rulesets, working.Value.BeatmapInfo)).ToArray(); |
There was a problem hiding this comment.
Capture BeatmapInfo before the async continuation to avoid cross-thread bindable access.
working.Value is accessed after ConfigureAwait(false), meaning this runs on a background thread. Bindable values should be read on the UI thread. Capture the reference before the await.
Proposed fix
+ BeatmapInfo beatmapInfo = working.Value.BeatmapInfo;
+
api.Queue(request);
List<MultiplayerScore> apiScores = await scoreLookup.Task.ConfigureAwait(false);
- ScoreInfo[] scores = apiScores.Select(s => s.CreateScoreInfo(scoreManager, rulesets, working.Value.BeatmapInfo)).ToArray();
+ ScoreInfo[] scores = apiScores.Select(s => s.CreateScoreInfo(scoreManager, rulesets, beatmapInfo)).ToArray();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| List<MultiplayerScore> apiScores = await scoreLookup.Task.ConfigureAwait(false); | |
| Schedule(() => | |
| { | |
| if (playerScoreTickChannel != null && playerScoreBar.Height >= playerScorePercent) | |
| playerScoreTickChannel.Looping = false; | |
| ScoreInfo[] scores = apiScores.Select(s => s.CreateScoreInfo(scoreManager, rulesets, working.Value.BeatmapInfo)).ToArray(); | |
| BeatmapInfo beatmapInfo = working.Value.BeatmapInfo; | |
| api.Queue(request); | |
| List<MultiplayerScore> apiScores = await scoreLookup.Task.ConfigureAwait(false); | |
| ScoreInfo[] scores = apiScores.Select(s => s.CreateScoreInfo(scoreManager, rulesets, beatmapInfo)).ToArray(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/ResultsScreen.cs` around
lines 92 - 94, Capture the BeatmapInfo from working.Value into a local variable
on the UI thread before awaiting scoreLookup.Task, then use that local variable
when constructing ScoreInfo objects; specifically, before awaiting
scoreLookup.Task assign var beatmapInfo = working.Value.BeatmapInfo and after
awaiting use beatmapInfo in the Select call (instead of accessing
working.Value.BeatmapInfo) so you don't read the bindable on a background thread
in ResultsScreen when building scores with scoreManager and rulesets.
Comment on lines
+124
to
+125
| PlayerDamageInfo = matchInfo.RoomState.Users[localUserId].DamageInfo!, | ||
| OpponentDamageInfo = matchInfo.RoomState.Users[opponentId].DamageInfo!, |
There was a problem hiding this comment.
Null-forgiving operator on nullable DamageInfo could cause runtime exception.
Per RankedPlayUserInfo, DamageInfo is nullable. While the doc states it's "only expected to be populated during the Results stage," using ! without validation risks a NullReferenceException if the server fails to provide damage info.
Consider adding a guard or assertion:
Proposed fix
+ Debug.Assert(matchInfo.RoomState.Users[localUserId].DamageInfo != null, "Expected DamageInfo to be populated during Results stage");
+ Debug.Assert(matchInfo.RoomState.Users[opponentId].DamageInfo != null, "Expected DamageInfo to be populated during Results stage");
+
LoadComponentAsync(new MainPanel
{
// ...
PlayerDamageInfo = matchInfo.RoomState.Users[localUserId].DamageInfo!,
OpponentDamageInfo = matchInfo.RoomState.Users[opponentId].DamageInfo!,
}, AddInternal);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| PlayerDamageInfo = matchInfo.RoomState.Users[localUserId].DamageInfo!, | |
| OpponentDamageInfo = matchInfo.RoomState.Users[opponentId].DamageInfo!, | |
| Debug.Assert(matchInfo.RoomState.Users[localUserId].DamageInfo != null, "Expected DamageInfo to be populated during Results stage"); | |
| Debug.Assert(matchInfo.RoomState.Users[opponentId].DamageInfo != null, "Expected DamageInfo to be populated during Results stage"); | |
| LoadComponentAsync(new MainPanel | |
| { | |
| PlayerDamageInfo = matchInfo.RoomState.Users[localUserId].DamageInfo!, | |
| OpponentDamageInfo = matchInfo.RoomState.Users[opponentId].DamageInfo!, | |
| }, AddInternal); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/ResultsScreen.cs` around
lines 124 - 125, The null-forgiving usage when assigning PlayerDamageInfo and
OpponentDamageInfo can throw if RankedPlayUserInfo.DamageInfo is null; update
the assignments in ResultsScreen (where you set PlayerDamageInfo =
matchInfo.RoomState.Users[localUserId].DamageInfo! and OpponentDamageInfo =
matchInfo.RoomState.Users[opponentId].DamageInfo!) to validate the nullable
DamageInfo first (e.g., check Users[localUserId].DamageInfo and
Users[opponentId].DamageInfo for null), and handle the null case by supplying a
safe default/empty DamageInfo, asserting/logging a descriptive warning, or
altering the downstream UI to accept null so no NullReferenceException occurs;
reference the RankedPlayUserInfo.DamageInfo property and the ResultsScreen
assignment locations when making this change.
Brings in new fresh icons for the toolbox (select, hitcircle, slider, spinner, grid) and toggles (new combo, whistle, finish, clap, grid/distance snap). Depends on ppy/osu-resources#416. | Before | After | |--------|--------| | <img width="300" height="897" alt="image" src="https://github.com/user-attachments/assets/279ea162-c3e2-4312-a0a2-39ad08df16f9" /> | <img width="304" height="889" alt="image" src="https://github.com/user-attachments/assets/aab2a143-0e6b-45c1-bf84-8998c12bd960" /> |
…ith some key counts (#37256) Closes #37232. The actual fix is e959b20; everything else is window dressing / test harness to ensure I don't try and do a wrong change like #37251 did. I recommend reviewing commit-by-commit. See [this desmos](https://www.desmos.com/calculator/a5yjpacvxa) for visual explanation of change, I think it does a better job at explaining this than any words I could type here. Of note: - In the end this did only affect 14K but that should never be assumed when floating point is involved. - Test cases generated here were generated in stable manually. - Except for 11 / 13 / 15 / 17K which are not officially supported and which don't work in lazer due to orthogonal reasons (see comment added in this PR in `ManiaBeatmapConverter`), decoding in lazer was always fine. - My worry was that the old encoding method before this PR could potentially cause stable to move a note from one column to another but thankfully that is not the case. The old method of encoding columns as X positions does not cause issues wherein lazer reads them back differently than stable after encode. I checked this by checking out `master`, re-encoding all of the test stair-pattern nK beatmaps added in this PR on `master`, exporting that as compatibility, re-importing to stable, and cross-checking that the decoded beatmap is visually the same on lazer and on stable. This is important to check because if this wasn't the case, we'd potentially have cases of actual online beatmaps (remember that we have BSS now) wherein a beatmap plays differently on stable than on lazer due to notes moving between columns, and would need to screen for this being the case and potentially apply corrective / reconciliatory action.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@osu.Game.Rulesets.Mania.Tests/ManiaLegacyBeatmapEncoderTest.cs`:
- Line 13: Replace the direct DllResourceStore instantiation in the static field
beatmaps_resource_store of ManiaLegacyBeatmapEncoderTest with the same
resource-store pattern used by the parent LegacyBeatmapEncoderTest: call
TestResources.GetStore() to obtain the centrally managed osu.Game.Tests resource
store (or alternatively add explicit <EmbeddedResource> entries for
Resources/Testing/Beatmaps/*.osu in the project file), so that resource loading
matches the parent class behavior and embedded beatmap resources are resolved
consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a22cae3-dee9-4d78-ba79-571d8deb80da
📒 Files selected for processing (52)
osu.Game.Rulesets.Mania.Tests.Android/osu.Game.Rulesets.Mania.Tests.Android.csprojosu.Game.Rulesets.Mania.Tests.iOS/osu.Game.Rulesets.Mania.Tests.iOS.csprojosu.Game.Rulesets.Mania.Tests/ManiaBeatmapConversionTest.csosu.Game.Rulesets.Mania.Tests/ManiaLegacyBeatmapEncoderTest.csosu.Game.Rulesets.Mania.Tests/Mods/TestSceneManiaModNoRelease.csosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/10K-expected-conversion.jsonosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/10K.osuosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/11K-expected-conversion.jsonosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/11K.osuosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/12K-expected-conversion.jsonosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/12K.osuosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/13K-expected-conversion.jsonosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/13K.osuosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/14K-expected-conversion.jsonosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/14K.osuosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/15K-expected-conversion.jsonosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/15K.osuosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/16K-expected-conversion.jsonosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/16K.osuosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/17K-expected-conversion.jsonosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/17K.osuosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/18K-expected-conversion.jsonosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/18K.osuosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/1K-expected-conversion.jsonosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/1K.osuosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/2K-expected-conversion.jsonosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/2K.osuosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/3K-expected-conversion.jsonosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/3K.osuosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/4K-expected-conversion.jsonosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/4K.osuosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/5K-expected-conversion.jsonosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/5K.osuosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/6K-expected-conversion.jsonosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/6K.osuosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/7K+1.osuosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/7K-expected-conversion.jsonosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/7K.osuosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/8K-expected-conversion.jsonosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/8K.osuosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/9K-expected-conversion.jsonosu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/9K.osuosu.Game.Rulesets.Mania.Tests/TestSceneHoldNoteInput.csosu.Game.Rulesets.Mania.Tests/TestSceneMaximumScore.csosu.Game.Rulesets.Mania.Tests/TestSceneOutOfOrderHits.csosu.Game.Rulesets.Mania.Tests/TestSceneReplayRewinding.csosu.Game.Rulesets.Mania.Tests/TestSceneTimingBasedNoteColouring.csosu.Game.Rulesets.Mania.Tests/osu.Game.Rulesets.Mania.Tests.csprojosu.Game.Rulesets.Mania/Beatmaps/ManiaBeatmapConverter.csosu.Game.Tests/Beatmaps/Formats/LegacyBeatmapEncoderTest.csosu.Game/Beatmaps/Formats/LegacyBeatmapEncoder.csosu.Game/Tests/Visual/EditorSavingTestScene.cs
✅ Files skipped from review due to trivial changes (39)
- osu.Game.Rulesets.Mania.Tests/osu.Game.Rulesets.Mania.Tests.csproj
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/13K-expected-conversion.json
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/10K-expected-conversion.json
- osu.Game.Rulesets.Mania/Beatmaps/ManiaBeatmapConverter.cs
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/18K.osu
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/12K-expected-conversion.json
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/3K-expected-conversion.json
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/4K-expected-conversion.json
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/2K.osu
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/5K-expected-conversion.json
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/1K.osu
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/7K-expected-conversion.json
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/14K-expected-conversion.json
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/9K.osu
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/7K.osu
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/11K-expected-conversion.json
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/17K-expected-conversion.json
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/5K.osu
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/6K-expected-conversion.json
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/8K-expected-conversion.json
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/4K.osu
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/15K.osu
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/17K.osu
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/7K+1.osu
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/15K-expected-conversion.json
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/16K-expected-conversion.json
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/2K-expected-conversion.json
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/9K-expected-conversion.json
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/1K-expected-conversion.json
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/6K.osu
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/10K.osu
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/16K.osu
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/18K-expected-conversion.json
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/12K.osu
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/8K.osu
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/14K.osu
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/11K.osu
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/13K.osu
- osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps/3K.osu
| [TestFixture] | ||
| public class ManiaLegacyBeatmapEncoderTest | ||
| { | ||
| private static readonly DllResourceStore beatmaps_resource_store = new DllResourceStore(typeof(ManiaLegacyBeatmapEncoderTest).Assembly); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify that test resource files exist for each test case
# Find resource files in the Mania tests project
echo "Looking for Mania test beatmap resources..."
fd -e osu . --search-path osu.Game.Rulesets.Mania.Tests/Resources/Testing/Beatmaps 2>/dev/null || echo "Directory not found or empty"
# Check csproj for embedded resources
echo -e "\nChecking .csproj for embedded resource configuration..."
fd -e csproj . --search-path osu.Game.Rulesets.Mania.Tests --exec cat {} | grep -A2 -B2 "EmbeddedResource\|Resources"Repository: winnerspiros/osu
Length of output: 2361
🏁 Script executed:
# Check the .csproj file for resource configuration
find osu.Game.Rulesets.Mania.Tests -name "*.csproj" -exec cat {} \;Repository: winnerspiros/osu
Length of output: 784
🏁 Script executed:
# Read the props file that's imported
cat osu.TestProject.propsRepository: winnerspiros/osu
Length of output: 586
🏁 Script executed:
# Read the test file to see complete context
cat osu.Game.Rulesets.Mania.Tests/ManiaLegacyBeatmapEncoderTest.csRepository: winnerspiros/osu
Length of output: 1895
🏁 Script executed:
# Search for other DllResourceStore usages in test files
rg "DllResourceStore" --type cs | head -20Repository: winnerspiros/osu
Length of output: 2507
🏁 Script executed:
# Read TestResources.cs to see how resources are configured
cat osu.Game.Tests/Resources/TestResources.csRepository: winnerspiros/osu
Length of output: 9767
🏁 Script executed:
# Read the parent test class
cat osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapEncoderTest.cs | head -100Repository: winnerspiros/osu
Length of output: 4317
Use the same resource store pattern as the parent test class.
The DllResourceStore is initialized from the Mania test assembly, but resource embedding is not explicitly configured in the project file. The parent class LegacyBeatmapEncoderTest uses TestResources.GetStore() to load from the centrally managed osu.Game.Tests assembly, which properly handles resource embedding. Either:
- Configure explicit
<EmbeddedResource>items in the.csprojforResources/Testing/Beatmaps/*.osu - Or align with the parent pattern by using
TestResources.GetStore()instead
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@osu.Game.Rulesets.Mania.Tests/ManiaLegacyBeatmapEncoderTest.cs` at line 13,
Replace the direct DllResourceStore instantiation in the static field
beatmaps_resource_store of ManiaLegacyBeatmapEncoderTest with the same
resource-store pattern used by the parent LegacyBeatmapEncoderTest: call
TestResources.GetStore() to obtain the centrally managed osu.Game.Tests resource
store (or alternatively add explicit <EmbeddedResource> entries for
Resources/Testing/Beatmaps/*.osu in the project file), so that resource loading
matches the parent class behavior and embedded beatmap resources are resolved
consistently.
- Stage changes (with ducking): https://github.com/user-attachments/assets/1ed1cf5a-759b-487a-9028-314accc15a6a - Messages (with debouncing): https://github.com/user-attachments/assets/c8c38d18-4fcc-4654-887b-be3de3e84949 Message sample might be a bit too clicky, but consider this a first pass. --- - [x] depends on ppy/osu-resources#417 --------- Co-authored-by: Dean Herbert <pe@ppy.sh>
The main goal here is to: - Make keyboard selection play previews just like hovering does with mouse. - Make sure cards don't play previews when they are in animation. - Drive by fix to fix toggling discard not working via keyboard. (I really want to rewrite all these classes, the structure is not great) --------- Co-authored-by: Dan Balasescu <smoogipoo@smgi.me>
- [x] Depends on #37226 - [x] Depends on ppy/osu-server-spectator#464 This adds two new components to the queue screen: - A listing of the most recently completed matches (global). - A rank distribution graph. It looks something like this (fake data / test scene): <img width="1669" height="1005" alt="image" src="https://github.com/user-attachments/assets/caa57119-4267-4c6e-9898-2f414de865bf" /> It's completely dev-design(TM), but I used Lichess as inspiration for the graph, and the original design document as inspiration for the panels. As for the history, because these are _completed_ matches one of the player life points will always be 0, but I've designed it so as to possibly support showing ongoing matches too in the future. It's only supported for ranked play right now, though there is no reason we couldn't track quick play rooms too (it's just... I'm not sure how to design the panels for quick play). --------- Co-authored-by: Dean Herbert <pe@ppy.sh>
This commit adds an always present overlay to all ranked play screens, meant to indicate to the user that a ranked play session in currently in progress. This has been largely inspired by the pre-shader argon healthbar code. It shouldn't have the same performance concerns, however, since the paths are only calculated once when loading the drawable (and eventually when it is resized, if ever). `RankedPlayScreen`: <img width="1838" height="1353" alt="image" src="https://github.com/user-attachments/assets/c621d759-a88f-49f0-b0df-3a6da90eca65" /> Gameplay: <img width="1838" height="1353" alt="image" src="https://github.com/user-attachments/assets/ee848d06-3878-4cbb-bd4b-de2c4bcfa688" /> Currently the drawable overlaps with some components, but it will be resolved in later pull requests. --------- Co-authored-by: Dean Herbert <pe@ppy.sh>
I was kind of lazy with the disappear/appear stuff. Made it properly set the required state at the correct time now. Made a second fix to change it into a visibility container, so that bindable states are deduped. On `master` it would re-appear from the bottom with every stage change.
The panels look up the online APIUser models. Maybe I could do this by doing the lookups async inside `RankedPlayMatchPanel`, but this will probably do for now?
Has been failing on master for some time now because .NET 6 is not supported anymore.
I don't know how to reproduce this: ``` [runtime] 2026-04-15 05:43:26 [verbose]: 📺 OsuScreenStack#478(depth:4) exit from ScreenQueue#281 [runtime] 2026-04-15 05:43:26 [verbose]: 📺 OsuScreenStack#478(depth:4) resume to ScreenIntro#687 [runtime] 2026-04-15 05:43:26 [verbose]: 📺 BackgroundScreenStack#692(depth:1) exit from MatchmakingBackgroundScreen#393 [runtime] 2026-04-15 05:43:26 [verbose]: 📺 BackgroundScreenStack#692(depth:1) resume to BackgroundScreenDefault#210 [runtime] 2026-04-15 05:43:26 [verbose]: 📺 OsuScreenStack#478(depth:3) exit from ScreenIntro#687 [runtime] 2026-04-15 05:43:26 [verbose]: 📺 OsuScreenStack#478(depth:3) resume to MainMenu#505 [runtime] 2026-04-15 05:43:26 [verbose]: 🌅 Global background change queued [runtime] 2026-04-15 05:43:26 [verbose]: ButtonSystem's state changed from EnteringMode to TopLevel [runtime] 2026-04-15 05:43:26 [debug]: Focus changed from nothing to DialogOverlay. [runtime] 2026-04-15 05:43:26 [error]: An unhandled error has occurred. [runtime] 2026-04-15 05:43:26 [error]: System.NullReferenceException: Object reference not set to an instance of an object. [runtime] 2026-04-15 05:43:26 [error]: at osu.Game.Screens.OnlinePlay.Matchmaking.Queue.RatingDistributionGraph.get_TooltipContent() in /home/smgi/Repos/osu/osu.Game/Screens/OnlinePlay/Matchmaking/Queue/RatingDistributionGraph.cs:line 434 [runtime] 2026-04-15 05:43:26 [error]: at osu.Framework.Graphics.Cursor.IHasCustomTooltip`1.osu.Framework.Graphics.Cursor.IHasCustomTooltip.get_TooltipContent() [runtime] 2026-04-15 05:43:26 [error]: at osu.Framework.Graphics.Cursor.TooltipContainer.hasValidTooltip(ITooltipContentProvider target) [runtime] 2026-04-15 05:43:26 [error]: at osu.Framework.Graphics.Cursor.TooltipContainer.Update() [runtime] 2026-04-15 05:43:26 [error]: at osu.Framework.Graphics.Drawable.UpdateSubTree() ``` As unplausible as it may seem, the only thing that can be null here is `GetContainingInputManager()`. The point of this exercise is to simply remove it by relying on `OnMouseMove` events instead.
https://github.com/user-attachments/assets/edfc8d06-4f04-4876-84a5-dfc83a18f160 Of note: - Supports both native beatmaps and converts - Supports key mods (changing key mods will trigger song select refilter when key count grouping is engaged) - The option to group by keys is only visible when mania ruleset is active - If the user selects key count grouping and then switches to another ruleset, song select will fall back to no grouping, but this change will not be written back to config. Only the user changing the grouping mode manually will reflect in config changes. This is done so that key grouping persists across ruleset changes, and this even survives game restarts. --- I've only done some light behaviour testing on this because this feature needs a lot of subjective shot calls and I don't want to commit too deep before I get a temperature check on the shot calls I made here. In particular some performance profiling of 7de8f70 may be warranted.
Fixes https://discord.com/channels/188630481301012481/188630652340404224/1493678774540304505 The rating distribution is updated once every 5 minutes, so there are periods where it may not include the local user's rating. This is simply a workaround where it's considered if it's bounded by it. Am a little surprised this is as easy to handle as it appears to be, even if not the cleanest presentation (it's an edge case).
Noticed in passing when doing something else.
…36285) In some cases `SliderPath.GetPathToProgress` used to compute the whole path when it can be not needed since it can be already stored inside `calculatedPath` list. Also some of these use cases will no longer require additional array wheen only readonly access is all we need.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
UI/UX Improvements
Tests
Summary by Gitar
SearchUsersRequestandUserSearchDisplaycomponentRankedPlayStageOverlayfor displaying stage information during ranked playRankedPlayBackgroundcustom shader implementation with simplerTrianglesandTrianglesV2compositionBackgroundMusicManagerstate handling withshouldBePlayingflagStarRatingSequenceUI layout and animation logicBeginPlayinginvocationThis will update automatically on new commits.