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 #10574 -- revert the change and don't list unavailable road types to game script. #10585

Merged
merged 3 commits into from May 12, 2023

Conversation

PeterN
Copy link
Member

@PeterN PeterN commented Mar 30, 2023

Motivation / Problem

See #10574.

Description

Re-allow town-buildable to override hidden, but do check for availability for game script listing.

Limitations

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

@PeterN
Copy link
Member Author

PeterN commented Apr 19, 2023

Updated to remove anything to do with rail types.

I don't know if this change is the way forward, it's just a potential solution.

@PeterN PeterN changed the title Fix #10574 -- revert the change and don't list unavailable rail/road types to game script. Fix #10574 -- revert the change and don't list unavailable road types to game script. Apr 19, 2023
rubidium42
rubidium42 previously approved these changes Apr 19, 2023
@Andrew350
Copy link

Andrew350 commented Apr 19, 2023

FWIW, as a NewGRF developer affected by the initial change this would be my preferred solution compared to the other options so far, although I'm obviously a bit biased here since this means no changes necessary on our side.

My only question is if this solution has any negative effect on the GS side of things? From what @rubidium42 explained, the reason for the change in the first place was that a GS could have issues determining which roadtypes were actually viable to build based on the way we define some of them with both the HIDDEN and TOWN_BUILD flags.

If that was the main issue and this solution solves it for both sides in a good way (which it does from my perspective, at least) then this seems like the ideal fix to me. However I don't really understand the GS side of things so perhaps there is another view here.

@Brickblock1
Copy link

I would also prefer this option, while I haven't made a roadset I still think that this option is more flexible and therefore better.

@michicc
Copy link
Member

michicc commented May 1, 2023

Needs a rebase after the recent CI changes.

@2TallTyler 2TallTyler removed the work: minor details This Pull Request has some minor details left to do label May 12, 2023
@michicc michicc merged commit 1a93618 into OpenTTD:master May 12, 2023
19 checks passed
@PeterN PeterN deleted the fix-10574 branch May 14, 2023 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants