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 #7062: Remove ship max order distance. #7279

Merged
merged 3 commits into from Mar 31, 2019
Merged

Conversation

@PeterN
Copy link
Member

PeterN commented Feb 25, 2019

It is skipped when NPF is in use.
It is trivial to work around by adding and removing dummy orders.
It is mostly alleviated by the ship path cache in YAPF.

PeterN added 2 commits Feb 25, 2019
It is skipped when NPF is in use.
It is trivial to work around by adding and removing dummy orders.
It is mostly alleviated by the ship path cache in YAPF.
@PeterN

This comment has been minimized.

Copy link
Member Author

PeterN commented Feb 25, 2019

#7272 and #7245 would be useful for this.

@PeterN PeterN marked this pull request as ready for review Mar 3, 2019
@PeterN

This comment has been minimized.

Copy link
Member Author

PeterN commented Mar 3, 2019

Ideally buoys should still be used but with this change there's no reason, from a player POV, to bother. So I'm not sure.

@nielsmh nielsmh added this to the 1.10.0 milestone Mar 3, 2019
@nielsmh

This comment has been minimized.

Copy link
Contributor

nielsmh commented Mar 3, 2019

Putting this on 1.10 since it pretty much requires #7245 to be sensible. I believe one of the major reasons for the max order distance is the original pathfinder.

@PeterN PeterN added the needs review label Mar 7, 2019
@PeterN

This comment has been minimized.

Copy link
Member Author

PeterN commented Mar 7, 2019

Perhaps I should split this into two PRs, one to revert the earlier change, and then the second to remove the distance checks?

Copy link
Member

LordAro left a comment

It's all the same set of changes, I don't see any need to split it any further than per-commit

@LordAro LordAro removed the needs review label Mar 24, 2019
@michicc

This comment has been minimized.

Copy link
Member

michicc commented Mar 24, 2019

#7272 would still make sense, but @PeterN had some outstanding comments on that.

@PeterN

This comment has been minimized.

Copy link
Member Author

PeterN commented Mar 24, 2019

I was thinking the first commit may be a backport candidate.

@PeterN PeterN merged commit f0336f1 into OpenTTD:master Mar 31, 2019
8 checks passed
8 checks passed
OpenTTD CI Build #20190225.7 succeeded
Details
OpenTTD CI (Linux commit-checker) Linux commit-checker succeeded
Details
OpenTTD CI (Linux linux-amd64-clang-3.8) Linux linux-amd64-clang-3.8 succeeded
Details
OpenTTD CI (Linux linux-amd64-gcc-6) Linux linux-amd64-gcc-6 succeeded
Details
OpenTTD CI (Linux linux-i386-gcc-6) Linux linux-i386-gcc-6 succeeded
Details
OpenTTD CI (MacOS) MacOS succeeded
Details
OpenTTD CI (Windows Win32) Windows Win32 succeeded
Details
OpenTTD CI (Windows Win64) Windows Win64 succeeded
Details
@PeterN PeterN deleted the PeterN:fix-7062 branch Mar 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.