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 #9523: Display icon or text in vehicle list or in vehicle window indicating whether a vehicle is lost #9543

Merged

Conversation

@SamuXarick
Copy link
Contributor

@SamuXarick SamuXarick commented Sep 5, 2021

Motivation / Problem

#9523

Description

While VF_PATHFINDER_LOST is set for a vehicle, a warning icon is displayed on vehicle window and vehicle list. On the vehicle window, status bar text will change from "heading to" to "can't reach".

Aberhattan Springs Transport, 1935-03-03

Limitations

I am reusing a sprite! Pretty sure this is not the way to go.

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')
@SamuXarick SamuXarick force-pushed the vehicle-is-lost-in-vehicle-guis branch from 2b109e9 to 12a066a Sep 5, 2021
@glx22
Copy link
Contributor

@glx22 glx22 commented Sep 5, 2021

Reusing a sprite is totally fine here.

Loading

Copy link
Member

@LordAro LordAro left a comment

LGTM, other than a couple of minor points

Loading

src/vehicle_gui.cpp Outdated Show resolved Hide resolved
Loading
src/lang/english.txt Outdated Show resolved Hide resolved
Loading
…cle window indicating whether a vehicle is lost
@SamuXarick SamuXarick force-pushed the vehicle-is-lost-in-vehicle-guis branch from 12a066a to 5616fba Sep 7, 2021
@TrueBrain TrueBrain added this to the 12.0 milestone Sep 12, 2021
@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Sep 14, 2021

Are you happy with the result? Can we un-draft this? :D

Loading

@SamuXarick
Copy link
Contributor Author

@SamuXarick SamuXarick commented Sep 14, 2021

It's a bit messed up. It doesn't immediately pick up the status when the route is fixed. And in some cases, it can reach the target while still saying that it cannot be reached. I did a few testings with trains and a missing single track that gets fixed meanwhile, and it took the train some time to clear up the train is lost status.

Loading

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Sep 14, 2021

It's a bit messed up. It doesn't immediately pick up the status when the route is fixed. And in some cases, it can reach the target while still saying that it cannot be reached. I did a few testings with trains and a missing single track that gets fixed meanwhile, and it took the train some time to clear up the train is lost status.

Is this because the GUI doesn't update? Or because the PF just caches information and takes a bit of time to notice the track is restored? The first we can fix. The second .. I personally can accept that as a "known-bug". We cannot instantly update all trains if you put down a single rail :) I think the benefit of this PR out wages the possible questions that might give.

Loading

@SamuXarick
Copy link
Contributor Author

@SamuXarick SamuXarick commented Sep 14, 2021

The second.

Loading

@SamuXarick SamuXarick marked this pull request as ready for review Sep 14, 2021
@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Sep 14, 2021

beep beep IRC to GitHub bot here:

<TrueBrain> Samu: not sure what others think, but I fully expected the icon to remain even after the route was fixed .. that is just how OpenTTD works :D
<TrueBrain> the news also remains despite the route being fixed
<TrueBrain> for me that really is not an issue .. but others might disagree :)
<TrueBrain> having some indication is already widely better than nothing, even if it can be there while the route is fixed :)
<FLHerne> TrueBrain: I think it's a bug that the news doesn't vanish too
<FLHerne> but yeah, even with this bug it's a big improvement
<_dp_> train stops being lost when it finds a path, not when tracks are "fixed", train may be on the other side of the map by that point
<_dp_> also would be very helpful if it also showed somewhere exact tile where it became lost
<_dp_> but, yeah, any indication is better than no idication

In other words, yes, this PR would "introduce a bug", but not one we might want to solve in this PR :)

Loading

@TrueBrain TrueBrain merged commit a57c2b0 into OpenTTD:master Sep 14, 2021
16 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants