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 #10511: Delay 'go to nearest depot' orders #11548

Conversation

SamuXarick
Copy link
Contributor

@SamuXarick SamuXarick commented Dec 5, 2023

Motivation / Problem

#10511

A lone Road Vehicle with a "go to nearest depot" order on a large road network and no depot nearby causes constant calls to the pathfinder every tick, hindering game performance.

This is happening in ProcessOrders and not in IndividualRoadVehicleController as it's been in the previous reported cases with similar symptoms. I think that the ideal solution here would be to call the pathfinder upon reaching the end of a segment, as that would in theory limit the number of calls significantly. But in this case, it really wants to know what's the destination of the order before handling it over to the IndividualRoadVehicleController.

I don't know how to do that, so I thought of a different approach.

Description

Delay the nearest depot order search for a day if the destination has been set to zero, which happens when it has already attempted to do so and failed to find a valid destination.

Closes #10511.

Limitations

It may break other "legitimate" uses for v->dest_tile == 0

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, game_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@glx22

This comment has been minimized.

@SamuXarick SamuXarick force-pushed the optionally-find-closest-depot-within-a-limit branch from b0ac816 to c46407b Compare December 6, 2023 18:46
glx22
glx22 previously approved these changes Dec 6, 2023
Copy link
Contributor

@glx22 glx22 left a comment

Choose a reason for hiding this comment

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

Squash required, but final diff looks good to me.

@SamuXarick SamuXarick force-pushed the optionally-find-closest-depot-within-a-limit branch from c46407b to a6597af Compare December 6, 2023 21:16
@glx22 glx22 dismissed their stale review December 6, 2023 21:46

Seems default penalty is actually too small

@SamuXarick SamuXarick marked this pull request as draft December 6, 2023 22:55
@SamuXarick

This comment has been minimized.

@SamuXarick SamuXarick force-pushed the optionally-find-closest-depot-within-a-limit branch from a6597af to ed89a52 Compare December 7, 2023 13:28
@SamuXarick SamuXarick changed the title Fix #10511: Limit 'go to nearest depot' orders Fix #10511: Delay 'go to nearest depot' orders Dec 7, 2023
@SamuXarick SamuXarick force-pushed the optionally-find-closest-depot-within-a-limit branch from ed89a52 to dcf026b Compare December 7, 2023 13:30
@SamuXarick SamuXarick marked this pull request as ready for review December 7, 2023 13:49
@SamuXarick SamuXarick force-pushed the optionally-find-closest-depot-within-a-limit branch from dcf026b to 1678a5e Compare December 16, 2023 21:09
@SamuXarick
Copy link
Contributor Author

A better savegame with 74 vehicles going to nearest depot.
go to nearest road vehicle depot x74.zip

image

src/order_cmd.cpp Outdated Show resolved Hide resolved
@SamuXarick SamuXarick force-pushed the optionally-find-closest-depot-within-a-limit branch from 1678a5e to c55bbb0 Compare December 22, 2023 15:18
@2TallTyler
Copy link
Member

@SamuXarick This looks fine to me, but can you test your demo savegame and verify it still works, and I haven't given you some wrong code? 😉

@SamuXarick
Copy link
Contributor Author

@SamuXarick This looks fine to me, but can you test your demo savegame and verify it still works, and I haven't given you some wrong code? 😉

TimerGameCalendar::date_fract worked the same as if it was TimerGameTick::counter % Ticks::DAY_TICKS.
One starts from 0 and goes up to 73. On the 74th, it becomes 0 again. The values I'm interested are in the range 0-73.
The other starts from 0 and goes to infinity. Using % 74 assures I am also getting the desired values in the range 0-73.

If there's some other functionality I'm not aware that could skew this check, then I must be aware.

2TallTyler
2TallTyler previously approved these changes Dec 22, 2023
src/order_cmd.cpp Outdated Show resolved Hide resolved
src/order_cmd.cpp Outdated Show resolved Hide resolved
AircraftNextAirportPos_and_Order(a);
/* If the vehicle can't find its destination, delay its next search.
* In case many vehicles are in this state, use the vehicle index to spread out pathfinder calls. */
if (v->dest_tile != 0 || TimerGameCalendar::date_fract == (v->index % Ticks::DAY_TICKS)) {
Copy link
Contributor

@glx22 glx22 Jan 2, 2024

Choose a reason for hiding this comment

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

Suggested change
if (v->dest_tile != 0 || TimerGameCalendar::date_fract == (v->index % Ticks::DAY_TICKS)) {
if (v->dest_tile == 0 && TimerGameCalendar::date_fract != (v->index % Ticks::DAY_TICKS)) break;

should work too without needing to touch indentation.
(Maybe return false instead of break, but I didn't analyse all the function)

Delay the nearest depot order search for a day if the vehicle can't find its destination, which happens when it has already attempted to do so and failed to find a valid destination.
@SamuXarick SamuXarick force-pushed the optionally-find-closest-depot-within-a-limit branch from 75d9d91 to a1efb1f Compare January 2, 2024 18:08
@PeterN
Copy link
Member

PeterN commented Jan 5, 2024

What happens to the road vehicle in this state? Does it just carry on driving randomly?

@glx22
Copy link
Contributor

glx22 commented Jan 5, 2024

Yes for at most a day, it's usually a user error if the vehicle can't determine what is the nearest depot, and the change is just so it has less impact on gameloop.
Before the change the search would happen every tick.

@PeterN PeterN merged commit 847f3f6 into OpenTTD:master Jan 5, 2024
20 checks passed
@SamuXarick SamuXarick deleted the optionally-find-closest-depot-within-a-limit branch February 25, 2024 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Go to nearest road vehicle depot constantly running the pathfinder
4 participants