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

Ship cpu hog workaround for #6145 #6784

Merged
merged 7 commits into from Jan 14, 2019

Conversation

PeterN
Copy link
Member

@PeterN PeterN commented May 18, 2018

Set of small changes that attempt to mitigate excessive CPU consumption of ships when pathfinding.

@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented May 19, 2018

This looks like it also addresses #6145 ?

src/ship_cmd.cpp Outdated Show resolved Hide resolved
src/ship_cmd.cpp Outdated Show resolved Hide resolved
if (track != TRACK_X && track != TRACK_Y) track = TrackToOppositeTrack(track);
if (!HasBit(tracks, track)) {
/* Can't continue in same direction so pick first available track. */
track = FindFirstTrack(tracks);
Copy link
Contributor

@J0anJosep J0anJosep May 28, 2018

Choose a reason for hiding this comment

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

Does it take into account "forbid 90 degrees turns"?
Anyway, I would prefer leaving the ship stopped (stuck) instead of going to any direction.

Copy link
Member Author

@PeterN PeterN May 29, 2018

Choose a reason for hiding this comment

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

Probably not but 'forbid 90 degree turns' is a bad feature for ships anyway.
No other vehicle type gets stopped when lost, so that would be inconsistent.

Copy link
Contributor

@J0anJosep J0anJosep May 30, 2018

Choose a reason for hiding this comment

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

I don't consider 'forbid 90 degree turns' for ships a bad feature and it would be great the PR takes that into account, but you are right about the inconsistency of stopping a lost vehicle.

Copy link
Contributor

@nielsmh nielsmh Jun 9, 2018

Choose a reason for hiding this comment

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

I think I would prefer if ships that had all orders deleted while out of depot just stopped in place, and only lost ships kept moving in a random direction.

Copy link
Member

@TrueBrain TrueBrain Jul 19, 2018

Choose a reason for hiding this comment

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

I noticed that YAPF also has a 'forbid 90 degree' for ships; would it be difficult to add it here? I agree that ships are weird anyway, but to keep some consistency would be nice. But it has to be reasonable. So if it is 2 lines of code, I think we should add it. If it is 6 days of work, I don't think it is worth it :)

Copy link
Contributor

@andythenorth andythenorth Jul 19, 2018

Choose a reason for hiding this comment

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

'Forbid 90 degree' in YAPF is the cause of routing bugs for ships, they turn the wrong way from dock and get lost in rivers.

Copy link
Member

@TrueBrain TrueBrain Jul 23, 2018

Choose a reason for hiding this comment

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

Just to keep this conversation clear: good remark, but that shouldn't keep us from trying to keep things consistent; better to have it broken everywhere, than not implemented at some places :) Given YAPF implements it (how ever broken it might be), means we should, given it can be done in a reasonable amount of time, take it into account too :)

Copy link
Contributor

@J0anJosep J0anJosep Jul 25, 2018

Choose a reason for hiding this comment

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

I haven't look thoroughly, but it seems OPF doesn't take into account "forbid_90_deg_turns". Maybe these lines of code after line 461 or 465 would do the trick:

if (_settings_game.pf.forbid_90_deg && _settings_game.pf.pathfinder_for_ships != VPF_OPF) {
    tracks &= (TrackdirBits)~TrackCrossesTracks(TrackdirToTrack(v->GetVehicleTrackdir()));
}

I haven't checked whether this works or not. And it would need to check if tracks == TRACKBIT_NONE after that.

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Aug 11, 2018

Friendly poke. Any chance the final straw will be tackled? Tnx!

@TrueBrain
Copy link
Member

@TrueBrain 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 andythenorth added the stale label Jan 6, 2019
@PeterN PeterN force-pushed the ship-cpu-hog-workaround-bug6145 branch from a95d6cd to 325765f Compare Jan 13, 2019
@andythenorth andythenorth removed the stale label Jan 13, 2019
@PeterN PeterN force-pushed the ship-cpu-hog-workaround-bug6145 branch from 355c242 to 5e4d5f9 Compare Jan 14, 2019
@LordAro LordAro merged commit 6b0a467 into OpenTTD:master Jan 14, 2019
1 check passed
case VPF_YAPF: track = YapfShipChooseTrack(v, tile, enterdir, tracks, path_found); break;
default: NOT_REACHED();

if (v->dest_tile == 0 || DistanceManhattan(tile, v->dest_tile) > SHIP_MAX_ORDER_DISTANCE + 5) {
Copy link
Contributor

@SamuXarick SamuXarick Jan 14, 2019

Choose a reason for hiding this comment

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

Fentwood Transport, 1954-11-03.zip

This ship becomes lost with this patch applied, but it's not lost in 1.8.0

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.

None yet

8 participants