In-gameplay pp counter is inaccurate when difficulty hit objects are not created for every hit object in a ruleset

[bdach]

Type

Game behaviour

Bug description

This was reported on the performance points discord, with the symptom that the count of hit objects in the beatmap as calculated from OsuDifficultyAttributes was 1 smaller than the count of hit results collected at each judgement. I have confirmed this by breakpointing at this line:

osu/osu.Game/Screens/Play/HUD/PerformancePointsCounter.cs

Line 136 in 2d1cc2e

scoreProcessor.PopulateScore(scoreInfo);

and looking at what the state of attrib and scoreProcessor is. attrib will have one object less.

I believe that the reason that this is happening is the following incorrect implicit assumption in DifficultyCalculator in the loop that computes incremental difficulty attributes:

osu/osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs

Lines 104 to 115 in 2d1cc2e

	foreach (var hitObject in getDifficultyHitObjects())
	{
	progressiveBeatmap.HitObjects.Add(hitObject.BaseObject);
	foreach (var skill in skills)
	{
	cancellationToken.ThrowIfCancellationRequested();
	skill.ProcessInternal(hitObject);
	}
	attribs.Add(new TimedDifficultyAttributes(hitObject.EndTime * clockRate, CreateDifficultyAttributes(progressiveBeatmap, playableMods, skills, clockRate)));
	}

By constructing partial beatmaps in this way, DifficultyCalculator is implicitly assuming that each real hitobject has a corresponding difficulty hitobject. But this is not the case, because osu! diffcalc skips the first object and only creates difficulty hitobjects from the second object onwards:

osu/osu.Game.Rulesets.Osu/Difficulty/OsuDifficultyCalculator.cs

Lines 88 to 100 in 2d1cc2e

	protected override IEnumerable<DifficultyHitObject> CreateDifficultyHitObjects(IBeatmap beatmap, double clockRate)
	{
	// The first jump is formed by the first two hitobjects of the map.
	// If the map has less than two OsuHitObjects, the enumerator will not return anything.
	for (int i = 1; i < beatmap.HitObjects.Count; i++)
	{
	var lastLast = i > 1 ? beatmap.HitObjects[i - 2] : null;
	var last = beatmap.HitObjects[i - 1];
	var current = beatmap.HitObjects[i];
	yield return new OsuDifficultyHitObject(current, lastLast, last, clockRate);
	}
	}

This is not generally easy to fix. One way to fix this in the general case would entail providing some sort of builder object that can receive hitobjects in a stream-like manner, and spit out difficulty objects on-the-fly (or information that one was not constructed because the object is ignored for diffcalc purposes). But this would require new API at best, and substantial restructuring of how the difficulty objects are created at worst.

Another possibility would be to require 1:1 hitobject-difficulty hitobject correspondence, and force pp developers to return 'ignored' difficulty objects for hitobjects they don't care about.

Screenshots or videos

n/a

Version

current master

Logs

n/a

[peppy]

Another possibility would be to require 1:1 hitobject-difficulty hitobject correspondence, and force pp developers to return 'ignored' difficulty objects for hitobjects they don't care about.

Without looking into this too deeply, I agree with this direction to keep things simple. Curious to hear what @smoogipoo thinks.

[apollo-dw]

The 1:1 correspondance is likely not going to be feasible - a direction going forward is that some calculations may involve splitting one hitobject into multiple difficulty hitobjects. This is pretty much always going to be via nested hitobjects, so for mania this is likely to be splitting long notes into head notes and tail notes... or for osu! it'd be splitting a slider into sliderhead, ticks, etc.

[peppy]

What's the motivation behind that? Is it something that could be solved by changing the structure of DifficultyHitObject?

[apollo-dw]

Well, the motivation is to award strain and use strain decay for those nested hitobjects. Difficulty processing in skills is ran on the difficulty hitobjects list sequentially, so the nested hitobjects are simply added to the list there in the creation method - which is important because of the whole lastObject and deltaTime stuff... so seems somewhat awkward to me currently...

It might become easier though with some WIP refactoring changes that StanR and I are working on, so maybe worth revisiting that specific avenue then?

[smoogipoo]

@apollo-dw I'm not sure DifficultyHitObjects are intended to be used for nested hitobjects... To be clear, there's no clear definition, but it'd make things a bit harder to manage imo.

For evaluating the strain of heads and tails separately, I'd imagine that as evaluating the strain of a hold note in respect to the head - so a Skill that responds to HoldNote and computes the strain for the head. And a separate Skill doing the same for the tail if needed.

Another possibility would be to require 1:1 hitobject-difficulty hitobject correspondence, and force pp developers to return 'ignored' difficulty objects for hitobjects they don't care about.

I generally also agree with this.

[apollo-dw]

The suggestion with different skills for each type of nested hit object doesn't exactly sound easy to manage... I've tested nested hit objects in the difficulty hit object loop before and it works as expected for osu!, and I know that the mania folks are also looking into doing it which is why I'm pushing against requiring the 1:1 thing. I could go into the advantages here but they're essentially summarised by 'convenience'. I'd be interested in hearing your reasoning against the nested hit objects if you absolutely believe that that should be the use case @smoogipoo

Would a one-to-many relationship here also throw off the pp counter? is my next question. If it doesn't, then a potential solution in my head is return the ignored difficulty objects (as suggested), and to only add hitobjects to the progressive beatmap if they are /not/ nested.

EDIT: Another big reason why adding nested hit objects to the ODHO list would be good is for reasons of strain: A difficult slider with many repeats and ticks will not be represented properly as one huge strain value with respect to strain decay and strain aggregation

[bdach]

#27808 is close enough to a resolution for this.