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

Remove poorly-fitted "legacy combo counter" abstraction #29838

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Sep 11, 2024

This is probably going to be hell to review, and I apologise for this, but I believe it is best to course correct now before this gets worse.

The long and short of it is that #26254, namely probably somewhere around 78cb6b6, regressed the "default legacy combo counter" display logic. When reviewing the code for #29811 and attempting to determine why, I found the code quality of the counters frankly shocking, with stuff like this:

this.Delay(big_pop_out_duration - 140).Schedule(() =>
{
base.OnCountIncrement();
DisplayedCountText.ScaleTo(1).Then()
.ScaleTo(1.1f, small_pop_out_duration / 2, Easing.In).Then()
.ScaleTo(1, small_pop_out_duration / 2, Easing.Out);
}, out scheduledPopOut);

protected virtual void OnCountIncrement()
{
if (DisplayedCount < Current.Value - 1)
DisplayedCount++;
DisplayedCount++;
}

This, dear reader, is a scheduled base call which mutates internal state in a manner that seems inconsistent with previous code and frankly arbitrary, which to me is NOPE NOPE NOPE territory. I want stuff like that gone from the codebase as soon as possible because it is not sane and we have lived several examples of this already.

To that end, this PR reverts LegacyDefaultComboCounter to what it was at e7b4a2f, fixing the issue, and moves enough of the machinery that used to live in the abstract eldritch entity into LegacyManiaComboCounter so that it can function standalone.

Had it not been for this review thread I probably would have searched for a more moderate resolution, but this being brought up already at review time definitely has me wanting to nip this in the bud before the abstract entity grows to inscrutability.

@peppy
Copy link
Member

peppy commented Sep 11, 2024

In hindsight, trying to salvage that PR rather than fix clear mistakes was a bad decision.

@peppy peppy disabled auto-merge September 11, 2024 11:12
@peppy peppy merged commit b5afca9 into ppy:master Sep 11, 2024
13 checks passed
@bdach bdach deleted the do-not-use-abstract-do-NOT-use-abstract-DO-NOT-USE-ABSTRACT branch September 11, 2024 11:36
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.

combo counter & longest combo counter miscount
2 participants