Replace indexed skill access with `skills.First(s is ...)`

[stanriders]

Something that shouldve been done long ago

[stanriders]

I guess this is an improvement but I'm not sure this goes far enough? As in, if we're going to such lengths, then why not a custom thin wrapper data structure, e.g. your basic container?

I'm not sure if that's really necessery honestly, since all this tries to achieve is save someone from inevitabely shooting themselves in a foot by messing up indices

[Givikap120]

I guess this is an improvement but I'm not sure this goes far enough? As in, if we're going to such lengths, then why not a custom thin wrapper data structure, e.g. your basic container?

I'm not sure if that's really necessery honestly, since all this tries to achieve is save someone from inevitabely shooting themselves in a foot by messing up indices

this change is very good when you have something like Flashlight and Hidden at the same time because in this case using indices is very painful

[smoogipoo]

Is there any path where we can remove CreateSkills() and have them managed by the class instead? The overall idea is to break down DifficultyCalculator into more but simpler units of work.

With regards to the other recent issues surrounding the timed attributes, perhaps we should look towards restructuring things along the following:

// Internal implementation - usage entrypoint.
public void Calculate()
{
}

// Ruleset implementation - reset skills and anything else in preparation for a new calculation.
protected abstract void Reset(IBeatmap beatmap, Mod[] mods)
{
    // skill = new MySkill();

    // skill.Reset();
}

// Ruleset implementation - process a single hitobject with all skills.
protected abstract void ProcessSingle(DifficultyHitObject hitObject)
{
    // skill.Process(hitObject);

    // foreach (var skill in skills)
    //    skill.Process(hitObject)
}

// Ruleset implementation - create the difficulty attributes from the current state.
protected abstract DifficultyAttributes CreateAttributes()
{
}

Skills would exist as auxiliary concepts - they are provided for if they serve your needs but you don't need to use them. You can do whatever you want in ProcessSingle().

I strongly believe we should not be passing in the beatmap at all to CreateAttributes and instead store constants (hitwindows/etc) in Reset() and calculate combo as hitobjects arrive during ProcessSingle().

It may also be worthwhile to look into making ProcessSingle work on HitObject and to decide how to link them up (last, lastLast, lastLastLast, etc) itself. There's several paths that could be tried here with consideration to clockRate. This is not initially required and can be saved for a later optimisation.

The hitobject sorting stuff also needs to go as it's a mania-specific hack that should probably just go directly in Reset() and mutate the incoming beatmap - it's already local to the difficulty calculator so nobody else cares about it. It's unnecessary processing for every other ruleset.

[smoogipoo]

Ignore the above suggestion - I'll be working on it as a separate solo effort.

[smoogipoo]

!difficulty RULESET=osu

[smoogipoo]

!difficulty RULESET=taiko

[smoogipoo]

!difficulty RULESET=catch

[smoogipoo]

!difficulty RULESET=mania

[bdach]

@stanriders please double check merge conflict resolution in https://github.com/ppy/osu/pull/30034/commits/9818f90537d093fbef1b09e762e334b600dbfc73

!difficulty RULESET=taiko

[bdach]

I can't with the merge conflicts here........

[stanriders]

I can't with the merge conflicts here........

I'm planning to resolve and merge it after we're done actively finishing up and merging reworks, to not interfere with that