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

Timing distribution graph shows slider head hits in an unintuitive way on classic scores #24274

Closed
bdach opened this issue Jul 17, 2023 · 2 comments · Fixed by #27994
Closed
Assignees
Labels
area:results priority:1 Very important. Feels bad without fix. Affects the majority of users. ruleset/osu! type:behavioural

Comments

@bdach
Copy link
Collaborator

bdach commented Jul 17, 2023

Type

Game behaviour

Bug description

Originally reported on discord. Opening as issue because I'm not sure what to do with this one.

The slider head circle is judged either as a full circle (with classic mod off), or as a large slider tick (with classic mod on):

protected override HitResult ResultFor(double timeOffset)
{
Debug.Assert(HitObject != null);
if (HitObject.JudgeAsNormalHitCircle)
return base.ResultFor(timeOffset);
// If not judged as a normal hitcircle, judge as a slider tick instead. This is the classic osu!stable scoring.
var result = base.ResultFor(timeOffset);
return result.IsHit() ? HitResult.LargeTickHit : HitResult.LargeTickMiss;
}

Notably, when classic mod is on, the head circle is judged as a large slider tick, but contrary to normal slider ticks, it still has non-empty hit windows due to inheriting from HitCircle : OsuHitObject:

public class SliderHeadCircle : HitCircle

protected override HitWindows CreateHitWindows() => new OsuHitWindows();

The hit event distribution graph discards hit events for objects with empty hit windows:

this.hitEvents = hitEvents.Where(e => !(e.HitObject.HitWindows is HitWindows.EmptyHitWindows) && e.Result.IsHit()).ToList();

However, with classic active, slider heads are objects that are judged as ticks but have non-empty hit windows, leading them to be included on the stacked bar graph (in the topmost position on each bar). What compounds the confusion is the fact that they will always be painted on the graph using blue:

case HitResult.SmallTickHit:
case HitResult.LargeTickHit:
case HitResult.Great:
return Blue;

With classic off, this just isn't an issue because the slider heads are judged as standard circles.

Screenshots or videos

osu_2023-07-17_20-25-28

Version

current master

Logs

n/a

@peppy
Copy link
Member

peppy commented Jul 26, 2023

Is the fix here as simple as returning empty windows when classic mod is enabled (probably via a flag set somewhere on the OsuHitObject, or applied by the mod – HitWindows is publicly settable)?

@bdach
Copy link
Collaborator Author

bdach commented Jul 27, 2023

Is the fix here as simple as returning empty windows when classic mod is enabled (probably via a flag set somewhere on the OsuHitObject, or applied by the mod – HitWindows is publicly settable)?

I'm 99% sure it is not. Doing this would essentially mean that with classic mod active the slider head would need to be hit with zero offset to be considered as hit. As in precise to the millisecond.

This isn't an issue for normal slider ticks as they are generally not timed objects and judgements on them are set via their parent objects using slider tracking state.

@peppy peppy added the priority:1 Very important. Feels bad without fix. Affects the majority of users. label Jul 28, 2023
@bdach bdach self-assigned this Apr 25, 2024
bdach added a commit to bdach/osu that referenced this issue Apr 25, 2024
Closes ppy#24274.

Bit of an ad-hoc resolution but maybe fine? This basically proposes to
bypass the problem described in the issue by just not showing tick hits
at all on the distribution graph.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:results priority:1 Very important. Feels bad without fix. Affects the majority of users. ruleset/osu! type:behavioural
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants