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

Feature: Close adjacent level crossings as if they were one large crossing #8444

Closed
wants to merge 2 commits into from

Conversation

Eddi-z
Copy link
Contributor

@Eddi-z Eddi-z commented Dec 27, 2020

Motivation / Problem

Level crossings on parallel tracks can be dangerous, especially for articulated road vehicles, or if multiple road vehicles follow each other. Often, a vehicle is waiting for a level crossing to open, when it suddenly gets struck in its "tail" by another train.

Description

Updated patch from https://www.tt-forums.net/viewtopic.php?f=33&t=46091

This patch tries to avoid the situation that a vehicle gets stuck inbetween two tracks by closing all level crossings simultaneously, and instead allowing road vehicles to leave a crossing they already entered. Best combined with a path signal some tiles ahead of a level crossing, to give vehicles time to leave if a train approaches.

Limitations

on loading an old savegame, road vehicles that were previously waiting for a crossing to open, may now immediately start up, causing them to run straight into a train. some savegame conversion update is needed.

also, a call to SndPlayTileFx(SND_0E_LEVEL_CROSSING, tile); fell off and i don't remember where it went, or why i removed it.
Update: level crossing sound is now changed to only appear when a train approaches the crossing, instead of sometimes when it is being closed.

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 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')

@michicc michicc added the preview This PR is receiving preview builds label Dec 27, 2020
@DorpsGek DorpsGek temporarily deployed to preview-pr-8444 December 27, 2020 14:25 Inactive
@michicc
Copy link
Member

michicc commented Dec 27, 2020

Your missing SndPlayTileFx seems to be done in UpdateLevelCrossingTile. I haven't tried it yet, but I think it will now play a sound for each crossing tile that is closed, which should probably be reduced to only the triggering tile.

@Eddi-z
Copy link
Contributor Author

Eddi-z commented Dec 27, 2020

I vaguely remember that there was a second source of that sound, when a train was approaching a crossing tile

@TrueBrain TrueBrain added this to the 1.11.0 milestone Jan 5, 2021
@TrueBrain TrueBrain changed the title Feature: Close adjacent level crossings as if they were one large cro… Feature: Close adjacent level crossings as if they were one large crossing Jan 6, 2021
@TrueBrain TrueBrain added candidate: yes This Pull Request is a candidate for being merged size: small This Pull Request is small, and should be relative easy to process labels Jan 6, 2021
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda stopped my review half-way through. Please spend some more time on code quality here. So many things that appear to be in a half-state, that it made me check if this was a draft PR (it was not).

src/road_cmd.cpp Outdated Show resolved Hide resolved
src/road_cmd.cpp Outdated Show resolved Hide resolved
src/train_cmd.cpp Outdated Show resolved Hide resolved
src/train_cmd.cpp Outdated Show resolved Hide resolved
src/train_cmd.cpp Outdated Show resolved Hide resolved
src/train_cmd.cpp Outdated Show resolved Hide resolved
src/train_cmd.cpp Outdated Show resolved Hide resolved
@TrueBrain TrueBrain added the work: needs more work This Pull Request needs more work before it can be accepted label Jan 6, 2021
@Eddi-z
Copy link
Contributor Author

Eddi-z commented Jan 6, 2021

it made me check if this was a draft PR (it was not).

it probably should be.

@Eddi-z Eddi-z marked this pull request as draft January 6, 2021 17:48
@TrueBrain TrueBrain removed this from the 1.11.0 milestone Jan 6, 2021
@TrueBrain TrueBrain added work: still in progress (draft) This Pull Request is a draft and removed work: needs more work This Pull Request needs more work before it can be accepted labels Jan 6, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8444 January 9, 2021 23:17 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-8444 January 9, 2021 23:21 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-8444 January 10, 2021 14:45 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-8444 January 10, 2021 14:48 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-8444 January 10, 2021 15:14 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-8444 January 10, 2021 16:02 Inactive
@Eddi-z
Copy link
Contributor Author

Eddi-z commented Jan 10, 2021

i revisited the crossing sound effect. before this PR, crossing sound was played in two different ways

a) when a crossing without a PBS nearby gets closed
b) when a crossing with PBS is approached by a train

case a) was a bit awkward in my patch, since that might result in multiple crossing sounds being played at once, so i opted instead to drop case a) and change case b) to be in all cases

i opted to do this in a separate commit, in case it was somehow controversial

@DorpsGek DorpsGek temporarily deployed to preview-pr-8444 January 15, 2021 13:13 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-8444 January 15, 2021 13:14 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: yes This Pull Request is a candidate for being merged preview This PR is receiving preview builds size: small This Pull Request is small, and should be relative easy to process work: still in progress (draft) This Pull Request is a draft
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants