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: Improve graph period markings #8732

Merged
merged 1 commit into from Feb 24, 2021
Merged

Conversation

@2TallTyler
Copy link
Member

@2TallTyler 2TallTyler commented Feb 23, 2021

Motivation / Problem

Periods of graphs are hard to read, due to lots of small text.

graph_period_current

Description

This PR simplifies the text and adds brighter grid lines separating each year.

Closes #8632.

graph_period_proposed

There is also some code cleanup of colour definitions which I missed in #8690.

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')
@ldpl
Copy link
Contributor

@ldpl ldpl commented Feb 23, 2021

Yeah, that's better. Also, I like to look at highcharts for inspiration as it's pretty good at doing charts. Here is a somewhat similar chart for example image

So one thing to consider is centering labels with their grid lines. Centering year label on the whole year may not be a bad idea as well.

Another thing to notice is that highcharts rarely do vertical grid lines, so that it mb a good idea to remove them as well to reduce visual clutter. At least quarterly lines can go, yearly ones are more than enough to clearly see periods.

@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Feb 23, 2021

I didn't center labels on their column because they skip three months at a time. Left-align labels seem to denote the left grid line as the start of that three-month data period without confusing the user who might think the data point is only January data, for example.

I tried the Roman numerals suggested in #8690 but it was a bit messy. Financial quarter markings (Q1, etc) would have been nice but fiscal years IRL don't often match calendar years and differ by country.

Centering years might look odd when you're partway through a year, and would have to center on however many periods exist.

I'm not opposed to eliminating vertical grid lines, but right now my skills support iterative improvement much better than reinventing the wheel. 😀 I'd welcome some other opinions before I go changing long-established things left and right.

@ldpl
Copy link
Contributor

@ldpl ldpl commented Feb 23, 2021

I didn't center labels on their column because they skip three months at a time. Left-align labels seem to denote the left grid line as the start of that three-month data period without confusing the user who might think the data point is only January data, for example.

I meant centering labels on the grid line, not on the column/data point itself. Having the point itself in the middle to mean the period is fine. Highcharts image I posted is a monthly chart so ofc I don't suggest copying it completely.

Copy link
Member

@TrueBrain TrueBrain left a comment

I really appreciate you thought this through; the additional information you give in your comment would be nice to have in the motivation next time, but creating PRs is also an iterative process :D

Personally:

  • The lighter lines per year are a great addition, much more readable.
  • Removing "Mar", "Jun", "Sep" and "Dec" does not yield any loss of data, so I really like that too. Less is more :)

So +1 for me! (and of course, @ldpl has some nice suggestions to further improve it, but I am so-so if that should be done in this PR or in a later; for me, that is up to you :))

src/graph_gui.cpp Outdated Show resolved Hide resolved
@2TallTyler 2TallTyler force-pushed the 2TallTyler:graph_periods branch from 3a7a5ef to 8b3449b Feb 23, 2021
@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Feb 23, 2021

Ah, I misunderstood your centering suggestion.

Centered labels would mean the very first month (Apr) would collide with the text for $0. Even if that were solved, I am not a fan of centered text in this case, but won't argue against it if you or someone else makes a PR to change it in the future.

@TrueBrain Separate PRs are good, I think. Ready for review. 😄

@ldpl
Copy link
Contributor

@ldpl ldpl commented Feb 23, 2021

Tried to implement what is suggested: https://pastebin.com/DnK9QPij (also removes vert lines in cargo payment graph but I think it's fine)
Screenshot from 2021-02-23 16-17-25

Also noticed a bug that if the font is large enough year gets clipped (it's been there before as well).

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Feb 23, 2021

Tried to implement what is suggested: https://pastebin.com/DnK9QPij (also removes vert lines in cargo payment graph but I think it's fine)
Screenshot from 2021-02-23 16-17-25

Also noticed a bug that if the font is large enough year gets clipped (it's been there before as well).

Personally, if you go this route, you should also move the points to the left. Now you see Jan in the middle of the line, but on the line is no dot. This is a bit to the right :D Highcharts does this better, and have them both center on each other.

Edit: correction, my issue is not with the location of the dot, but with the fact it is a dot while it is a value of the whole quarter. But that is a completely different can of worms :P

@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Feb 23, 2021

I like the centered years, but am still not a fan of centered months.

Edit: I don't disagree with TB that the points could be moved to the vertical grid lines (which are no longer drawn) but then it almost looks like the data points are cherry-picked from January, April, July, and October, with the months in between discarded, rather than taking the average of each three-month period.

To keep vertical lines in the cargo payment graph, just put the vertical line code inside
if (this->month != 0xFF) {}.

@2TallTyler 2TallTyler force-pushed the 2TallTyler:graph_periods branch from 8b3449b to 17d5c40 Feb 23, 2021
@2TallTyler 2TallTyler force-pushed the 2TallTyler:graph_periods branch from 17d5c40 to 74d1808 Feb 23, 2021
@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Feb 23, 2021

I did some experimentation with @ldpl's suggestions and ran into some design issues. I'd like to move forward as-is and leave those for a future PR.

  • With left-aligned month labels, removing vertical grid lines feels disorienting to me.
  • Centered year labels are problematic when a year end enters the chart at right, or disappears at left. Aligning them with January isn't something I've seen on other graphs, but on the wonky OpenTTD graphs I feel it actually works well. Again, something for a future PR.

I did fix one more colour constant and included my changes in UpdateWidgetSize, which I had forgotten before.

Copy link
Member

@LordAro LordAro left a comment

LGTM. Are there eints considerations regarding the translations?

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Feb 24, 2021

I checked with boss-man, there is not.

@TrueBrain TrueBrain merged commit 8476f12 into OpenTTD:master Feb 24, 2021
11 checks passed
11 checks passed
Emscripten
Details
Commit checker
Details
Check preview needs update Check preview needs update
Details
Linux (clang, clang++, libsdl2-dev)
Details
Linux (gcc, g++, libsdl2-dev)
Details
Linux (gcc, g++, libsdl1.2-dev)
Details
Mac OS (x64, x86_64)
Details
Windows (windows-latest, x86)
Details
Windows (windows-latest, x64)
Details
Windows (windows-2016, x86) Windows (windows-2016, x86)
Details
Windows (windows-2016, x64) Windows (windows-2016, x64)
Details
@2TallTyler 2TallTyler deleted the 2TallTyler:graph_periods branch Feb 24, 2021
@2TallTyler 2TallTyler restored the 2TallTyler:graph_periods branch Feb 24, 2021
@2TallTyler 2TallTyler deleted the 2TallTyler:graph_periods branch Feb 27, 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.

4 participants