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

Add #6410: [YAPF] Use a max penalty for finding the nearest road vehicle depot #6929

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@SamuXarick
Copy link
Contributor

SamuXarick commented Oct 1, 2018

Implemented a maximum path cost penalty when searching for the closest road vehicle depot for YAPF
#6410

Add: [YAPF] Use a max penalty for finding the nearest road vehicle depot
Implemented a maximum path cost penalty when searching for the closest road vehicle depot for YAPF

@SamuXarick SamuXarick changed the title Add: [YAPF] Use a max penalty for finding the nearest road vehicle depot Add #6410: [YAPF] Use a max penalty for finding the nearest road vehicle depot Oct 2, 2018

@PeterN

This comment has been minimized.

Copy link
Member

PeterN commented Oct 2, 2018

What does this solve?

@SamuXarick

This comment has been minimized.

Copy link
Contributor

SamuXarick commented Oct 2, 2018

Apparently it changes nothing, but makes the pathfinder quit earlier if it goes past the max_penalty.

Seems that the penalty is only used for automatic servicing. Manual sending to depot still uses unlimited penalty.

@J0anJosep

This comment has been minimized.

Copy link
Contributor

J0anJosep commented Oct 2, 2018

@SamuXarick Yes, it leaves earlier and the max_distance penalty is used. For consistency, either max_distance should be used or removed from the code. I forgot to add a patch for it, but I wrote them two years ago (YAPF and NPF):
J0anJosep@d6a311b
J0anJosep@3680d95
Sorry for forgetting about it. Double work done here.

Anyway, I don't see why a change in max_distance is needed.

@SamuXarick

This comment has been minimized.

Copy link
Contributor

SamuXarick commented Oct 2, 2018

Because

/**
* Used when user sends road vehicle to the nearest depot or if road vehicle needs servicing using YAPF.
* @param v vehicle that needs to go to some depot
* @param max_penalty max distance (in pathfinder penalty) from the current vehicle position
* (used also as optimization - the pathfinder can stop path finding if max_penalty
* was reached and no depot was seen)
* @return the data about the depot
*/
FindDepotData YapfRoadVehicleFindNearestDepot(const RoadVehicle *v, int max_penalty);

Oh, I did that for NPF too, included it in my other patch request #6928

e556519#diff-fe68d910d6c0b0a8d81eb65e88e60fb1

@J0anJosep

This comment has been minimized.

Copy link
Contributor

J0anJosep commented Oct 2, 2018

Because

OpenTTD/src/pathfinder/yapf/yapf.h

Line 70 in 25a060b
FindDepotData YapfRoadVehicleFindNearestDepot(const RoadVehicle *v, int max_penalty);

Oh, I did that for NPF too, included it in my other patch request #6928

e556519#diff-fe68d910d6c0b0a8d81eb65e88e60fb1

In the file you referenced, both max_distance and max_penalty are used and their meaning is the same. It is explained in the comments. If it is an inconsistency, it shouldn't be dealt here but in a separate commit.

@TrueBrain

This comment has been minimized.

Copy link
Member

TrueBrain commented Jan 5, 2019

We recently switched from Jenkins as CI to Azure Pipelines as CI. This means you need to rebase before this Pull Request will pass its checks. Sorry for the troubles!

@andythenorth

This comment has been minimized.

Copy link
Contributor

andythenorth commented Jan 11, 2019

Same issue is addressed by #7009, which is merged.

@SamuXarick SamuXarick deleted the SamuXarick:max-penalty-for-yapf-find-closest-road-vehicle-depot- branch Jan 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment