Refactor of osu!taiko difficulty calculation code #31636
Conversation
|
!diffcalc |
|
oof, something is wrong |
|
!diffcalc |
|
!diffcalc |
| @@ -111,6 +117,27 @@ public TaikoDifficultyHitObject(HitObject hitObject, HitObject lastObject, HitOb | |||
| NoteIndex = noteObjects.Count; | |||
| noteObjects.Add(this); | |||
| } | |||
|
|
|||
| // Using `hitObject.StartTime` causes floating point error differences | |||
| double normalizedStartTime = StartTime * clockRate; | |||
There was a problem hiding this comment.
Just for consistency we use non-american everywhere else
| /// <summary> | ||
| /// Represents a group of <see cref="TaikoDifficultyHitObject"/>s with no rhythm variation. | ||
| /// </summary> |
There was a problem hiding this comment.
Is this a sequential grouping of hitobjects that share the same rhythm? Would probably mention that for the reader.
There was a problem hiding this comment.
It is.
Represents a group of sequential <see cref="TaikoDifficultyHitObject"/>s with no rhythm variation.
| /// </summary> | ||
| public class SameRhythmGroupedHitObjects : IHasInterval | ||
| { | ||
| public List<TaikoDifficultyHitObject> Children { get; private set; } |
There was a problem hiding this comment.
Would probably name this "HitObjects" or "Objects".
This, along with the class renaming comment above, apply to the other class as well. Over there I think "Groups" makes more sense than "Children".
|
|
||
| public TaikoDifficultyHitObject FirstHitObject => Children[0]; | ||
|
|
||
| public SameRhythmGroupedHitObjects? Previous; |
There was a problem hiding this comment.
Publicly settable? Also for a few of the fields below.
|
|
||
| /// <summary> | ||
| /// The interval in ms of each hit object in this <see cref="SameRhythmGroupedHitObjects"/>. This is only defined if there is | ||
| /// more than two hit objects in this <see cref="SameRhythmGroupedHitObjects"/>. |
There was a problem hiding this comment.
Maybe just me but I find it weird that this is only valid for >2 objects yet the class itself is used when there's ">1" hitobjects.
This code in particular breaks my brain:
osu/osu.Game.Rulesets.Taiko/Difficulty/Evaluators/RhythmEvaluator.cs
Lines 44 to 53 in 13c956c
Reading as "if the previous group has >2 objects and the current group has >1 object, do this thing". Perhaps there needs to be a more informative bool-returning method along the lines of "isvalid" to denote the valid use-cases of this class...
My original comment is that I would expect this to be valid for two hitobjects, and to be 0 for one hitobject.
There was a problem hiding this comment.
sameRhythmGroupedHitObjects.Children.Count > 1 ensures the current group (with the previous having an interval has more than one object to ensure there's an interval to compare, instead of a single object, I'll agree that my comment is quite confusing however.
If a previous interval exists, and there is a current interval to compare
| /// <summary> | ||
| /// Represents a group of <see cref="TaikoDifficultyHitObject"/>s with no rhythm variation. | ||
| /// </summary> | ||
| public class SameRhythmGroupedHitObjects : IHasInterval |
There was a problem hiding this comment.
Would probably prefer something more along the lines of SameRhythmHitObjectGrouping than a pluralised class containing a plural of objects.
| namespace osu.Game.Rulesets.Taiko.Difficulty.Preprocessing.Rhythm.Data | ||
| { | ||
| /// <summary> | ||
| /// Represents <see cref="SameRhythmGroupedHitObjects"/> grouped by their <see cref="SameRhythmGroupedHitObjects.StartTime"/>'s interval. |
There was a problem hiding this comment.
This xmldoc doesn't do much for me without reading into how it works. Can it be explained in less code-y terms?
For example, does every grouping in this class have the same rhythm?
There was a problem hiding this comment.
It represents a group of hit objects grouped by the same spacing between their start times (interval). This then is calculated as a singular rhythmic group or segment. Every grouping in this class will be grouped by having the same interval, with the changes garnering the difficulty between them.
|
|
||
| namespace osu.Game.Rulesets.Taiko.Difficulty.Preprocessing.Rhythm | ||
| { | ||
| public static class TaikoRhythmDifficultyPreprocessor |
There was a problem hiding this comment.
This class is way too much to understand for me without any comments. How are we grouping things? What is a "pattern" grouping and how does that differ from a "rhythm" grouping?
i.e. if we have the objects aa b b b aa b b b do we create 4 "rhythm" groupings (aa, b b b, aa, b b b) and one "pattern" grouping containing all 4 of those? Or maybe 2 "pattern" groupings as aa b b b?
There was a problem hiding this comment.
Would be grouped as the first example. Can admit my lack of comments in this isn't ideal.
Rhythm grouping follows the individual note intervals - putting the hit objects intervals when they match, essentially objects that share a basic spacing in time.
Pattern grouping is the parent class of this, it looks at the groups of notes within the rhythm groups, and put them together when the repetition of intervals is consistently the same across multiple sections of maps. The first matches the time gap between individual notes, whereas the parent compares the whole patterns of intervals among the groups of notes.
Below would be more inline comment friendly.
Rhythm grouping = Groups of individual notes that share the same interval.
Pattern grouping = Groups of rhythm groups that share repeating interval changes.
|
Requesting review from @peppy because you reviewed the taiko changes in the primary PR. |
|
Looping in @Lawtrohux to address some of these review comments as a lot of the things commented stand prior to my refactor so I think he's better tasked at explaining some of these workings than I am. |
General refactor of some of the recent osu!taiko changes to be a bit more maintainable and less weird.