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 #7226: No ship track due to "forbid 90 deg turns"-> Do not call pathfinders. #7230

Merged
merged 1 commit into from Feb 18, 2019

Conversation

@J0anJosep
Copy link
Contributor

J0anJosep commented Feb 14, 2019

Don't call ship pathfinders if there is no available track due to "forbid 90 deg turns".

Don't know if OPF will be affected in a negative way by this patch. Probably it forbids 90 degrees turns for this pathfinder, and it may be unable of finding routes.

… due to "forbid 90 deg turns".
@J0anJosep
Copy link
Contributor Author

J0anJosep commented Feb 14, 2019

Couldn't trigger the assert with trains. The 90 deg turns were already checked before calling any pathfinder. But ships didn't check the 90 deg turns setting before calling them.

@PeterN
Copy link
Member

PeterN commented Feb 14, 2019

Why can we not call the pathfinder depending on a pathfinder setting? I don't see why it's not the pathfinders responsibility to say there's no path at this point.

@J0anJosep
Copy link
Contributor Author

J0anJosep commented Feb 14, 2019

It could be up to the pathfinder to return INVALID_TRACKDIR to say there is no path ahead, there is no problem with that.

But most controllers (ship and train at least) don't call the pathfinder if there is no available option (if there is only one track or no track at all, they don't call the pathfinder because there is no necessity of doing so). The train controller checks for the 90 deg turns setting before executing its pathfinders. This commit tries to be consistent with the approach in train_cmd and part of the code in ship_cmd.

So, I think it is better to be consistent with the approach in train_cmd. Moreover, we can keep the asserts on NPF by now, to ensure everything on the pathfinder (or in previously executed functions) work as expected.

I know the asserts on NPF are really tight, but those asserts are a good way of finding out situations where NPF (or something outside the pathfinder) don't work well. I would keep them for the time being and expect bug reports. If someone finds situations where it is necessary to remove them, they will be removed in the future, but not yet.

To sum up, those asserts are there to find out pathfinder bugs instead of sweeping them under the carpet. And in my opinion, this approach is consistent with the existing code and gives NPF the responsibility of throwing an assert in some cases where something goes wrong.

@PeterN
Copy link
Member

PeterN commented Feb 14, 2019

Thanks for explaining.

@PeterN
Copy link
Member

PeterN commented Feb 16, 2019

Hmm, yes, OPF does ignore the forbid 90 deg turns setting. Wonder what the effect is.

@J0anJosep
Copy link
Contributor Author

J0anJosep commented Feb 16, 2019

Well... I have tested OPF with this patch and it works well with a big savegame I have with lots of ships. Moreover, now OPF doesn't ignore the forbid 90 deg turns setting.

@michicc michicc merged commit 6ca637b into OpenTTD:master Feb 18, 2019
8 checks passed
8 checks passed
OpenTTD CI Build #20190214.6 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
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

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