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: Towns don't build dead-end road bridges #8395

Closed
wants to merge 7 commits into from

Conversation

2TallTyler
Copy link
Member

Currently, towns build bridges over ocean tiles or tracks without checking if the tile past the bridge will allow them to continue building the road. This frequently leads to bridges dead-ending at industries, houses, tracks, etc.

I have created a test savegame to demonstrate each case where the bridge should be allowed, and each case where it should be prohibited. You can run it on fast-forward to let the towns grow (I have included buses to drive growth). You can tell that a bridge has been rejected when the town grows the road to the end of the tile without bridging.

Bridge_Test_Save.zip

TrueBrain suggested using a DoCommand to try building a road in lieu of the industry/object/house/railway tile checks, but after some experimentation I decided that separate checks rejecting the bridge is more maintainable than one big if chain with a single return false at the end. Feel free to correct me, as I'm pretty new to all of this. :)

@2TallTyler
Copy link
Member Author

I can't see the whitespace which is failing CI in Visual Studio. Any tips?

@Milek7
Copy link
Contributor

Milek7 commented Dec 18, 2020

You need to squash these commits.

src/town_cmd.cpp Outdated Show resolved Hide resolved
src/town_cmd.cpp Outdated Show resolved Hide resolved
@Milek7
Copy link
Contributor

Milek7 commented Dec 18, 2020

TrueBrain suggested using a DoCommand to try building a road in lieu of the industry/object/house/railway tile checks, but after some experimentation I decided that separate checks rejecting the bridge is more maintainable than one big if chain with a single return false at the end.

I don't quite understand that, why it would result in big if chain?
Couldn't you just do if (!IsRoadAllowedHere(t, tile_past_bridge, bridge_dir)) return false;? (or maybe other conditions than IsRoadAllowedHere does, but with simple DoCommand instead of trying various situations)

@2TallTyler
Copy link
Member Author

The problem with returning false if no road is allowed to be built, is that there are several conditions in which the bridge should be allowed and returning false short-circuits those next checks. Specific situations are when a valid road tile already exists in the next tile: a road station, depot, bridge, tunnel, or a complete road tile facing the correct direction.

@2TallTyler 2TallTyler force-pushed the no_dead_end_bridges branch 2 times, most recently from 4657d69 to 9b88688 Compare December 18, 2020 17:22
src/town_cmd.cpp Outdated Show resolved Hide resolved
src/town_cmd.cpp Outdated Show resolved Hide resolved
@TrueBrain
Copy link
Member

I also send you some words on Discord too, but in summary here too:

You now define all the possible ways the bridge would be invalid, which is a pretty lengthy list. Possibly it is better to do the reverse: define what are valid scenarios, as I think that list would be a lot shorter:

  • Road already connecting
  • Can build road that connects
  • Station right orientation
  • Depot right orientation
  • Other tunnel or bridge in right orientation

C'est tout!

This would be, I think, much easier code to read, but also much less likely to have a flaw somewhere in the logic :)

@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 Dec 18, 2020
@2TallTyler
Copy link
Member Author

Per my conversation with TrueBrain on Discord, I will be rewriting this to use a new function. Stay tuned.

@2TallTyler 2TallTyler marked this pull request as draft December 18, 2020 18:35
@2TallTyler 2TallTyler force-pushed the no_dead_end_bridges branch 2 times, most recently from c675d1b to f997fac Compare December 18, 2020 21:22
src/town_cmd.cpp Outdated Show resolved Hide resolved
@2TallTyler 2TallTyler marked this pull request as ready for review December 19, 2020 03:10
@2TallTyler 2TallTyler marked this pull request as draft December 19, 2020 03:12
@2TallTyler 2TallTyler closed this Dec 19, 2020
@2TallTyler
Copy link
Member Author

2TallTyler commented Dec 19, 2020

I need to sleep before I break anything further. I'll try this again in the morning.

The correct commit should be cc53efa

TrueBrain and others added 7 commits December 19, 2020 18:26
Co-authored-by: Owen Rudge <owen@owenrudge.net>
This in preparation for other architectures, like arm64.
This has several ways of being triggered:
- When creating a new release via the GitHub interface. Fully
  automated that will produce new binaries, upload them, and it
  will even update the website to tell about the new version.
- When triggered in an automated way from OpenTTD/workflows to
  start a nightly.
- Manually via the Release workflow, which accepts branches,
  Pull Requests and tags to build.

Rerunning a job is safe and should be without issues. Everything
retriggers and updates what-ever might have been broken. In fact,
except for dates, it should produce identical results.

Co-authored-by: Charles Pigott <charlespigott@googlemail.com>
Azure Pipelines has build our releases for two years now, but we
are finally switching to GitHub Actions. This to bring the full
workflow to a single place, making it easier for people to see
what is going on and how to influence the process.
Signed-off-by: 2TallTyler <tyler@tylertrahan.com>
@2TallTyler 2TallTyler reopened this Dec 19, 2020
@2TallTyler
Copy link
Member Author

Per my discussion with TrueBrain on Discord, I appear to have broken GitHub and the easiest fix is starting a new branch and making a new PR.

@2TallTyler 2TallTyler closed this Dec 19, 2020
@2TallTyler 2TallTyler deleted the no_dead_end_bridges branch December 24, 2020 23:47
@2TallTyler 2TallTyler restored the no_dead_end_bridges branch February 24, 2021 21:45
@2TallTyler 2TallTyler deleted the no_dead_end_bridges branch February 27, 2021 16:46
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 size: small This Pull Request is small, and should be relative easy to process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants