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

Allow a JudgementProcessor to provide a rate-scaled time offset of a JudgementResult #30842

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tsunyoku
Copy link
Member

Closes #30828.

I'm not convinced this is the absolute cleanest solution, but I've made it in such a way that it doesn't require too much messing around with hit/judgement processing.

Result (verifies the unstable rate is correctly scaled when compared to the issue)

image

…a `JudgementResult`

Closes ppy#30828.

I'm not convinced this is the absolute cleanest solution, but I've made it in such a way that it doesn't require too much messing around with hit/judgement processing.
/// <param name="hitObject">The <see cref="HitObject"/> that triggered the event.</param>
/// <param name="lastHitObject">The previous <see cref="HitObject"/>.</param>
/// <param name="position">A position corresponding to the event.</param>
public HitEvent(double timeOffset, double? gameplayRate, HitResult result, HitObject hitObject, [CanBeNull] HitObject lastHitObject, [CanBeNull] Vector2? position)
public HitEvent(double timeOffset, double scaledTimeOffset, HitResult result, HitObject hitObject, [CanBeNull] HitObject lastHitObject, [CanBeNull] Vector2? position)
Copy link
Member

@frenzibyte frenzibyte Nov 23, 2024

Choose a reason for hiding this comment

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

This feels really bad. Having two fields that are tied together by some factor does not sound like a good structure.

Looking at the diff at face value, is it not possible to just keep the gameplayRate variable but override ManiaScoreProcessor to always provide 1 instead of the actual gameplay rate itself? This of course comes with repurposing of the variable's name and docs so the code doesn't look weird. Something like renaming gameplayRate to timeScale or otherwise with explanatory docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Your suggestion was my first approach but it felt weird due to it being called gameplayRate. Something like timeScale sounds a lot better.

@pull-request-size pull-request-size bot added size/M and removed size/L labels Nov 24, 2024
@tsunyoku
Copy link
Member Author

Worth noting there is debate in the original issue on whether it's valid in the first place. I don't have any personal opinion on the matter, but I'll wait for any decision on that before pushing this further.

@bdach bdach added the blocked Don't merge this. label Nov 25, 2024
@bdach
Copy link
Collaborator

bdach commented Nov 25, 2024

Marking as blocked pending conclusion of discussion in issue

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.

Unstable Rate shouldn't be converted in osu!mania
3 participants