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

Move "town name" selection into map generator GUI #8566

Merged
merged 6 commits into from Feb 14, 2021

Conversation

frosch123
Copy link
Member

@frosch123 frosch123 commented Jan 13, 2021

Motivation / Problem

There have been plenty of attempts to redesign the map generation window. Most get derailed into wanting to change everything.
This PR deals with two "world generation" settings:

  • "Town names": currently only in "game options".
  • "Road drive side": currently in "game options" and in the "settings tree".

Both settings do not belong into "game options". The most reported issue related to NewGRF town names is, that players do not "find" the "town name" dropdown to enable them.

Description

Main change:

  • "Town name" selection is removed from "game options", and put into the map generation window.
    • This setting is "interesting" enough to put it into a prominent GUI location.

Collateral changes:

  • "Road drive side" is removed from "game options", and demoted to "settings tree only".
    • This setting does not fit in with the other "basic" settings in "game options". I don't think anyone would look for that setting in "game options", unless you are used to it being there.
    • It gets the same "basic" visibility as "train signal side".
  • "Tree plant algorithm" is removed from the map generation window, and demoted to "settings tree only".
    • The difference between "original" and "improved" is hardly a popular setting.
    • "none" is probably popular, but only makes sense when "in-game growth" is also disable, which is in the settings tree already.
    • It gets the same "basic" visibility as the "in-game growth" setting.

Limitations

None.

Checklist for review

N/A

@frosch123 frosch123 added the preview This PR is receiving preview builds label Jan 13, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8566 January 13, 2021 19:33 Inactive
@andythenorth
Copy link
Contributor

Easy +1 from me.

I did use the 'thumbs up' on the PR, but I don't really trust that as a process yet.

Comment on lines 950 to 951
STR_GAME_OPTIONS_ROAD_VEHICLES_DROPDOWN_LEFT :Drive on left
STR_GAME_OPTIONS_ROAD_VEHICLES_DROPDOWN_RIGHT :Drive on right
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the remaining string ids should be renamed, given they're no longer in the game options

Copy link
Member

Choose a reason for hiding this comment

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

I agree that might be a good thing for future-us :D

uint gen = _settings_newgame.game_creation.town_name;
StringID name = gen < BUILTIN_TOWNNAME_GENERATOR_COUNT ?
STR_GAME_OPTIONS_TOWN_NAME_ORIGINAL_ENGLISH + gen :
GetGRFTownNameName(gen - BUILTIN_TOWNNAME_GENERATOR_COUNT);
Copy link
Member

Choose a reason for hiding this comment

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

pedantry: not sure i agree with the line splitting here. All on one line, or split with the ?/: as the first character

@LC-Zorg
Copy link

LC-Zorg commented Jan 17, 2021

Town names
It's a good change, but what about creating scenarios? Here, choosing a setting in the create new game window doesn't seem like a logical place.

  • Currently: important that changing the setting also should works for the editor
  • In the future: it would be nice to add options for selecting city names in the scenario editor
    City creation window v1 0

Traffic direction on the roads
It also seems like a good change. My only doubts are that this setting maybe should be in the Vehicles / Routing section - I haven't thought too much about it, but maybe it's a more appropriate place?
Detail: In the scenario editor, when there are moving vehicles, this option could be disabled.

Trees algorithm
The choice of an algorithm really seems unnecessary here. In my opinion, this is one of those rarely or never used settings that would fit into the real "Weird" category. ;)
However, the very selection of whether the map is to be generated with or without trees is one of the very basic settings that should be available in the map creation window.
If possible deletion of option is dictated by lack of space for "Town names", "Land generator" seems to be a much less used setting.

  • Currently: I would leave "Trees: Yes / No"
  • In the future: I would have a proposal that it would be possible to define the limit of the number of trees on the map. I wrote a little more about it on the forum.

World generation window layout
Since you are changing the layout a bit, it would be a good idea to group the settings a bit better. :)

World Generation window v1 0
-without Land generator
-Smoothness setting in terrain section
-River and Sea level close together (water section)
-Town names and No. of towns also close together (towns section)

World Generation window v2 0
-with Land generator

World Generation window v3 0
-another layout, rather not the best but just for inspection

Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

#shipit

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

Successfully merging this pull request may close these issues.

None yet

6 participants