Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add "Start cinema mod with Ctrl+Shift+Enter" #11047

Closed
wants to merge 7 commits into from

Conversation

orozso
Copy link
Contributor

@orozso orozso commented Dec 1, 2020

Add hotkey to "cinema" mod as in osu!stable.

Closes #9221.

@bdach
Copy link
Collaborator

bdach commented Dec 1, 2020

This has been attempted once before in #9224, and as far as I can see, this implementation suffers from the exact same bug that prevented that one from being merged.

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above - please add automated test coverage for the aforementioned edge case if you decide to resolve

@pull-request-size pull-request-size bot added size/L and removed size/M labels Dec 1, 2020
@peppy
Copy link
Sponsor Member

peppy commented Dec 2, 2020

I'm not even sure we want this. Cinema is such a rare use case that I feel it probably doesn't need its own dedicated (hidden) hotkey..

@orozso orozso requested a review from bdach December 2, 2020 19:56
Comment on lines +134 to +135
if (Mods.Value.Any(m => m.GetType() == Ruleset.Value.CreateInstance().GetAutoplayMod()?.GetType())) return Ruleset.Value.CreateInstance().GetAutoplayMod();
else if (Mods.Value.Any(m => m.GetType() == Ruleset.Value.CreateInstance().GetCinemaMod()?.GetType())) return Ruleset.Value.CreateInstance().GetCinemaMod();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't agree with this being handled locally as done here. The higher-level logic of mod selection/deselection should be deselecting one when the other is selected, via IncompatibleMods or similar. (see #7155) Otherwise we're going to end up with local fixes every time an issue like this arises.

That was also another problem the previous attempt at this had.

{
notifications?.Post(new SimpleNotification
{
Text = "The current ruleset doesn't have an autoplay mod avalaible!"
Text = "The current ruleset doesn't have an autoplay/cinema mod avalaible!"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this text should depend on the hotkey pressed, not be an either/or catch-all

@orozso orozso closed this Dec 3, 2020
orozso added a commit to orozso/osu that referenced this pull request Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cinema mod shortcut not implemented
4 participants