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 #8506: Towns shouldn't add junctions to NewGRF roads they cannot build #8535

Merged
merged 1 commit into from Jan 9, 2021

Conversation

2TallTyler
Copy link
Member

@2TallTyler 2TallTyler commented Jan 8, 2021

Fixes #8506

Motivation / Problem

Per #8506, towns add junctions to NewGRF roads which have the TOWN_BUILD flag disabled.

If a town cannot build a road type, I would assume that it should not be able to add junctions to roads of that type.

Description

I have created a savegame to test. You will need the RattRoads NewGRF, available from Bananas.
HighwayRoadTest.zip

While the town is not able to build junctions on the highway, the town growth algorithm can still walk down the highway to build houses past the highway.

Note that while the RattRoads Highway also has the NO_HOUSES flag set, this is not checked as I think it is irrelevant to towns building said road.

Limitations

There could be side effects I'm not aware of. Please help test this!

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

@2TallTyler 2TallTyler changed the title Fix #8506: Towns don't add junctions to NewGRF roads they cannot build Fix #8506: Towns shouldn't add junctions to NewGRF roads they cannot build Jan 8, 2021
@glx22 glx22 added the preview label Jan 8, 2021
@TrueBrain TrueBrain merged commit b045666 into OpenTTD:master Jan 9, 2021
9 checks passed
@JGRennison
Copy link
Contributor

@JGRennison JGRennison commented Jan 9, 2021

I unfortunately ran out of time to comment on this yesterday, but in any case, there are some potential issues:

  1. There may be no road types where ROTF_TOWN_BUILD is set, in this case towns will build using ROADTYPE_ROAD (type 0), see GetTownRoadType. In this case towns would be unable to build junctions.
    Try the GRF "U&RaTT" from bananas, this now causes town generation to fail entirely.
  2. The fact that a road type is not buildable by town growth does not in itself imply that towns should be prevented from creating junctions. This is not what the flag in the GRF specification advertises and does not make sense in the context of some existing NRT GRFs.
    For example in the GRF "RattRoads" from bananas, ROTF_TOWN_BUILD is not set on most of the player-buildable road types, including innocuous sounding types like "Dirt Road" and "Basic Asphalt Road". There are separate types specifically for towns to use which also have ROTF_HIDDEN set such that players can't build them directly.
    This would make it very awkward for players to be able to add road to a town without accidentally clobbering town growth, as whether ROTF_TOWN_BUILD is set is not actually indicated anywhere in the UI.

Adding a new road type flag to enum RoadTypeFlags and therefore to the GRF API is comparatively trivial, and avoids changing the meaning of an existing flag to do something different.
(For what it's worth I did add such a new flag in my branch, but I don't think that any GRFs are using it so far).

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 9, 2021

In light of the above comment, I am going to revert this change for now, so it can be done properly. Tnx @JGRennison !

TrueBrain added a commit that referenced this issue Jan 9, 2021
…oads they cannot build (#8535)"

As per #8535 (comment),
jumped the gun here.
TrueBrain added a commit that referenced this issue Jan 9, 2021
…oads they cannot build (#8535)" (#8541)

As per #8535 (comment),
jumped the gun here.
@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Jan 9, 2021

Thanks, @JGRennison! I'll take a look at your codebase to see about adding a new flag specifically for no town intersections.

@2TallTyler 2TallTyler deleted the no_highway_junctions branch Feb 27, 2021
@jdeepankur
Copy link

@jdeepankur jdeepankur commented Mar 6, 2021

Just asking, if this thing is eventually patched, could it perhaps be something the user can toggle? I understand why some people don't like towns adding junctions to newGRF roads they're not supposed to build on, but some of us actually like the feature. Especially since one way road mechanism allows you to prevent junctions from being built too.

@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Mar 6, 2021

This approach has been reverted, since I didn't properly understand other use cases for this flag. It is not being implemented this way.

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

Successfully merging this pull request may close these issues.

6 participants