Refactor OsuDifficultyHitObject for readability/understandability #15774

smoogipoo · 2021-11-24T05:02:51Z

I recommend going through this PR per-commit as the diff is a little bit of a mess.

The primary issue I had is that TravelDistance and TravelTime stored information about the previous hitobject, in the current hitobject. I've made these two store information about the current hitobject only.
The rest is a bit of refactoring to clean things up - property ordering, naming (American vs British English), and expanding on the comment describing how MovementDistance is calculated.

I've tested that SR and difficulty attributes don't change compared to current master.

peppy · 2021-11-24T05:08:31Z

osu.Game.Rulesets.Osu/Difficulty/Skills/Aim.cs

@@ -50,7 +50,7 @@ private double strainValueOf(DifficultyHitObject current)
            if (osuLastObj.BaseObject is Slider && withSliders)


Confirm that this is still correct as osuLastObj here? Looks weird with the changes inside this conditional now reading from one index back while the conditional itself isn't.

(it probably is, just a hard change to read due to how complex things are)

This is one of the reasons for me changing it, because it was using osuCurrObj.TravelDistance to access the travel distance of osuLastObj.

So yes, this condition is still fine as it is, the only change is that the properties apply directly to the relevant hitobject now.

To add on as to why - all these blocks of code try to do (perhaps not explained enough in the leading comments?) is to calculate the velocity through a previous slider into the current object. One part is stored in the previous object (travel), and the other is stored in the current object (jump / movement, however you want to think about it).

smoogipoo · 2021-11-24T07:16:14Z

Don't merge this yet as I realised my testing was flawed. Will unblock after re-confirming SR is unaffected.

smoogipoo · 2021-11-24T07:50:57Z

Found an issue and re-tested, looks okay now.

stanriders · 2021-11-24T12:09:46Z

osu.Game.Rulesets.Osu/Difficulty/Preprocessing/OsuDifficultyHitObject.cs

+        /// <remarks>
+        /// This is bounded by <see cref="JumpDistance"/>, but may be smaller if a more natural path is able to be taken through a preceding slider.
+        /// </remarks>
+        public double MovementDistance { get; private set; }


Maybe rename it to something like MinimumMovementDistance? Overall distance/time fields could use better naming so that it can be more obvious which does what without reading docs every time.

Renamed to MinimumJumpDistance.

bdach

Haven't looked at diffcalc for a while but the transformation generally looks correct to me. Just want to clarify one xmldoc (there's also a nitpick in this review but I don't care about it very much)

osu.Game.Rulesets.Osu/Difficulty/Preprocessing/OsuDifficultyHitObject.cs

osu.Game.Rulesets.Osu/Difficulty/Skills/Aim.cs

smoogipoo · 2021-12-07T14:16:22Z

I've applied some renames + documented a bit more thoroughly. Let me know if this is understandable now.

stanriders · 2021-12-07T14:25:54Z

Yep, seems much better to me now

bdach · 2021-12-07T18:21:29Z

The latest update to docs/names looks good to me now, but there is a merge conflict here and I'm unsure whether my resolution is correct... I think this would be the correct one?

diff --cc osu.Game.Rulesets.Osu/Difficulty/Skills/Speed.cs
index 75a9b13bdf,4f87767fa7..ca89236258
--- a/osu.Game.Rulesets.Osu/Difficulty/Skills/Speed.cs
+++ b/osu.Game.Rulesets.Osu/Difficulty/Skills/Speed.cs
@@@ -154,8 -154,7 +154,7 @@@ namespace osu.Game.Rulesets.Osu.Difficu
              if (strainTime < min_speed_bonus)
                  speedBonus = 1 + 0.75 * Math.Pow((min_speed_bonus - strainTime) / speed_balancing_factor, 2);
  
-             double travelDistance = osuPrevObj?.TravelDistance ?? 0;
-             double distance = Math.Min(single_spacing_threshold, travelDistance + osuCurrObj.LazyJumpDistance);
 -            double distance = Math.Min(single_spacing_threshold, osuCurrObj.TravelDistance + osuCurrObj.MovementDistance);
++            double distance = Math.Min(single_spacing_threshold, osuCurrObj.TravelDistance + osuCurrObj.MinimumJumpDistance);
  
              return (speedBonus + speedBonus * Math.Pow(distance / single_spacing_threshold, 3.5)) / strainTime;
          }

peppy · 2021-12-08T05:00:54Z

osu.Game.Rulesets.Osu/Difficulty/Preprocessing/OsuDifficultyHitObject.cs

+                // 2.  <======o==>---o
+                //            |______|
+                //
+                // Where "<==>" represents a slider, and "o" represents where the cursor needs to be for either hitobject (for a slider, this is the lazy cursor position).


I'd recommend representing the cursor location with something that isn't a "o". This reads as if it's hitobjects, which can make for confusion here. How about using a * or x or something?

peppy · 2021-12-08T06:09:46Z

osu.Game.Rulesets.Osu/Difficulty/Preprocessing/OsuDifficultyHitObject.cs

                //            | /
-                //            o
+                //            x


I still don't quite understand what this diagram is trying to explain, but as long as others can maybe it's okay.

The diagrams show how the natural movement path of a player changes depending on what the angle of a follow-up circle is, and how using the lazy cursor end is completely fine in diagram 1 ("players will cut the slider short in order to move to the next object"), but will overestimate the perceived jump distance in diagram 2 ("players will follow the slider through to its visual extent").

I don't know how to better explain it.

Am I supposed to know that there's a second object below the slider in the first diagram? So the "x" represents the position of the slider ball and also the position of the next object here?

No, it represents the final position your cursor needs to be in a slider given the most lazy movements (following at the edge of the follow circle at all times) + the -36ms offset for the last slider tick.

But the downwards movement implies there's something beneath it right? If so it's be nice to show that next object to give more context.

I misunderstood what you meant, I thought you were referring to the "x" on the slider track.

How should I show that the "x" below the slider is another hitobject? Any suggestions? Would this be better?

I can't easily add the same text to the second diagram...

Maybe they should be shown using o after all, and the documentation should not mention anything about the cursor position. The cursor movement is shown by the single-width line, which should be ample to explain the concept here.

Might help to split the diagram out into two versions beside each other: one with the "lazy" movement and one with the "correct" movement (ie the one denoted by /). Feels weird showing both on the same diagram.

I've restructured it, see if it's better now?

Yep, much better.

smoogipoo · 2021-12-08T06:56:28Z

@bdach I've resolved conflicts, note that your patch is slightly incorrect because this PR moves TravelDistance to the hitobject which is actually being traveled.

…iculty-hit-object" This reverts commit 603ba61, reversing changes made to b393f83.

smoogipoo added 6 commits November 24, 2021 12:14

Reorder members a bit

e67d9b1

Make TravelDistance/TravelTime apply to the current object

402de75

Reword comment with a more diagrammatical explanation

e07c44d

Normalized -> Normalised

a081038

Reword xmldocs

b5747f3

Add additional information to diagram

274444e

smoogipoo added type:code-quality ruleset/osu! area:difficulty labels Nov 24, 2021

pull-request-size bot added the size/L label Nov 24, 2021

smoogipoo mentioned this pull request Nov 24, 2021

Make speed skill consider only the shortest movement distance #15758

Merged

peppy reviewed Nov 24, 2021

View reviewed changes

peppy previously approved these changes Nov 24, 2021

View reviewed changes

peppy requested a review from bdach November 24, 2021 05:34

smoogipoo added the blocked Don't merge this. label Nov 24, 2021

Ensure travel distance is calculated for all sliders

b20ff22

smoogipoo dismissed peppy’s stale review via b20ff22 November 24, 2021 07:50

smoogipoo removed the blocked Don't merge this. label Nov 24, 2021

stanriders reviewed Nov 24, 2021

View reviewed changes

bdach reviewed Nov 24, 2021

View reviewed changes

osu.Game.Rulesets.Osu/Difficulty/Preprocessing/OsuDifficultyHitObject.cs Outdated Show resolved Hide resolved

osu.Game.Rulesets.Osu/Difficulty/Skills/Aim.cs Show resolved Hide resolved

smoogipoo added 3 commits November 25, 2021 14:08

Invert lines for better chronological order

3e4b774

Merge branch 'master' into refactor-osu-difficulty-hit-object

7c3d1d6

Rename + better documentation

ded8628

peppy reviewed Dec 8, 2021

View reviewed changes

Use "x" for cursor position in diagrams

7c0f7b1

peppy reviewed Dec 8, 2021

View reviewed changes

smoogipoo added 3 commits December 8, 2021 15:17

Use new LazyJumpDistance terminology in documentation

814f072

Restructure doc for easier readability

1110412

Merge branch 'master' into refactor-osu-difficulty-hit-object

1a09a34

Minor cleanups, unifying wording a bit more

99991a6

bdach approved these changes Dec 8, 2021

View reviewed changes

bdach requested a review from peppy December 8, 2021 18:20

peppy approved these changes Dec 9, 2021

View reviewed changes

peppy merged commit 603ba61 into ppy:master Dec 9, 2021

smoogipoo added a commit to smoogipoo/osu that referenced this pull request Jan 17, 2022

Revert "Merge pull request ppy#15774 from smoogipoo/refactor-osu-diff…

594ffd9

…iculty-hit-object" This reverts commit 603ba61, reversing changes made to b393f83.

smoogipoo deleted the refactor-osu-difficulty-hit-object branch September 11, 2023 02:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor OsuDifficultyHitObject for readability/understandability #15774

Refactor OsuDifficultyHitObject for readability/understandability #15774

smoogipoo commented Nov 24, 2021

peppy Nov 24, 2021 •

edited

Loading

smoogipoo Nov 24, 2021

smoogipoo Nov 24, 2021

smoogipoo commented Nov 24, 2021

smoogipoo commented Nov 24, 2021

stanriders Nov 24, 2021

smoogipoo Dec 7, 2021

bdach left a comment

smoogipoo commented Dec 7, 2021

stanriders commented Dec 7, 2021

bdach commented Dec 7, 2021

peppy Dec 8, 2021

smoogipoo Dec 8, 2021

peppy Dec 8, 2021

smoogipoo Dec 8, 2021

peppy Dec 8, 2021

smoogipoo Dec 8, 2021

peppy Dec 8, 2021

smoogipoo Dec 8, 2021

peppy Dec 8, 2021

smoogipoo Dec 8, 2021

peppy Dec 8, 2021

smoogipoo commented Dec 8, 2021

		@@ -50,7 +50,7 @@ private double strainValueOf(DifficultyHitObject current)
		if (osuLastObj.BaseObject is Slider && withSliders)

Refactor OsuDifficultyHitObject for readability/understandability #15774

Refactor OsuDifficultyHitObject for readability/understandability #15774

Conversation

smoogipoo commented Nov 24, 2021

peppy Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoogipoo commented Nov 24, 2021

smoogipoo commented Nov 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdach left a comment

Choose a reason for hiding this comment

smoogipoo commented Dec 7, 2021

stanriders commented Dec 7, 2021

bdach commented Dec 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoogipoo commented Dec 8, 2021

peppy Nov 24, 2021 •

edited

Loading