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

Change: Don't apply forbid 90 deg turn settings to ships. #7232

Merged
merged 1 commit into from Mar 3, 2019

Conversation

@PeterN
Copy link
Member

PeterN commented Feb 14, 2019

Is forbidding 90 degree turns for ships useful? Especially now that ships have extra handling for rotating on the spot.

@J0anJosep
Copy link
Contributor

J0anJosep commented Feb 14, 2019

If it is not useful, it may be disabled by users.

It works as expected right now. I see no problem with it. I see more problems with OPF.

I use this feature and I like to use it with other patches I created. Can we keep this one? I see no problem in keeping it. Probably, the setting should be converted into two different settings (trains and ships), but that is another different thing that shouldn't be discussed on this PR.

@PeterN
Copy link
Member Author

PeterN commented Feb 15, 2019

I've been working on adding a penalty for 90 degree turns instead, so that ships prefer not to, but still can.

@glx22
Copy link
Contributor

glx22 commented Feb 15, 2019

Forbid 90° was a train thing anyway. It's the only transport type where it makes sense.

@PeterN
Copy link
Member Author

PeterN commented Feb 15, 2019

Ok, so adding a penaltly for 90° turns only works when TrackDir is used as NodeKey instead of ExitDir, which is apparently a performance option. I think it'll mean it will search more tile combinations, which is not ideal. Might not be particularly problematic with the ship path cache now, though.

@PeterN
Copy link
Member Author

PeterN commented Feb 15, 2019

This latest change only works properly with pf.yapf.disable_node_optimization on

@J0anJosep
Copy link
Contributor

J0anJosep commented Feb 16, 2019

Isn't it better to address path penalties in a separate branch? One thing is removing 90 deg setting for ships and another is add path penalties.

@PeterN
Copy link
Member Author

PeterN commented Feb 16, 2019

Yes, you are right, I got carried away. I will split that off.

@TrueBrain
Copy link
Member

TrueBrain commented Mar 3, 2019

Make this a non-draft and lets merge it! (or possibly for 1.10?)

Side question: possibly we can also rename the variable to indicate it is trains only? Not worth it if it is a lot of effort, but it might avoid this conversation in the future :)

@PeterN PeterN marked this pull request as ready for review Mar 3, 2019
@TrueBrain TrueBrain added this to the 1.10.0 milestone Mar 3, 2019
@TrueBrain TrueBrain merged commit 3f32711 into OpenTTD:master Mar 3, 2019
8 checks passed
8 checks passed
OpenTTD CI Build #20190216.21 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

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