Certain float settings are saved weirdly in the editor

[64ArthurAraujo]

Type

Game behaviour

Bug description

I saw a comment about this in the recent post of the new release, apparently when adjusting some sliders to certain float numbers for example 2.6 it instead sets it to 2.600001

This causes a false positive problem with the abnormal difficulty check

exported .osu (it seems the slider multiplier is affected as well but im not sure if it matters)

Screenshots or videos

n/a

Version

2024.412.1

Logs

n/a

[peppy]

This causes a false positive problem with the abnormal difficulty check

I wouldn't classify this as false positive.

[peppy]

This is floating point imprecision (in this case, both double and float). The SetValue implementation sees an input from the slider bar of something like 5.7350206, with a precision setting of 0.1. The precision adjusted doubleValue is 5.700000084936619, which is converted back to a float resulting in 5.7000003.

Short of using decimal type, we have to be happy with this level of precision, so the fix I'd see would be on writing to the file.

I also cannot see this being a recent regression as the issue implies. I've tested back months and the issue still appears (at least back to 2024.219.0). Can't easily test before .net8 switch, but if anything that could have played a role I guess.

[peppy]

And I guess the annoying-as-fuck part is I have zero faith that rounding on .osu write is correct because there's probably user beatmaps which have taken the format into their own hands and used decimal in one or more of these fields. So if I round on data export, it's likely going to clobber some beatmaps. Do we just clobber on export specifically (at least it won't break existing ranked beatmaps as parsing logic will not change).

@ppy/team-client

[bdach]

The unconventional but probably mighty risky workaround would be to apply generic math framework-side. I'm not sure what exact encantation you'd use here but the hope is that you could get rid of the ToDouble() conversion and do all of that rounding within the specific T, making the issue go away entirely because you wouldn't have to do the same double-to-float demotion that causes the fp error to manifest in the first place.

I seem to vaguely recall @huoyaoyuan mentioning wanting to make it happen?

[huoyaoyuan]

I seem to vaguely recall @huoyaoyuan mentioning wanting to make it happen?

Yeah I already have a branch that enables generic math for Bindable numbers. Just so busy these days.

[bdach]

Would you be able to at least draft PR that so we can look at it and work it over without involving you in the review back-and-forth maybe?

[huoyaoyuan]

Opened https://github.com/ppy/osu-framework/pull/6248. I wrote it years ago, just rebased. I didn't write game side change but it should be straight, just changing generic constraints.

[bdach]

@peppy So I actually would say that this is a framework bug in BindableNumber and would be in favour of using decimal for the rounding as you stated. Is there a reason you don't want to go there (other than performance, presumably)?

Just something like

diff --git a/osu.Framework/Bindables/BindableNumber.cs b/osu.Framework/Bindables/BindableNumber.cs
index 46491e5eb..6e9452b8c 100644
--- a/osu.Framework/Bindables/BindableNumber.cs
+++ b/osu.Framework/Bindables/BindableNumber.cs
@@ -7,6 +7,7 @@
 using System.Diagnostics;
 using System.Globalization;
 using JetBrains.Annotations;
+using osu.Framework.Extensions.TypeExtensions;
 using osu.Framework.Utils;

 namespace osu.Framework.Bindables
@@ -77,16 +78,16 @@ private void setValue(T value)
         {
             if (Precision.CompareTo(DefaultPrecision) > 0)
             {
-                double doubleValue = ClampValue(value, MinValue, MaxValue).ToDouble(NumberFormatInfo.InvariantInfo);
-                doubleValue = Math.Round(doubleValue / Precision.ToDouble(NumberFormatInfo.InvariantInfo)) * Precision.ToDouble(NumberFormatInfo.InvariantInfo);
+                decimal accurateResult = ClampValue(value, MinValue, MaxValue).ToDecimal(NumberFormatInfo.InvariantInfo);
+                accurateResult = Math.Round(accurateResult / Precision.ToDecimal(NumberFormatInfo.InvariantInfo)) * Precision.ToDecimal(NumberFormatInfo.InvariantInfo);

-                base.Value = convertFromDouble(doubleValue);
+                base.Value = convertFromDecimal(accurateResult);
             }
             else
                 base.Value = value;
         }

-        private T convertFromDouble(double value)
+        private T convertFromDecimal(decimal value)
         {
             if (typeof(T) == typeof(sbyte))
                 return (T)(object)Convert.ToSByte(value);
@@ -106,8 +107,10 @@ private T convertFromDouble(double value)
                 return (T)(object)Convert.ToUInt64(value);
             if (typeof(T) == typeof(float))
                 return (T)(object)Convert.ToSingle(value);
+            if (typeof(T) == typeof(double))
+                return (T)(object)Convert.ToDouble(value);

-            return (T)(object)value;
+            throw new InvalidCastException($"Cannot convert from decimal to {typeof(T).ReadableName()}");
         }

         protected override T DefaultMinValue

appears to fix it, and will probably fix 20 other instances of this everywhere else we use decimal precision. I just don't see rounding on encode as a valid solution to what is ostensibly BindableNumber's problem.

I am guessing the reason why the above fixes is that the actual problem is not the double-to-float demotion, the problem is that Precision is inaccurately stored for floating point types, and thus it turns out being off by just enough to matter when doing the multiplication in the rounding op above, as in this goes something like

5.2f
> is really 5.199999809265136718750000000000

0.1f
> is really 0.100000001490116119384765625000

Math.Round(5.2f / 0.1f)
> is really 52.000000000000000000000000000000

52 * 0.1f
> is really 5.200000286102294921875000000000

52 * 0.1f - 5.2f
> is really 0.000000476837158203125000000000

[huoyaoyuan]

Should we consider to use decimal values instead? System.Decimal is not perfectly performant, but it should not be a big problem.

[peppy]

Is there a reason you don't want to go there (other than performance, presumably)?

Using it as per your patch seems fine. I just didn't want to have to Bindable<decimal> everywhere.

[shiumano]

When was it completed?

[bdach]

@shiumano you'll have to explain how you captured this screenshot.

if this is a beatmap that you have locally saved via lazer before the fix was released (2024.418.0) then it won't have been retroactively fixed, you'll have to change CS again and save again.

[shiumano]

This is occurring on maps created with the latest version.

https://github.com/ppy/osu/assets/75152752/07591a82-3e0e-444a-9d1b-c3ed8de5da21

logs.zip

[bdach]

Actually now that I think of it again the fix I did probably did not make it into the latest release because of all of the SDL reverting that took place (that also inadvertently hit the fix too because resolving conflicts was too annoying). So I'm not gonna investigate further until next release.