ppy / osu

rhythm is just a *click* away!
https://osu.ppy.sh
MIT License
15.01k stars 2.21k forks source link

Importing beatmap in lazer give different object position from stable. #12109

Closed IkaWaAyuMu closed 3 years ago

IkaWaAyuMu commented 3 years ago

Describe the bug: Some (right now I saw only 1) beatmaps object position is different from stable in weird way.

Example map use in screenshot : https://osu.ppy.sh/beatmapsets/1207624#osu/2514799 Timestamp : ~1:54 Screenshots or videos showing encountered issue: On lazer Lazer

On stable Stable

osu!lazer version: 2021.320.0

Logs: This log is start from open the game, download the map, then open the editor, seek to that position, exit the editor and exit the game. database.log, network.log, performance.log, runtime.log, updater.log

bdach commented 3 years ago

Looks like an almost-straight perfect slider gone wrong somehow. The control points are shown correctly, but the slider itself is drawn wrong.

osu_2021-03-20_16-02-33

Offending HO is:

493,92,114993,2,0,P|472:181|442:308,1,180,12|0,0:0|0:0,0:0:0:0:
Wieku commented 3 years ago

I noticed this in my project as well. It's a pretty weird edge case. As I don't have local osu-framework and my code use the same CircularArc approximator as lazer, decided to find what's wrong there.

So I decided to add a couple of prints:

aSq := pt2.DstSq(pt3)
bSq := pt1.DstSq(pt3)
cSq := pt1.DstSq(pt2)

log.Println("aSq:", aSq, "   bSq:", bSq, "   cSq:", cSq)

if math32.Abs(aSq) < 0.001 || math32.Abs(bSq) < 0.001 || math32.Abs(cSq) < 0.001 {
    arc.Unstable = true
}

s := aSq * (bSq + cSq - aSq)
t := bSq * (aSq + cSq - bSq)
u := cSq * (aSq + bSq - cSq)

sum := s + t + u

log.Println("s:", s, "   t:", t, "   u:", u, "   sum:", sum)

if math32.Abs(sum) < 0.001 {
    arc.Unstable = true
}

centre := pt1.Scl(s).Add(pt2.Scl(t)).Add(pt3.Scl(u)).Scl(1 / sum) //nolint:misspell

dA := pt1.Sub(centre)
dC := pt3.Sub(centre)

r := dA.Len()

log.Println("centre:", centre, "    radius:", r)

After running the code for the offending slider I got those results:

aSq: 17029    bSq: 49257    cSq: 8362
s: 6.912071e+08    t: -1.1755676e+09    u: 4.8436048e+08    sum: -32
centre: {482304 114688}     radius: 495251.53

High values of s, t and u immediately told me that the issue is caused by float32 rounding, so I decided to switch aSq, bSq and cSq to float64 (double):

aSq := float64(pt2.DstSq(pt3))
bSq := float64(pt1.DstSq(pt3))
cSq := float64(pt1.DstSq(pt2))

log.Println("aSq:", aSq, "   bSq:", bSq, "   cSq:", cSq)

if math.Abs(aSq) < 0.001 || math.Abs(bSq) < 0.001 || math.Abs(cSq) < 0.001 {
    arc.Unstable = true
}

s := aSq * (bSq + cSq - aSq)
t := bSq * (aSq + cSq - bSq)
u := cSq * (aSq + bSq - cSq)

sum := s + t + u

log.Println("s:", s, "   t:", t, "   u:", u, "   sum:", sum)

if math.Abs(sum) < 0.001 {
    arc.Unstable = true
}

centre := pt1.Scl(float32(s)).Add(pt2.Scl(float32(t))).Add(pt3.Scl(float32(u))).Scl(float32(1 / sum)) //nolint:misspell

dA := pt1.Sub(centre)
dC := pt3.Sub(centre)

r := dA.Len()

log.Println("centre:", centre, "    radius:", r)

I got following results:

aSq: 17029    bSq: 49257    cSq: 8362
s: 6.9120711e+08    t: -1.175567562e+09    u: 4.84360488e+08    sum: 36
centre: {-428714.66 -101944.89}     radius: 441169.72

And slider is being displayed correctly now: danser_2021-03-21_01-41-09

I'm also thinking if switching from area-based centre calculation to https://en.wikipedia.org/wiki/Circumscribed_circle#Cartesian_coordinates_2 will be more stable.

bdach commented 3 years ago

Thanks for looking into it. That said, I'd be loath to increase precision and potentially risk a performance hit just for this.

What gets my attention in the approximator code is this area-based cutoff. I think that Precision.Equals check there doesn't have much chance of working in the cases where the points are pretty far apart. I think either normalising it somehow - for instance by dividing the area computed there by the bounding box of the control points - may be the way to go there?

Another thing I'm wondering there, is that there's a collinearity check present:

https://github.com/ppy/osu/blob/f14a8c21d1fa50209d41b7696000340a15f0e990/osu.Game/Rulesets/Objects/Legacy/ConvertHitObjectParser.cs#L313-L323

A similar one is already in stable. I'd be curious to see whether the stable one is catching this. If so, this may explain the issue (I've already checked that the lazer check does not fire in this case).

Wieku commented 3 years ago

I think it's more likely that stable uses the formula I linked previously (which is less prone to go haywire) instead of 16s^2 one that lazer uses (which is present in the same wiki page).

bdach commented 3 years ago

No, both stable and lazer use the Cartesian formulae. They're not 1:1 code-wise though, which might well affect numerical stability. I haven't done a rigorous analysis yet, just took a quick peek.

bdach commented 3 years ago

...Never mind, on checking again stable does seem use this. There was a misleading link near that source. But it looks that framework uses the same formula, too. So I'm not sure which one @Wieku is claiming to be safer here, and I think it'll just be all down to floating point issues.

Edit: Confirmed privately that it is pretty much just all down to FP precision.

Wieku commented 3 years ago

Lazer is using that formula: 84cf64f366d880f475e55e1da32192130a9f2827

As we see we're operating on a^2 (b^2 +- ...) which in some cases can give absurdly large numbers. The formula that stable uses seems safer because we operate in a^2b ranges.