0.01 is not a sensible minimum - I would have thought perhaps 0.5 is as far as we need to go.

Screenshot with 1/gui.scale=0.01:

At 1/gui.scale=0.5 it's still stupidly small but may be useful on extremely low dpi setups - text is just about readable.

Mh, I think there should be a failsafe or some type of resolution check for the maximum. If you zoom in too much, you can't see the buttons anymore so you can't change-it back from in-game, and then some people might get sucked "zoomed in" if they can't find the config file.

Maybe not a real problem, but I'm wondering.

In D332#13158, @domdomegg wrote:

I think people will realise it must be gui.scale and reset the options accordingly

Most people don't know about the config file, so it would be good to have a confirm dialog with automatically reverts back after 10/15 secs.
(Otherwise, we'll get many bug reports about that...)

Wouldn't add code for a specific config option. We just got options.js agnostic of options.json.
Adding min & max shouldn't be an issue, find suitable numbers for the greatest displays on the market.
Incidentally min/max are also required for the slider this should use.

REQUIRES GAME RESTART should be avoided where possible.
All it takes is updating g_GuiScale if the gui.scale config entry changes.

(One idea would be to remove the global altogether and read the config variable each render call. But that sounds like performance loss.)
(I don't see a global propagation of config changes in the C++ code, so it seems we'd just have to)

add a new entry to the GUI manager functions area of ScriptFunctions.cpp that would just update that one variable.

In D332#29326, @elexis wrote:

REQUIRES GAME RESTART should be avoided where possible.
All it takes is updating g_GuiScale if the gui.scale config entry changes.

I agree, but wasn't there some texture creations depended on the gui scale? Need to check its uses.

In D332#29332, @vladislavbelov wrote:

In D332#29326, @elexis wrote:

REQUIRES GAME RESTART should be avoided where possible.
All it takes is updating g_GuiScale if the gui.scale config entry changes.

I agree, but wasn't there some texture creations depended on the gui scale? Need to check its uses.

I can't find any texture dependencies on g_GuiScale (unless the texture creation caches GetDefaultGuiMatrix() in some obscure way). All dependencies I have found are indeed used on g_GuiScale in 'real time'.

In D332#29326, @elexis wrote:

add a new entry to the GUI manager functions area of ScriptFunctions.cpp that would just update that one variable.

I have attached a patch which registers a ReloadUserConfig() call, which reloads the saved configuration on the fly and additionally can do sanity checking of the configuration parameters.
I think it is impossible to 'shoot oneself in the foot' using this patch, as it won't let the UI scaling to be larger than what a native 1024x768 resolution look like.

A drawback is that you would have to press 'Save' in order to see what the actual 'maximum scaling' is allowed to be. (but this should not matter in practice)

Perhaps not the most elegant solution, but it was the easiest thing I could think of which hopefully does not mess with other parts of the code.

Notice that UpdateResolution` also sends the WindowResized event` even though the window size didn't change. Probably won't matter as long as there is no code that may not be run if the change is kept the same.
Changing the UI setting without saving it should apply it just as saving does. Make sure to keep options.js agnostic of options.json (i.e. when adding support for a setting, make sure all settings can use that).

In D332#29362, @elexis wrote:

Notice that UpdateResolution` also sends the WindowResized event` even though the window size didn't change. Probably won't matter as long as there is no code that may not be run if the change is kept the same.
Changing the UI setting without saving it should apply it just as saving does. Make sure to keep options.js agnostic of options.json (i.e. when adding support for a setting, make sure all settings can use that).

Regarding the call to UpdateResolution(), this is required in order to flush the cached window sizes. I guess this would be equivalent to changing the window size in a semantic way (even though the physical window size is kept the same), so I don't really think there is a problem here. Otherwise we need another way to flush the cached window sizes.

I just found out that options.json already allows for function callbacks, so here is my second (less hacky) attempt at doing the same thing, but applying the gui.scale even before the Save button has been pressed.

Updating the gui scaling this way makes it really hard to use a slider for this setting though, so it is a number box for now.

The only thing that I noticed is missing from @joskar's patch is that the cursor doesn't get resized until the game is restarted. The cursor needs to be recreated, because the scaling is done at creation time.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1762/ for more details.

Should also add the programming.json change from D749, in case the latter hasn't committed already (we can commit it as a patch by both of you).

In D332#29460, @Dariost wrote:

The only thing that I noticed is missing from @joskar's patch is that the cursor doesn't get resized until the game is restarted. The cursor needs to be recreated, because the scaling is done at creation time.

Good catch. Fixed in attached patch. However, there doesn't seem to be a "good (exposed) way" to do this other than to remove the cursors which are already loaded and relying on the asset handling system to reload it on the next cursor_draw() call. But maybe relying on this behaviour isn't terrible?

In D332#29472, @elexis wrote:

Should also add the programming.json change from D749, in case the latter hasn't committed already (we can commit it as a patch by both of you).

Done.

In D332#inline-13546, @elexis wrote:

Attention when changing this.value in order to show the most recent value. This also triggers an onValueChange event which we might not want to call in any case, but here it seems to not have any side-effect.

Please tell me if I am wrong, but it seems that onValueChange() is only called if a mouse motion event is acting on the slider (as coded in gui/CSlider.cpp). Thus I can't really see how the this.value = ...; could trigger an extra call of onUpdate (which I assume is what you were worried about), because I can't find any overloading of the setter for this.value anywhere either.

Mostly asking because I get this "You have unsaved changes, do you want to close this window?" each time I open the options dialog and want to close it again without having touched anything.

I cannot reproduce this behaviour when running a new instance of 0ad. However, if I change something once and then close the Options menu without saving, then it will keep asking me the next time I enter and then try to leave the Options menu (without changing anything). I think this is an unrelated issue, and it seems to be because the settings are not automatically reverted upon exiting the menu (and this is probably even the intended behaviour?).

In D332#inline-13550, @elexis wrote:

Should explain what it actually does.

Fixed. (improved, rather)

in D332#inline-13566, @vladislavbelov wrote:

Why not call value as guiScale? As value isn't const. So it's unnecessary variable.

I kept both of these to keep track of whether I had sanitized the value. I don't keep track of that anymore in this new patch, so I renamed value to guiScale as suggested.

in D332#inline-13565, @vladislavbelov wrote:

What's a reason of these checks? Also it's invalid in case, when user have the windowed mode

The reason was to sanitize gui.scale so that the scaling does not get too big (making the scaling too big makes it impossible to revert any changes, because everything will be off-screen). And I set the limit to not exceed the equivalent of a resolution of 1024x768 (which is the current 'minimal supported resolution').

I added a comment in the code to clarify this.

Also, I think it is valid even though the user is in windowed mode. Otherwise the user might have to be forced to enter fullscreen mode in order to revert unplayable settings. (which the user might be required to know/remember the keyboard shortcut for, because the setting might be unreachable after making the scaling too big...)

In D332#inline-13545, @elexis wrote:

Why is the value saved to the config file if it was sanitized, but not if it differs from the config file value without having been sanitized?

The value was already 'saved' to the config file from options.js (by Engine.ConfigDB_CreateValue), and I was only updating the value in case it had to be updated. In this new patch, I always update it. Less code, so it is perhaps cleaner now.

In D332#inline-13543, @elexis wrote:

std::to_string

Thanks! I was looking for the C++ way to do it, but couldn't find it. C++11 to the rescue!

In D332#29501, @Dariost wrote:

Updated with the latest diff

I can confirm the issue where after changing the UI scale and saving the settings it still open the dialog for unsaved settings when closing the options menu

Oh, now I see. I misunderstood it before. I'll look into it.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1764/ for more details.

Marking some stuff as 'done', while leaving a couple still open for discussion (no clear 'Definition of Done' on those?).

In D332#inline-13578, @vladislavbelov wrote:

I think it should be more readable, because the current implementation can show it in wrong way. I think this is shorter:

guiScale = std::max({guiScale, g_xres / 1024, g_yres / 768.0});

Also hard-coded things are bad. I can suggest to use a const variable.

It sure is shorter, and updated the diff now to see if there are other comments on it. It should be std::min({}) though in order to have the same behaviour as before.

Updated patch accoring to the comments below:

Option type:
Slider:

• Shouldn't this be a slider? The problems seems to be that the cursor is positioned on a different location relative to the slider coordinates. So when increasing or decreasing the value, there will be an unreasonable GUI scale change. -> Impossible to use for now.

Number:

• rP20091 made an existing issue more visible. Currently it's impossible to just type "0.5" in the text field, because it sanitizes on each key press, i.e. the first character (0) becomes 0.5, then after typing a ., the value would be 0.5., which is parsed to NaN. Before that commit the issue occured only if cursor was moved outside of the window in between. It had never become visible until now. I'm annoyed by the fact that we can't simply type 0.6. It will require a good solution without workarounds. Using it as is might be considered good enough.

Dropdowns are off the table because the values would have to be translated and there are only few values that we can provide. It's a free numeric range, so it shouldn't be a dropdown.

C++ part:

• If we had a JSInterface_GUI.cpp, I'd say move the code there. But we don't. So don't.
• The contents of that ScriptFunction could still be moved to the GUI manager.
• When I set a NaN, I get one of these weird errors on every frame. Can be caught trivially.

ERROR: Trying to use a sprite that doesn't exist ("BackgroundSessionTooltip").

But the sesion code is totally not loaded (these sprites shouldn't be in gui/common/ independent of that).
Amazingly that NaN is transported into the C++ function as nan:
http://en.cppreference.com/w/cpp/numeric/math/nan
So we should have a check for that!

Options sanitization feedback:

• (The sanitization feedback should work for all functions that can call into the engine (at least as of D805, all option types support it), so not only for slider and input. BUT,)
• I'm not a fan of transporting the sanitized value back to the GUI. All config value sanitization should be (or stay) unified. That means either we only do it in options.js(on) and remove that feedback in the proposed patch, or we change options.js to not come with a predefined set of sanitization constants. Considering that the user can still edit a completely messed up value into the config, the C++ code using config values needs additional sanity checks either way. As soon as these actually change the value, the config value is not updated. So the proposed feedback seems like the wrong place. Better would be doing so when initially loading the config value. Hence I prefer to just remove those lines from the patch and remove the post-sanitization feedback in the options page of features provided by the patch.

Value range:

• If the user wants to edit the config file and set an extremely small gui scale, then it might be considered a feature to be able to do so.
• Someone concerned about users setting a too low value could change the comment in default.cfg
• Why aren't we concerned about someone setting a too big UI scale? I certainly run into that issue. If the UI scale is too large, only the middle part of the dialog will be visible, so it's not good to put it at the top of the dialog either. Putting it into the middle of the dialog wont work either, as the options should remain logically grouped.

Rendering issues:

• The red buttons look a bit odd after scaling (I get few 1px wide black horizontal and vertical lines across the button). Perhaps that could be fixed by reloading sprites somewhere somehow.
• My mouse cursor is flickering

The number input auto-correct issue was addressed by the commit above.
Rebased patch:

Added a scale=0, the UI really didn't like that value, froze and spammed errors.

g_OptionType still needs to convert it to a number before calling Engine[option.function](value);.

I'm still not happy enough to commit this. The random black lines and fonts look really ugly.

12:12 < Dariost> i think it's some filtering issue (like some GL_NEAREST instead of GL_LINEAR in the texture filter)

In D332#35255, @elexis wrote:

Notice posix has an isnan macro, so we can't use std::isnan (http://en.cppreference.com/w/cpp/numeric/math/isnan)

#undef isnan should work, then again just doing if (a != a) { /* a is NaN */ } would also work.

(And why is it surprising that IEEE 754 floating point numbers are correctly passed from JS to C++?)

In D332#35261, @leper wrote:

In D332#35255, @elexis wrote:

Notice posix has an isnan macro, so we can't use std::isnan (http://en.cppreference.com/w/cpp/numeric/math/isnan)

#undef isnan should work, then again just doing if (a != a) { /* a is NaN */ } would also work.

(And why is it surprising that IEEE 754 floating point numbers are correctly passed from JS to C++?)

(isnan passes (on my platformat least), just std::isnan doesn't)

(isnan passes (on my platformat least), just std::isnan doesn't)

Because POSIX isnan is a macro. If I'd have to implement that it'd be #define isnan(x) ((x)!=(x)), which still has issues if the passed x has side effects (so there'd have to be some internal function eg int __isnan(sometype x) which is then just used by the isnan macro). But as the preprocessor does its work before the compiler does anything you might end up with something like std::((bar)!=(bar)), which is not going to compile. Thus you would need #undef isnan to remove isnan from the list of things the preprocessor will try to replace.

I would commit the patch despite the remaining graphical glitches, but only if it became possible to reset the UI scale to 1 in case the user set it too low or too high to control anything.
I don't see a solution for that other than putting the control right into the center of the screen and (ab)using the fact that the options window is always centered.

In D332#47970, @elexis wrote:

I would commit the patch despite the remaining graphical glitches, but only if it became possible to reset the UI scale to 1 in case the user set it too low or too high to control anything.
I don't see a solution for that other than putting the control right into the center of the screen and (ab)using the fact that the options window is always centered.

(Assuming the setting is applied directly) Why not just a dialog with a timeout, that reverts the setting to 1 after e.g. 10 secs?