Skip to content

Adjust hitwindows to match stable #30244

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

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

Conversation

Givikap120
Copy link
Contributor

This is fixing #11311 and #28744
Unfortunately all already set replays can't be fixed

From my testing - Standard and Taiko work correctly
Mania requires additional testing

@bdach
Copy link
Collaborator

bdach commented Oct 12, 2024

Just for reference for anyone reading #26452 also exists, probably was tested way better than this is, but is also likely overcomplicated.

As touched on in #11311 (comment), I'm not making a shot call on this alone without a serious talk within @ppy/team-client on how we wanna proceed with the problem at hand. That said this PR needs to be fully tested (see op comment about mania) before I'd be willing to review it.

@sz-da
Copy link
Contributor

sz-da commented Oct 12, 2024

I think the title could be changed to better reflect the purpose of this pr.
Based on the discussion on discord, I'm assuming the main goal here is to match lazer gameplay with lazer replays, not to match stable hitwindows.

Matching stable hitwindows should be considered a different issue that doesn't need as much urgency imo.

@Givikap120
Copy link
Contributor Author

Givikap120 commented Oct 12, 2024

I think the title could be changed to better reflect the purpose of this pr. Based on the discussion on discord, I'm assuming the main goal here is to match lazer gameplay with lazer replays, not to match stable hitwindows.

Matching stable hitwindows should be considered a different issue that doesn't need as much urgency imo.

Title shows exactly what this PR does - changing hitwindows to match stable
Replays in lazer are encoded the same way stable replays are encoded
And this creates a desync between replays and gameplay
By making hitwindows as in stable - you're fixing lazer replays and stable replays being wrong at the same time

@Givikap120
Copy link
Contributor Author

Givikap120 commented Oct 12, 2024

Additional note

Right now:
ScoreInfo is correct from lazer perspective and incorrect from stable perspective.
Replay is incorrect from any perspective.

After this change:
ScoreInfo of old lazer replays (pre-change) will be plainly incorrect as they were judged on different hitwindows.
Replay will be correct from both lazer and stable perspectives.

This means that rerunned old lazer replays will output correct judgements.

@sz-da
Copy link
Contributor

sz-da commented Oct 13, 2024

By making hitwindows as in stable - you're fixing lazer replays and stable replays being wrong at the same time

Don't think it's a good idea to bring in the complexity of matching stable hitwindows for a quickfix to lazer replays. Looking at the table in #11311 (comment), stable hitwindows are very inconsistent between modes. And I don't know if it's a good idea to have lazer replicate this inconsistency. Not to mention the lazer hitwindow for mania Perfect is intentionally changed to be different from stable.

If you still want to match stable hitwindows, here are some test replays that are all edge hits where timeOffset == HitWindow
The plays were set in stable, then played back in lazer.

Beatmap:
circles.osz

Replay result (stable) result (master) result (PR) hitwindow (stable) hitwindow (master) hitwindow (PR)
osu100.osr 100 300 100 < 21 <= 21.2 <= 20.5
osu50.osr 50 100 100 < 61 <= 61.6 <= 61.5
osumiss.osr miss 50 50 < 101 <= 102 <= 101.5
taiko100.osr 100 300 300 < 20 <= 20.6 <= 20.5
taikomiss.osr miss 100 miss < 51 <= 51.2 <= 50.5
taikomissearly.osr miss miss miss <= 70 <= 71 <= 70.5
mania300.osr 300 300 300 <= 34 <= 34.6 <= 34.5
mania200.osr 200 200 200 <= 67 <= 67.6 <= 67.5

@Givikap120
Copy link
Contributor Author

Givikap120 commented Oct 13, 2024

By making hitwindows as in stable - you're fixing lazer replays and stable replays being wrong at the same time

Don't think it's a good idea to bring in the complexity of matching stable hitwindows for a quickfix to lazer replays. Looking at the table in #11311 (comment), stable hitwindows are very inconsistent between modes. And I don't know if it's a good idea to have lazer replicate this inconsistency. Not to mention the lazer hitwindow for mania Perfect is intentionally changed to be different from stable.

If you still want to match stable hitwindows, here are some test replays that are all edge hits where timeOffset == HitWindow The plays were set in stable, then played back in lazer.

Beatmap: circles.osz

Replay result (stable) result (master) result (PR) hitwindow (stable) hitwindow (master) hitwindow (PR)
osu100.osr 100 300 100 < 21 <= 21.2 <= 20.5
osu50.osr 50 100 100 < 61 <= 61.6 <= 61.5
osumiss.osr miss 50 50 < 101 <= 102 <= 101.5
taiko100.osr 100 300 300 < 20 <= 20.6 <= 20.5
taikomiss.osr miss 100 miss < 51 <= 51.2 <= 50.5
taikomissearly.osr miss miss miss <= 70 <= 71 <= 70.5
mania300.osr 300 300 300 <= 34 <= 34.6 <= 34.5
mania200.osr 200 200 200 <= 67 <= 67.6 <= 67.5

Actually any hitwindows that make timings end in .5 will fix the main problem - desync between gameplay and replay.
It's just would be convenient to also fix stable replays being wrong.
I will look into a formula more precisely.

Mania obviously can't be fixed in this way.

@Givikap120
Copy link
Contributor Author

Givikap120 commented Oct 13, 2024

If you still want to match stable hitwindows, here are some test replays that are all edge hits where timeOffset == HitWindow The plays were set in stable, then played back in lazer.

I fixed it and now your tests pass.

@sz-da
Copy link
Contributor

sz-da commented Oct 13, 2024

Rechecked the hitwindows just to make sure and they seem good to me.
I did get the hitwindows slightly wrong for master though, since I didn't account for the float to double conversion.

Replay hitwindow (stable) hitwindow (master) hitwindow (PR)
osu100.osr < 21 <= 21.1999988555908 <= 20.5
osu50.osr < 61 <= 61.5999984741211 <= 60.5
osumiss.osr < 101 <= 101.999998092651 <= 100.5
taiko100.osr < 20 <= 20.5999994277954 <= 19.5
taikomiss.osr < 51 <= 51.1999988555908 <= 50.5
taikomissearly.osr <= 70 <= 70.9999990463257 <= 70.5
mania300.osr <= 34 <= 34.5999994277954 <= 34.5
mania200.osr <= 67 <= 67.5999994277954 <= 67.5

@bdach
Copy link
Collaborator

bdach commented Oct 14, 2024

tests failing now

@pull-request-size pull-request-size bot added size/M and removed size/S labels Oct 14, 2024
@Givikap120
Copy link
Contributor Author

Givikap120 commented Oct 14, 2024

tests failing now

fixed

@Givikap120
Copy link
Contributor Author

Also, BIG warning - this changes pp outputs because pp is converting OD to hitwindows

@Givikap120
Copy link
Contributor Author

Givikap120 commented Apr 23, 2025

I would like to comment on newly added tests in #32770 and #32810.

Considering that the TestSceneReplayStability test cases are created for current hitwindows - they're failing, as this PR changes hitwindows. But the TestSceneLegacyReplayPlayback ones for osu and taiko passing as expected.

Mania legacy tests are failing because their overly-complicated behavior was not replicated in this PR. But considering that hitwindows are now rounded - they're consistent with it's own replays. I would like to fix mania to be also 1-to-1, but I would like to hear what approach I should take for problem of converts having different hitwindows without scorev2.

@bdach
Copy link
Collaborator

bdach commented Apr 23, 2025

@Givikap120 can you leave the matter of this PR to me and the rest of the team? thanks, appreciated in advance.

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.

4 participants