Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update osu! spinner ticks calculation method to better match with osu!(stable) #24661

Merged
merged 3 commits into from
Sep 12, 2023

Conversation

frenzibyte
Copy link
Member

osu!(stable) calculate spins requirement for completion in "half spins" unit, meanwhile lazer tries to do roughly the same but applies a 0.6x factor to "convert" into full spins. This PR updates the logic to calculate as full spins directly.

@peppy
Copy link
Member

peppy commented Aug 30, 2023

@smoogipoo to confirm, you are on board with this change correct?

@smoogipoo
Copy link
Contributor

Contrary to #23732 (comment), I still believe it's fine to go ahead with this even if it's going to deviate even further in the future.

@bdach
Copy link
Collaborator

bdach commented Aug 30, 2023

I'm not sure anything in that comment is saying that it's not fine to go ahead with this, for whatever that's worth.

@peppy
Copy link
Member

peppy commented Aug 30, 2023

Cool, let's get this one in after it's confirmed to be (more) correct.

@bdach bdach self-requested a review August 30, 2023 19:06
@bdach
Copy link
Collaborator

bdach commented Aug 30, 2023

Review resources

Relevant stable bits

// HitObjectManager.UpdateVariables() /////////////////////////////////////////////////////////////////////////////////

            SpinnerRotationRatio =
                MapDifficultyRange(Beatmap.DifficultyOverall, 3, 5, 7.5);

// SpinnerOsu.UpdateTransformations() /////////////////////////////////////////////////////////////////////////////////

            rotationRequirement = (int)((float)Length / 1000 * hitObjectManager.SpinnerRotationRatio);
            // note: this is in HALF SPINS, as:

// SpinnerOsu.Update() ////////////////////////////////////////////////////////////////////////////////////////////////

                editorCircleRotation.EndFloat = (float)(rotationRequirement * Math.PI);
                // ...
                floatRotationCount += Math.Min(1, Math.Abs((float)(rotationAddition / Math.PI)));

// SpinnerOsu.rotationCount ///////////////////////////////////////////////////////////////////////////////////////////

        internal int rotationCount => (int)floatRotationCount;

// SpinnerOsu.GetScorePoints() ////////////////////////////////////////////////////////////////////////////////////////

            //If we have actually progressed, let's return some score...
            if (rotationCount == lastRotationCount) return IncreaseScoreType.Ignore;

            scoringRotationCount++;

            IncreaseScoreType score = IncreaseScoreType.Ignore;

            if (SkinManager.Current.SpinnerFrequencyModulate)
                AudioEngine.UpdateSpinSample((float)scoringRotationCount / rotationRequirement);

            if (scoringRotationCount > rotationRequirement + 3 && (scoringRotationCount - (rotationRequirement + 3)) % 2 == 0)
            {
                if (spriteGlow != null)
                    spriteGlow.FlashColour(Color.White, 200);

                score = IncreaseScoreType.SpinnerBonus;
                if (!ModManager.CheckActive(Mods.Cinema))
                    AudioEngine.PlaySample(@"spinnerbonus", AudioEngine.VolumeSample, SkinSource.All);
                SpriteBonusCounter.Text = (1000 * (scoringRotationCount - (rotationRequirement + 3)) / 2).ToString();
                SpriteBonusCounter.Transformations.Clear();
                SpriteBonusCounter.Transformations.Add(new Transformation(TransformationType.Fade, 1, 0, AudioEngine.Time, AudioEngine.Time + 800));
                SpriteBonusCounter.Transformations.Add(new Transformation(TransformationType.Scale, 2f, 1.28f, AudioEngine.Time, AudioEngine.Time + 800));
                SpriteBonusCounter.Transformations[0].Easing = EasingTypes.Out;
                SpriteBonusCounter.Transformations[1].Easing = EasingTypes.Out;
                //Ensure we don't recycle this too early.
                SpriteBonusCounter.Transformations.Add(new Transformation(TransformationType.Fade, 0, 0, EndTime + 800, EndTime + 800));
            }
            else if (scoringRotationCount > 1 && scoringRotationCount % 2 == 0)
                score = IncreaseScoreType.SpinnerSpinPoints;
            else if (scoringRotationCount > 1)
                score = IncreaseScoreType.SpinnerSpin;
                
            lastRotationCount = rotationCount;

// SpinnerOsu.fullScoreRequirement /////////////////////////////////////////////////////////////////////////////////////

        private int fullScoreRequiement => useOldAlgorithm ? rotationRequirement + 1 : rotationRequirement;

// SpinnerOsu.Hit() ////////////////////////////////////////////////////////////////////////////////////////////////////
// (new algorithm only, I'm intentionally ignoring the old one) ////////////////////////////////////////////////////////

                if (rotationRequirement == 0)
                {
                    PlaySound();
                    return IncreaseScoreType.Hit300;
                }

                if (scoringRotationCount < minimumRequirement)
                    return IncreaseScoreType.Miss;

                PlaySound();

                if (scoringRotationCount > fullScoreRequiement) return IncreaseScoreType.Hit300;
                if (scoringRotationCount > fullScoreRequiement - 2) return IncreaseScoreType.Hit100;
                return IncreaseScoreType.Hit50;

Empirical testing

Spinner Test - Spinner Test.osz.zip (remove last .zip extension to use) contains 6 difficulties, each has 2 spinners each. Each difficulty has a different OD value (from 0 to 10, step 2).

To empirically examine the spin counts, apply this to stable:

diff --git a/osu!/GameplayElements/HitObjects/Osu/SpinnerOsu.cs b/osu!/GameplayElements/HitObjects/Osu/SpinnerOsu.cs
index a806c79..0e70f36 100644
--- a/osu!/GameplayElements/HitObjects/Osu/SpinnerOsu.cs
+++ b/osu!/GameplayElements/HitObjects/Osu/SpinnerOsu.cs
@@ -16,6 +16,7 @@ using osu.Helpers;
 using osu.Input;
 using osu.Input.Handlers;
 using osu_common;
+using osu_common.Helpers;
 using Un4seen.Bass;
 
 namespace osu.GameplayElements.HitObjects.Osu
@@ -227,6 +228,7 @@ namespace osu.GameplayElements.HitObjects.Osu
 
             floatRotationCount = 0;
             rotationRequirement = (int)((float)Length / 1000 * hitObjectManager.SpinnerRotationRatio);
+            Logger.Log($"Spinner @ {StartTime}: {nameof(rotationRequirement)} = {rotationRequirement} (HALF spins)");
             maxAccel = 0.00008 + Math.Max(0, (5000 - (double)Length) / 1000 / 2000);
         }
 

and this to lazer:

diff --git a/osu.Game.Rulesets.Osu/Objects/Spinner.cs b/osu.Game.Rulesets.Osu/Objects/Spinner.cs
index f32c6ae979..8e233326ee 100644
--- a/osu.Game.Rulesets.Osu/Objects/Spinner.cs
+++ b/osu.Game.Rulesets.Osu/Objects/Spinner.cs
@@ -5,6 +5,7 @@
 using System.Collections.Generic;
 using System.Linq;
 using System.Threading;
+using osu.Framework.Logging;
 using osu.Game.Audio;
 using osu.Game.Beatmaps;
 using osu.Game.Beatmaps.ControlPoints;
@@ -48,6 +49,7 @@ protected override void ApplyDefaultsToSelf(ControlPointInfo controlPointInfo, I
             double minimumRotationsPerSecond = IBeatmapDifficultyInfo.DifficultyRange(difficulty.OverallDifficulty, 1.5, 2.5, 3.75);
 
             SpinsRequired = (int)(secondsDuration * minimumRotationsPerSecond);
+            Logger.Log($"Spinner @ {StartTime}: SpinsRequired = {SpinsRequired} (FULL spins)");
             MaximumBonusSpins = (int)((maximum_rotations_per_second - minimumRotationsPerSecond) * secondsDuration);
         }
 

osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSpinner.cs Outdated Show resolved Hide resolved

double secondsDuration = Duration / 1000;

double minimumRotationsPerSecond = stable_matching_fudge * IBeatmapDifficultyInfo.DifficultyRange(difficulty.OverallDifficulty, 3, 5, 7.5);
double minimumRotationsPerSecond = IBeatmapDifficultyInfo.DifficultyRange(difficulty.OverallDifficulty, 1.5, 2.5, 3.75);
Copy link
Collaborator

@bdach bdach Aug 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine and all, but the rather large elephant in the room is 2 lines below:

SpinsRequired = (int)(secondsDuration * minimumRotationsPerSecond);

Stable counts in half-spins. lazer counts in full spins, which is reflected by the fact that the fudge is now factored into the difficulty range here. But then lazer floors the spin count, which means that this happens:

Spinner 1 from test maps (@ 2000ms)

┌────┬─────────────────────────────────┬───────────────┐
│ OD │ stable                          │ lazer         │
├────┼─────────────────────────────────┼───────────────┤
│  0 │ 12 half spins =  6   full spins │  6 full spins │
│  2 │ 15 half spins =  7.5 full spins │  7 full spins │
│  4 │ 18 half spins =  9   full spins │  9 full spins │
│  6 │ 22 half spins = 11   full spins │ 11 full spins │
│  8 │ 26 half spins = 13   full spins │ 13 full spins │
│ 10 │ 30 half spins = 15   full spins │ 15 full spins │
└────┴─────────────────────────────────┴───────────────┘

Spinner 2 from test maps (@ 7000ms)

┌────┬─────────────────────────────────┬───────────────┐
│ OD │ stable                          │ lazer         │
├────┼─────────────────────────────────┼───────────────┤
│  0 │ 27 half spins = 13.5 full spins │ 13 full spins │
│  2 │ 34 half spins = 17   full spins │ 17 full spins │
│  4 │ 41 half spins = 20.5 full spins │ 20 full spins │
│  6 │ 49 half spins = 24.5 full spins │ 24 full spins │
│  8 │ 58 half spins = 29   full spins │ 29 full spins │
│ 10 │ 67 half spins = 33.5 full spins │ 33 full spins │
└────┴─────────────────────────────────┴───────────────┘

wherein "stable" is stable's rotationRequirement and "lazer" is lazer's SpinsRequired.

tl;dr: this implementation is still potentially half a spin off (too low).

Is this fine? I don't know. But this wasn't brought up anywhere, and I figured it needs attention.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also for consideration is that if we change to half spins, @smoogipoo's work for anti-cheese may need changes to implementation (@smoogipoo clarification on this if possible).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't say I even have an implementation for anti-cheese at this point. But as for whether it's okay... probably?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear I'm not necessarily saying that we need to do the half spin thing. It's probably fine for lazer spinners to be half a spin easier than stable's. I just... don't know if we wanna do that and stay there, or do something else.

Copy link
Member Author

@frenzibyte frenzibyte Sep 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good observation. In my opinion, I think it's better to have the full spins count floored, mainly for compatibility reasons. See, with #24662 in mind, flooring the full spins count would keep the bonus count matching with stable in the "odd required half spins" case, and less than stable by 1 half spin in the "even required half spins" case.

In the "odd required half spins" case, the reason why it matches stable is because in stable there's a 1.5x spin gap between completing the spinner and receiving the first bonus, and we're accommodating that by having the spinner finish 1 half spin early and add a 2x spin gap instead. Therefore the first bonus would be received at the exact moment between stable and lazer, along with all the bonus ticks afterwards.

To put the above in practice, here's a frame-by-frame shot on the first spinner of the OD0 test map, in stable:

CleanShot.2023-09-01.at.22.07.59.mp4

And here it is in lazer:

CleanShot.2023-09-01.at.22.03.13.mp4

Meanwhile, in the "even required half spins" case, it's less than stable by 1 half spin due to the fact that the first bonus is gained at a half spin point, which is impossible to accommodate for in lazer while it is using full spins, so bonus gains in lazer will be shifted forward by an extra half spin compared to stable. I don't believe it's worthy of fixing in my own opinion.

In summary, the state this PR is in should be best with regards to matching stable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still leaning towards fixing the half spin, but haven't had a chance to compare visuals yet.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "fixing the half spin" mean here? Is it to bring half spin mechanism into osu!(lazer) and have lazer match stable calculations? Or is it to round the full spin count evenly rather than flooring it / casting to int?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in satisfying the change in the other PR. Making sure the spinner matches behaviour wise to stable in terms of timings.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be the case already, I've explained everything in #24661 (comment). Basically there should be nothing to change in this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you'd misunderstanding. I was responding directly to @bdach above. It has nothing to do with this PR but I was providing feedback on it as it may help progress this PR's change.

@peppy peppy merged commit 7db4b3e into ppy:master Sep 12, 2023
12 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

4 participants