New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make song select ensure current beatmap is always playable in the active ruleset #2159

Merged
merged 11 commits into from Mar 10, 2018

Conversation

2 participants
@naoey
Contributor

naoey commented Mar 3, 2018

Alternative to #1830 and fixes #1724 and #2055

naoey added some commits Feb 28, 2018

Make song select ensure current beatmap is always playable in the act…
…ive ruleset.

 - Add a to TestCasePlaySongSelect testing this scenario

@peppy peppy added the resolves issue label Mar 5, 2018

@@ -194,6 +199,8 @@ private void load(BeatmapManager beatmaps, AudioManager audio, DialogOverlay dia
if (this.beatmaps == null)
this.beatmaps = beatmaps;

rulesetConversionAllowed = config.GetBindable<bool>(OsuSetting.ShowConvertedBeatmaps);

This comment has been minimized.

@peppy

peppy Mar 8, 2018

Member

i think we want to bind rulesetConversionAllowed.ValueChanged += ensurePlayableRuleset too, right?

This comment has been minimized.

@naoey

naoey Mar 8, 2018

Contributor

the carousel itself seemed to handle it by changing filters

@@ -147,8 +233,9 @@ private BeatmapSetInfo createTestBeatmapSet(int i)
new BeatmapInfo
{
OnlineBeatmapID = 1235 + i,
Ruleset = rulesets.AvailableRulesets.First(),
Path = "hard.osu",
Ruleset = rulesets.AvailableRulesets.First(r => r.ID != 0),

This comment has been minimized.

@peppy

peppy Mar 8, 2018

Member

probably use ElementAt() here too

@@ -443,6 +456,34 @@ private void ensurePlayingSelected(bool preview = false)
}
}

private void ensurePlayableRuleset()

This comment has been minimized.

@peppy

peppy Mar 8, 2018

Member

Is there any reason for this to be here rather than in BeatmapCarousel? You're duplicating the "can we convert" conditional out of the carousel scope (and doing so twice in the same method). Need to avoid duplicating such code wherever possible.

I'd suggest trying to work this in to BeatmapCarousel's SelectBeatmap, which is already called in the necessary code paths.

Then you can probably already check if the beatmap to be selected is currently filtered, and if so, perform the required actions. Also I'm not sure whether we want it to trigger a ruleset change, maybe leave that part out (and call SelectNextRandom instead).

This comment has been minimized.

@naoey

naoey Mar 8, 2018

Contributor

the ruleset change was to handle the scenario where the next track in the music controller might be an incompatible ruleset. when pressing "next" in music controller loads a random beatmap instead it'd be a bit disconcerting (like in #2055)

This comment has been minimized.

@peppy

peppy Mar 8, 2018

Member

I think the more common scenario is the one we should cater to initially – entering song select from the main menu after a random beatmap starts playing. We don't want this to switch a user's active ruleset.

naoey and others added some commits Mar 9, 2018

Remove previous fix and move filtered logic to carousel.
- Add an optional bool parameter to SelectBeatmap to skip selecting
filtered maps
Make song select choose random when initial selection fails.
- Revert TestCasePlaySongSelect to master
@peppy

peppy approved these changes Mar 10, 2018

@peppy peppy merged commit 3a5283f into ppy:master Mar 10, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@naoey naoey deleted the naoey:fix-unplayable-beatmaps branch Mar 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment