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 #5713: Use pathfinder to find closest ship depot #6928

Closed

Conversation

SamuXarick
Copy link
Contributor

When ships are asked to find the closest depot, the depot that is provided is not always reachable. This patch provides the closest reachable ship depot, by utilizing the pathfinder (Original, NPF or Yapf) first. Only if it fails, it would then be provided by the original method, distance manhattan based.

https://www.tt-forums.net/viewtopic.php?f=33&t=76137

Copy link
Contributor

@nielsmh nielsmh left a comment

Choose a reason for hiding this comment

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

I don't like this approach at all. It's far too intrusive and adds far too many special cases to the pathfinding code, which is already very complex. Start over with something simpler that doesn't need to change code in all the pathfinders.

@nielsmh
Copy link
Contributor

nielsmh commented Nov 1, 2018

An idea for an alternate approach could be to insert the logic in ShipController instead. If after ChooseShipTrack the VF_PATHFINDER_LOST flag is set, and the current order is go to depot (and it's not a scheduled order) then select a different depot and try again. It's probably necessary to keep a list of depots already tried (and failed), but it can just be local to the ship controller function.

Of course that won't handle the case of OPF, since that never signals no path found. I would suggest not doing anything about that for now, but maybe make a separate change that keeps track of how far a ship has sailed since it last completed an order, and compare that to the expected distance, and send a warning to the player if the ship appears to be taking too long.

@nielsmh
Copy link
Contributor

nielsmh commented Nov 2, 2018

I tried implementing the approach I outlined above, it's probably not complete like this, but it's significantly shorter and touches much less code: nielsmh@fd90477

@TrueBrain
Copy link
Member

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!

@SamuXarick SamuXarick force-pushed the find-closest-reachable-ship-depot branch from e556519 to b8b7878 Compare January 10, 2019 01:15
@SamuXarick
Copy link
Contributor Author

@nielsmh
I just experimented your approach and it causes stalls during gameplay when there's too many unreachable depots nearby. It calls the pathfinder for everyone of them.

@J0anJosep has already implemented part of what I had in mind for npf, which got into master, and some of the complexity that was on npf.cpp is gone. Unless you mean the other changes...

@SamuXarick SamuXarick force-pushed the find-closest-reachable-ship-depot branch from b8b7878 to e4a026e Compare January 10, 2019 01:48
Copy link
Contributor

@nielsmh nielsmh left a comment

Choose a reason for hiding this comment

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

Still think this looks like too much complexity to attempt to improve a few corner cases.

src/pathfinder/opf/opf_ship.cpp Outdated Show resolved Hide resolved
src/pathfinder/opf/opf_ship.cpp Outdated Show resolved Hide resolved
src/pathfinder/opf/opf_ship.cpp Outdated Show resolved Hide resolved
src/pathfinder/opf/opf_ship.cpp Outdated Show resolved Hide resolved
src/pathfinder/opf/opf_ship.h Outdated Show resolved Hide resolved
src/ship_cmd.cpp Outdated Show resolved Hide resolved
src/ship_cmd.cpp Outdated Show resolved Hide resolved
src/pathfinder/opf/opf_ship.cpp Outdated Show resolved Hide resolved
src/pathfinder/opf/opf_ship.cpp Outdated Show resolved Hide resolved
src/pathfinder/opf/opf_ship.cpp Outdated Show resolved Hide resolved
@SamuXarick SamuXarick force-pushed the find-closest-reachable-ship-depot branch 3 times, most recently from 3060a64 to a5c5df2 Compare January 12, 2019 02:49
@SamuXarick
Copy link
Contributor Author

NoNoCAB v5 (O) 1.7.2.zip
Savegame with 5000 ships for testing

src/pathfinder/npf/npf.cpp Show resolved Hide resolved
src/pathfinder/opf/opf_ship.cpp Outdated Show resolved Hide resolved
src/ship_cmd.cpp Outdated Show resolved Hide resolved
@SamuXarick SamuXarick force-pushed the find-closest-reachable-ship-depot branch from a5c5df2 to 74e3985 Compare January 13, 2019 03:07
@SamuXarick
Copy link
Contributor Author

@SamuXarick SamuXarick force-pushed the find-closest-reachable-ship-depot branch from 4ba1fe3 to 7a9d5c1 Compare January 17, 2019 14:32
@nielsmh nielsmh dismissed their stale review January 19, 2019 12:36

Code issues resolved

Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

I'm with @nielsmh on this - this is far too much code to fix this very minor issue. Perhaps @nielsmh's method could be limited to try the first N (where N is small) number of depots before it reverts to the old Manhattan method?

Also there's currently conflicts

When ships are asked to find the closest depot, the depot that is provided is not always reachable. This patch provides the closest reachable ship depot, by utilizing the pathfinder (Original, NPF or Yapf) first. Only if it fails, it would then be provided by the original method, distance manhattan based.
@SamuXarick SamuXarick force-pushed the find-closest-reachable-ship-depot branch from 7a9d5c1 to 9ad74e8 Compare January 20, 2019 20:59
@SamuXarick
Copy link
Contributor Author

Just resolved conflicts

@andythenorth
Copy link
Contributor

This one appears to be stuck, and reviewers are finding it difficult to providing constructive reviews. Also it appears difficult to be confident that the solutions proposed are correct. On this basis I am closing this one. Thanks for contributing!

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

5 participants