Game Settings Overview Window

[Eagle_rainbow]

Hi everyone,

Playing OTTD now for a while, I start participating in multiplayer sessions on the internet. Though slowly "becoming domestic" on one server, what I like most is, that you can play on one server today, while you will be playing on another tomorrow. Doing so, you will notice that the server settings may be quite deviating from each other, which means that you have to adjust your building style. However, I find it nerve-wracking to browse through all the three (well, in most cases it's only two) setting screens to find out what's the current settings of the server you are logged on to not spoil your game with a wrong focus right from the beginning. That's why I was searching for a "brief settings overview" window somewhere that was telling me the most important settings of the server currently logged on at a single spot. However, I was not able to find anything that came close to my needs.
Trying to get familiar with the development style in OTTD, I started to implement a first prototype for the idea above. Here's a screenshot where I currently landed after some iterations:



Note that:

• This overview window is read-only and does not show anything else that you couldn't get out of the rest of the screens already.
• The collection of settings to be shown is my perferred list only and is kept short by concious decision
• The window is reachable via a dedicated setting in the Settings Dropdown menu.

What's available there currently is entirely satisfying my needs. Yet, I'd like to receive feedback from you, if you find that useful as well or where things can still be improved, for example:

• Layout Design
• Labelling/Language consistency
• Which settings to show (and which to leave out and why)
• Coding Style used and where I still have glitches

One feature that looks interesting to me is that the window should open up automatically once you have logged on to a server successfully (to me only network games make sense, as with a local game, you have chosen those values yourself a couple of minutes before anyway...). However, I am still searching for the right coding location yet...

For reference to you, developers, please find attached a patch file against current trunk revision 24586. Furthermore, there is also a patch version against branches/v1.2.

Thanks for your feedback and having mercy with me for my "first patch"

Regards,
Eagle_Rainbow

gso-r24586.diff

game settings overview window patch against trunk (as of writing)

(12.68 KiB) Downloaded 198 times

gso-branch-1.2.diff

game settings overview window patch against branch/1.2

(12.8 KiB) Downloaded 173 times

[minecraftfan]

I would love this to be implemented. The number of times I have logged onto a server and wasted 30 seconds trying to find the useful options means something like this would be brilliant in trunk. What do yuo say Owen?

[frosch]

The selection of settings to display in such a window is quite arbitrary. I mean, imagine a server disabling "build on slopes". You most certainly want to see that, but then the window would be huge.

So, how about a different approach: Display all non-client settings which deviate from the local client's new-game settings?
That would also work for single player and scenarios to see the settings deviating from the values the player is used to.

Also you can generate that window from the settings descriptions, it would not require any decision on what settings are considered "important" (which is quite subjective).

[Eagle_rainbow]

I see your point. During implementation I also had my doubts on the fact that my selection might be very personal - and thus arbitrary for others. My initial thought on that was to allow making the list of settings user-configurable (for example, via .ini entries or something like that). Already there I felt a bit uneasy and that's why I did not implement it, but first wanted to have some feedback from you here (there were other reasons as well to make this suggestion, though). And as we see, your valid point with "building on slopes" would also not be covered by my .ini-approach (as the user most likely won't have had added it to the list).

On the other hand, your approach also has a weakness, which I currently don't feel comfortable with: Assume that you have set your local settings to something totally odd, because you wanted to check out a variant how this feels like playing with. Joining a server which you really know well thereafter, you will then get a huge strange list of about "trivial and very well-accepted" setting values. The list then will be filled with a lot of clutter. Having joined there before, you remember that the list was really short... This behaviour can also appear to be quite erratic to the end user and could provoke bug reports with something like "Settings Overview screen provides random selection".

What about the idea to not make it a "diff against your local settings" but "against the default settings" (as shipped via release packages)? This is a globally defined fixed set and - normally - every player will become familiar with them at least once in a lifetime

[frosch]

I wondered whether something like this could be integrated into the existing adv. setting dialog.

Something like:

• A dropdown to select between "show all settings", "show deviations from new game setting" (only in game), "show deviations from default settings", "show deviations from default settings, not considering client/gui settings".
• Collapse/expand all
• Make these settings the new game settings. (only in game)
• Revert settings to default. (only in main menu)

This would cover your GUI when triggering the "expand all" upon selection of the "show derivations from xxx settings", and it would also cover the often-suggested "revert to default" thing.

I am not sure about the "not considering client/gui settings". You do not want to see them when checking a server because they will be your settings. But you want to see them when generally checking your settings. So I made it two options for now :s

[Yexo]

...

I like that idea

[Eagle_rainbow]

I wondered whether something like this could be integrated into the existing adv. setting dialog [...]

Could also be an option.
However, to be honest, already from the beginning I had my troubles with all these settings screens. First of all, I never really got the point why there are three of them (game options, difficulty and adv. settings). And you are right in that sense, that a fourth one (which then is r/o) doesn't make the world better either. Especially the advanced settings window (affecting both visualization and difficulty) is hard to chew. The tree there tries to make this long list more digestable. However, what is causing me headache, is that the grouping is not really orthogonal and distinct. Let's look at an example for what I mean:

The option "Show timetable in ticks rather than days" are below "Interface" only. On the other hand this is just a "display thingy" which - to my mind - would qualify it to the settings page "interface -> Display Options". However, you also could argue that it should go somewhere to the "vehicles", for instance, next to the "warn if a vehicle's income is negative" (which is just another thing, that only affects if/how something is shown). Moreover, you could think of putting it to another setting page after all ("debugging" or "technical"), as I would assume that most standard users will not like to see rather technical game ticks, but would prefer having something tangable on the screen.

To make a long story short: though I appreciate the idea of having a tree there, the logic behind the grouping for this tree never became obvious to me (if there is a certain hidden logic behind it, then I am eager to listen to it!). Taking the example above, I also have my doubts that it will be easy to find one that fits all. Adding buttons and features to the list, will not make this thing more logical, but for sure by providing a mechanism to filter the list of rows would reduce the complexity for the user.
However, the most important part IMHO would be to group all those settings better: Today you always need to look very closely if the buttons are disabled if you are on a network game opening the adv. settings window and try to see what you can adjust "locally" (because it does not affect the other players on the server) and what has been set by the server, because it affects gameplay. Only those will have an impact on the "settings overview" at all.
Going for an integration into the advanced settings window we need to consider this - otherwise it could become risky that we overload an already tricky screen. And that is exactly what I wanted to prohibit with my suggestion at the first place...

[planetmaker]

I do agree with your analysis that the three different settings windows (options, difficulty, adv. settings) is rather sub-optimal - a historically grown jungle of settings in various places.

Personally I'd prefer a split in user-settings and in game settings, maybe a 3rd network setting for cases where you start a (non-dedicated) server.

Wrt grouping: that's a difficult thing IMHO - any grouping is somewhat subjective, I fear. I once tried to (re-)group them... but always found that some settings belong in at least two categories. But doing so certainly won't help conciseness of the already long list - even when it's split into the above mentioned user, game and network parts.

That said, I also like frosch's suggestion on how the important changes of a network game could be visualized. Basically I think that that change can be done independent of a re-grouping of the settings windows, though; it's two related tasks, but can be done independent (and probably should - or neither is done ever)

[Eagle_rainbow]

I just saw that with rev. 24631-24633 the filter capability (by search text) in the advanced options screen was added. I just tested it and think, that it solves the search issue I mentioned above.
Currently, I am thinking/trying to implement on the other idea frosch mentioned, namely, to filter on the entries which deviate from other settings (vs. new game/default)..

[Eagle_rainbow]

Hi everyone,
Having found some hours to read up to the advanced options screen, I also managed to get something running as well. I have attached a patch file plus some screenshots based on the current state where I am. The patch is against rev. 24634.

asrestriction_ddb-against-24634.patch

Advanced Setting GUI with restriction on default values etc.

(17.05 KiB) Downloaded 205 times

I also got it working in conjunction with frosch's latest enhancement, the search edit box. Also, I could test that this enhancement works very well in a local game; yet, I was not able to check it in a network game (simply didn't find the time yet).
The only little glitch that does not pleasure me yet, is that the drop down arrow of the drop down box is not being reset properly after you have chosen a value yet (see also second screenshot below). However, I think that this is something really tiny which I just don't find to fix easily as I am still quite a greenhorn... Can someone of the more experienced developers assist here from where this is coming?

Any feedback on this already?
I think it would also solve my initial request described in this thread.

[Eagle_rainbow]

Update: Now, I was also able to do a test in network mode - and it works like a charm.
Yet, I found the labels in the dropdown box a bit odd and changed them; here's the updated patch.

[Yexo]

Nice work!

I do have some comments on the code though, most of them are minor and should be very easy to fix.

- I see no reason to move the complete ResolveVariableAddress function, just declared it near the top of the .cpp file like this:

Code: Select all

static const void *ResolveVariableAddress(const GameSettings *settings_ptr, const SettingDesc *sd);

Code: Select all

StringID restrictionMode

-While I understand your reason for doing this, I dislike it. Please create a new enum with the restriction mode and an easy way to map that enum to/from a string. It makes it a lot easier to check those functions if they implement all possible values from that enum, instead of having to check the language file with the risk of overlooking one.

Code: Select all

if ( (restrictionMode == STR_CONFIG_SETTING_RESTRICT_CHANGED_AGAINST_DEFAULT)

- The coding style says to remove the space after the first (, so

Code: Select all

if ((restrictionMode == STR_CONFIG_SETTING_RESTRICT_CHANGED_AGAINST_DEFAULT)

Code: Select all

// this entry shall only be visible, if the value deviates from its default value

- The coding styles says use /* */-style comments for comments not on the same line as code.

Code: Select all

		assert ( (this->flags & SEF_KIND_MASK) == SEF_SETTING_KIND );

- Again, space. This should be:

Code: Select all

		assert((this->flags & SEF_KIND_MASK) == SEF_SETTING_KIND);

Code: Select all

+		if (settings_ptr == &_settings_newgame)
+			// we are changing the new game's settings and shall compare against the new game's settings
+			// ==> we never will find any difference there
+			return false; // situation should not occur anyway, but we can recover from here without causing damage

- Better make sure the code really never comes there by disabling this filter in the main menu. Place an assert at this location to make sure it never happens.

Code: Select all

+			if (force_visible) {
+				visible = true;
+			} else if (!filter.IsEmpty()) {

Could be this since force_visible is already initialized to true above.

Code: Select all

			if (!force_visible && !filter.IsEmpty()) {

If nobody else objects and you fix the above issues I think this can go in trunk very soon.

[Eagle_rainbow]

Hi Yexo,

Nice work!

Thanks, especially as this is the first patch which I've submitted

- I see no reason to move the complete ResolveVariableAddress function, ...

done.

Code: Select all

StringID restrictionMode

-While I understand your reason for doing this, I dislike it. Please create a new enum with the restriction mode and an easy way to map that enum to/from a string. It makes it a lot easier to check those functions if they implement all possible values from that enum, instead of having to check the language file with the risk of overlooking one.

Funny. When I came to that part, I first had implemented it with an enum, but finally refrained from doing so before uploading the patch, as I did not consider this entity to be strong enough. It was just a 1:1 mapping without additional value to me. But as you requested it, here we go
Please note that I have skipped implementing a RestrictionMode->StringID routine, as it would not be used actively in this coding. Thus, I did not want to introduce any unused code lines which just can date out... Moreover, this inverse direction is also really a low-brainer anyway, so that anyone may write it, if it should be become necessary at some later point in time...

Code: Select all

if ( (restrictionMode == STR_CONFIG_SETTING_RESTRICT_CHANGED_AGAINST_DEFAULT)
... 
// this entry shall only be visible, if the value deviates from its default value
...
assert ( (this->flags & SEF_KIND_MASK) == SEF_SETTING_KIND );

sorry, on the code style thingy... In the last three weeks I have implemented stuff in three different languages (ABAP, Java and now C++) and everyone of them has a slightly different style guide... It's just too easy to mix them up.
of course, fixed as well.

- Better make sure the code really never comes there by disabling this filter in the main menu. Place an assert at this location to make sure it never happens.
Could be this since force_visible is already initialized to true above.

done those two things as well

If nobody else objects and you fix the above issues I think this can go in trunk very soon.

Both I would be glad and it would be an honor for me
Here's the revised version of the patch.

asrestriction_ddb-v3-against-24634.patch

revised version of patch for restricting by default values etc. in the Advanced Setting GUI

(17.77 KiB) Downloaded 196 times

Now, the only pending thing is about the hanging drop down box arrow to the right. Do you have an idea what I missed there?

[Yexo]

I've fixed the bug with the dropdown box. It was not really something you forget, the advanced settings gui just is a special window in that it handles the dropdown boxes itself instead of leaving it to the more generalized code in the Window class. My fix was to call the general function for your new dropdown box.

More in general I've also made some changes to the rest of your patch. The StringID->RestrictionMode function was not required, you were doing the conversion twice without realizing it. I've also done a few more changes to make the code suit the style of OpenTTDs codebase more.

You can find my changed version of your patch here: http://devs.openttd.org/~yexo/filter_changes.diff
The strings in english.txt will still have to be changed, they're not consistent with capitalization/punctuation and I think they can be written in a more clear way. This patch is now up for review by other devs. Depending on review I'll commit it later this week.

[Michi_cc]

This patch is now up for review by other devs.

There are a couple if's with the body on a new line without braces. And I'd include a full stop in the function doxygen comments wherever there is a real sentence.

-- Michael Lutz

[Yexo]

Thanks, I hope I got them all this time. Same url, new diff.

[Alberth]

Looks fine to me

[Yexo]

And in trunk since r24647. Please have a look at the final commit to see what was changed since your last diff. I think the most important points are:
- if() xyz; needs to be on a single line. If split over two lines, add braces.
- Comments need capitalization and punctuation.
- Strings in english.txt were rewritten.

[Eagle_rainbow]

Thanks! I'll try putting your comments in my pipe and smoke it...
Moreover, I will check my other patch posted in the other thread right away on these learnings.

[ChillCore]

After screwing up my config file, (*), I have found that it would be nice to have two more options in the dropdown menu:
- Show all settings that have a default value.
- Show all client setting whatever their value is.

The first on might perhaps be tricky though.
We do not want the setting to dissappear from the gui as soon as the value is changed, perhaps someone wanted to click an button twice or maybe selected the wrong dropdown option.


(*) calling SaveToConfig() from a button in scenario editor mode was a bad, bad idea.