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

Fix 1 million score being unachievable on some mania beatmaps #23917

Merged
merged 7 commits into from
Jul 13, 2023

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Jun 14, 2023

RFC. Closes #23911.

I'll level with you, I kinda hate this diff, but I'm not sure how to do better given the constraints in place. The constraints in question being:

  • We want to be using Score V2.
  • The value of Score V2 for a play depends heavily on ordering of judging objects, due to the combo component of each object's score being tied to current combo after judgement of said object.
  • Therefore, autoplay simulation must match actual gameplay 1:1 wrt judgement ordering for 1M score to be achievable by the user given perfect gameplay.
  • Using gameplay code for autoplay simulation is infeasible for reasons of (lack of) speed.
  • Judgements are scored individually by each drawable hitobject.
  • Therefore, the specifics of judgement ordering depend heavily on the implementation of a ruleset.
  • Changing the above (so that order of judging during gameplay can be somehow centralised to one place that decides order of judgement) requires rewriting every ruleset ever.
  • In mania specifically, during a single update frame, notes are judged before ticks. This is due to the implementation detail of note hits/releases being triggered by IKeyBindingHandler<ManiaAction>.On{Pressed,Released}() (1, 2), which is being handled by a parenting KeyBindingContainer + InputManager, which propagates input events to the notes earlier than they can actually be updated themselves (due to being their parent). Ticks are judged via the normal UpdateResult() flow, in DrawableHitObject.UpdateAfterChildren() (3).
  • The above means that attempting to apply any sort of non-ruleset-specific fix in JudgementProcessor (which I thought I may be able to do but which ended up failing) is fooling ourselves, as there is no guarantee that every ruleset ever will follow this particular judgement ordering. Which is why I opted to expose a method that mania uses exclusively to jiggle autoplay simulation order into the order that it needs things to be in to match actual gameplay.

None of this is even touching the issue of inter-column judgement order dependence, as for the matters of autoplay it has no bearing on the final result (to figure out why, consider that all judgements that happen on the same time instant and receive the same score are basically interchangeable, so they can be reordered in any way among each other without influencing the end result). But that is absolutely a concern for actual gameplay where judgements may not be perfect.

Nobody overrides this, and with the structure given, overriders would
have to rewrite half of this code anyway. The fact that the class has 2
other overridable members (`CreateResult()`, `GetSimulatedHitResult()`)
which cease to have any meaning if `SimulateAutoplay()` is overridden
also contributes to taking this decision.
@bdach bdach requested a review from a team June 14, 2023 19:57
@bdach bdach changed the title Fix max score being unachievable on some mania beatmaps Fix 1 million score being unachievable on some mania beatmaps Jun 14, 2023
Comment on lines +174 to +175
protected virtual IEnumerable<HitObject> EnumerateHitObjects(IBeatmap beatmap)
=> enumerateRecursively(beatmap.HitObjects);
Copy link
Collaborator Author

@bdach bdach Jun 15, 2023

Choose a reason for hiding this comment

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

Someone reported on discord that a graved osu! map also has an issue where 1M score is unachievable, and it's because this implementation is still naive (relies on no concurrent objects to work correctly). I could go and fix that case here by partially mirroring the mania implementation (flatten all objects, sort by end time) but honestly not sure it's worth the performance hit for a graved map.

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 be against this since it's to guarantee correctness. The performance hit shouldn't be noticeable. But the current changeset is also fine for now.

@peppy peppy requested a review from smoogipoo June 16, 2023 05:16
@peppy
Copy link
Member

peppy commented Jun 16, 2023

I think this is fine for now. We're probably going to come across more cases that need addressing (aka judgements which change ordering due to how the user plays the beatmap – one of the things we solved with the old standardised scoring but alas) but small steps forwards makes sense.

@peppy peppy added the next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! label Jun 20, 2023
@peppy peppy merged commit 9cba24e into ppy:master Jul 13, 2023
@bdach bdach deleted the fix-imperfect-simulation branch July 13, 2023 17:14
peppy added a commit to peppy/osu that referenced this pull request Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:scoring next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! ruleset/osu!mania size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Perfect play didn't get max score in specific beatmap
3 participants