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: [Linkgraph] Show a tooltip with statistics when hovering a link #9760

Merged
merged 4 commits into from
Oct 18, 2022

Conversation

nchappe
Copy link
Contributor

@nchappe nchappe commented Dec 22, 2021

Motivation / Problem

Cargodist considers many parameters to determine how cargo should be routed.
These parameters are not all exposed to the player:
the linkgraph overlay shows the saturation of links, but there is nothing
for the capacities, or for the travel times.

Description

This PR shows a tooltip when a link of the overlay is hovered.
The tooltip contains the source and destination stations, the link capacity, its usage and its average travel time.
For bidirectional links, data are shown for both directions of travel.

I believe this could be useful for various reasons:

  • The station names help differentiate close but distinct links, and clarify which color corresponds to each direction, mitigating Linkgraphs don't show direction of each link #9167.
  • The link usage is expressed as a percentage, which is more precise than the color code. It mitigates Linkgraphs are not colourblind-friendly #9168 too.
  • When e.g. adding a vehicle to a saturated route, it is interesting to see how the capacity of the link evolves.
  • Travel times can help players update their timetables after replacing vehicles.
  • It can help debugging linkgraph code (that was actually my motivation to write the first version of this patch a few months ago)

Limitations

Corner cases

  • The tooltip is only shown if only one cargo type is selected. Showing information for all cargo types would need a huge tooltip.
  • For the same reason, in case several links cross, the tooltip only shows information for one of them.
  • As all cargo types are always shown in the minimap, it does not benefit from this feature.

Limitations

I had to declare a C-like string as static because TooltipsWindow segfaults if its given string has a temporary RAW_STRING argument that does not outlive the TooltipsWindow.

A possible alternative would be to add a GetString call in the constructor of TooltipsWindow and store the resulting string once and for all in a buffer. This is how JGR's patch pack solves the same problem.

Alternatives

A window would be more flexible than a tooltip, but it would raise many questions about its layout, etc.
I preferred to keep this PR relatively simple.

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 touches english.txt or translations? Check the guidelines
  • 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')

Copy link
Contributor

@nielsmh nielsmh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things I'm worried about (having not tested the code):

Is the LinkGraphOverlay::ShowTooltip sufficiently fast on big networks?

Does this even work when the tooltip delay is zero so you need to hold RMB to show them?

src/linkgraph/linkgraph_gui.cpp Outdated Show resolved Hide resolved
src/linkgraph/linkgraph_gui.h Outdated Show resolved Hide resolved
@nchappe
Copy link
Contributor Author

nchappe commented Dec 23, 2021

Thanks for the feedback!
I fixed the coding style and RMB tooltips. For this one I had to change a bit the mouse handling logic of window.cpp, I'm not entirely sure what I did is valid so I put it in a separate commit.

On the performance of LinkGraphOverlay::ShowTooltip, its complexity is linear in the number of links for the selected cargo type.
I agree this is a bit much for large networks, so now hover events are only generated every _settings_client.gui.hover_delay_ms when the cursor is static (they were generated at each frame before). With the default setting of 250ms, ShowTooltip will be called at most 4 times per second.

@2TallTyler
Copy link
Member

Might we get a Preview label to make testing this easier?

@nielsmh nielsmh added the preview This PR is receiving preview builds label Dec 23, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-9760 December 23, 2021 14:26 Inactive
@nielsmh
Copy link
Contributor

nielsmh commented Dec 23, 2021

Examples of the tooltip:
image
image

It appears to only work when you have exactly one cargo type selected in the cargo flow legend, if I select more than one cargo type the tooltips won't show up.

@DorpsGek DorpsGek temporarily deployed to preview-pr-9760 December 23, 2021 16:13 Inactive
@nchappe
Copy link
Contributor Author

nchappe commented Dec 23, 2021

It appears to only work when you have exactly one cargo type selected in the cargo flow legend, if I select more than one cargo type the tooltips won't show up.

I just pushed a new version that displays statistics for the most saturated one when a link has several cargo types. This is already how the color to use for a link is determined, so the two will always match.

@2TallTyler
Copy link
Member

In my opinion, this and #9866 close #9167 and #9168.

One point of minor confusion for me regarding links which are over capacity — e.g. 219 passengers transported back (547% of capacity) in 10 days.

To me, this reads like 219 passengers is 547% of capacity. Obviously it's not, but perhaps there's a better way to word this. What about 219 passengers transported back in 10 days (547% of capacity)?

@nchappe
Copy link
Contributor Author

nchappe commented May 15, 2022

219 passengers/month is 547% of capacity. Even without this PR, all dark red links in the overlay correspond to links with a 200%+ usage/capacity ratio.

The "capacity" of a link is only a rough estimate, based on the capacity of the vehicles that took the link in the past few months.
As for the usage of a link, it is also approximative, and it's a bit more complex as the value used is actually max(planned, usage), with 'planned' an (even rougher) estimate of the future demand based on the number of passengers waiting for this link.

By construction, the estimated usage of a link never exceeds 100% of its capacity, but the demand can, thus the occasional >100% usage ratios. This is suboptimal, but reworking the formulae to be more accurate is definitely out of scope for this PR. I may give it a try at some point in the future though :)

In any case, I realize the tooltip text is a bit confusing as is.
I read your proposed wording as '219 passengers crossed this link the last 10 days' when the intended meaning of the tooltip is actually 'about 219 passengers cross this link each month, and the journey takes them 10 days on average'.
Maybe 219 passengers transported back per month (547% of capacity) in an average of 10 days would be less ambiguous? I'm open to alternative wording suggestions!

@Moth-Tolias
Copy link

it looks like there are two different stats trying to be condensed into a single line here.
how does this sound?
219 passengers (547% of current capacity) to be transported back per month
average return journey time: 10 days

that stops it from sounding like more than 100% capacity is being transported, and also helps prevent capacity and time from being confused.

@DorpsGek DorpsGek temporarily deployed to preview-pr-9760 July 22, 2022 22:14 Inactive
@nchappe
Copy link
Contributor Author

nchappe commented Jul 22, 2022

Thanks for the suggestions. I slightly reworded the tooltip:

X passengers to be transported per month from A to B (x% of capacity)
Y passengers to be transported back (y% of capacity)
Average travel time: n days

The travel time is now on a separate line, and is an average for the two directions of travel.

@2TallTyler
Copy link
Member

The to be transported helps a lot, indicating demand versus actual stats.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview This PR is receiving preview builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants