Fix spinner drain not correctly considered after a break

[smoogipoo]

Fixes #37046

Background

1. Between two objects separated by a break, there is no drain between the first object and the start of the break, and between the end of the break and the start of the second object.

This is a mechanic that is carry-forward from osu!stable.

For drain, the implementation of this lies here:

osu/osu.Game/Rulesets/Scoring/DrainingHealthProcessor.cs

Lines 107 to 120 in 101cfff

	noDrainPeriodTracker = new PeriodTracker(
	beatmap.Breaks.Select(breakPeriod =>
	new Period(
	beatmap.HitObjects
	.Select(hitObject => hitObject.GetEndTime())
	.Where(endTime => endTime <= breakPeriod.StartTime)
	.DefaultIfEmpty(double.MinValue)
	.Last(),
	beatmap.HitObjects
	.Select(hitObject => hitObject.StartTime)
	.Where(startTime => startTime >= breakPeriod.EndTime)
	.DefaultIfEmpty(double.MaxValue)
	.First()
	)));

And for simulation, the implementation lies here:

osu/osu.Game/Rulesets/Scoring/DrainingHealthProcessor.cs

Lines 184 to 191 in 101cfff

	while (currentBreak < Beatmap.Breaks.Count && Beatmap.Breaks[currentBreak].EndTime <= currentTime)
	{
	// If two hitobjects are separated by a break period, there is no drain for the full duration between the hitobjects.
	// This differs from legacy (version < 8) beatmaps which continue draining until the break section is entered,
	// but this shouldn't have a noticeable impact in practice.
	lastTime = currentTime;
	currentBreak++;
	}

2. Bonus objects are not required to be hit towards drain.

This is an osu!lazer-specific mechanic implemented since #9528 and intended to fix osu!catch bananas over-weighting drain.

osu/osu.Game/Rulesets/Scoring/DrainingHealthProcessor.cs

Lines 133 to 143 in 101cfff

	protected override void ApplyResultInternal(JudgementResult result)
	{
	base.ApplyResultInternal(result);
	if (IsSimulating && !result.Type.IsBonus())
	{
	healthIncreases.Add(new HealthIncrease(
	result.HitObject.GetEndTime() + result.TimeOffset,
	GetHealthIncreaseFor(result)));
	}
	}

Problem

The issue arises in the following scenario:

(c = "combo object", - = "break", b = "bonus object")

                    c ------------------- bbbbbbbbbbbbc
no drain:             ^^^^^^^^^^^^^^^^^^^
gameplay drain:    ^^                     ^^^^^^^^^^^^^^
simulation drain:  ^^                                 ^^

In this case, the second drain section would be simulated to start only at the second combo object, but in gameplay it actually starts with the leading bonus objects. This occurs because bonus objects are not tracked at all as a result of (2) from above.

Demonstration: broken.zip

Fix

I've chosen to track the points in time where the bonus objects occur while keeping the osu!lazer-specific bonus object exclusion.

[bdach]

Running some checks in the background, but in the mean time

[bdach]

I am not entirely sure I agree with the removal of this test.

Checking blame, it was added in #10253, to cover off another edge case. I would be loath to see such test coverage removed without replacement.

Instead of 064aac7, I would apply

diff --git a/osu.Game.Tests/Gameplay/TestSceneDrainingHealthProcessor.cs b/osu.Game.Tests/Gameplay/TestSceneDrainingHealthProcessor.cs
index 1914622e71..7d69573e3c 100644
--- a/osu.Game.Tests/Gameplay/TestSceneDrainingHealthProcessor.cs
+++ b/osu.Game.Tests/Gameplay/TestSceneDrainingHealthProcessor.cs
@@ -230,7 +230,7 @@ public void TestSingleLongObjectDoesNotDrain()
             assertHealthEqualTo(1);
 
             setTime(5000);
-            assertHealthEqualTo(1);
+            AddAssert("health positive", () => processor.Health.Value, () => Is.GreaterThan(0));
         }
 
         [Test]

onto c10cf21 which covers off the failure scenario.

[smoogipoo]

What are we testing here...? That health drains? The edge case here exists exactly because not all judgements are accounted for, which this PR resolves.

I've replaced this with 7749e7a which also showcases a failure that occurs even on master + d463adf that fixes said failure.

Is that suitable?

[bdach]

What are we testing here...? That health drains?

I would say that what the test appeared to be exercising before was that in the edge case mentioned the health does not immediately go to zero and thus cause instant failure. As in, if you go to master and then revert the healthIncreases.Count <= 1 check, the test fails, with the final HP of zero.

Which is why I suggested replacing the assertion of being 1 with the assertion of being positive, because then it's not an instant fail.

I've replaced this with 7749e7a which also showcases a failure that occurs even on master + d463adf that fixes said failure.

That looks vaguely okay to me but this drain code is rather inscrutable in general so my confidence levels are not high.

[smoogipoo]

While that's true, even if you remove that check on this branch the test still wouldn't fail. So it was improperly constructed to test the edge case, and that's why I don't feel good just modifying the test.

(and in fact, that test is plain wrong because it should fail on master - a single hitobject does exhibit drain in stable)

I believe the added unit tests to be more applicable in this scenario.

[bdach]

Because I believe that I lack the wherewithal to evaluate this directly via code, I decided to go the empirical route and thus I ran this program over the May .osu file dump from https://data.ppy.sh and the (trimmed) results are here.

TL;DR: unless I did the data manipulation wrong, all measurable differences are in osu! ruleset only, ~4000 maps are affected, and in all cases the drain rate decreased. Which seems good.

The full comparison without filtering to scope down to the relevant bits is here: comparison.csv.zip. I'd have pasted it into the gist but when I tried that GitHub 500'd at me so I assume it can't handle the size.

Notably I did all this before d463adf, but I'm not sure I can be bothered to repeat this process because generating this data takes literal hours.

[smoogipoo]

One concern I have with this is that it may make maps easier than they should be due to bonus objects being excluded entirely. /b/4847648 is perhaps one such case.

osu!stable has catch-specific handling for bananas (I think it basically assumes the user can hit 1/4 of the bananas). Not sure if we should do the same thing here instead or what...

[bdach]

osu!stable has catch-specific handling for bananas (I think it basically assumes the user can hit 1/4 of the bananas). Not sure if we should do the same thing here instead or what...

As far as I can tell this PR does not materially affect any ranked catch map or convert and I am not aware of any specific complaints of the catch player base with respect to health at this time.

[smoogipoo]

It doesn't affect catch yes, but the reason why bonus objects are excluded in the first place is due to catch - see point (2) in the OP.

[smoogipoo]

Gimme a bit longer with this PR. We since have LegacyDrainingHealthProcessor which is the only health processor that catch uses and I want to check if it handles bananas correctly.

osu! is the only ruleset that uses the non-legacy health processor, for whatever reason...