Fix circle scale not matching stable due to missing multiplier #25167
Conversation
This multiplier has to exist. I'm not guaranteeing that the rest is correct here. Should we be doing proper cross-testing on this? Maybe, but it's going to be hard to get right. We could likely check as far as "game pixels", but there's still a chance that the osu-framework could be doing something weird in the rest of the hierarchy where playfield scale is involved. Closes ppy#25162. Tested to fix the linked replay.
|
Test failures here are gonna suck... The diffcalc changes I guess are expected and can be locally undone, but this also appears to be impacting hyperdashing. So this is gonna need a crossreference against stable. I'll see what I can get done there... |
|
The above is from superficial review. Remaining questions in my head:
|
|
Okay so I did just a bit of further work on this and here is what follows from empirical evidence: osu! input radiusOsuRadiusTest.zip contains a beatmap ( private const float FudgeInside = 1.0004f;
private const float FudgeMiddle = 1.00041f;
private const float FudgeOutside = 1.00042f;
// `OsuHit`
new OsuReplayFrame(0, new Vector2(-256, 500)),
new OsuReplayFrame(3000, new Vector2(OsuHitObject.OBJECT_RADIUS * 0.5f * FudgeInside, 0), OsuAction.LeftButton),
new OsuReplayFrame(3001, new Vector2(OsuHitObject.OBJECT_RADIUS * 0.5f * FudgeInside, 0))
// `OsuMissed`
new OsuReplayFrame(0, new Vector2(-256, 500)),
new OsuReplayFrame(3000, new Vector2(OsuHitObject.OBJECT_RADIUS * 0.5f * FudgeOutside, 0), OsuAction.LeftButton),
new OsuReplayFrame(3001, new Vector2(OsuHitObject.OBJECT_RADIUS * 0.5f * FudgeOutside, 0))Results:
Conclusion: This PR is conclusively more correct. catch hyperdash generationCatchHyperTests.zip contains two beatmaps: Results:
Conclusion: No perceivable difference in constructed test case. In testing, shifting the coordinates of the second object by sub-unit amounts did nothing. This is likely due to the flooring that stable applies. That also means that the results of this test may be overly optimistic and mismatches may occur on certain sub-unit alignments. I probably wouldn't go too deep into it in this PR. catcher capturing widthCatchRangeTests.zip contains two replays for // NoCatch
new CatchReplayFrame(0, 0),
new CatchReplayFrame(3000, catcherWidthFudge * 0.8f - 0.001f)
// Catch
new CatchReplayFrame(0, 0),
new CatchReplayFrame(3000, catcherWidthFudge * 0.8f)These were chosen to satisfy stable - the Results:
Conclusion: lazer is wrong, but is probably made no more wrong by this PR and this should be handled separately if anything. In short, next step is probably a sheet and seeing the impact on SR this has. If it's not too egregious, we can probably proceed with this as-is. |
|
Thanks for the extended checking. I've applied the two proposed changes. |
|
!diffcalc |
|
!diffcalc |
There was a problem hiding this comment.
Will wait for the above sheets to finish, but at face value this looks fine. On a personal level I find it a little bit weird (in that I can't explain it) that the scale is divided by 2, but this PR isn't changing that.
Oh also diffcalc tests need to be fixed.
That's what the diffcalc runs are for - to ensure that we can just do that and not inadvertently cause some weird star rating/pp jumps ;) |
|
Quite a few large changes in the catch spreadsheet. Worth looking into to see if they can be justified. |
|
I dunno. Sounds like the old values were just wrong? Probably means hyperdashes were being generated where they shouldn't. I'd probably do a comparison of the beatmap and look for any differences like that. Should I be doing this or are you already looking into it @smoogipoo ? |
peppy
added
the
next release
|
I'm not looking into this at the moment. By-beatmap-comparison is the way I'd go about it as well. |
…nding allowance to catch As discussed, it isn't used in stable like this. Was a mistake.
|
While I didn't find any immediate changes in the beatmaps (I may have overlooked something), @bdach brought it to my attention that this isn't actually applied to catch in stable code. I've removed the usage of the fudge factor there as a result, and updated the osu! diffcalc tests to fix the fractional differences. |
This multiplier has to exist. I'm not sure how we missed it until now.
Closes #25162. Tested to fix the linked replay.
Note that I unfortunately found something very unfortunate this is likely going to cause us nightmares for eternity, but I don't know if we can aim to fix it. Stable applies mod multipliers at a very low level, as part of the
doublemath. Note the remaining discrepancies:I propose we ignore these.
code used for testing