aa - #137
Merged
Merged
Conversation
Adds the ability to drag and reorder cards. Card order is preserved between rounds and is synchronized between both players (each player can see the other player drag around and reorder their cards). To make this possible I had to rewrite the card layout algorithm to be stateless (e0a46fe), there wasn't really a way around that since I needed a way to calculate a layout position based on a card's index. Should hopefully be a lot easier to read now though. Some noteworthy stuff: - I didn't really know what the best place to store the card order is, so I put it on `RankedPlayCardWithPlaylistItem` since that one will stay the same instance per round. - To prevent the opponent's cards from dragged into the middle of the playfield, only the x axis of the drag gets synchronized for the `OpponentHandOfCards` with a fixed y value. - I adjusted the replay recorder/player parameters a little. With the drag events happening every frame the replay recorder record new frames every 25ms and end up dropping half the replay frames per flush interval, so I increased the sample interval to 50ms so the buffer size matches the sample rate exactly (50ms -> 20 samples per flush every 1000ms). I also increased the buffer size in the replay player a bit so slight fluctuations in latency won't make it start to drop frames. https://github.com/user-attachments/assets/b810cb85-db02-4edf-a63e-bfc96cf59665 https://github.com/user-attachments/assets/4d2f884d-fcce-4948-9659-fbb314634cb8 --------- Co-authored-by: Dean Herbert <pe@ppy.sh>
Intended to add toggle but forgot. This also fixes #37012 via a convoluted refactor of a lot of stuff. The basic overview is: - Moved all replay overlay concerns out of `HUDOverlay`. We can display this above everything confidently (i think). - Split out `ReplayOverlay` and `ReplaySettingsOverlay` so the base class can handle the visibility, hotkeys and everything that should be shared with *all* replay overlay components going forward. `Ctrl+H` is supposed to hide any of these kinds of details, and I'm sure we'll add more in the future. - Reorganised some things in `Player` so the new structure would work. Mainly the overlays which add a black layer during fade out.
- Closes #31684 Uses the global action variant of up and down for dropdown menus. It is already used in multiplayer/playlist room/beatmap navigation and gameplay menu overlays. Comment wording is derived from: https://github.com/ppy/osu/blob/fc817627e56b7db1c46e2f00fc9a3bd14af51211/osu.Game/Graphics/UserInterface/FocusedTextBox.cs#L73-L74
Noticed this not updating when working on tests.
Using a bespoke music loop, fades in during the intro: https://github.com/user-attachments/assets/8db7e30c-2a7c-4fd0-8385-fa77a6e0a506 Ducks when hovering cards for previews, with debouncing to smooth it: https://github.com/user-attachments/assets/b3557c08-c675-4591-963c-95866d45107f --- - [x] depends on ppy/osu-resources#414 --------- Co-authored-by: Dean Herbert <pe@ppy.sh>
- Fixes CI failures due to completely wrong test (sorry) - Fixes #37167 (oops)
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (37)
📝 WalkthroughWalkthroughThis PR refactors the replay settings UI layer by extracting Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Refactors gameplay/replay overlays and ranked play UI interactions, introducing a dedicated replay overlay layer and enhancing ranked play card-hand behaviour / music handling.
Changes:
- Introduced
ReplayOverlayand migrated replay settings + “watching” banner out ofHUDOverlay/Player.CreateOverlayComponents(). - Added ranked play hand enhancements (staggered enters, draggable reorder/display order propagation) and tuned hand replay recording/queueing.
- Added ranked play background music management and improved dropdown keyboard navigation (with new test coverage).
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| osu.Game/osu.Game.csproj | Bumps ppy.osu.Game.Resources package version. |
| osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs | Updates test client to broadcast playlist/settings changes when playing a ranked-play card. |
| osu.Game/Screens/Play/SpectatorPlayer.cs | Adds optional replay/settings overlay + routes “watching … live” message via ReplayOverlay. |
| osu.Game/Screens/Play/ReplayPlayer.cs | Integrates ReplayOverlay, routes settings/message into it, and removes reliance on CreateOverlayComponents(). |
| osu.Game/Screens/Play/Player.cs | Removes CreateOverlayComponents() plumbing and rehomes hotkey overlays into GameplayClockContainer. |
| osu.Game/Screens/Play/HUDOverlay.cs | Removes replay settings overlay + toggle handling from HUD overlay. |
| osu.Game/Screens/Play/HUD/ReplayOverlay.cs | Adds new ReplayOverlay (message + replay settings, toggleable via global action). |
| osu.Game/Screens/Play/HUD/PlayerSettingsOverlay.cs | Renames PlayerSettingsOverlay to ReplaySettingsOverlay and adjusts internals. |
| osu.Game/Screens/OnlinePlay/Multiplayer/Spectate/MultiSpectatorScreen.cs | Renames overlay usage to ReplaySettingsOverlay. |
| osu.Game/Screens/OnlinePlay/Multiplayer/Spectate/MultiSpectatorPlayer.cs | Disables settings overlay and adds top-centre username display for multiplayer spectating. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayScreen.cs | Adds and controls a BackgroundMusicManager across stage changes/suspends/exits. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayCardWithPlaylistItem.cs | Adds DisplayOrder for player-preferred card ordering. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/PickScreen.cs | Reorders hand construction and adds staggered entrance logic. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/OpponentPickScreen.cs | Adds staggered entrance + missing card-add sample; refines start positions. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Intro/IntroScreen.cs | Removes music ducking/disposal logic. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Hand/PlayerHandOfCards.cs | Adds drag-reorder behaviour; updates state-change signature and keyboard guard. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Hand/PlayerHandOfCards.PlayerHandCard.cs | Adds drag handling + callback to propagate reordering. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Hand/OpponentHandOfCards.cs | Adds constrained dragged-card layout for opponent hand. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Hand/HandReplayRecorder.cs | Adjusts record interval default (25 → 50ms). |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Hand/HandReplayPlayer.cs | Increases max queued frames (20 → 30). |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Hand/HandOfCards.cs | Major relayout: draw-order sorting, drag layout, spring-driven card movement targets. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Hand/HandOfCards.HandCard.cs | Adds drag/order state, spring-based movement + stagger helper methods. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/DiscardScreen.cs | Uses new insert positions + staggered movement; improves drawn-card movement setup. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Components/BackgroundMusicManager.cs | Adds ranked play BGM with crossfade and preview-aware volume adjustments. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Card/RankedPlayCard.SongPreview.cs | Improves preview looping on hover + adjusts ripple binding approach. |
| osu.Game/Rulesets/Mods/ModCinema.cs | Updates cinema mod overlay hiding to new replay overlay approach. |
| osu.Game/Online/RankedPlay/RankedPlayCardState.cs | Adds dragged/order/drag position fields for hand reordering/drag visuals. |
| osu.Game/Graphics/UserInterface/OsuDropdown.cs | Adds global-action keyboard navigation support to dropdown menu. |
| osu.Game/Audio/PreviewTrackManager.cs | Exposes IsPlayingPreview bindable for other components (eg. BGM manager). |
| osu.Game.Tests/Visual/SongSelect/TestSceneSongSelect.cs | Adds visual test for dropdown keyboard navigation without changing beatmap selection. |
| osu.Game.Tests/Visual/RankedPlay/TestScenePlayerCardHand.cs | Refactors setup and adds test ensuring Contract() moves cards outside bounds. |
| osu.Game.Tests/Visual/RankedPlay/TestSceneHandReplay.cs | Updates default record interval and slider default to 50ms. |
| osu.Game.Tests/Visual/Navigation/TestSceneSkinEditorNavigation.cs | Updates test to ReplaySettingsOverlay type. |
| osu.Game.Tests/Visual/Navigation/TestSceneScreenNavigation.cs | Updates mania test to use TargetTimeRange. |
| osu.Game.Tests/Visual/Multiplayer/TestSceneMultiSpectatorScreen.cs | Updates spectator test to ReplaySettingsOverlay type. |
| osu.Game.Tests/Visual/Gameplay/TestSceneReplayPlayer.cs | Updates replay test to ReplaySettingsOverlay type. |
| osu.Game.Rulesets.Mania/Edit/DrawableManiaEditorRuleset.cs | Updates scroll speed target assignment to TargetTimeRange. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
116
to
119
| overlayLayer.Add(new RippleVisualization(cardColours.Border) | ||
| { | ||
| TrackRunning = trackRunning.GetBoundCopy(), | ||
| TrackRunning = { BindTarget = trackRunning } | ||
| }); |
There was a problem hiding this comment.
TrackRunning = { BindTarget = trackRunning } requires TrackRunning to be a Bindable<bool> (or compatible type exposing BindTarget). As currently declared on RippleVisualization, this is very likely a compile error (if TrackRunning is IBindable<bool>) or at least inconsistent with the initializer. Consider either (1) changing TrackRunning to a BindableBool/Bindable<bool> and keeping the BindTarget initializer, or (2) reverting to assigning a bound copy (TrackRunning = trackRunning.GetBoundCopy()).
|
|
||
| player.BreakOverlay.Hide(); | ||
| player.OverlayComponents.Hide(); | ||
| (player as ReplayPlayer)?.ReplayOverlay.Hide(); |
There was a problem hiding this comment.
This only hides the new overlay for ReplayPlayer. SpectatorPlayer also creates a ReplayOverlay (and previously relied on OverlayComponents.Hide()), so cinema mode may leave spectator overlays visible. A more robust approach would be to hide any ReplayOverlay in the player hierarchy (eg. query player.ChildrenOfType<ReplayOverlay>()) or to expose a common API on Player/SpectatorPlayer to access and hide the overlay.
Suggested change
| (player as ReplayPlayer)?.ReplayOverlay.Hide(); | |
| foreach (var replayOverlay in player.ChildrenOfType<ReplayOverlay>()) | |
| replayOverlay.Hide(); |
| { | ||
| var cards = Cards.OrderBy(static c => c.Order).ToArray(); | ||
|
|
||
| int newIndex = cardIndexInLayout(cards, card.ScreenSpaceDrawQuad.Centre); |
There was a problem hiding this comment.
The screenSpacePosition parameter is unused, and the reindexing uses card.ScreenSpaceDrawQuad.Centre instead of the dragged cursor position. This can produce incorrect ordering if the visual centre lags behind the mouse position (or if the card is being constrained). Use screenSpacePosition when computing newIndex (and/or remove the unused parameter if it's not intended).
Suggested change
| int newIndex = cardIndexInLayout(cards, card.ScreenSpaceDrawQuad.Centre); | |
| int newIndex = cardIndexInLayout(cards, screenSpacePosition); |
Comment on lines
+183
to
+196
| delay = 0; | ||
|
|
||
| foreach (var item in matchInfo.OpponentCards) | ||
| { | ||
| double currentDelay = delay; | ||
|
|
||
| opponentHand.AddCard(item, c => | ||
| { | ||
| c.Position = ToSpaceOfOtherDrawable(new Vector2(DrawWidth / 2, 0), playerHand); | ||
| c.DelayMovementOnEntering(currentDelay); | ||
| }); | ||
| } | ||
|
|
||
| playerHand.UpdateLayout(stagger: 50); | ||
| opponentHand.UpdateLayout(stagger: 50); | ||
| delay += 50; | ||
| } |
There was a problem hiding this comment.
Opponent cards are being inserted using coordinates converted into playerHand space while being added to opponentHand, which is inconsistent with the new insertion helpers (BottomCardInsertPosition) and can yield incorrect entry positions. Also, the stagger uses a hardcoded 50 instead of the stagger constant used earlier in the method. Consider using opponentHand.BottomCardInsertPosition (or converting to opponentHand space) and incrementing with the shared stagger constant for consistency.
Comment on lines
+42
to
+46
| isPlayingPreview = previewTrackManager.IsPlayingPreview.GetBoundCopy(); | ||
| isPlayingPreview.BindValueChanged(playing => | ||
| { | ||
| bgm.VolumeTo(playing.NewValue ? 0 : 1, hover_fade_duration); | ||
| }); |
There was a problem hiding this comment.
The BindValueChanged subscription doesn’t run an initial update. If IsPlayingPreview is already true at the time BGM starts (eg. a preview is already playing and no further value change occurs), the BGM volume won’t be reduced. Consider binding with the initial notification (passing true) and/or applying bgm.VolumeTo(isPlayingPreview.Value ? 0 : 1, …) when starting playback.
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
Improvements
Bug Fixes