DysonSphereMod / QOL

64 stars 36 forks source link

Bugfix if number's decimal separator is . instead of , #101

Closed ThomasBlt closed 3 years ago

ThomasBlt commented 3 years ago

Bugfix attempt for #100

Side note : 1)This is my first attempt at C# development and 2) I haven't found how to compile it (nor test it) ==> @brokenmass could you do it for me ?

Last note : does anyone have a good tuto on how to import and compile such bepinEx mods in a brand new Visual Studio ?

starfi5h commented 3 years ago

BepInEx document has some basic tutorials. You can also join DSP Modding discord server to find some resources.

ThomasBlt commented 3 years ago

Compiled and tested. Works like a charm.

brokenmass commented 3 years ago

thanks for the pr, will merge and release later

mveselov-de commented 3 years ago

Hi, sorry for chasing, but you could you please merge and release this ?

starfi5h commented 3 years ago

This is not a proper way to fix the bug. For example, it will change string "10.6" to "10,6", and float.Parse("10,6") result is 106, making number 10 times bigger.

Dimava commented 3 years ago

The correct way would be either

ThomasBlt commented 3 years ago

Nope, now i see some discrepancies in my stats ... still need to work on it

ThomasBlt commented 3 years ago

@brokenmass yours to choose : I've done it in a more stable way in 6010b19 ... but it's way more complex than f829833 for little to no more stability. Everything is due to the fact that Youthcat Studio is using a ',' as a decimal separator for values <1000 and a '.' for values above (with a letter suffix)

starfi5h commented 3 years ago

',' as a decimal separator is because of your CultureInfo.CurrentCulture setting. What Youthcat Studio do is that they have StringBuilderUtility.WriteKMG() that use '.' as decimal separator when value is over 10000 regardless of culture setting, making format inconsistent for cultures that use other decimal separators. You can test that by adding Thread.CurrentThread.CurrentCulture = new CultureInfo("en"); in Awake()
So a proper way to do that will be either:

ThomasBlt commented 3 years ago

@starfi5h I'm new to C# so there is no "easy" solution from my point of view. Beside, sticking my fingers in the heart of YouthCat's developement will never do any good as they might change something and thus we break everything.

My first solution was not CurrentCulture-aware but both solutions are just a workaround that will (and are known to) break at YouthCat's first change. In any case it's not a real problem as it will only break the statistics page whereas your proposed change could mess up with the inner-workings of the game. If my solution breaks something this only means that we would need to do another release which I already anticipated with the following line : https://github.com/DysonSphereMod/QOL/blob/f829833d75ec0791751462d843daa20b1b03982a/BetterStats/BetterStats.cs#L168

mveselov-de commented 3 years ago

Hi. Unfortunately, this issue still exists in 1.2.1. At least for me.