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

Jump to bottom

Ship cpu hog workaround for #6145 #6784

Merged
merged 7 commits into from Jan 14, 2019
Merged

Ship cpu hog workaround for #6145 #6784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cc3c511
Change: Don't leave ship depot with no orders.
PeterN May 18, 2018
d4d6de1
Change: Don't pathfind with no destination or if destination is known…
PeterN May 18, 2018
aeacd42
Change: Allow only one ship to leave depot at a time.
PeterN May 18, 2018
8aca131
Codechange: Use const instead of magic number for ship order distance…
PeterN May 20, 2018
392c518
Codechange: Replace trackbit lookup table with existing functions.
PeterN May 23, 2018
70346ef
Change: When ship is lost, pick first available track instead of a ra…
PeterN May 28, 2018
5e4d5f9
Change: Obey forbid_90_deg_turns when choosing available track with n…
PeterN Jan 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/order_cmd.cpp
View file
Open in desktop
Expand Up @@ -23,6 +23,7 @@
#include "core/random_func.hpp"
#include "aircraft.h"
#include "roadveh.h"
#include "ship.h"
#include "station_base.h"
#include "waypoint_base.h"
#include "company_base.h"
Expand Down Expand Up @@ -929,7 +930,7 @@ CommandCost CmdInsertOrder(TileIndex tile, DoCommandFlag flags, uint32 p1, uint3
dist = GetOrderDistance(prev, &new_order, v);
}

if (dist >= 130) {
if (dist >= SHIP_MAX_ORDER_DISTANCE) {
return_cmd_error(STR_ERROR_TOO_FAR_FROM_PREVIOUS_DESTINATION);
}
}
Expand Down
2 changes: 2 additions & 0 deletions src/ship.h
View file
Open in desktop
Expand Up @@ -48,6 +48,8 @@ struct Ship FINAL : public SpecializedVehicle<Ship, VEH_SHIP> {
void UpdateCache();
};

static const uint SHIP_MAX_ORDER_DISTANCE = 130;

/**
* Iterate over all ships.
* @param var The variable used for iteration.
Expand Down
41 changes: 36 additions & 5 deletions src/ship_cmd.cpp
View file
Open in desktop
Expand Up @@ -319,6 +319,14 @@ void Ship::UpdateDeltaXY()
this->z_extent = 6;
}

/**
* Test-procedure for HasVehicleOnPos to check for a ship.
*/
static Vehicle *EnsureNoVisibleShipProc(Vehicle *v, void *data)
{
return v->type == VEH_SHIP && (v->vehstatus & VS_HIDDEN) == 0 ? v : NULL;
}

static bool CheckShipLeaveDepot(Ship *v)
{
if (!v->IsChainInDepot()) return false;
Expand All @@ -330,6 +338,13 @@ static bool CheckShipLeaveDepot(Ship *v)
return true;
}

/* Don't leave depot if no destination set */
if (v->dest_tile == 0) return true;

/* Don't leave depot if another vehicle is already entering/leaving */
/* This helps avoid CPU load if many ships are set to start at the same time */
if (HasVehicleOnPos(v->tile, NULL, &EnsureNoVisibleShipProc)) return true;

TileIndex tile = v->tile;
Axis axis = GetShipDepotAxis(tile);

Expand Down Expand Up @@ -443,11 +458,27 @@ static Track ChooseShipTrack(Ship *v, TileIndex tile, DiagDirection enterdir, Tr

bool path_found = true;
Track track;
switch (_settings_game.pf.pathfinder_for_ships) {
case VPF_OPF: track = OPFShipChooseTrack(v, tile, enterdir, tracks, path_found); break;
case VPF_NPF: track = NPFShipChooseTrack(v, tile, enterdir, tracks, path_found); break;
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

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

/* No destination or destination too far, don't invoke pathfinder. */
track = TrackBitsToTrack(v->state);
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. */
if (_settings_game.pf.forbid_90_deg) {
tracks &= ~TrackCrossesTracks(TrackdirToTrack(v->GetVehicleTrackdir()));
if (tracks == TRACK_BIT_NONE) return INVALID_TRACK;
}
track = FindFirstTrack(tracks);
Copy link
Contributor

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

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

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

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

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

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

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 •
edited

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.

}
path_found = false;
} else {
switch (_settings_game.pf.pathfinder_for_ships) {
case VPF_OPF: track = OPFShipChooseTrack(v, tile, enterdir, tracks, path_found); break;
case VPF_NPF: track = NPFShipChooseTrack(v, tile, enterdir, tracks, path_found); break;
case VPF_YAPF: track = YapfShipChooseTrack(v, tile, enterdir, tracks, path_found); break;
default: NOT_REACHED();
}
}

v->HandlePathfindingResult(path_found);
Expand Down