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

Various slider-related difficulty changes proposal #27303

Closed
wants to merge 29 commits into from

Conversation

Givikap120
Copy link
Contributor

@Givikap120 Givikap120 commented Feb 21, 2024

List of the changes:

  • Sliderless velocity is now calculated correctly, using JumpDistance instead of LazyJumpDistance
  • Stream spacing now calculated correctly, using JumpDistance instead of MinimalJumpDistance+TravelDistance
  • Penalty for missing sliderends now calculated correctly
  • Aim now have 1.08 multiplier for sliderjumps
  • Speed now have 1.25 multiplier for sliderstreams
  • HD now awards slideraim 2 times less (oshama scramble EZHD fix)

What to expect:

This rework doesn't fix the sliders completely, and it doesn't touch complex slidershapes problem. It's main point is to fix the some flaws of the current slider system, until better system can be made.

to keep the same scale
@Givikap120

This comment was marked as outdated.

@peppy
Copy link
Member

peppy commented Feb 22, 2024

  • Please change the title to something more descriptive.
  • Please link discussions where these changes were decided.

@Givikap120 Givikap120 changed the title Sliders mini-rework Sliders difficulty calculation fix Feb 22, 2024
@Givikap120
Copy link
Contributor Author

  • Please change the title to something more descriptive.
  • Please link discussions where these changes were decided.

This is not "decided", this is rework proposal (rework proposals are not in the osu PRs anymore?)

@Givikap120 Givikap120 changed the title Sliders difficulty calculation fix Sliders difficulty calculation fix proposal Feb 22, 2024
@bdach bdach requested a review from a team February 22, 2024 06:55
added angle and velocity bonuses to sliderjump
fixed alternating bonus
@Xexxar
Copy link
Contributor

Xexxar commented Feb 26, 2024

While I do believe that sliders are hugely problematic in their current state, I personally don't have much feedback to give on this PR as I believe a more fundamental rewrite of sliders is needed to properly assess their difficulty anyway, which is an ongoing effort of mine. This likely does improve things from a values standpoint, but it seems like something that would only be a stop gap solution until something more elaborate can be created. With that in mind, I personally don't see the need to push this within the context of what else is being worked on.

@Givikap120
Copy link
Contributor Author

While I do believe that sliders are hugely problematic in their current state, I personally don't have much feedback to give on this PR as I believe a more fundamental rewrite of sliders is needed to properly assess their difficulty anyway, which is an ongoing effort of mine. This likely does improve things from a values standpoint, but it seems like something that would only be a stop gap solution until something more elaborate can be created. With that in mind, I personally don't see the need to push this within the context of what else is being worked on.

this rework is a free value improve if we won't have new slider aim algo in next deploy

@smoogipoo
Copy link
Contributor

!diffcalc

Copy link

github-actions bot commented Mar 7, 2024

@smoogipoo
Copy link
Contributor

!diffcalc

Copy link

github-actions bot commented Mar 13, 2024

now it's applied correctly
Most notable is on this map (was 8.2* in previous version)
https://osu.ppy.sh/beatmapsets/2005921#osu/4172054
@pull-request-size pull-request-size bot added size/L and removed size/M labels Mar 18, 2024
@smoogipoo
Copy link
Contributor

!diffcalc

Good time to test out some updates I've made to the sheet generator :)

Copy link

github-actions bot commented Apr 9, 2024

@smoogipoo
Copy link
Contributor

!diffcalc
SCORE_PROCESSOR_A=ppy/osu-queue-score-statistics#263
SCORE_PROCESSOR_B=ppy/osu-queue-score-statistics#263

Copy link

github-actions bot commented Apr 26, 2024

// Take just slider heads into account because we're computing sliderjumps, not slideraim
sliderJumpBonus = slider_jump_multiplier * aimStrain * sliderJumpsAdjustingMultiplier;

// Reward more if sliders and circles are alternating (actually it's still lower than several sliders in a row)
Copy link
Member

Choose a reason for hiding this comment

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

What is the rationale behind this? If we're alternating between sliders and circles then you're saying it's more difficult to aim than being slider jumps only.. but why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in general - the meaning of buffing slider jumps is accounting for the fact that slider jumps are more difficult to process/read in comparison to normal jumps
but the cases where there's some notes in the middle of slider jump section or vice versa - it's not becoming any easier to read than just all sliders

Copy link
Contributor Author

@Givikap120 Givikap120 May 17, 2024

Choose a reason for hiding this comment

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

you can create jumps test map with three cases:

  • only circles
  • alternating
  • only sliders

in my experience last two have similar difficulty, but maybe i'm just skill-issued

Copy link
Contributor Author

@Givikap120 Givikap120 May 17, 2024

Choose a reason for hiding this comment

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

I just tested the jumps here:
With alternating bonus:

  • normal - 3.38*
  • alternating - 3.48*
  • sliders - 3.58*

Without:

  • normal - 3.38*
  • alternating - 3.45*
  • sliders - 3.58*

Copy link
Contributor Author

@Givikap120 Givikap120 May 17, 2024

Choose a reason for hiding this comment

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

maybe i actually should revert alternating multiplier to 1.0x, cuz even 3.48 still feels too low

Copy link
Contributor Author

Choose a reason for hiding this comment

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

even with 2.0x it's still only 3.55*

Comment on lines 148 to 152
// Punish too short sliders to prevent cheesing (cheesing is still possible, but it's very rare)
double sliderLength = slider.Velocity * slider.SpanDuration;
if (sliderLength < slider.Radius)
sliderJumpBonus *= sliderLength / slider.Radius;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

rename this variable to sliderDuration as length can ambiguously be understood to mean distance here.

i would be expecting something to do with follow circle radius here (2.4x). did you just not find that to be necessary? your comment does state that cheesing still occurs with this code, so something to consider

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sliderjumps is not connected with sliderbody difficulty in any way, so there's no sense in using follow circle radius
cheesing can occur cuz you can hide long sliderbody under the circle, so the slider will appear like a circle
but i haven't seen it in ranked maps so it's good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also, it's indeed length, not a duration
do you misunderstanding something?
velocity * time = distance
velocity * duration = length

Copy link
Contributor

@apollo-dw apollo-dw May 19, 2024

Choose a reason for hiding this comment

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

sliderjumps is not connected with sliderbody difficulty in any way, so there's no sense in using follow circle radius

but slider leniency will allow short enough sliders to effectively allow the movement as if it were circles. mappers have used this technique for over a decade at this point

also, it's indeed length, not a duration

yeah I think I just get tripped up by people using the term "length of time" in sentences. duration is just less ambiguous IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but slider leniency will allow short enough sliders to effectively allow the movement as if it were circles. mappers have used this technique for over a decade at this point

this is literally the point of sliderjumps
even tho technically they allow the same movement - they're still harder because they're harder to read

yeah I think I just get tripped up by people using the term "length of time" in sentences. duration is just less ambiguous IMO

how duration is less ambiguous if duration literally means time, when this is clearly length in pixels

Copy link
Contributor

Choose a reason for hiding this comment

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

... i think i confused myself on the duration and length thing again. i just read myself and i am not making any sense, disregard

Comment on lines 144 to 146
// If slider was slower than notes before - punish it
if (osuCurrObj.StrainTime > osuLastObj.StrainTime)
sliderJumpBonus *= Math.Pow(Math.Min(osuCurrObj.StrainTime, osuLastObj.StrainTime) / Math.Max(osuCurrObj.StrainTime, osuLastObj.StrainTime), 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm confused by the comment and formula here. the comment talks about notes before the slider, but the slider is osuLastObj. so wouldn't the notes before be osuLastLastObj?

the comment could be less ambiguous too since when i read "slider was slower", i am interpreting this as the slider's velocity. but the code seems to be regarding a slowing down in rhythm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this line exist to not buff the cases like "jumps jumps jumps, 1/2 slider, jumps jumps jumps"
now I'm think about it - it's still have 0.25 multiplier, maybe i should drop it to 0

double currVelocity = osuCurrObj.JumpDistance / osuCurrObj.StrainTime;

// This multiplier exists to prevent slideraim having sliderjumps bonus
double sliderJumpsAdjustingMultiplier = 1.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to sliderJumpNerfFactor or similar.

also why not have slider body bonus and slider jump bonus coexist? are there any particular cases you can point to where this doesn't make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because lost umbrella +100

sliderBonus = osuLastObj.TravelDistance / osuLastObj.TravelTime;
}

aimStrain += sliderBonus * slider_aim_multiplier;
Copy link
Contributor

Choose a reason for hiding this comment

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

if the slider jump bonus and slider aim bonus are not meant to be applied together, then i would suggest throwing them into a max function. this would communicate their purpose much better, and it might render sliderJumpsAdjustingMultiplier redundant.

so i'd be expecting something like:
aimStrain += Math.max(sliderBonus * slider_aim_multiplier, sliderJumpBonus);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they meant to be applied together
sliderjumps account for the fact that aiming sliderhead is harder than normal circle
sliderbody is aiming the body after you hit the circle
sliderJumpsAdjustingMultiplier exist because i don't want sliderbody difficulty to be multiplied by sliderjumps bonus

Copy link
Contributor

Choose a reason for hiding this comment

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

so they are meant to be applied together, but also there is a nerf factor that "exists to prevent slideraim having sliderjumps bonus" and you "don't want sliderbody difficulty to be multiplied by sliderjumps bonus". this seems to be a pretty confusing contradiction on a conceptual basis, and i wouldn't expect anyone coming in with a fresh pair of eyes to be able to comprehend what's going on based on the code alone...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in my opinion it's pretty simple to understand that sliderjump bonus is for aiming to the circle, while slideraim is for aiming the body
aiming the body doesn't gets any harder if previous object was a slider, so i can't just use raw aim strain that includes body difficulty

Copy link
Contributor

Choose a reason for hiding this comment

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

i fully understand what slider jump bonus and slider aim bonus is, that doesn't really address my point of there seemingly being contradictions in approach here. if there are no contradictions here and i am mistaken, then the changes here need to be made much clearer

Comment on lines 133 to 142
// Reward more if sliders and circles are alternating (actually it's still lower than several sliders in a row)
if (osuLastLastObj.BaseObject is HitCircle)
{
double alternatingBonus = slider_jump_multiplier * osuLastObj.JumpDistance / osuLastObj.StrainTime;

if (osuLastObj.StrainTime > osuLastLastObj.StrainTime)
alternatingBonus *= Math.Pow(Math.Min(osuCurrObj.StrainTime, osuLastObj.StrainTime) / Math.Max(osuCurrObj.StrainTime, osuLastObj.StrainTime), 2);

sliderJumpBonus += alternatingBonus;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think i'm convinced with this being a factor in difficulty -- could you explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

backread a bit

Copy link
Contributor

Choose a reason for hiding this comment

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

i can get behind the notion of "slider jumps are more difficult to process" just fine, but i don't agree with alternating between circles and sliders. in my experience, that offers a respite from the increased processing strain more than anything.

but the cases where there's some notes in the middle of slider jump section or vice versa - it's not becoming any easier to read than just all sliders

the strain algorithm works for this purpose. this shouldn't need any additional bonuses

Copy link
Contributor Author

Choose a reason for hiding this comment

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

try to do what i did:
compare the difficulty of alternating and sliders-only in test diff
alternating case will be considerably underweight compared to sliders-only

/// <summary>
/// Normalised distance from the start position of the previous <see cref="OsuDifficultyHitObject"/> to the start position of this <see cref="OsuDifficultyHitObject"/>.
/// </summary>
public double JumpDistance { get; private set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

this naming is very confusing, because in theory this is even more "lazy" than LazyJumpDistance since it's ignoring sliderbodies on sliders

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rename to HeadJumpDistance? CircleJumpDistance?

Copy link
Contributor

Choose a reason for hiding this comment

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

i don't like either of those. maybe swapping JumpDistance and LazyJumpDistance makes more sense. might need more opinions

Copy link
Contributor Author

@Givikap120 Givikap120 May 19, 2024

Choose a reason for hiding this comment

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

i don't like it cuz it will be confusing for people who already know pp variables
maybe changing LazyJumpDistance to TailJumpDistance?

Copy link
Contributor

Choose a reason for hiding this comment

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

that's a fair point. TailJumpDistance doesn't quite get there IMO, because it should be general to every object type. something that covers the notion of it being lazy and from the end position would be good. LazyJumpFromEndDistance would be ideal for me, but i don't mind long variable names that are descriptive

@Givikap120
Copy link
Contributor Author

Making this Draft because I split-up all changes

@Givikap120 Givikap120 marked this pull request as draft September 25, 2024 13:37
@stanriders
Copy link
Member

This PR has been split into smaller ones

@stanriders stanriders closed this Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants