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

Feature: Hide block signals in GUI by default #8688

Merged
merged 1 commit into from Sep 5, 2021

Conversation

@2TallTyler
Copy link
Member

@2TallTyler 2TallTyler commented Feb 18, 2021

Motivation / Problem

New players to OpenTTD are confused by signals, partly because there are so many choices available.

Description

signal_toolbar

Path signals are the only signals needed by the majority of players, so this PR hides all non-path signals from the signal GUI by default. For the players who use them for signal logic, priority merges, or stubborn habits, they can be enabled via an Advanced-level setting, Show signal types.

In an attempt to simplify the confusing combination of settings affecting signals, I have made additional changes:

  • Settings > Company > Signal type to build by default: Path signals has been removed and hard-coded to one-way path signals. Note that players only see this when they first open a game -- after that, the toolbar opens with the last signal used selected.
  • Settings > Company > Cycle through signal types now defaults to Path signals only, and the option for Block signals only has been removed.
  • Settings > Construction > Enable the signal GUI has been removed. You can still control-click the signal button in the Rail construction menu to build signals without creating a toolbar.

I am open to debate on any of these changes.

This PR was inspired by #7504 but mostly builds on @JGRennison's work hiding pre-signals for his Realistic Braking feature.

Limitations

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')
src/widgets/rail_widget.h Outdated Show resolved Hide resolved
Loading
src/rail_gui.cpp Outdated Show resolved Hide resolved
Loading
src/rail_gui.cpp Outdated Show resolved Hide resolved
Loading
@2TallTyler 2TallTyler force-pushed the hide_advanced_signals branch from 2f7537f to c6b563d Feb 18, 2021
@TrueBrain TrueBrain added this to the 1.11.0 milestone Feb 27, 2021
@frosch123
Copy link
Member

@frosch123 frosch123 commented Feb 27, 2021

You can use BuildSignalWindow::UpdateWidgetSize() to adjust the size of individual widgets.

if (widget == WID_BS_...) {
  size->width += WD_FRAMETEXT_LEFT + WD_FRAMETEXT_RIGHT;
} else ...

You will have to give the WWT_CAPTION a WID_BS_... identifier.

Loading

src/table/settings.ini Outdated Show resolved Hide resolved
Loading
@2TallTyler 2TallTyler force-pushed the hide_advanced_signals branch from c6b563d to 6ee7a88 Feb 27, 2021
@2TallTyler 2TallTyler marked this pull request as ready for review Feb 27, 2021
@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Feb 28, 2021

What happens if one settings allows cycling through all signals and the GUI only shows path based?

Loading

src/lang/english.txt Outdated Show resolved Hide resolved
Loading
src/settings_type.h Outdated Show resolved Hide resolved
Loading
@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Feb 28, 2021

I am all in favor of simplifying settings, as long as you'll help me out by posting https://xkcd.com/1172/ at the appropriate time! 😀

Loading

@2TallTyler 2TallTyler force-pushed the hide_advanced_signals branch from 6ee7a88 to c4da168 Mar 1, 2021
@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Mar 2, 2021

I have merged three signal settings into one, now called "Signal GUI mode," with three options:

  • Path signals only (recommended)
    • Cycles through path signals only
  • All signals
    • Cycles through all signals
  • None
    • Cycles through path signals only

In all three modes, the default signal to place when opening the signal GUI for the first time is the standard path signal. After that, the tool defaults to the last selected signal, as normal.

Not every configuration possible with the old settings is still provided, but I think the selection memory will suffice for most edge cases.

Loading

@ldpl
Copy link
Contributor

@ldpl ldpl commented Mar 2, 2021

@2TallTyler IMO you removed two very useful options - one-way pbs as default and show all cycle pbs.

And cycling all is useless except for opencoop playstyle as it's the only case I can think of that may require fast access to all signal types.

Loading

@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Mar 2, 2021

I can add an option to show all, cycle path. I don't want to remove show all, cycle all, because inevitably someone will complain.

Because the signal GUI remembers the last signal you built, the only case where the default signal really matters is when the GUI is disabled entirely. Perhaps defaulting to one-way PBS would be better here?

Loading

@ldpl
Copy link
Contributor

@ldpl ldpl commented Mar 2, 2021

@2TallTyler it doesn't remember for long. You reconnect to the server and it's already back to two-way, very annoying. I remember I was very happy when I found there is a setting for default signal type because I was already preparing to implement it myself.

And while I'm ok with one-way pbs by default I'm pretty sure there are fans of other options.

Btw, I just noticed, there is another pretty pointless and quite confusing signal type there - semaphores. Though I guess that's a story for another pr ;)

Loading

@2TallTyler 2TallTyler force-pushed the hide_advanced_signals branch from c4da168 to 8bdceb4 Mar 2, 2021
@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Mar 2, 2021

I added the setting for "show all, cycle path" and made one-way PBS the default signal type.

What's your concern with semaphores? Relevant to anything changed in this PR or entirely separate?

Loading

@ldpl
Copy link
Contributor

@ldpl ldpl commented Mar 2, 2021

Not really relevant to this pr. Semaphores are just a completely redundant choice that only serves to confuse new players.

Loading

@2TallTyler 2TallTyler force-pushed the hide_advanced_signals branch from 8bdceb4 to 1ee813d Mar 2, 2021
src/rail_gui.cpp Outdated Show resolved Hide resolved
Loading
src/table/settings.ini Outdated Show resolved Hide resolved
Loading
src/rail_gui.cpp Outdated Show resolved Hide resolved
Loading
@2TallTyler 2TallTyler force-pushed the hide_advanced_signals branch from 1ee813d to 331a4f1 Mar 6, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8688 Mar 10, 2021 Inactive
@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Mar 10, 2021

First of all, I did not look at the code. I just started the preview and did some play-testing. My findings:

  • Changing the signal setting closes the signal GUI. This is really frustrating me for some reason.
  • The "None" option feels weird; it is rather undocumented what signal it will put down. I do understand what you try to achieve here, but the user feedback is so minimal, it just isn't very clear.
  • The mentioning of "Cycle" is completely unclear unless you know exactly what you are talking about. It took me quite some time to find the tooltip that mentions this. Even then it took me some time to parse that information :D This was already the case before your PR, but with your PR it becomes even fuzzier.
  • The "None" option seems to be cycling through PBS signals. This is not mentioned to the player
  • The blue "Recommended" is impossible to read when it is not selected in the dropdown. Not sure if we do this on other places, but just mentioning it here.

All in all, for me, this PR makes the signal more confusing than it already is, especially when looking at the settings. Do not get me wrong: I am all in favour of reducing the amount of settings. But it seems some feature-creep happened while doing so, making it all feel slightly more out of place when looking at it as a whole.
For example, if the setting was just: PBS, Block, All, and it allowed cycling through your selections, this would have made perfect sense to me. But by combining what you can cycle through in combination with a "None" setting, it becomes rather unclear to me, and it really feels like we are trying to fit a square peg in a round hole.

I also understand why this happened. There is always a part of our community that wants to fine control everything, so you get an infinite matrix of options and selections :) I am surprised nobody asked for an option to only have "one-way" but for PBS and Block signals :P We just have to be challenging, also to ourselves, what makes for a good addition for the few, but doesn't confuse the many. I think that balance is a bit lost in the current state of this PR. (this is normal btw; happens a lot to my PRs too if I am not careful about it :P)

This brings me to a conclusion that was already made in the closing comment of the previous attempt on this: I think the GUI needs a redesign to make visually more clear what is going on with what signals can be selected and cycled through. I think it is really good to only have PBS visible by default, but the current solution to allow non-PBS via settings doesn't cut it for me (personally opinion, of course).
To be a bit more constructive here:
I can imagine that you only see PBS signals by default, but that there is a button or dropout where you can still select other signals. This would make clear that cycling only happens on PBS signals, and still allow people that once in a while want to build other signals, can do so too. This would most likely work for 90% of our playerbase.
Additionally, we can add a setting that shows all signals by default; that would remove the button or dropout or what-ever too, of course.
For the "None" setting, CTRL+Click on the signal already does this. I do not know if we need a special setting for this. If we do, an alternative approach would be: reverse CTRL behaviour on signals, setting. So by default it puts down PBS, and when you CTRL+Click it opens the GUI again.
To me, such solution would make more sense, but does need a visually solid solution for such dropout; and I do not have a suggestion for that :D But it does mean that we only have 2 settings: show "PBS-only" or "All" by default, and reverse CTRL behaviour yes/no. The rest can all by done from the GUI itself.

Either way, I really think this needs more time and more trying out other possibilities.
Again, but you know this already, I do really appreciate these PRs, as if we never try, we will never innovate, we will never progress. So please keep doing PRs like this: they are really nice and valuable. I just think this one is not (yet) hitting the mark :D But possibly me (long) story gives you enough inspiration :D

Loading

@TrueBrain TrueBrain removed this from the 1.11.0 milestone Mar 10, 2021
@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Mar 10, 2021

Thanks for the detailed explanation. I'm going to close this PR and step back to think about a more thorough signal GUI redesign. I'm still learning when to iterate following precedent and when to chart a new course entirely. :)

Incidentally, I've been learning a lot of engineering strategy from Martin Molin / Wintergaten's mechanical music machine build, and your comments sound a lot like this week's video. My approach of hiding GUI elements via a setting is just treating a symptom of bigger problems with the GUI itself, and treating symptoms is often much harder than just fixing the problem itself.

Loading

@2TallTyler 2TallTyler closed this Mar 10, 2021
@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Apr 3, 2021

@TrueBrain I've been thinking about this recently, and I still feel that the decision of which signals to show belongs in Settings, rather than a drop-down on the signal toolbar itself. Since it will default to only showing path signals, I expect users to change the setting only once, if ever. New players don't need to know about non-path signals, and players who use priority merges (including me), can opt-in to the current toolbar.

There are other issues you raised which would be solved separately:

  • I really wonder if anybody uses the disabled signal GUI. Can we delete this entirely?
  • I think the best way to solve the desire for user control of which signals are cycled through, is to keep that setting separate from which signals are shown. I was a bit too eager to delete settings earlier. Obviously, it would only cycle through the signals which are shown in the GUI.

Edit: See the edited PR description above for a detailed explanation of what I've changed from the last go.

Loading

@2TallTyler 2TallTyler reopened this Apr 3, 2021
@2TallTyler 2TallTyler marked this pull request as draft Apr 3, 2021
@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Aug 24, 2021

I think I updated everything properly to the new ways settings are handled, and tested reasonably well. More testing and a careful inspection of my gui_settings.ini changes would be appreciated!

Loading

Copy link
Member

@TrueBrain TrueBrain left a comment

Testing shows it works as advertised. I think this is the best solution for now.

I would appreciate it if another dev merges this, just to have a double-check this functionality is wanted.

Loading

src/rail_gui.cpp Outdated Show resolved Hide resolved
Loading
src/settings_table.cpp Outdated Show resolved Hide resolved
Loading
@2TallTyler 2TallTyler force-pushed the hide_advanced_signals branch from f7c00bb to fe50594 Aug 25, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8688 Aug 25, 2021 Inactive
@glx22
Copy link
Contributor

@glx22 glx22 commented Aug 25, 2021

Warnings (and annotations check failure) because

static void CloseSignalGUI(int32 new_value);

Loading

Copy link
Member

@TrueBrain TrueBrain left a comment

Nitpicking; sorry :)

Loading

src/rail_gui.cpp Outdated Show resolved Hide resolved
Loading
src/table/settings/gui_settings.ini Outdated Show resolved Hide resolved
Loading
@2TallTyler 2TallTyler force-pushed the hide_advanced_signals branch from fe50594 to f8fed98 Aug 26, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8688 Aug 26, 2021 Inactive
src/rail_gui.cpp Outdated Show resolved Hide resolved
Loading
@2TallTyler 2TallTyler force-pushed the hide_advanced_signals branch from f8fed98 to 60c6afa Aug 26, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8688 Aug 26, 2021 Inactive
Copy link
Member

@michicc michicc left a comment

Last shitstorm was too long ago 😛

Loading

@TrueBrain TrueBrain changed the title Feature: Hide block signal GUI by default Feature: Hide block signals in GUI by default Sep 5, 2021
@TrueBrain TrueBrain dismissed stale reviews from michicc and themself via d75ba56 Sep 5, 2021
@TrueBrain TrueBrain force-pushed the hide_advanced_signals branch from 60c6afa to d75ba56 Sep 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet