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

Public roads type selector #758

Merged
merged 18 commits into from
Oct 7, 2024

Conversation

bigyihsuan
Copy link
Contributor

@bigyihsuan bigyihsuan commented Sep 27, 2024

Motivation / Problem

Kale suggested in #jgr-patch-pack on the Discord to be able to select the road type of public roads. This was in the context of preventing towns from building on public roads; not doing so leads to these weird "tentacles" of housing along the public road:

image

Selecting a road type that prevents houses from being built next to it solves this problem; as the town is serviced and grows it will preferentially select its town roads over the public roads to build houses next to.

Plus it looks nicer to not have the town road stretch for miles between towns sometimes 😛

Description

This PR adds the ability to select a road type for public roads in the scenario editor.

When you are in the scenario editor, and click on the landscape generation button, the public roads window will open with it. This public roads window can be closed separately, and reopens when you click the landscape generation button again. When you close the main landscape generation window, the public roads window will also close.

The dropdown allows you to select a public road type. When you click the "Build public roads" button, it will use the selected road type instead of the default.

Images

Vanilla

image
image

With U&RaTT, and custom font

image
image

Limitations

The default road type is initialized on first opening the window, and will not change until you pick a new road type. When you build towns a different road type will be used. It is recommended to build towns before public roads.

Weird things happen when the available road types changes when you change GRFs. (Intended? since changing GRFs is always weird anyway)

Allowing public road type selection in a new game would require adding a new setting that depends on the available road types before they are fully loaded (i.e. when I tried migrating my code from the scenario editor to the new game menu, it was extremely glitchy and didn't work). This would come in a new PR.

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, game_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

…bel to scenario terraforming GUI

Currently does not display the selected roadtype, and generating public
roads still uses the default roadtype.
@bigyihsuan bigyihsuan changed the title Draft: Public roads type selector in the scenario editor Draft: Public roads type selector Sep 27, 2024
@JGRennison
Copy link
Owner

This looks pretty good.
A few minor things. There should probably be a bit of padding over the public road types label to separate it from the buttons above.
GetTownRoadType() is not necessarily in the list of user-buildable road types as returned in GetScenRoadTypeDropDownList(). This is only the case for some old/awkward GRFs (e.g. U&RATT 0.2), but it'd probably be worth adding it to the list if not present.

@bigyihsuan bigyihsuan force-pushed the public-roads-type-selector branch 2 times, most recently from 915f833 to 0dd9f3a Compare September 28, 2024 00:38
@bigyihsuan bigyihsuan force-pushed the public-roads-type-selector branch from 0dd9f3a to 43c34ad Compare September 28, 2024 00:40
@bigyihsuan bigyihsuan force-pushed the public-roads-type-selector branch from 8fae371 to 7361b77 Compare September 29, 2024 21:05
@bigyihsuan bigyihsuan marked this pull request as ready for review September 29, 2024 21:53
@bigyihsuan bigyihsuan changed the title Draft: Public roads type selector Public roads type selector Sep 29, 2024
@JGRennison
Copy link
Owner

I've pushed a few changes to tidy up some minor things.
In particular, the default selected road type should now be initialised to the right value for the current road type configuration.
It'll be reset when the road type configuration is changed.
This should fix the issue when changing GRF configuration or changing between scenarios.

I'm not really convinced about the button opening two windows which are partially linked. This seems somewhat strange and unexpected from a user interface point of view.
It would seem more straightforward to use the dropdown pattern like the other toolbar buttons with multiple functions.

Generally the public roads function only needs to be used once when setting up a scenario, whereas the landscape tools may be used frequently. Having the public roads window pop up every time the landscape window is requested is not likely to be appreciated by all scenario editor users.

@bigyihsuan
Copy link
Contributor Author

This seems somewhat strange and unexpected from a user interface point of view.

There is some precedent with the terraforming toolbar being linked with the rail/road/etc toolbars, but I see what you mean.

@JGRennison
Copy link
Owner

I've made the changes so that the scenario editor toolbar is used. If you're all right with things as they are, I'm happy to merge this.

@bigyihsuan
Copy link
Contributor Author

I've made the changes so that the scenario editor toolbar is used. If you're all right with things as they are, I'm happy to merge this.

I'm fine with your changes, go ahead.

@JGRennison JGRennison merged commit 809c2a2 into JGRennison:jgrpp Oct 7, 2024
10 checks passed
@JGRennison
Copy link
Owner

Thanks for this

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.

2 participants