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 osu!taiko Constant Speed mod #27341

Merged
merged 9 commits into from May 23, 2024
Merged

Conversation

Hiviexd
Copy link
Member

@Hiviexd Hiviexd commented Feb 23, 2024

resolves #17927

no-longer-relevant blurb about previous implementation

initially tried implementing it through changing the (DrawableTaikoRuleset)drawableRuleset's ScrollVisualisationMethod to Constant similar to osu!mania's Constant Speed mod, but that caused me 2 distinct issues:

  1. overall scroll speed was extremely low
  2. Difficulty Adjust's scroll speed setting does not work with this mod (EZ/HR may also be affected but I didn't test)

so, I've implemented this in a more straightforward manner (which follows the original proposal's intention) by simply setting every effect point's scroll speed (every inherited point's SV value in osu!stable terms) to 1, which achieves the desired effect of the mod (neutralizing all SV changes relative to the map's base SV) + makes it compatible with other scroll speed changes made by mods like DA/HR/EZ.

I've figured out how to implement this correctly using ScrollVisualisationMethod, the method above doesn't account for maps with multiple BPM changes which results in those having "broken" scroll speed changes which negatively impacts gameplay. however, I still couldn't figure out how to allow for HR/EZ to change the base scroll speed when ScrollVisualisationMethod is set to Constant, so I'll be excluding them from working with this mod unless someone figures out how to make this work, as i believe this is currently a fair compromise to have correct implementation + clean gameplay in variable BPM maps.

mod should be working fine with other mods that change scroll speed (EZ/HR/DA) and should have no issues with variable BPM maps now.

this also makes constant scroll speed look much more accurate in the editor.

@Hiviexd
Copy link
Member Author

Hiviexd commented Feb 23, 2024

after more tinkering i resolved all the potential issues with this implementation (variable BPM maps and compatibility with mods that change scroll speed), should be ready for a 2nd pass/merge i reckon.

bdach
bdach previously approved these changes Mar 7, 2024
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.

seems fine source-wise but not merging without at least one more approval

@peppy peppy self-requested a review March 8, 2024 02:46
@Hiviexd
Copy link
Member Author

Hiviexd commented May 23, 2024

small bump on this in case it slipped through the cracks, considering it seems like it's in a ready state now 👀

Copy link
Sponsor Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

Can't comment on the mod multiplier but code looks fine.

@bdach
Copy link
Collaborator

bdach commented May 23, 2024

What is going to happen to this PR at this point then? If we're not making calls on multiplier ourselves (which I agree with, I have absolutely no clue if it's right or not) then do we just merge? summon the pp committee or something?

Copy link
Contributor

@smoogipoo smoogipoo left a comment

Choose a reason for hiding this comment

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

What we do is I merge :) (we can adjust later if necessary)

@smoogipoo smoogipoo merged commit 6304a5e into ppy:master May 23, 2024
15 of 17 checks passed
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.

None yet

4 participants