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

Fix road pathfinder behaviour with speed limits on roadtypes/bridges #8710

Merged
merged 2 commits into from Feb 21, 2021

Conversation

@LordAro
Copy link
Member

@LordAro LordAro commented Feb 21, 2021

Motivation / Problem

#8594

Also noted that road vehicles were always picking wooden bridges over faster ones, regardless of whether the (slightly) longer route was better or not.

Description

Get the speed limit from the roadtype if there is one, same as for railtypes.

Account for bridge length in path cost calculation, same as is done for rail

Limitations

Might change/break some existing setups? Possibly?

Path cache can take a few months to fully clear for some road vehicles.

I don't think it's perfect yet. Road vehicles still seem to pick the an unexpected path sometimes - the wooden bridge is still being chosen more often than I'd expect

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')
@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Feb 21, 2021

Might change/break some existing setups? Possibly?

I would consider this likely for large RV games, and their timetables will be out of whack for sure. So maybe this has to be mentioned clearly in the changelog. But in the end, fixing bugs often means that who-ever was "using" the bug in his game will notice this. It cannot and should not hold us back :)

Path cache can take a few months to fully clear for some road vehicles.

For a moment I wanted to suggest: bump savegame version and clear cache .. but nah .. this should be fine.

I don't think it's perfect yet. Road vehicles still seem to pick the an unexpected path sometimes - the wooden bridge is still being chosen more often than I'd expect

Guess we should make a debug-tool that shows path-cost between two arbitrary points, so we can diagnose path-finder issues better. Not for now of course, but it saves you from doing all that debugging stuff you had to do now. Should be relative easy to implement .. just some food for thought :)

PS: really cool you pick up this problem .. "pathfinder, SCARY" is the most common response, but it is cool that with some tenacity we can fix bugs that might have been there for years :P

@LordAro LordAro merged commit 1d6a0c7 into OpenTTD:master Feb 21, 2021
10 checks passed
@LordAro LordAro deleted the bug8594 branch Feb 21, 2021
@@ -161,8 +161,8 @@ class CYapfCostRoadT
int min_speed = 0;
int max_veh_speed = v->GetDisplayMaxSpeed();
int max_speed = F.GetSpeedLimit(&min_speed);
if (max_speed < max_veh_speed) segment_cost += 1 * (max_veh_speed - max_speed);
if (min_speed > max_veh_speed) segment_cost += 10 * (min_speed - max_veh_speed);
if (max_speed < max_veh_speed) segment_cost += YAPF_TILE_LENGTH * (max_veh_speed - max_speed) * (4 + F.m_tiles_skipped) / max_veh_speed;
Copy link
Contributor

@stormcone stormcone Feb 21, 2021

Choose a reason for hiding this comment

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

Maybe a little bit late, but where the 4 coming from and why? :)

Copy link
Member Author

@LordAro LordAro Feb 21, 2021

Choose a reason for hiding this comment

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

From the rail pathfinder! No, I'm not sure why it's 4 either...

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