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. |
There was a problem hiding this comment.
As touched on on discord, I don't have any major issues with this PR. It fixes some things but not the overarching issue I have.
Which is to say we can probably merge this and then do a second pass of every line of code in a review session?
|
|
||
| for (; i < data.Count - 1; i++) | ||
| { | ||
| // An interval change occured, add the current data if the next interval is larger. |
There was a problem hiding this comment.
How does this work? Groupings include the next object if the interval increased but don't if it decreased (as per line 38)? Is there rationale for this decision?
There was a problem hiding this comment.
The rationale behind it is basically - Even though the spacing is getting a bit wider, it’s still a continuation of the same pattern.
When an interval decreases, it indicates the shift to a faster, or the start of a new rhythmic pattern, whereas the increase implies a slowdown, thus being apart of the same pattern. For example 1/4 - 1/2 is considering easy, whereas 1/4 - 1/6 isn't, we use the grouping logic to indicate the start of a 'new' or in this case a change in difficulty.
There was a problem hiding this comment.
That sounds fine, but from what I read, it only includes the next object / grouping in this case (ie. it exits the loop) – is this intended or a potential oversight?
Put another way, if the interval decreases twice, this will cause a new grouping to be created, even though there have been no increases in tempo.
There was a problem hiding this comment.
Seems fine - when i first did the grouping - the double decrease was added as it was considered drastic enough by players for a new grouping to form (i.e. going from 1/6 to 1/3 to 1/1, major slowdowns can cause some discomfort, which was the original intention. Example maps like Sun and Spirit deservedly gain as a result of this.
There was a problem hiding this comment.
Okay, if that's the intention then it needs to be documented, else it looks like a bug.
There was a problem hiding this comment.
Another potential bug: the first two objects in a group are not being compared for interval checks. Is this intentional..?
There was a problem hiding this comment.
Another subject to my terrible commenting - I chose this approach to simplify the logic to ensure the behaviour that the first object is the seed, then compare the pairs thereafter - as the second object's interval change would still be considered when looking at the third.
|
!diffcalc |
|
Difficulty calculation failed: https://github.com/ppy/osu/actions/runs/13171493303 |
|
Difficulty calculation failed: https://github.com/ppy/osu/actions/runs/13171493303 |
|
Difficulty calculation failed: https://github.com/ppy/osu/actions/runs/13171493303 |
General refactor of some of the recent osu!taiko changes to be a bit more maintainable and less weird.