Skip to content
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

ConfigManager.SetDefault should not fail when the stored bindable is of a different type #6042

Closed
LumpBloom7 opened this issue Nov 7, 2023 · 7 comments

Comments

@LumpBloom7
Copy link
Contributor

SetDefault should not fail when the old datatype of a lookup doesn't match the new datatype, but instead it should fallback to restoring the lookup to a valid state. (Particularly in InitializeDefaults)

SetDefault already has a fallback case for when GetOriginalBindable() is not the same type (indicating non-existent lookup), but GetOriginalBindable will throw an exception preventing the fallback case from being executed.

Since the throw in GetOriginalBindable to prevent type mismatches when binding to configs. A try-catch in SetDefault, or a non-throwing version of GetOriginalBindable can be introduced to allow the configs to restore to a sane state in such a scenario.

@Susko3
Copy link
Member

Susko3 commented Nov 7, 2023

Particularly in InitializeDefaults

In what scenario are you encountering this issue? InitialiseDefaults() is called in the constructor, and I would expect the entries to be empty (making any conflicts impossible).

SetDefault already has a fallback case for when GetOriginalBindable() is not the same type

Not really, this code is meant to "upgrade" the returned bindable to a more useful type. Eg: Bindable<float>BindableFloat, the underlying T of both bindables is the same.
And in the general case, there is no "fallback case".

@LumpBloom7
Copy link
Contributor Author

LumpBloom7 commented Nov 7, 2023

Hmmm. I might have misread intentions then on the fallback stuff.

I encountered this issue in a Ruleset context, and I swapped out the type of a lookup from Bindable<double> to Bindable<float> while experimenting. This caused a problem loading the ruleset settings subsection which never resolves itself until I reverted the type.

I now see that RulesetConfigManager calls Load before InitialiseDefaults in the ctor (my ide somehow showed me decompiled version rather than the one from the referenced project dir). I'd expect that SetDefault/InitializeDefaults would try to restore invalid lookups to valid defaults if things aren't valid after load for some reason.

@peppy
Copy link
Sponsor Member

peppy commented Nov 8, 2023

I disagree that it should change.

@LumpBloom7
Copy link
Contributor Author

If it doesn't happen automatically, is there a process I can do on my side to "migrate" the underlying data type if I need to do so for some reason? As it is right now, either I never change the type, use a brand new lookup to avoid the problem, or require adjustment of the stored db setting.

@peppy
Copy link
Sponsor Member

peppy commented Nov 8, 2023

This sounds like an osu! side issue at this point. Potentially we want to reassess the execution order of Load and InitialiseDefaults?

@Susko3
Copy link
Member

Susko3 commented Nov 8, 2023

I encountered this issue in a Ruleset context, and I swapped out the type of a lookup from Bindable<double> to Bindable<float> while experimenting. This caused a problem loading the ruleset settings subsection which never resolves itself until I reverted the type.

I was not able to reproduce your scenario. The settings are stored to realm as strings. double and float have compatible stringified representations, so there is no issue when changing between the two.

image

The code below is how I understood your scenario that should cause issues:

diff --git a/osu.Game.Rulesets.Osu/Configuration/OsuRulesetConfigManager.cs b/osu.Game.Rulesets.Osu/Configuration/OsuRulesetConfigManager.cs
index 2056a50eda..f541b23792 100644
--- a/osu.Game.Rulesets.Osu/Configuration/OsuRulesetConfigManager.cs
+++ b/osu.Game.Rulesets.Osu/Configuration/OsuRulesetConfigManager.cs
@@ -11,6 +11,8 @@ namespace osu.Game.Rulesets.Osu.Configuration
 {
     public class OsuRulesetConfigManager : RulesetConfigManager<OsuRulesetSetting>
     {
+        private readonly bool firstRun = true; // run tests once and change to false
+
         public OsuRulesetConfigManager(SettingsStore settings, RulesetInfo ruleset, int? variant = null)
             : base(settings, ruleset, variant)
         {
@@ -24,6 +26,13 @@ protected override void InitialiseDefaults()
             SetDefault(OsuRulesetSetting.ShowCursorTrail, true);
             SetDefault(OsuRulesetSetting.ShowCursorRipples, false);
             SetDefault(OsuRulesetSetting.PlayfieldBorderStyle, PlayfieldBorderStyle.None);
+
+            if (firstRun)
+                // double
+                SetDefault(OsuRulesetSetting.SomeBoolOrDoubleValue, 5.0, 0.0, 10.0);
+            else
+                // float
+                SetDefault(OsuRulesetSetting.SomeBoolOrDoubleValue, 5.0f, 0.0f, 10.0f);
         }
     }
 
@@ -34,5 +43,6 @@ public enum OsuRulesetSetting
         ShowCursorTrail,
         ShowCursorRipples,
         PlayfieldBorderStyle,
+        SomeBoolOrDoubleValue,
     }
 }

When changing to an incompatible type, I get errors about parsing the stored string.
This happens when I change the type of the first bindable from double to bool:

2023-11-08 14:38:34 [error]: An unhandled error has occurred.
2023-11-08 14:38:34 [error]: System.FormatException: Input string was not in a correct format.
2023-11-08 14:38:34 [error]: at System.Number.ThrowOverflowOrFormatException(ParsingStatus status, TypeCode type)
2023-11-08 14:38:34 [error]: at System.String.System.IConvertible.ToSingle(IFormatProvider provider)
2023-11-08 14:38:34 [error]: at System.Convert.ChangeType(Object value, Type conversionType, IFormatProvider provider)
2023-11-08 14:38:34 [error]: at osu.Framework.Bindables.Bindable`1.Parse(Object input)
2023-11-08 14:38:34 [error]: at osu.Game.Rulesets.Configuration.RulesetConfigManager`1.AddBindable[TBindable](TLookup lookup, Bindable`1 bindable) in C:\Users\Mateo\Code\git\osu\osu.Game\Rulesets\Configuration\RulesetConfigManager.cs:line 87
2023-11-08 14:38:34 [error]: at osu.Framework.Configuration.ConfigManager`1.SetDefault(TLookup lookup, Single value, Nullable`1 min, Nullable`1 max, Nullable`1 precision)
2023-11-08 14:38:34 [error]: at osu.Game.Rulesets.Osu.Configuration.OsuRulesetConfigManager.InitialiseDefaults() in C:\Users\Mateo\Code\git\osu\osu.Game.Rulesets.Osu\Configuration\OsuRulesetConfigManager.cs:line 35
2023-11-08 14:38:34 [error]: at osu.Game.Rulesets.Configuration.RulesetConfigManager`1..ctor(SettingsStore store, RulesetInfo ruleset, Nullable`1 variant) in C:\Users\Mateo\Code\git\osu\osu.Game\Rulesets\Configuration\RulesetConfigManager.cs:line 39
2023-11-08 14:38:34 [error]: at osu.Game.Rulesets.Osu.Configuration.OsuRulesetConfigManager..ctor(SettingsStore settings, RulesetInfo ruleset, Nullable`1 variant) in C:\Users\Mateo\Code\git\osu\osu.Game.Rulesets.Osu\Configuration\OsuRulesetConfigManager.cs:line 17
2023-11-08 14:38:34 [error]: at osu.Game.Rulesets.Osu.OsuRuleset.CreateConfig(SettingsStore settings) in C:\Users\Mateo\Code\git\osu\osu.Game.Rulesets.Osu\OsuRuleset.cs:line 265
2023-11-08 14:38:34 [error]: at osu.Game.Rulesets.RulesetConfigCache.LoadComplete() in C:\Users\Mateo\Code\git\osu\osu.Game\Rulesets\RulesetConfigCache.cs:line 39
2023-11-08 14:38:34 [error]: at osu.Framework.Graphics.Drawable.loadComplete()
2023-11-08 14:38:34 [error]: at osu.Framework.Graphics.Drawable.UpdateSubTree()
2023-11-08 14:38:34 [error]: at osu.Framework.Graphics.Containers.CompositeDrawable.UpdateSubTree()
2023-11-08 14:38:34 [error]: at osu.Framework.Graphics.Containers.CompositeDrawable.UpdateSubTree()
2023-11-08 14:38:34 [error]: at osu.Framework.Graphics.Containers.CompositeDrawable.UpdateSubTree()
2023-11-08 14:38:34 [error]: at osu.Framework.Graphics.Containers.CompositeDrawable.UpdateSubTree()
2023-11-08 14:38:34 [error]: at osu.Framework.Graphics.Containers.CompositeDrawable.UpdateSubTree()
2023-11-08 14:38:34 [error]: at osu.Framework.Graphics.Containers.CompositeDrawable.UpdateSubTree()
2023-11-08 14:38:34 [error]: at osu.Framework.Platform.GameHost.UpdateFrame()
2023-11-08 14:38:34 [error]: at osu.Framework.Threading.GameThread.processFrame()

@LumpBloom7 I suggest you check which types you're using and that the ones used in InitialiseDefaults() match other usages in the code. If the problems persist, please open a new discussion at ppy/osu. Provide your code and logs from the crashes/hangs.

Migrating settings of incompatible types is a real issue and should be reported as such against ppy/osu. osu! has migration support, but I'm not sure if it's exposed for ruleset settings.

@LumpBloom7
Copy link
Contributor Author

I can't reproduce the scenario I described anymore. I must have missed a small thing somewhere when I attempted yesterday which led to the failure and my debugger pointed me at the wrong direction...

I'll close this one for now, and open a discussion for ruleset setting migration support if that is needed.

@LumpBloom7 LumpBloom7 closed this as not planned Won't fix, can't repro, duplicate, stale Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants