Refactor difficulty calculation flow to move control from DifficultyCalculator to Skills - #38529
Refactor difficulty calculation flow to move control from DifficultyCalculator to Skills#38529stanriders wants to merge 2 commits into
DifficultyCalculator to Skills#38529Conversation
…Calculator` to `Skill`s
stanriders
added
code quality
tsunyoku
left a comment
tsunyoku
left a comment
There was a problem hiding this comment.
I think I prefer this flow overall - in that skills processing the entire map in whichever way it decides definitely has its benefits for mania at the very least. I also like that this means skills can take control of attributes, rather than having to maintain state for its entire existence and expose methods to fetch related data.
That said, some of the details around how Process and the attributes don't feel ideal. I feel like I'd want something with stronger typing but I'm not sure how that'd look like immediately. Maybe my specific review comments will inspire.
| public class AimAttributes : VariableLengthStrainSkillAttributes | ||
| { | ||
| public required bool WithSliders { get; init; } | ||
| public required double DifficultSlidersCount { get; init; } | ||
| public required double TopWeightedSlidersCount { get; init; } | ||
|
|
||
| public AimAttributes(VariableLengthStrainSkillAttributes baseAttributes) | ||
| { | ||
| Difficulty = baseAttributes.Difficulty; | ||
| ObjectDifficulties = baseAttributes.ObjectDifficulties; | ||
| TopWeightedStrainsCount = baseAttributes.TopWeightedStrainsCount; | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure I like this idea of a class inheriting from T whilst also having a constructor that takes in an object of type T. I suppose your intention here was not to have to assign every attribute in the base skill attributes class but I'm not sure.
There was a problem hiding this comment.
My intention here was to guarantee that base class properties don't get lost on inherited classes initialisation. Maybe I'm trying to be too smart here and we can just leave the responsibility of doing that on the inheritor, I'm not sure
There was a problem hiding this comment.
As long as every property has a required on it then it should be fine shouldn't it?
There was a problem hiding this comment.
It somehow completely slipped my mind. Changed in bf7cbfe
| public override ISkillAttributes Process() | ||
| { | ||
| var baseAttributes = (VariableLengthStrainSkillAttributes)base.Process(); | ||
|
|
||
| return new AimAttributes(baseAttributes) | ||
| { | ||
| WithSliders = IncludeSliders, | ||
| DifficultSlidersCount = getDifficultSliders(), | ||
| TopWeightedSlidersCount = countTopWeightedSliders(baseAttributes.Difficulty) | ||
| }; | ||
| } |
There was a problem hiding this comment.
Hmm. I'm not sure. I don't love having to cast base.Process, which may be fixable with some generics on the skill class, but I feel like I'd want to go yet another step further and use interfaces to achieve this instead?
I'm not 100% sure what design I have in my head but it feels weird to have every skill forcefully override Process, call the base Process, cast the result and then return it's own attributes.
There was a problem hiding this comment.
I have tried making ISkill have a generic param for skill attributes, but gave up on it pretty early. If I remember correctly it didn't really solve the issue of multiple layers of inheritence having to do some nasty casts
2 participants
RFC.
Currently difficulty calculation is completely controlled by the
DifficultyCalculatorandSkills exist only to store state while theDifficultyCalculatorgoes through everyDifficultyHitObject. It then can callDifficultyValueand other methods ofSkills to get the final aggregated difficulty values, which it usually does inCreateDifficultyAttributes.Current flow looks pretty much like this:
This is pretty convoluted so this change is proposing flow that looks roughly like this:
As you can see it makes all object iteration and aggregation move to skills instead of being in the calculator itself.
CreateDifficultyAttributesthen gets a final result of skill processing in a form ofISkillAttributesand only does final difficulty attributes creation instead of doing parts of the difficulty calculation.Benefits of this approach apart from untangling the flow is allowing skills to define how exactly they want to process the map instead of forcing sequential per-object approach (which for example would greatly benefit potential mania changes since mania often has multiple objects at the same time which should be evaluated together) and allowing skills to expose more information to diffcalc consumers like osu-tools and potentially clientside stuff like map difficulty graphs.
Consider this PR to be sort of an PoC - I've made sure it works for the most part but don't expect it to be merge-ready in any way. I'm interested in having more eyes on the new flow itself before ironing out stuff like tests or timed calculation inconsistencies. Would appreciate hearing feedback sooner than later so that we can have all the major refactoring work done before any other changes.
Benchmarks:
Old:
New: