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

Hide scroll speed changes in std and ctb editor #29448

Merged
merged 6 commits into from
Aug 29, 2024

Conversation

OliBomby
Copy link
Contributor

@OliBomby OliBomby commented Aug 16, 2024

closes #29336

Test is broken because you cant make a DrawableRuleset of a TestBeatmap. Is there a better way to extract ruleset information from the beatmap?

@bdach
Copy link
Collaborator

bdach commented Aug 19, 2024

Yeah I dunno. The conditional seems generally sane, but I'm not sure how to fix the test aside from just... fixing the test itself to not use a TestBeatmap.

The fact that instantiating a DrawableRuleset to check this worries me. I'm also acutely aware that the other rulesets will want to have SVs shown likely on both timelines, so I'm not super sure this is the correct direction in general. Maybe the thing here is to have settable or overridable flags, and override them on a per-ruleset basis rather than trying to backwards-reason the flag's value from drawable rulesets...? @ppy/team-client would like to hear thoughts

@pull-request-size pull-request-size bot added size/M and removed size/S labels Aug 22, 2024
@OliBomby OliBomby marked this pull request as ready for review August 22, 2024 18:17
@OliBomby
Copy link
Contributor Author

I think I found a decent solution.

To draw scroll speed changes is now determined by an overridable flag in Ruleset. This eliminates the need to instantiate a drawable ruleset entirely and removes the need for IDrawableScrollingRuleset.VisualisationMethod. It seems that interface value was only being used for determining whether to draw scroll speed in the editor timing section.

I also fixed an unrelated issue of missing scroll speed visualisation at the start of kiai sections. They were being drawn off-screen because of a wrong positioning calculation.
NVIDIA_Share_Z0WT4wPJ9d

@bdach bdach requested a review from a team August 27, 2024 08:06
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.

I'm a bit concerned about the scope creep in polluting Ruleset, but I suppose I'm fine with this for the time being.

@smoogipoo smoogipoo enabled auto-merge August 29, 2024 15:41
@smoogipoo smoogipoo merged commit b1530e4 into ppy:master Aug 29, 2024
9 checks passed
@bdach
Copy link
Collaborator

bdach commented Aug 30, 2024

I'm a bit concerned about the scope creep in polluting Ruleset

That's why I was expecting more discussion to happen here rather than a quick merge.

I think we'll need to do a very API-breaking pass on Ruleset anyway at some point. The API is very haphazard and needs to be structured better.

@OliBomby OliBomby deleted the scroll-speed-std branch August 30, 2024 08:00
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.

Scroll speed changes show up in editor for standard and ctb gamemodes
4 participants