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

Improve pathfinder behaviour for finding road depots (fix #7001; see #6410, #6928, #6929) #7009

Merged
merged 5 commits into from Jan 6, 2019

Conversation

@J0anJosep
Copy link
Contributor

J0anJosep commented Jan 4, 2019

This PR tries to fix that:

  • Road vehicles can reverse at standard road stops.
  • Road vehicles can reverse on some occasions, but not necessarily can reverse when they want to find a road depot. Fix that for NPF.
  • Use the max penalty when trying to find a depot when appropriate (for automatic servicing; user-requested find-a-depot won't use the max penalty).

Also, some minor spelling mistakes on comments.

Please, note that this PR needs an accurate review.

Fix #7001
See #6410 #6928 #6929

@J0anJosep

This comment has been minimized.

Copy link
Contributor Author

J0anJosep commented Jan 4, 2019

Road vehicles CAN turn around. But NPF didn't reverse the vehicle when the best depot was found on the reversed dir.
I will change the PR and take this into account.

@LordAro

This comment has been minimized.

Copy link
Member

LordAro commented Jan 4, 2019

Probably a good idea to test this on some large savegames to check for no unexpected changes in behaviour. Maybe an OTTDCoop save?

@J0anJosep

This comment has been minimized.

Copy link
Contributor Author

J0anJosep commented Jan 4, 2019

Even as road vehicles can turn around, I wouldn't check two-way for road vehicles when looking for a depot:

  • If road vehicles turn around for finding a depot, why they don't try to turn around when finished loading on a go-through road stop?
  • Road vehicles cannot turn around on tunnels or bridges, and that would be an issue if a road vehicle on a bridge tried to find a depot.
  • When changing the orders of a vehicle (or just skipping and order), the vehicle controller doesn't check for the opposite direction.
  • Trams on straight tracks. A tram on a straight track can reverse if the user asks for it (even when there is no rail to reverse), but it looks ugly. Allowing vehicles to reverse when trying to find a depot would show this ugly detail.

I think that disabling two-way checks (as in the current PR) will simplify the whole issue. But I am not fully convinced of this solution. It would be ok that road vehicles checked the opposite direction in some cases, but I don't know whether it is worth it.

Copy link
Member

LordAro left a comment

The commit messages could possibly be improved ("[NPF] NPF..." seems a bit odd), but the code itself looks fine, to my eyes

Copy link
Contributor

nielsmh left a comment

Please fix the commit messages (rebase -i).

The first commit message talks about "road vehicles" when it should be "road stations". (Road vehicles can never be tiles.)

The last commit should be "Docs" rather than "Fix", suggestion: Docs: Fix spelling in some comments

Also try to find a way to not repeat YAPF and NPF in the other commit messages, it looks very silly with it first in a tag and then first word in the sentence.

@J0anJosep J0anJosep force-pushed the J0anJosep:DepotFixes branch from 8770d89 to a3bdbe8 Jan 6, 2019
@nielsmh
nielsmh approved these changes Jan 6, 2019
@nielsmh nielsmh merged commit effb7da into OpenTTD:master Jan 6, 2019
1 check passed
1 check passed
OpenTTD CI Build #20190106.5 succeeded
Details
@J0anJosep J0anJosep deleted the J0anJosep:DepotFixes branch Jan 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.