Remove osu!mania hold note ticks

[smoogipoo]

Resolves #24584
Resolves #24618

This adds a new ComboBreak hit result. A correctly-held hold note results in an IgnoreHit judgement, and an early release (a hold break) results in this new ComboBreak judgement.

[smoogipoo]

I wonder if we can agree on this, and of the override-ability of MinResult that prompted this. I'm accepting of alternatives...

[bdach]

I'm not really sure there's a feasible alternative that wouldn't require disproportionate effort...

Like, theoretically, you could leverage the type system for this a little bit. Separate HitResults that are hits from misses. But that only saves you the first two batches of checks. The pairing of "is this a valid max result for this min result" is not really directly encodeable in the C# type system without overcomplications.

You'd need a proper typescript/haskell-like union type system to encode this nicely. Unfortunately we don't get one to work with. At least one that isn't handrolled and completely clunky to work with.

That said, the method itself is an absolute behemoth and can be slimmed down. The best I can do is something like:

        public static void ValidateHitResultPair(HitResult maxResult, HitResult minResult)
        {
            if (maxResult == HitResult.None || !IsHit(maxResult))
                throw new ArgumentOutOfRangeException(nameof(maxResult), $"{maxResult} is not a valid maximum judgement result.");

            if (minResult == HitResult.None || IsHit(maxResult))
                throw new ArgumentOutOfRangeException(nameof(minResult), $"{minResult} is not a valid minimum judgement result.");

            if (maxResult.IsBonus() && minResult != HitResult.IgnoreMiss)
                throw new ArgumentOutOfRangeException(nameof(minResult), $"{HitResult.IgnoreMiss} is the only valid minimum result for a {maxResult} judgement.");

            if (maxResult == HitResult.LargeTickHit && minResult is not (HitResult.LargeTickMiss or HitResult.IgnoreMiss or HitResult.ComboBreak))
                throw new ArgumentOutOfRangeException(nameof(minResult), $"{minResult} is not a valid minimum result for a {maxResult} judgement.");

            if (maxResult == HitResult.SmallTickHit && minResult is not (HitResult.SmallTickMiss or HitResult.IgnoreMiss or HitResult.ComboBreak))
                throw new ArgumentOutOfRangeException(nameof(minResult), $"{minResult} is not a valid minimum result for a {maxResult} judgement.");

            if (maxResult.IsBasic() && minResult is not (HitResult.Miss or HitResult.IgnoreMiss or HitResult.ComboBreak))
                throw new ArgumentOutOfRangeException(nameof(minResult), $"{minResult} is not a valid minimum result for a {maxResult} judgement.");
        }

And yes this slightly bends our ban on or pattern expressions but I think this is one of the rarer cases where this actually vastly benefits readability so I'd be personally fine with it.

[smoogipoo]

Hmm, the part I’m more curious about is we used to have judgements be fully customisable at one point, with no validation whatsoever. This is putting a foot back into that pond with MinResult once again being customisable.

Whether everyone’s okay with that…

An alternative I have is to separate it into a property like OnlyComboBreakOnMiss and keep MinResult sealed/handle everything internally. It would remove the need for this method altogether (could be a test, but not a live assertion).

I’m not sure if there’s any other alternatives.

[smoogipoo]

To illustrate my point here, I've added the commit 04ce223, which I believe is more correct than the rules I had prior. It sounds weird to have a pairing of <Perfect, IgnoreMiss> - I don't know when this sort of pairing would ever come about or if it makes sense from the point of ScoreInfo.Statistics.

So, the only thing really added in this PR is the pairing <IgnoreHit, (IgnoreMiss | ComboBreak)>

Which, could be handled by a third alternative - making HitResult a flags enum and implicitly attaching | ComboBreak to IsBasic()/LargeTickHit results and allowing it to be attached for IgnoreHit. For example...

class MyCustomJudgement : Judgement
{
    public override HitResult MaxResult => IgnoreHit | ComboBreakOnMiss;
}

Again, with the theme of keeping MinResult sealed.

[bdach]

the part I’m more curious about is we used to have judgements be fully customisable at one point, with no validation whatsoever. This is putting a foot back into that pond with MinResult once again being customisable.

Whether everyone’s okay with that…

I see no issue with that.

An alternative I have is to separate it into a property like OnlyComboBreakOnMiss and keep MinResult sealed/handle everything internally. It would remove the need for this method altogether (could be a test, but not a live assertion).

Where would this property live? In Judgement? I thought that was infeasible because there are server-side components that depend on being able to calculate the max combo of a score from just the hit results alone?

I also generally don't like that flag, it feels much too specific and targeted to this weird usecase.

making HitResult a flags enum and implicitly attaching | ComboBreak to IsBasic()/LargeTickHit results and allowing it to be attached for IgnoreHit. For example...

class MyCustomJudgement : Judgement
{
    public override HitResult MaxResult => IgnoreHit | ComboBreakOnMiss;
}

I really don't like that one. The fact that the max result now influences min result in this roundabout way feels really bad.

Put it this way: I much prefer explicitly spelling out min/max result with appropriate validation to introducing such implicit rules.

[bdach]

Initial pass, haven't ran properly yet

[bdach]

I'm not really sure there's a feasible alternative that wouldn't require disproportionate effort...

Like, theoretically, you could leverage the type system for this a little bit. Separate HitResults that are hits from misses. But that only saves you the first two batches of checks. The pairing of "is this a valid max result for this min result" is not really directly encodeable in the C# type system without overcomplications.

You'd need a proper typescript/haskell-like union type system to encode this nicely. Unfortunately we don't get one to work with. At least one that isn't handrolled and completely clunky to work with.

That said, the method itself is an absolute behemoth and can be slimmed down. The best I can do is something like:

        public static void ValidateHitResultPair(HitResult maxResult, HitResult minResult)
        {
            if (maxResult == HitResult.None || !IsHit(maxResult))
                throw new ArgumentOutOfRangeException(nameof(maxResult), $"{maxResult} is not a valid maximum judgement result.");

            if (minResult == HitResult.None || IsHit(maxResult))
                throw new ArgumentOutOfRangeException(nameof(minResult), $"{minResult} is not a valid minimum judgement result.");

            if (maxResult.IsBonus() && minResult != HitResult.IgnoreMiss)
                throw new ArgumentOutOfRangeException(nameof(minResult), $"{HitResult.IgnoreMiss} is the only valid minimum result for a {maxResult} judgement.");

            if (maxResult == HitResult.LargeTickHit && minResult is not (HitResult.LargeTickMiss or HitResult.IgnoreMiss or HitResult.ComboBreak))
                throw new ArgumentOutOfRangeException(nameof(minResult), $"{minResult} is not a valid minimum result for a {maxResult} judgement.");

            if (maxResult == HitResult.SmallTickHit && minResult is not (HitResult.SmallTickMiss or HitResult.IgnoreMiss or HitResult.ComboBreak))
                throw new ArgumentOutOfRangeException(nameof(minResult), $"{minResult} is not a valid minimum result for a {maxResult} judgement.");

            if (maxResult.IsBasic() && minResult is not (HitResult.Miss or HitResult.IgnoreMiss or HitResult.ComboBreak))
                throw new ArgumentOutOfRangeException(nameof(minResult), $"{minResult} is not a valid minimum result for a {maxResult} judgement.");
        }

And yes this slightly bends our ban on or pattern expressions but I think this is one of the rarer cases where this actually vastly benefits readability so I'd be personally fine with it.

[bdach]

I'm not sure this is a particularly good place to assert this, though. I'd want it closer to the actual construction of the judgement itself so that it fails as early as possible.

Also there's an argument here that this method throwing is overkill and it should stop at asserting. Dunno about that.

[smoogipoo]

The reason I didn't put this in Judgement's constructor is because someone could theoretically change its value at run time.

We already do some validation in DHO.ApplyResult(), maybe it could go alongside it?

osu/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs

Lines 675 to 678 in 6b38600

	if (!Result.Type.IsValidHitResult(Result.Judgement.MinResult, Result.Judgement.MaxResult))
	{
	throw new InvalidOperationException(
	$"{GetType().ReadableName()} applied an invalid hit result (was: {Result.Type}, expected: [{Result.Judgement.MinResult} ... {Result.Judgement.MaxResult}]).");

Any other suggestions?

[bdach]

The reason I didn't put this in Judgement's constructor is because someone could theoretically change its value at run time.

That would be vile, but you do have a point...

We already do some validation in DHO.ApplyResult(), maybe it could go alongside it?

That would probably be the other best place to put it, yeah. I did have an alarm bell ringing in my head that doing so may cause a performance hit, but I don't think it's gonna be significant anywhere outside of like frame-stable replay catchup maybe. At most.

[smoogipoo]

Consider this temporary for now. DrawableHitObject is not properly annotated for its nullable ctor argument, and I don't want to use null! in this case.

[peppy]

I'm pretty fine with this change overall.

[bdach]

I've found and attempted a fix a couple of foibles:

3f29f27: Assign empty hit windows to HoldNoteBody

It's not a timed object, so following precedent, it should have empty hitwindows.

This is not actually just aesthetics; several components check whether a hitobject has empty hitwindows to determine whether to include it on various HUD displays and results screen components where timed objects are explicitly involved.

041318e: Assign red colour to ComboBreak hit result

For a judgement result to show up on the colour hit error meter, it has to be two things: be scorable, and not be bonus.

ComboBreak happens to meet both of those criteria - it impacts score indirectly via decreasing the combo portion, and isn't bonus. Therefore it would show up on the colour hit error meter, but because OsuColour.ForHitResult() wasn't handling it explicitly, it was painting the result in light blue, which isn't ideal for a miss type judgement. Therefore, use red instead.

There is possibly an argument to be made that this judgement should not show up on the colour hit error meter at all, but I think it's actually okay for it to be this way. In any case it doesn't appear to be anything so major as to warrant blocking the hold note tick removal at this time.

---

Other than that, no issues as far as I can tell. Please double-check that you're OK with the aforementioned changes though.

[peppy]

Changes look good, going to keep this moving.