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 #8797: Use logical rail length when placing signals #9652

Merged
merged 1 commit into from Nov 19, 2021

Conversation

ldpl
Copy link
Contributor

@ldpl ldpl commented Oct 24, 2021

Motivation / Problem

As described in #8797 signal placer uses some visual distance instead of logical distance to place signals requiring players who need a certain logical gap to place additional signals manually.

Description

Interprets signal distance in the UI as the logical length of that many X/Y tiles of rail. And uses the logical length of the track pieces (i.e. 192 or 128 units) when calculating the distance between signals placed.

When "keep fixed distance" setting is off (i.e. minimize_gaps = true) ensures that gap between signals never exceeds the required signal distance except for when it's on a bridge or tunnel.
Without minimize_gaps (i.e. "pretty" mode) just tries to place signals as soon as accumulated gap >= required distance regardless of whether it succeeds or not.
Nundinghall Transport, 15th Jan 2031

Also, this placer is mostly ready to work with segments of arbitrary logical length if that ever makes into the game.

Limitations

  • Does not account for train elongation on diagonals as there is nothing signal placer can do without knowing exact train stats.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@LordAro LordAro added the preview This PR is receiving preview builds label Oct 24, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-9652 October 24, 2021 18:58 Inactive
@Rau117

This comment was marked as abuse.

@ldpl
Copy link
Contributor Author

ldpl commented Oct 24, 2021

This is what it currently does with minimize_gaps and gap 2. Also there is an emscripten preview so you can try whatever you want yourself right in the browser.
Screenshot from 2021-10-25 00-37-41

@LC-Zorg
Copy link

LC-Zorg commented Oct 25, 2021

Generally it's not bad, but with the current positioning of the signaling it looks ugly. Without aligning signals, it won't be a good change for many players.

Currently (at the top), the distances between signals are 18 and 42 pixels. In addition, one of the signals is a pixel lower than the other. At the bottom, it is equal to 30 and 30.
signals on flat rails

Likewise for vertical tracks.
signals on vertical rails

I don't see why these signals should remain so unevenly placed.

Btw. I would see another method here that would help keep traffic flowing without the need of ugly signals compaction. There could be even less of this signals... In short: "Start braking when you see the red light"

@ldpl
Copy link
Contributor Author

ldpl commented Oct 25, 2021

Signal sprite offsets are a separate issue. Signal placer doesn't even build signals that close in a "pretty" mode anyway.

@LC-Zorg
Copy link

LC-Zorg commented Oct 25, 2021

Separate, but not entirely so, because this change would significantly emphasize these inequalities. For spacing settings of 3 or more, it looks fine. Setting 1 and 2 is problematic.
If these signaling positions were corrected, both modes would be pretty. ;) Besides, some players (I don't know how many, there can be a lot of them or even most, for sure me) like evenly arranged signaling, but at the same time prefer it when the game places it in front of bridges when the distance is too large. This change will remove that possibility.

Another significant problem is the continuation of the game with the changed building rules. So maybe instead of an on/off switch, three points to choose would be better?

@glx22
Copy link
Contributor

glx22 commented Oct 25, 2021

If the alignment is fixed, people will complain because trains don't stop in front of signals.
Signals are placed on the tile edge for a good reason (even if it may look ugly).

@LC-Zorg
Copy link

LC-Zorg commented Oct 25, 2021

If people were to complain about it, they should be doing it now. :D

Currently
signals position - current

After change
signals position - new

In front of the station
signals position - on station

@LordAro
Copy link
Member

LordAro commented Oct 25, 2021

The real reason for the "misalignment" is so that signals do not overlap when train lines run next to each other

image

Now, that's enough off-topic discussion, please keep further discussion to the actual PR subject.

@Rau117

This comment was marked as abuse.

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.

#YOLO. Code looks fine. Would recommend against backporting though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview This PR is receiving preview builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants