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 "Synesthesia" mod for osu! ruleset #23520

Merged
merged 10 commits into from
Jun 19, 2023
Merged

Conversation

jtbiddle
Copy link
Contributor

This is a mod for osu!standard that colors hit circles and sliders based on their beat division in the song. For example, objects that are on full notes are white, half notes are red, and quarter notes are blue. This is a simple implementation using the function for the snap coloring preference in osu!mania, and shares the same colors. I implemented this in an attempt to make reading complicated rhythms easier, especially for low AR or hidden.

snapcolour.mov

Compatibility with other mods has been tested. I could imagine this being combined with the "Incognito" mod which also changes hit object colors ( #22271) to avoid cluttering the mod menu.

@peppy
Copy link
Member

peppy commented May 13, 2023

It's a touch weird that this is introduced as a mod while it exists as a setting in osu!mania. Not necessarily the wrong approach, but food for thought.

Copy link
Contributor

@ItsShamed ItsShamed left a comment

Choose a reason for hiding this comment

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

Quick code quality check

osu.Game.Rulesets.Osu/Mods/OsuModSnapColour.cs Outdated Show resolved Hide resolved
osu.Game/Rulesets/Mods/ModSnapColour.cs Outdated Show resolved Hide resolved
osu.Game.Rulesets.Osu/Mods/OsuModSnapColour.cs Outdated Show resolved Hide resolved
osu.Game.Rulesets.Osu/Mods/OsuModSnapColour.cs Outdated Show resolved Hide resolved
…sable, fixed incorrect LocalisableString, removed incorrect dependency injection for OsuColour, fixed nullable dependency for IBeatmap

Removed unnecessary usage of "this." caught by the CI code quality check
@bdach
Copy link
Collaborator

bdach commented May 13, 2023

It's a touch weird that this is introduced as a mod while it exists as a setting in osu!mania.

I really think this is the big issue here and one that doesn't really warrant per-ruleset considerations. The way I see it, either toggling the colour snapping is visual preference and as such should be a game setting, or it actually changes the difficulty of gameplay, and as such should be a mod. I'm not sure it should be allowed to be a setting in one ruleset but a mod in the other.

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.

Additionally:

osu.Game.Rulesets.Osu/Mods/OsuModSnapColour.cs Outdated Show resolved Hide resolved
osu.Game.Rulesets.Osu/Mods/OsuModSnapColour.cs Outdated Show resolved Hide resolved
@peppy
Copy link
Member

peppy commented May 16, 2023

I really think this is the big issue here and one that doesn't really warrant per-ruleset considerations. The way I see it, either toggling the colour snapping is visual preference and as such should be a game setting, or it actually changes the difficulty of gameplay, and as such should be a mod. I'm not sure it should be allowed to be a setting in one ruleset but a mod in the other.

From that angle it could be argued that the mania setting is moved to exist as a mod. It's the kind of thing I think should be shared in replays, so that might be a good direction?

@bdach
Copy link
Collaborator

bdach commented May 16, 2023

From that angle it could be argued that the mania setting is moved to exist as a mod. It's the kind of thing I think should be shared in replays, so that might be a good direction?

I can't say I have particularly strong feelings as to which one it should be, personally. I'm mostly looking for consistency of treatment if possible.

I wouldn't disagree in principle with moving the mania thing to a mod, if only to unclutter the settings sidebar just a tiny bit.

Moved accent color assignment from OnUpdate to ApplyCustomUpdateState. In order to get this to work, a flag needed to be added to DrawableHitObject.cs to disable combo color updates also being applied.
@jtbiddle
Copy link
Contributor Author

Thanks for the reviews so far, let me know if there are any changes you would like to see to it. I wanted to keep this PR simple and to see if there was interest in this addition.

Some ideas for this mod that I would be happy to refine with the team and implement in the future:

  • Moving it to a preference instead of a mod if that is more appropriate
  • Color customization, possibly with an option to read colors from the current skin. (Fixing Cannot show or hide a popover without a parent PopoverContainer in the hierarchy #23684 would make adding a color picker to the mod customization menu much easier)
  • Either disabling or implementing the "Combo Colour Normalisation" and "Beatmap Colours" options when the mod is selected
  • Allowing beatmap colors to be used if the number of different time divisions in a beatmap is equal to or less than the number of beatmap colors (would require pre-processing the beatmap on loading)
  • A toggle to disable slider tail coloring. It currently provides more timing information as it is, but may look nicer if sliders are a uniform color

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.

Let's keep away from the scope creep mentioned above and focus on what we have right now - still a few things I'm not sure about

osu.Game.Rulesets.Osu/Mods/OsuModSnapColour.cs Outdated Show resolved Hide resolved
Comment on lines 13 to 14
public override string Name => "Snap Colour";
public override string Acronym => "SC";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure on the naming of this thing. The existing mania setting is named "timing-based colouring". I would like to keep that naming consistent, but "timing-based colouring" a bit too much of a mouthful to use for a mod name. "Snap" is a bit of an editor term and I'm not sure it should be used in the mod name.

Maybe something like "Coloured Rhythm" could work? Or - going off the wall a bit - "Synesthesia"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the name was just taken from the function name - I figured that was some "standard" name for it in VSRGs but I'm not tied to it at all. I really like the suggestion of "Synesthesia", a cool reference and in-line with the other mod names. I've renamed it to Synesthesia in the latest commit.

osu.Game/Rulesets/Mods/ModSnapColour.cs Outdated Show resolved Hide resolved
Fixed null checking in ApplyToDrawableHitObject
Renamed mod to "Synesthesia"
Moved to the "Fun" mod category
@bdach
Copy link
Collaborator

bdach commented Jun 17, 2023

I don't have any problems with this anymore, but I also don't feel confident enough to proceed with this mod as it is written on the basis of my judgement alone. Going to need a second approval for this to go forward.

bdach
bdach previously approved these changes Jun 17, 2023
@peppy peppy self-requested a review June 19, 2023 10:48
peppy
peppy previously approved these changes Jun 19, 2023
@peppy
Copy link
Member

peppy commented Jun 19, 2023

@bdach please double-check my changes. I wanted to avoid the expensive colour lookup (including a TimingPointAt) multiple times per frame.

@peppy peppy marked this pull request as ready for review June 19, 2023 11:12
peppy
peppy previously approved these changes Jun 19, 2023
@peppy peppy added the next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! label Jun 19, 2023
@bdach bdach changed the title Add "Snap Colour" mod to osu!standard Add "Synesthesia" mod for osu! ruleset Jun 19, 2023
Only visually apparent on legacy skins.
@bdach
Copy link
Collaborator

bdach commented Jun 19, 2023

Fixed one more issue with slider tails that was only visible on legacy skins (see discord conversation). As stated there, not sure what the reaction to differently-coloured slider pieces is going to be, but let's try this and see what it will be.

@bdach bdach enabled auto-merge June 19, 2023 15:54
@bdach bdach merged commit 2d8a74d into ppy:master Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:mods next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! ruleset/osu! size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants