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

Change: Recolour graph windows to brown #8700

Merged
merged 1 commit into from Mar 11, 2021

Conversation

@2TallTyler
Copy link
Member

@2TallTyler 2TallTyler commented Feb 19, 2021

Motivation / Problem

The dark graphs introduced in #8557 look great, but the windows don't match stylistically with other GUI elements. @ldpl suggested changing the window color to brown to match the map.

EDIT: I had attempted to recolour a bunch of other windows, but realized the scope creep made reviewing difficult, so I've reduced the scope to just recolouring graph and other information windows brown.

Closes #8633.

Description

brown_graph

The following graph windows are now brown:

  • Operating profit graph
  • Income graph
  • Cargo delivery graph
  • Company performance graph
  • Company value graph
  • Cargo payment rates graph

Also, several non-graph windows make sense to be brown too:

  • Detailed performance rating
    • Note: I also made the company performance numbers orange, to match other brown windows
  • Company league table
  • Sign list

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')
@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Feb 19, 2021

Screenshots or it didn't happen! :D

@DorpsGek DorpsGek temporarily deployed to preview-pr-8700 Feb 19, 2021 Inactive
@frosch123
Copy link
Member

@frosch123 frosch123 commented Feb 19, 2021

Nice idea. Some notes:

  1. Your list is missing the industry directory and fund-industry window :)
  2. I think town GUI and local authority should have the same color. Giving the local authority the same color as the toolbars sounds weird to me. For me those two windows belong together like vehicle view and vehicle details.

3. I would expect town directory to have the same colour as the town view. Though argueably that is not the case for the industry directory. Not sure whether industry and town directory would be readable with the cream colour.

For reference, an earlier page about the same idea: https://wiki.openttd.org/en/Development/GUI%20Style%20Guide#window-colours

Edit:
4. Sign-rename window: Brown, grey, grey+1CC ?
5. The graph windows have plenty of colours in the curves themself. A neutral background works better.

So, I tend to agree with ldpl. The changes do not really improve the visibility. I guess they are inconsistent for a reason.

@ldpl
Copy link
Contributor

@ldpl ldpl commented Feb 19, 2021

This would've been much easier to discuss as several separate changes but here's my opinion:

  • "Consistency" - mostly irrelevant. Just pick whatever looks nicer as that categorization is questionable anyway.
  • Graphs, Sign list and league table - Brown is better.
  • Detailed performance - was better in grey. and you didn't even recolor buttons.
  • Towns - awful change, cream is ugly af, quite aggressive for a background in general and red and green text is nearly unreadable on it. IMO industries should be switched to brown as well.
  • Local authority - no, god no. Leave dark green for toolbars if anything :p

UPD. I forgot I had night mode on, so changed my opinion a bit after disabling it xD

P.S. Story book also has 1cc brown.

@2TallTyler 2TallTyler force-pushed the recolour_windows branch from 6417b03 to 4a1df39 Feb 20, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8700 Feb 20, 2021 Inactive
@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Feb 20, 2021

Thank you both for the feedback. I have updated the original post.

I have reverted town and local authority windows to brown. Industries probably have to stay cream, since a lot of NewGRF industries insert custom text which we can't recolour orange to match town windows.

I've changed the buttons on the detailed performance window to brown, which required extending MakeCompanyButtonRows() to take a Colours parameter.

I also recoloured the buttons on the cargo payment graph from orange to brown. The closest parallel here I think is enabling/disabling industries in the small map, which of course are not buttons...but this inconsistency is out of scope here.

Sign-rename window: Brown, grey, grey+1CC ?

All rename windows are system grey including for player-owned things like stations and vehicles. I don't see a need to change this.

@2TallTyler 2TallTyler marked this pull request as ready for review Feb 20, 2021
@ldpl
Copy link
Contributor

@ldpl ldpl commented Feb 20, 2021

I still think detailed performance looked better in grey but brown is ok too now. And mb making numbers orange like in the town window will improve it even further.

Brown cargo buttons look nicer than orange.

Industries probably have to stay cream, since a lot of NewGRF industries insert custom text which we can't recolour orange to match town windows.

Well, if there ever was a good time to do that it would be now. As 1.11 already messes with industry sets in cargo colours. And gamescripts now can add a custom industry text too so they'll also become a hindrance in the future.

@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Feb 20, 2021

I agree that this is a good opportunity to change the industry window colour, since we are already changing the graph background. I'll include this in the next push. EDIT: Reverted this as it's out of scope.

@ldpl: And mb making numbers orange like in the town window will improve it even further.

Thoughts?

orange_performance

@2TallTyler 2TallTyler force-pushed the recolour_windows branch from 4a1df39 to a2a86da Feb 20, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8700 Feb 20, 2021 Inactive
@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Feb 20, 2021

Industries are now brown with black/orange text, and even NewGRFs which use yellow text don't look that bad. See the PR description for a screenshot. EDIT: This has since been reverted, as it's out of scope.

I went with the orange numbers for the detailed performance rating.

@stormcone
Copy link
Contributor

@stormcone stormcone commented Feb 20, 2021

Personally I don't like these changes. I find it better when the different thing's windows have different color, because I can easier distinguish them without taking a closer look. Like when the industry window has other color than the town window.

@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Feb 20, 2021

EDIT: I could go either way on the industry window, and the primary scope of this PR is graphs and related info windows being brown. I have simplified the PR to only graph windows.

If someone has a strong preference on the industry window colour, they can open a new PR. It is out of scope for this one.

@2TallTyler 2TallTyler force-pushed the recolour_windows branch from a2a86da to 2a521ba Feb 20, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8700 Feb 20, 2021 Inactive
@2TallTyler 2TallTyler changed the title Change: Recolour windows for consistency Change: Recolour graph windows to brown Feb 20, 2021
@TrueBrain TrueBrain added this to the 1.11.0 milestone Feb 27, 2021
@LordAro
Copy link
Member

@LordAro LordAro commented Mar 3, 2021

You're still changing the signs list window to brown. Seems out of scope for this - given industry directory is brown & station list is grey

@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Mar 3, 2021

I'm not opposed to splitting up this PR for each window type, but this isn't an oversight in colour choice. The sign list is global like the industry window, not split per company like the station list.

@LC-Zorg
Copy link

@LC-Zorg LC-Zorg commented Mar 8, 2021

Few comments:

  1. Why do you want to make a change that can be done with NewGRF?
  2. The issue of improving consistency is very debatable and up to the player. For example, graphs are part of finance for me - the finance window is gray, why should the graphs be brown? So does the sign list - the sign input window is gray.
  3. The issue of improving the aesthetics is even more debatable. To me, the brown window doesn't look bad. I'd say it's even quite nice, especially with the lines tweaked. Thanks. But the Detailed performance rating window doesn't look good anymore. Orange text on a brown background is not more legible at all, it is quite the opposite. See that here, in a very small group, there are voices against this change. Among players, many of whom may have become attached to the classic look, this change may also be perceived negatively.
  4. Haven't you thought about leaving the choice to the players?
    Extensive options could be created, but even if you gave a simple option: New / Old, this change would be neutral at worst and positive for many. An interesting solution could be adding the GUI style / theme selection option. There is also an option that I mentioned in the topic of city names, where the player could choose passive NewGRF - among them there could be those that change the appearance of the GUI.

@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Mar 8, 2021

A few answers:

  1. GUI window colours cannot be changed by NewGRFs.

  2. In the colour scheme I determined from other windows, windows about global game information (map, town and industry directories, town windows, goals, etc) are brown while windows about specific companies (finance, station and vehicle windows, etc) are grey with a 1CC title background. Grey windows are for system windows like save/load, rename dialogs, etc. This colour scheme could be changed or reinterpreted, but this is my proposal for the core developers to consider.

  3. I think it's important that the detailed performance window match the performance graph. And orange text on a brown background is very common in OpenTTD - see the town window, for example.

  4. NewGRF GUI themes is extraordinarily out of scope for this PR. :)

@LC-Zorg
Copy link

@LC-Zorg LC-Zorg commented Mar 10, 2021

  1. GUI window colours cannot be changed by NewGRFs.

NewCC Set

Zrzut ekranu 2021-03-08 092152

There are add-ons that do this. Probably the problem is that the window colors and the company colors are linked. Perhaps NewGRF cannot currently change a specific element. The question is, is it a big problem to make both changes independent and to create appropriate opportunities?

  1. Just because it's common doesn't mean it's more legible. If you want to change something, it should be for the better, not the worse. I agree that orange looks more interesting in terms of color, but it just worsens readability. A few separated numbers next to each other in a city window is not the same as a monolithic string in that window.

I checked what it would look like with black text and it seems... less colorful, but, in my opinion, more important, it is clearer.
Black letters

  1. I am just writing that there are other ways to do the same that will not cause anyone a sense of loss. For me, this change would not be bad. But I kinda dislike the fact that you don't include in this PR of players who like and prefer the current look.

@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Mar 10, 2021

Ah, I was not aware of that NewGRF. I would note, though, that it simply changes the entire colour palette of the game; it is still not possible to recolour specific GUI elements via NewGRF.

I welcome other thoughts on orange vs. black text in the detailed performance rating window.

It is not feasible to allow users to opt-out of every single change, and I have not heard from anybody that they prefer the current look over this change. Please understand that I am not unilaterally making changes with no concern for other users. I am trying to contribute to making OpenTTD a better game for all, and my proposals are all subject to approval or rejection by the core developers. I welcome input from anyone and often incorporate constructive suggestions into my proposals (see my back-and-forth with ldpl here and in my other recent PRs), but opposing change just because you fear a hypothetical user might be upset is not helpful feedback. Nor is it our job to worry about that; let's leave that to the core developers. :)

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Mar 10, 2021

Right, we talked it over a bit. A few minor requests:

  1. please keep the performance strings black. Orange doesn't fit the rest of the game, and looks a bit off/weird :) Also, so close to the release, changing strings is a bit troublesome. So I rather not, if we can :) (bit of a silly reason, I know)
  2. please rebase, so we no longer see those pesky CI warnings :D

Otherwise, good to go!

@2TallTyler 2TallTyler force-pushed the recolour_windows branch from 2a521ba to ade7243 Mar 10, 2021
@LC-Zorg
Copy link

@LC-Zorg LC-Zorg commented Mar 10, 2021

I think it would be worthwhile in the future to consider adding a color selection option for some windows - without going into details, just 2, 3 options like: Brown (default), Gray (classic). Changing the color could also be useful for purple windows, which are not very legible given the small GUI size. Anyway, this is a matter of a different PR. ;)

@TrueBrain TrueBrain merged commit de89123 into OpenTTD:master Mar 11, 2021
12 checks passed
@2TallTyler 2TallTyler deleted the recolour_windows branch Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

8 participants