aa - #107
Conversation
- Simplifies track handling by not attaching to the global beatmap, not looping, and moving into `GameplayWarmupScreen` where the beatmap is set. - The main idea here is that the transition period until gameplay is so short (~10 seconds) that we don't need to account for the track ever looping in the first place. - Fixes the track not playing from its preview point (feedback item mentioned in some meeting a while back). This is definitely going to conflict with @nekodex 's work, sorry about that. It's a bit of a much-of-a-muchness change (imo) if the conclusion is to wait for ongoing work first.
…eplace background is disabled (#37112) Man this "storyboard replaces background" baloney has taken hours of bugfixing alone. So many forehead indentations from stepping onto this stupid rake. This still fails in one more case: when you download a no-video variant of a beatmap that has video, but then edit it, all of the flags on storyboard will claim that the beatmap has a storyboard that replaces a background, but the video asset is missing, so the background will still be black. There's currently no way to check for this and the simplest way to address this as far as I can see would be reverting #37038 and going with the non-refactor route to fix #36875 instead. The alternative is adding all sorts of weird jingles and checks in the storyboard machinery that can be used to be able to tell that a video was supposed to be present in the storyboard but is missing. Also when entering editor on a map that has background video and storyboard enabled the background will be black until you hit play. Something to do with `Video` idiosyncrasies for sure. Closes #37104 maybe? Kind of? Partially? I don't know. This is all very low effort because I'm not confident about digging this ditch any deeper, but just PRing a direct revert would feel pretty offensive I guess?
|
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 (3)
📝 WalkthroughWalkthroughThe changes refactor audio playback management in ranked play screens by removing complex music looping logic from RankedPlayScreen and centralizing playback control in GameplayWarmupScreen, while EditorBackgroundScreen gains background sprite opacity animation based on storyboard visibility state. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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
Updates audio preview handling for Ranked Play flow and adjusts editor background visibility when toggling storyboards.
Changes:
- Remove RankedPlayScreen’s beatmap-track looping management and instead stop any preview playback on suspend/exit.
- Start playback from the beatmap preview point during Ranked Play gameplay warmup.
- Ensure the editor background sprite is shown when the storyboard is hidden.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayScreen.cs | Removes looping/track-handling hooks and standardises preview stop on suspend/exit. |
| osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/GameplayWarmupScreen.cs | Adds MusicController usage to start the selected beatmap’s track from its preview point. |
| osu.Game/Screens/Backgrounds/EditorBackgroundScreen.cs | Adds sprite fade logic intended to prevent missing backgrounds when storyboards are disabled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Play the new track from its preview point. | ||
| globalBeatmap.Value.PrepareTrackForPreview(false); |
There was a problem hiding this comment.
PrepareTrackForPreview(false) sets Track.Looping = false. If the warmup lasts long enough for the beatmap track to complete, MusicController will advance to the next track (see onTrackCompleted()), which would desync music from the selected beatmap. This should likely prepare the track with looping enabled (or otherwise ensure the track can’t complete while waiting).
| // Play the new track from its preview point. | |
| globalBeatmap.Value.PrepareTrackForPreview(false); | |
| // Play the new track from its preview point, looping during warmup to avoid completion. | |
| globalBeatmap.Value.PrepareTrackForPreview(true); |
| background?.Storyboard.FadeTo(showStoryboard.Value ? 1 : 0, withAnimation ? 500 : 0, Easing.OutQuint); | ||
| // if the storyboard is disabled, in some cases (e.g. involving `StoryboardReplacesBackground`) | ||
| // we still need to show the background sprite, because if we don't, then there will be no background shown at all | ||
| background?.Sprite.FadeTo(showStoryboard.Value ? 0 : 1, withAnimation ? 500 : 0, Easing.OutQuint); |
There was a problem hiding this comment.
updateState() now fades the background sprite to 0 whenever showStoryboard is enabled. This hides the beatmap background even for storyboards that don’t replace the background, which can result in a blank/incorrect background. Consider only hiding background.Sprite when the loaded storyboard actually replaces the background (eg background.Beatmap.Storyboard.ReplacesBackground), and otherwise keep the sprite visible while the storyboard is shown.
| background?.Sprite.FadeTo(showStoryboard.Value ? 0 : 1, withAnimation ? 500 : 0, Easing.OutQuint); | |
| bool storyboardReplacesBackground = background?.Beatmap?.Storyboard?.ReplacesBackground ?? false; | |
| float targetBackgroundAlpha = showStoryboard.Value && storyboardReplacesBackground ? 0 : 1; | |
| background?.Sprite.FadeTo(targetBackgroundAlpha, withAnimation ? 500 : 0, Easing.OutQuint); |
Summary by Gitar
This will update automatically on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Changes