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: darken the background of all graph to increase contrast #8557

Merged
merged 1 commit into from Jan 13, 2021

Conversation

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 11, 2021

Fixes #8539.

Motivation / Problem

Some company graph colours are very hard to read, making you want to change your company colour.

See #8539 for a detailed explanation.

Description

For the current choice, colours like Mauve, Dark Green and Purple
are nearly invisible in the graphs. Using a darker background
resolves that issue.

image

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')
Now lines like Mauve, Dark Green and Purple are much more visible
without hurting the other colours.
@TrueBrain TrueBrain added the preview label Jan 11, 2021
@TrueBrain TrueBrain marked this pull request as draft Jan 11, 2021
@LC-Zorg
Copy link

@LC-Zorg LC-Zorg commented Jan 12, 2021

It looks cute :)
I made a small attempt at how the dark field will look with a large window and I think it's ok.
However, I would have a few comments:

  1. Dark background is not suitable for displaying payment rates charts of more complex industrial sets - a light background should still be displayed there, unless it is possible to change the dark colors of lines defined by newGRF (I don't think so)
  2. For me, such a background is a good solution, but maybe others will still prefer bright - I remember that adding more settings is not the most desirable solution, but it seems that it would be good to give this option (light / dark / dark except rate chart)
  3. Details worth adding:
  • Readable period markings - the present one is unreadable and at least for me still confusing today.
  • Framing for the dark field - it doesn't matter for readability, but it gives an interesting, pleasant visual effect. In the case of a gray window, the frame is not as good.
  • Vertical grid lines of the chart separating individual years with a slightly lighter color or slightly darker ones - make it easier to find the data you are looking for
  • Changing the font of markings on the chart to a larger one - although the current one is very difficult to read, its change may not necessarily cause the expected changes in other windows (I can be wrong). You could also change the size of this font to medium, which might be even better.

Few another versions
New Graph windows

Copy link
Member

@LordAro LordAro left a comment

LGTM

@TrueBrain TrueBrain marked this pull request as ready for review Jan 13, 2021
@TrueBrain TrueBrain merged commit de44ce2 into OpenTTD:master Jan 13, 2021
8 checks passed
8 checks passed
Emscripten
Details
Commit checker
Details
Check for preview label Check for preview label
Details
Linux (clang, clang++)
Details
Linux (gcc, g++)
Details
Mac OS (x64, x86_64)
Details
Windows (x86)
Details
Windows (x64)
Details
@TrueBrain
Copy link
Member Author

@TrueBrain TrueBrain commented Jan 13, 2021

It looks cute :)
I made a small attempt at how the dark field will look with a large window and I think it's ok.
However, I would have a few comments:

1. Dark background is not suitable for displaying payment rates charts of more complex industrial sets - a light background should still be displayed there, unless it is possible to change the dark colors of lines defined by newGRF (I don't think so)

We talked it over, and having 2 backgrounds for similar windows is even worse (and hard to maintain). We think this improves the situation in most cases, but it indeed is a regression for a select few others. Pretty sure we can never catch them all :)

2. For me, such a background is a good solution, but maybe others will still prefer bright - I remember that adding more settings is not the most desirable solution, but it seems that it would be good to give this option (light / dark / dark except rate chart)

Themes etc would be nice, but that should be a generic thing, not only for this specific window :)

3. Details worth adding:

(..)

All nice ideas, but as you might have noticed by now, we are taking one thing at the time, not everything at once :) Otherwise things will never get finished ;) We accept Pull Requests for all these suggestions, as with any other feature request!

@TrueBrain TrueBrain deleted the TrueBrain:graph_colours branch Jan 13, 2021
@LC-Zorg
Copy link

@LC-Zorg LC-Zorg commented Jan 16, 2021

I checked revision 20210114 and I would have some comments:

  1. There is an error when changing the company's color - the color of the chart is not updated immediately, you need to move the view. In some cases, hovering the cursor over the line caused local color changes. In previous versions, there was no problem.
    Bug - Delayed color update

  2. There is a problem with the visibility of rate charts in sets such as FIRS. With XIS, the rates for Manganese are completely invisible.
    Bug - XIS Manganese
    The point is not to make it nice, but to make this change not harmful, and in many cases it would be so now. I don't see any particular relationship between the chart background color option and other windows here. Changing the colors of the windows themselves is a completely different topic. ;) If it is impossible to change the line colors for individual sets, and I suspect that it would not be trivial, it would be appropriate to add a background color option. Preferably with 3 settings: light / dark / dark except for the rates chart. Another solution, simpler but worse, would be to leave the light background for the rates window unchanged.
    Changing the background color to a bit lighter / slightly darker / blue etc. will not change anything, because some rates will always be invisible - the add-on creators have adapted the colors to the light background. Only new editions will be able to adapt to the new conditions.

  3. The color of the mesh is too bright and makes eyes tired - I was delighted too quickly. :) In the game, the color of the auxiliary lines turns out to be too bright and overlapping. It should be at least a shade darker, preferably two. This will give the option of adding lines to separate the years of the game. In such a layout, it will be very easy to observe the graph and find the data you are looking for.
    Graph window - v20210114

Graph window - v202104 1
The color of the lines for the value 0 should of course be lighter than the others - it may be a shade of the current lines.

  1. Details. I understand this approach, but I don't know if I should create new threads for each detail. For me, they form one topic of improvements to the appearance and readability of the window. I have many other comments on other topics, and I would not like to spam by breaking down each one into multiple elements. I'd rather leave the setting of themes for specific changes to other.
@TrueBrain
Copy link
Member Author

@TrueBrain TrueBrain commented Jan 16, 2021

1. There is an error when changing the company's color - (..)

image
Please use that lovely link and click
image

That lovely button afterwards. Again, we are not solving [insert everything] in a single Pull Request. That would only mean we would never merge anything. Unrelated problems need their own issue.
Especially reporting an unrelated problem in a merged Pull Request will not receive any further attention, as it is very likely we will not notice or forget about it :)

2. There is a problem with the visibility of rate charts in sets such as FIRS. With XIS, the rates for Manganese are completely invisible.

(..)

The FIRS author himself told me this will be fine ;) I believe him.

3. The color of the mesh is too bright and makes eyes tired (..)

See 1).

1. Details. (..)

Tickets that describe a single issue are more likely to be solved with a PR that solves that exact issue (as this PR did with the issue indicated).
If you want to improve a complete UI, creating a Discussion would be a good start, so everyone can pitch in if they like the new idea, what should be better, etc. Once there is consensus and it is feasible to do, someone with programming knowledge can make it a reality :)
We are very much against feature-creep, which is what you went for here: request a small thing, but while doing so, also request 10 other things. That is just very unlikely to ever happen, as it only results in an unmergeable PRs. Scoping beforehand is rather important, and we tend to make sure PRs keep a focus. Fixing [everything related] is not a focus.
So I appreciate you want to tackle UIs, and you are more than welcome to do so! But the best approach is to be upfront about that, start with all the things you want changed (in a Discussion would be best), and we can take it from there :)
To word it slightly different: trying to sneak in other changes while we fix a small thing, is unlikely to happen :) Again, this PR being a perfect example of that! This stance is purely meant to protect the author of a PR, as it is really easy to get sucked in, and only to surface in a few months, with something nobody was looking for :) We have many other PRs being an example of this, sadly. It is a common pitfall in software design :)

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.

4 participants
You can’t perform that action at this time.