Add lazy difficulty attribute calculation

[minisbett]

Second try at #36570 (continuing with their approval)

I've dissected the convoluted discussion that happened, and believe to have got the important things right:

1. The timed calculation logic was moved to CalculateTimedLazy
2. CalculateTimed calls CalculateTimedLazy, but continues to hold the 10 second timeout cancellation token, which is simply passed over
3. A cancellation of calculation will continue to throw in the inner loop (per-skill)
4. The behavior of CalculateTimed should be identical

I have also added a remark to the two CalculateTimed overloads, in order to highlight the timeout behavior that CalculateTimedLazy does not have. Whether the lazy method should actually hold the timeout logic can be discussed, but I personally don't see a need for it.

[peppy]

Looks quite sane to my eye now.

[bdach]

@storycraft does this work for you?

[storycraft]

@storycraft does this work for you?

Looks somewhat okay to me.

actually wait

[bdach]

Not sure how to feel about this being written this way. All work here is synchronous and blocks the caller, meaning this method isn't usable in async/await flows unless you know to wrap it with Task.Run() so that the sync work is marshaled off-thread to TPL. @minisbett did you intend this?

[minisbett]

This was sort-of intended. I was not sure you would want to see Task.Run here, and maybe rather want Task as some kind of result pattern.

What would be preferred? Both would work for me as I never intend to use asynchronous processing.

[storycraft]

This was sort-of intended. I was not sure you would want to see Task.Run here, and maybe rather want Task as some kind of result pattern.

What would be preferred? Both would work for me as I never intend to use asynchronous processing.

There's a reason why I said it's half ok to me.
As bdach already mentioned, the user would expect a task which is already started in parallel using Task.Run or isn't started at all.
But it blocks, which seems to defeat the purpose to me.

[bdach]

Probably wrap the entire thing in a Task.Run() so the caller can decide whether they want to async/await or they're fine with full blocking via .Result.

[peppy]

Would again, be useful to know the usages of this.

Using task wrapping is going to make things slower if not already in async-await code. And the whole idea here is performance I guess. As previously stated, without a usage this is all a lot of faffery. Not even a benchmark to be seen.

[minisbett]

Would again, be useful to know the usages of this.

I have mentioned the usage of it further up. I hadn't shown a specific code-example, because it would not really be helpful. But here you go:

[OsuNativeFunction]
[OsuNativeEnumerator<NativeTimedOsuDifficultyAttributes>]
public static ErrorCode CalculateTimedLazy(OsuDifficultyCalculatorHandle calcHandle, ModsCollectionHandle modsHandle, NativeOsuTimedDifficultyAttributesEnumeratorHandle* timedAttributesEnumeratorHandle)
{
    DifficultyCalculatorContext<OsuDifficultyCalculator> context = calcHandle.Resolve();
    Mod[] mods = modsHandle.IsNull ? [] : [.. modsHandle.Resolve().Select(x => x.ToMod(context.Ruleset))];

    IEnumerator<NativeTimedOsuDifficultyAttributes> enumerator = LazyDifficultyCalculationHelper.CalculateTimedLazy(context.Calculator, mods)
        .Select(x => new NativeTimedOsuDifficultyAttributes(x))
        .GetEnumerator();

    enumerator.MoveNext();

    *timedAttributesEnumeratorHandle = ManagedObjectStore.Store(enumerator);

    return ErrorCode.Success;
}

I'm essentially storing an enumerator of it, and a caller via a C FFI can enumerate through the timed difficulty attributes by calling a specific function.

This is far from any async context, and while both the current implementation and one using Task.Run would work, I'd personally favour the current one. But that is up to you. What could be considered is whether this method could become used in Lazers' Performance Counter skin component, as it suffers from the same performance issues on beatmaps with a lot of objects (the counter appears grayed out and stays at 0pp until the whole calculation finished).

[bdach]

Either we go with 8c5cb08 or I dunno. I'm kinda done with this discussion again. The whole method is just kooky behaviour start to end.

[minisbett]

I would be fine with that, changing it to use Task.Run could be done if the mentioned impact slow difficulty calculation has on the performance counter skin component is ever being addressed and changed to use this newly introduced method.

[minisbett]

I would like to suggest not merging this (for now?), because there is something only now deemed necessary for this to work for us: being able to skip attributes, preventing the call of CreateDifficultyAttributes but still processing all objects through the skills. Without this, getting the difficulty attributes of a specific point in time requires calculating all attributes before that time.

I'm not sure this can find it's way in here nicely? My current approach in my consumer library is heavily using reflection on the difficulty calculator, and therefore I'm curious if exposing required methods/properties would be okay? (instead of what this PR does, not additionally) That would probably save me from doing another PR in the future on something slightly related, and also make this thing in osu-tools less cheesy.

The following things would be required to be exposed:

• preProcess()
• CreateSkills()
• getDifficultyHitObjects()
• CreateDifficultyAttributes()
• Beatmap
• playableMods
• clockRate
• ProgressiveCalculationBeatmap

I do have doubts this is going to be acceptable, but wanted to ask since bdach had mentioned the possibility of it before. I personally feel like it should be okay to expose these, as that part of the osu!(lazer) code-base is pretty much the most likely to be a point of interest for third-parties to consume.

If this is acceptable, I would create a new PR for that and close this one.

---

Update: Another oversight is that unfortunately CreateDifficultyAttributes can be stateful, meaning you cannot run a new calculation and expect the enumerable to still be correct..