Fix JuiceStream velocity calculation

[smoogipoo]

• Closes Osu!Catch sliders overexaggerate the amount of drops in it, while editing in the editor #28534 (for the majority case, see Fix JuiceStream velocity calculation #25725 (comment))

This should match the calculations already present / recently made to Slider. Not sure why they weren't done for catch.

Spreadsheet here: https://docs.google.com/spreadsheets/d/1NA5JK947QmdUMv7Lnv56WXxCAuq-IOYsGWZ83GXAWB0/edit#gid=791502473 ([run]https://github.com/ppy/osu/actions/runs/7163776038)

Todo in this PR:

• Add at least one test case beatmap
• Validate results of spreadsheet above

[bdach]

So I generated expected conversions for all of the changes that changed SR with nomod: conversions.zip

And I have bad news. Some of the SR changes are fixes, and some are regressions. The detailed breakdown is thus:

Test	master	this PR	change
Test("3091364")	✔️	❌	regression
Test("556909")	❌	✔️	fixed
Test("828828")	❌	❌	still broken
Test("933615")	❌	✔️	fixed
Test("991733")	❌	❌	still broken
Test("1009022")	✔️	❌	regression
Test("1392244")	❌	❌	still broken
Test("1514076")	❌	❌	still broken
Test("1653504")	✔️	❌	regression
Test("1654305")	❌	✔️	fixed
Test("1875197")	✔️	❌	regression
Test("1962756")	❌	✔️	fixed
Test("2041893")	❌	❌	still broken
Test("2042351")	❌	✔️	fixed
Test("2077578")	❌	❌	still broken
Test("2165315")	❌	✔️	fixed
Test("2186317")	✔️	❌	regression
Test("2195985")	✔️	❌	regression
Test("2256396")	❌	✔️	fixed
Test("2339801")	✔️	❌	regression
Test("2389697")	❌	✔️	fixed
Test("2638311")	❌	✔️	fixed
Test("2789383")	❌	✔️	fixed
Test("3335242")	❌	❌	still broken
Test("3614534")	❌	❌	still broken
Test("3620288")	❌	❌	still broken
Test("3642792")	❌	✔️	fixed
Test("3652994")	❌	❌	still broken
Test("3922638")	❌	❌	still broken
Test("3942600")	❌	✔️	fixed
Test("3944076")	❌	❌	still broken
Test("4044925")	❌	❌	still broken
Test("4087902")	✔️	❌	regression

Note that "still broken" may also mean "broken even further". I'm pretty sure I saw at least one case of this already.

Detailed test result reports:
before.txt
after.txt

I will attempt to investigate later, but I need to break for a bit and just wanted to drop this here in the mean time...

[smoogipoo]

That's very helpful, thanks! If you get to it before me, it'd be good to test those with #25726 merged on top of these changes, since they touch related code.

[bdach]

More details:

Test	master	#25725	#25726	#25725 + #25726	change
Test("3091364")	✔️	❌	✔️	❌	broken by #25725
Test("556909")	❌	✔️	❌	✔️	fixed by #25725
Test("828828")	❌	❌	✔️	❌	fixed by #25726 and then broken by #25725
Test("933615")	❌	✔️	❌	✔️	fixed by #25725
Test("991733")	❌	❌	❌	❌	still broken
Test("1009022")	✔️	❌	✔️	❌	broken by #25725
Test("1392244")	❌	❌	❌	✔️	fixed by combination
Test("1514076")	❌	❌	❌	❌	still broken
Test("1653504")	✔️	❌	✔️	❌	broken by #25725
Test("1654305")	❌	✔️	❌	✔️	fixed by #25725
Test("1875197")	✔️	❌	✔️	❌	broken by #25725
Test("1962756")	❌	✔️	❌	✔️	fixed by #25725
Test("2041893")	❌	❌	❌	❌	still broken
Test("2042351")	❌	✔️	❌	✔️	fixed by #25725
Test("2077578")	❌	❌	❌	❌	still broken
Test("2165315")	❌	✔️	❌	✔️	fixed by #25725
Test("2186317")	✔️	❌	✔️	❌	broken by #25725
Test("2195985")	✔️	❌	✔️	❌	broken by #25725
Test("2256396")	❌	✔️	❌	✔️	fixed by #25725
Test("2339801")	✔️	❌	✔️	❌	broken by #25725
Test("2389697")	❌	✔️	❌	✔️	fixed by #25725
Test("2638311")	❌	✔️	❌	✔️	fixed by #25725
Test("2789383")	❌	✔️	❌	✔️	fixed by #25725
Test("3335242")	❌	❌	✔️	❌	fixed by #25726 and then broken by #25725
Test("3614534")	❌	❌	❌	❌	still broken
Test("3620288")	❌	❌	❌	❌	still broken
Test("3642792")	❌	✔️	❌	✔️	fixed by #25725
Test("3652994")	❌	❌	❌	❌	still broken
Test("3922638")	❌	❌	❌	✔️	fixed by both pulls
Test("3942600")	❌	✔️	❌	✔️	fixed by #25725
Test("3944076")	❌	❌	❌	❌	still broken
Test("4044925")	❌	❌	❌	✔️	fixed by both pulls
Test("4087902")	✔️	❌	✔️	❌	broken by #25725

Detailed test reports:
25726.txt
25725+25726.txt

The 828828 and 3335242 cases are especially maddening, only breaking down when both pulls are involved.

Either way, these results seem to indicate that the other pull seems to be a much less volatile improvement, as in it appears to both not introduce any star rating changes and lead to a net increase in test case passes, so I'll get that one in first, and see if I can get to any conclusions on this one later...

[bdach]

Looking into the failing cases more, every single one manifests by a last fruit in a juice stream moving temporally.

• In most cases, the last fruit is later by 1ms, and thus causes a hyperdash that was not there previously.
• In one case (3091364), the last fruit is earlier by ~0.000002ms, and becomes not-hyper.

You would ask, "why didn't this manifest in osu! if this PR brings the two in line?", and, well...

osu/osu.Game.Rulesets.Osu.Tests/OsuBeatmapConversionTest.cs

Lines 87 to 91 in 568bd85

	public bool Equals(ConvertValue other)
	=> Precision.AlmostEquals(StartTime, other.StartTime, conversion_lenience)
	&& Precision.AlmostEquals(EndTime, other.EndTime, conversion_lenience)
	&& Precision.AlmostEquals(X, other.X, conversion_lenience)
	&& Precision.AlmostEquals(Y, other.Y, conversion_lenience);

The test doesn't care about the object times being precisely equal. It just needs to be within 2ms. In fact, when the failing maps are plugged into the osu! conversion tests (as they are all converts), they would also fail conversion if the leniency on end time was turned off.

I'm not sure what to do with this information further yet, but there you go...

[smoogipoo]

Yeah... Unfortunately I don't see a way to get perfect conversion unless we use the exact calculations that osu!stable does (i.e. not using doubles).

Personally, just from a code PoV, both the new changes look like they should match osu!stable just because the osu! implementation is geared to do so (even if it doesn't exactly due to double timing).

As for how to make further progress, I'm thinking we should only consider the position of hitobjects and not consider what are effectively "pixeljumps". I wouldn't be opposed to reverting 6f73d78 either.

Any opinions? Also @peppy

[bdach]

From the standpoint of considering catch gameplay mechanics or balance or whatever, I don't feel qualified to answer that question.

From a code quality / programming standpoint, to some degree I'd be in favour of merging this simply because it brings osu! and catch in line. If it just broke maps, I would have more reservations about it, but the fact that it breaks some and fixes some has me thinking we're probably never getting 100% accuracy of reproduction of stable calculations anyway.

[peppy]

I guess one way or another we should make a call on this. I'd be curious to see what a "float calculation" solution would look like (as in how bad it will look).

[bdach]

I'm actually going to make a shot-call on this now to get it in, and that is because this fixes a majority case of an editor issue, namely #28534.

That bug is caused by TickDistanceMultiplier not being updated correctly. On master, it is expected that that value in catch specifically will be 1 pre-osu-file-format-v8 and equal to slider velocity post-osu-file-format-v8. This value not being updated correctly by the editor was causing ticks to generate using the wrong density.

This branch inverts the value, such that TickDistanceMultiplier is 1 over slider velocity pre-osu-file-format-v8 and 1 post-osu-file-format-v8. This means that this issue essentially disappears for post-v8 beatmaps, since they are loaded with the multiplier equal to 1 and will continue to use that multiplier undisturbed.

Note that there is still a 1% issue - if an either osu! or catch beatmap that is v8 or earlier is edited and saved, tick spacing will inadvertently change upon save, because of it being derived entirely based on osu! beatmap version, which is hardcoded in the encoder to be v128. That said this is a 1% issue and I'm not sure we need to act upon it until necessary.

See deeb2e9 for testament to all of the above (test will fail when cherry-picked onto master, and passes here - hopefully on CI, too).