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

Reverse at signal timeouts occur unexpectedly quickly, affects title game #7159

Closed
JGRennison opened this issue Feb 1, 2019 · 6 comments
Closed
Labels
regression It used to work, and now it's broken.
Milestone

Comments

@JGRennison
Copy link
Contributor

Version of OpenTTD

master: from commit e934f09 onwards

Expected result

When _settings_game.pf.reverse_at_signals is true, reversing of trains at a red signal occurs after the interval given by _settings_game.pf.wait_oneway_signal or _settings_game.pf.wait_twoway_signal.

Actual result

When _settings_game.pf.reverse_at_signals is true, reversing of trains at a red signal occurs at a shorter interval than expected.

Steps to reproduce

Watch the title game for a few minutes, diesel trains going over the bridge at the top occasionally reverse on the bridge or in the the signal block prior to the bridge, due to moderate delays which are less than that given by _settings_game.pf.wait_oneway_signal. This did not occur prior to commit e934f09 and is probably undesirable in the title game.

@PeterN PeterN added the regression It used to work, and now it's broken. label Feb 1, 2019
@PeterN
Copy link
Member

PeterN commented Feb 1, 2019

Seems to ultimately be caused by separate things using the same counter?

@J0anJosep
Copy link
Contributor

Is it caused by #7114?

@JGRennison
Copy link
Contributor Author

Seems to ultimately be caused by separate things using the same counter?

Train::wait_counter is only used for one thing in this case.
It looks to me like the change in e934f09 would cause Train::wait_counter to be incremented every tick as the signal is now checked every tick, whereas previously some progress would have to be made between checks. Adjusting the scaling constants in the tests with _settings_game.pf.wait_oneway_signal or _settings_game.pf.wait_twoway_signal ought to be sufficient.

Is it caused by #7114?

No, it is not a pathfinder issue. Reverting e934f09 is sufficient to resolve the issue.

@PeterN
Copy link
Member

PeterN commented Feb 1, 2019

I was referring to the use of progress, not wait_counter.

@LordAro LordAro added this to the 1.9.0 milestone Feb 3, 2019
@michicc
Copy link
Member

michicc commented Feb 19, 2019

With the current code, a train waiting at a one-way signal will increment wait_counter twice each tick, i.e. on both TrainLocoHandler calls.

Previously, the update rate of wait_counter depended on train speed and acceleration. For example train 2 from the title game (Turner Turbo) incremented wait_counter every ten ticks, while train 12 (Lev4) incremented every five ticks.

The easiest change would be to the factor in the ++v->wait_counter < _settings_game.pf.wait_oneway_signal * 20 line, to get an approximate and speed independent wait time. Factor 200 would be right for the Lev4, 400 for the Turner, but to avoid integer overflow 255 is the max factor anyway. So maybe just stick with it.

@PeterN
Copy link
Member

PeterN commented Feb 19, 2019

Apart from being twice as fast, this new behaviour also seems to match the setting description better. However I guess the comment was wrong before. Should be possible to at least not increment it on both TrainLocoHandler calls.

michicc added a commit to michicc/OpenTTD that referenced this issue Feb 19, 2019
…o short.

This is not an exact fix as previously, the wait time was speed/acceleration dependant. This simple fix ignores that and just makes it somewhat right.
michicc added a commit to michicc/OpenTTD that referenced this issue Feb 20, 2019
…o short.

This is not an exact fix as previously, the wait time was speed/acceleration dependant. This simple fix ignores that and just makes the 'days' from the settings comment to be actually days.
nielsmh pushed a commit to nielsmh/OpenTTD that referenced this issue Mar 11, 2019
…o short.

This is not an exact fix as previously, the wait time was speed/acceleration dependant. This simple fix ignores that and just makes the 'days' from the settings comment to be actually days.
douiwby pushed a commit to douiwby/OpenTTD that referenced this issue Apr 16, 2020
…o short.

This is not an exact fix as previously, the wait time was speed/acceleration dependant. This simple fix ignores that and just makes the 'days' from the settings comment to be actually days.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression It used to work, and now it's broken.
Projects
None yet
Development

No branches or pull requests

5 participants