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

Feature: [UI] Split AI/Game Script configuration windows and add them to world gen window #10058

Merged
merged 1 commit into from
Nov 10, 2022
Merged

Conversation

Arastais
Copy link
Contributor

@Arastais Arastais commented Sep 28, 2022

Motivation / Problem

A while ago, @frosch123 mentioned in the discord that adding Game Script, AI, and NewGRF settings buttons to the world gen window is an old request/idea but always gets swamped by other requests. As I personally also want this feature, I decided to do it myself. This will allow users to edit Game Script/AI settings, NewGRF settings, and Game Script configuration from both world generation windows (new game and heightmap).

What's most convenient is that users can now edit Game Script paramters directly from the world generation window, without having to go back to main menu > AI/Game Script Setttings > select Game Script > Configure, and can instead just click "Configure Game Script". This also aligns with OpenTTD's goal of improving the user interface; I made the changes in the way that they are not too intrusive (the new buttons do not over-complicate the world generation window) and have the same styling as the other buttons. This will be most enjoyed by any players who regularly use and change either their game script or their game script configuration/parameters.

Description

This commit simply adds 3 buttons to the bottom of the world generation window, each of which just show windows which are already in the game:
eb61390770a79342ee5799bee2c57d4a

The first two buttons do exactly the same as their main menu counterpart; the "Configure Game Script" button directly shows the Game Script Parameters window for the currently selected game script and allows players to directly configure the Game Script. As mentioned earlier, this allows players to easily and fully configure their NewGRFs and scripts right from the world generation screen.

The buttons are double the height of the other buttons so they are easy to see but are also at the bottom of the window so they do not intrude any part of the world gen interface (The idea to put them at the bottom was thanks to @ldpl). They are also colored the same as the other buttons (excluding the "Generate" button) to match OpenTTD's theme.

Limitations

This feature solves the problem of inconvenient menu navigation for all players. The feature is pretty much complete, and nothing else needs to be added to it. It is possible to change the look and feel of the buttons in the future, or add more buttons with similar functionality, but my aim was to make functionality that was already in the game more convenient and user-friendly. As this is a simple addition, there should be no bugs or problems that occur because of it.

The only notable source code change is that void ShowAISettingsWindow(CompanyID slot) in ai_gui.cpp was added via decleration to its corresponding header file (i.e. the header file for the AI gui) and is no longer static, so that it could be visible to the world gen GUI file (genworld_gui.cpp). This should not have a real effect on any part of the game however.

It is important to note however that I did add strings to english.txt, some of which were duplicates. I itentionally did not re-use the strings from the main menu (e.g. reusing "NewGRF Settings") because I did not want them to share the same strings (causing edits to them to relfect in both places) and possibly confuse people if they were looking for a seperate set of strings. It would also make the strings inconsistent: the first two buttons would use shared strings, while the "Configure Game Script" button would have its own string in a different section in english.txt. Not to mention, I don't see string codes being re-used anywhere in OpenTTD. Regardless, this could be easily changed if need be, but I ended up doing what I believed is best.

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

@2TallTyler
Copy link
Member

I'm very happy to see this being worked on, and it's always nice to see new contributors getting involved.

A couple thoughts:

  • I'm not sure about the button for configuring the current GS. NewGRF parameters are changed from inside the NewGRF settings window so these don't match. I like the convenience but the inconsistency bothers me. I could be convinced one way or the other. 🙂
  • I think the Generate button should be at the bottom of the window, underneath the new buttons. You might need to rearrange the contents of the columns to match the lengths. I do like how your new buttons are double-height for visibility.

@Arastais
Copy link
Contributor Author

I'm very happy to see this being worked on, and it's always nice to see new contributors getting involved.

A couple thoughts:

  • I'm not sure about the button for configuring the current GS. NewGRF parameters are changed from inside the NewGRF settings window so these don't match. I like the convenience but the inconsistency bothers me. I could be convinced one way or the other. 🙂

  • I think the Generate button should be at the bottom of the window, underneath the new buttons. You might need to rearrange the contents of the columns to match the lengths. I do like how your new buttons are double-height for visibility.

@2TallTyler, thanks for your interest. Here's what I think:

  1. Like you say, the NewGRF parameters are configured from inside the settings window, while the Game Script has its own dedicated window. Additionally, I wanted to put a dedicated button for the Game Script as there can only be one selected at a time, so having a button to open the dedicated window for it makes sense. For NewGRF on the other hand, you can have multiple active at the same time, so it doesn't make sense to have a dedicated button for configuring NewGRFs; it would basically just be the settings window anyway since you would have to chose which NewGRF to configure. While I do see the presented inconsistency, I think that whatever convenience the button brings is worth, and makes the interface feel more natural as a whole; this dedicated configure button only works for the Game Script like I said, and I believe that it is the most convenient part of this feature.

  2. Turns out, having the generate button underneath was my original idea, but I ruled it out as, either on the landscape generation or height map generation (as the 'Generate' button is of a different height between these two), it would cause the columns to be uneven in terms of length, even after rearrangement. Here is what I mean (I moved sea level to the right side):
    image
    This is for the "New Game" version of the window. The "Play Heightmap" version of this window is actually even, since the generate button is only double height. As you can see, there's a gap after the town names on this window; This also creates inconsistency as this gap is not present on the heightmap version of this window. The best solution I can see to the gap is centering the "Map edges" text and button, but again there's still the consistency of this not being present.
    Regardless, if you prefer this version (how it is in the screenshot above and below), let me know and I will commit this version instead. For reference, here's how it would look when playing a heightmap (I moved rivers to the right side):
    image

As a side note, would anyone happen to know why the commit check is saying that the commit message is in the incorrect format? It seems to me like it is, unless it's somehow too long. (I do know why the tabs check is failing, and I'll fix that at the end).

@glx22
Copy link
Contributor

glx22 commented Sep 28, 2022

As a side note, would anyone happen to know why the commit check is saying that the commit message is in the incorrect format? It seems to me like it is, unless it's somehow too long. (I do know why the tabs check is failing, and I'll fix that at the end).

It's because of the merge commit, your commit message is fine.

@2TallTyler
Copy link
Member

Good argument for the configure GS button. I'm sold.

I definitely prefer the generate button below. The gap below town names doesn't bother me — I'm pretty sure a previous version of the game had a similar gap somewhere in this menu which was only eliminated.

@Arastais
Copy link
Contributor Author

Alright, should be good to go. Just commited @2TallTyler's ideas (i.e. moving the generate button) and fixed some accidental tab issues.

Copy link
Contributor Author

@Arastais Arastais left a comment

Choose a reason for hiding this comment

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

The lines that Actions is complaining about has already been fixed in 33da4df; It seems to be complaining about an earlier commit. The current changes to genworld_widget.h only adds 4 lines with correct tab spacing. Also, the commit message complaint is referring to the commit that merges master into my branch.

@glx22
Copy link
Contributor

glx22 commented Sep 29, 2022

Indeed, commit checker checks each commit individually.

@2TallTyler
Copy link
Member

You'll want to do an interactive rebase to combine all six commits into one, then force-push.

Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

Although maybe not for this PR, but the 3rd button kinda bothers me. And I understand your reasoning for making that button, but the UI now feels weird.

NewGRF -> AI/GS -> GS

It is a bit confusing why GS is there twice. In an ideal world, I guess, the AI/GS should be come just AI, and the GS should allow changing the GS. That would give a much more solid flow, I would think.

But I can fully understand if that is a bit too much for a change at once, but I would love to read your view on it.

src/genworld_gui.cpp Outdated Show resolved Hide resolved
src/genworld_gui.cpp Outdated Show resolved Hide resolved
src/genworld_gui.cpp Outdated Show resolved Hide resolved
@PeterN
Copy link
Member

PeterN commented Oct 22, 2022

  1. I think the GS settings button is not needed as that comes under AI/GS configuration, much like configuring individual AIs.
  2. I also don't like the choice of double height buttons.
  3. Also I don't really see why you have moved the Sea Level dropdown and the Generate button, and made that really really huge.

@2TallTyler
Copy link
Member

Moving the Generate button was my suggestion above. 😛 It looked strange to have a lot of buttons below the Generate button. Maybe keeping it at the bottom, but making it normal or double height instead of triple, would look better.

@Arastais
Copy link
Contributor Author

Arastais commented Oct 22, 2022

  1. I think the GS settings button is not needed as that comes under AI/GS configuration, much like configuring individual AIs.
  2. I also don't like the choice of double height buttons.
  3. Also I don't really see why you have moved the Sea Level dropdown and the Generate button, and made that really really huge.
  1. The whole point of this is so that you can skip a bunch of steps. Instead of having to open up the menu, select the game script, then select the configure GS button, which is way smaller too, you can do all that in one click.
  2. It seems me and @2TallTyler agreed on double height buttons, but it's not unanimous. Maybe it's worthy of a discussion in the discord.
  3. This was @2TallTyler's suggestion and I honestly like it more than the original design, even with single height buttons. Do note that I started with the original design, which is seen in the pictures of the first post of this thread. Maybe this is also worthy of discussion

@Arastais Arastais changed the title Feature: [UI] Allow configuring AI/Game Script and NewGRF from world gen window Feature: [UI] Split AI/Game Script configuration windows and add them to world gen window Oct 23, 2022
@Arastais Arastais changed the title Feature: [UI] Split AI/Game Script configuration windows and add them to world gen window Feature, Change: [UI] Split AI/Game Script configuration windows and add them to world gen window Oct 23, 2022
@PeterN
Copy link
Member

PeterN commented Oct 23, 2022

I think splitting AI/GS windows should be a separate commit before adding the buttons to the worldgen window.

And please rebase to remove the merge.

Copy link
Contributor

@glx22 glx22 left a comment

Choose a reason for hiding this comment

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

I won't comment on all locations with coding style issues ;)
Most are caused by VS.

src/ai/ai_gui.cpp Outdated Show resolved Hide resolved
src/ai/ai_gui.hpp Show resolved Hide resolved
src/ai/gs_gui.cpp Outdated Show resolved Hide resolved
src/ai/gs_gui.cpp Outdated Show resolved Hide resolved
src/ai/gs_gui.cpp Outdated Show resolved Hide resolved
src/ai/gs_gui.cpp Outdated Show resolved Hide resolved
src/ai/gs_gui.cpp Outdated Show resolved Hide resolved
@Arastais
Copy link
Contributor Author

Arastais commented Oct 23, 2022

Sorry about all the force pushes, had to fix merge conflicts and add your suggestions

This is what the AI window looks like now:
wg-settings-ai

And this is the Game Script window:
wg-settings-gs

I have tested both of them as much as I could and they both work as intended. The reset button in the Game Script window works the same way it did before, so it doesn't reset values it's not supposed to in-game. Note however that because I changed an existing StringID in english.txt (instead of making a new one) the "Select Game Script"/"Select AI" texts will show up incorrectly (they'll be blue and won't say select), so translations should be updated properly.

To coincide with the split, the title menu now looks like this:
wg-settings-menu

And the in-game toolbar looks like this:
wg-settings-toolbar

The world gen windows look the same as before these recent force-pushes.

The title screen may look off now, but I did what felt the simplest in terms of changing the layout of the buttons. The layout of the title screen can obviously be discussed.

I think splitting AI/GS windows should be a separate commit before adding the buttons to the worldgen window.
If this is agreed upon, I can remove it being added to worldgen window and change this PR to just splitting the config windows.

I personally think it's not necessary, since the worldgen window change was the original goal, and now it's a much smaller change relatively to splitting the windows. The whole purpose of splitting the windows is to properly add them to the worldgen window anyway. But of course, more opinions on this would be appreciated.

Copy link
Contributor

@glx22 glx22 left a comment

Choose a reason for hiding this comment

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

Seems VS doesn't want to cooperate ;)

src/ai/ai_gui.cpp Outdated Show resolved Hide resolved
src/ai/gs_gui.cpp Outdated Show resolved Hide resolved
src/ai/gs_gui.cpp Outdated Show resolved Hide resolved
@Arastais
Copy link
Contributor Author

As a side note for the main menu/title screen UI, a good option would be to remove the highscore table (similar to what #7786 does) and thus remove the button from the main menu and replace it with the "NewGRF Settings" button. This way "AI Settings" and "Game Script Settings" will be on their own row and the UI will be properly lined up again. Keep in mind that this of course would interfere with #7786 since the "NewGRF Settings" button goes where the "Help and Manuals" button would go in #7786.

@LC-Zorg
Copy link

LC-Zorg commented Oct 25, 2022

I like the idea of ​​shortcuts in the new game window. I also like the idea of ​​creating separate windows for AI and GS selection and settings. It could use other changes there as well, but that's a separate topic. However, I don't know if the proposed changes are the best way. What I definitely miss in this window, apart from these three shortcuts, is also a shortcut to gameplay settings - not all settings, including those related to the interface, but those that define the gameplay. In general, there are some interesting concepts that would be worth considering.

Some time ago, Andrew350 proposed, probably not the first, to combine the NewGRF and AI/GS setup windows. This solution solves the problem of the three buttons in the intro.
https://user-images.githubusercontent.com/50259667/102848133-e639cd00-43c8-11eb-8697-23125bd8c6e0.png

Over a decade ago andythenorth created a very interesting and inspiring mockup for me, showing the concept of a new game settings window (I think).
https://www.tt-forums.net/download/file.php?id=128801

Also TrueBrain introduced the concept of redesigning the new game window as well as the intro, where the main change would be to add a preset selection option, which would be the player's primary choice and which I think is a really enjoyable and useful solution.
https://user-images.githubusercontent.com/1663690/108421875-14575400-7236-11eb-8468-78dbebe6e2c7.png
https://user-images.githubusercontent.com/1663690/108423928-c859de80-7238-11eb-99e0-20e5ffe00474.png

I also have my own concept largely inspired by the ideas above. It is not complete and not everything is consistent.
In brief:

  1. By pressing "Singleplayer" you open a new game creation window
  2. The basic choice are presets, that is, saved various configurations of map, game, AI, GS and NewGRF settings
  3. Random maps are under the "Heightmap" tab
  4. At the bottom of the "Heightmap" tab you have shortcuts to AI, GS and NewGRF settings, as well as in a separate line to gameplay settings
  5. The shortcuts AI, GS and NewGRF open the same window with a different tab
  6. A shortcut to gameplay settings opens the window of all merged settings with the "Gameplay" tab open.
    Presets and setting shortcuts

Sorry, I'm not presenting you with a ready-made concept that you can simply approve or reject. However, I mention it because I think it is worth considering what it should look like. :)

@Arastais
Copy link
Contributor Author

Arastais commented Oct 26, 2022

@LC-Zorg The main goal of this PR was to add the GS/AI settings to the world gen window. I split GS and AI after the fact because there was a disagreement about the GS parameter config button, and I agreed with the suggestion to split them. However, I wanted to split them as minimally as possible and keeping to the same layout/design as much as I could. Thus, while your suggestions are valid, I feel they are beyond the scope of this pr, especially when trying to drastically alter the title menu or NewGRF settings as well. Your suggestions would be a good PR on its own, especially since they are all concepts right now.

@2TallTyler 2TallTyler added this to the 13.0 milestone Oct 27, 2022
@2TallTyler 2TallTyler added the preview This PR is receiving preview builds label Nov 3, 2022
@DorpsGek DorpsGek temporarily deployed to preview-pr-10058 November 3, 2022 14:23 Inactive
@2TallTyler
Copy link
Member

Needs a rebase 🙂

src/ai/CMakeLists.txt Outdated Show resolved Hide resolved
@frosch123
Copy link
Member

I like this a lot :)

Is there a reason to keep the 3 buttons in the title menu?
They only affect new games, and now they are in the mapgen gui. So I would remove them from the title menu.

I am not sure how the scenario editor should work (I never use it myself). Maybe the easiest option would be to also add the three buttons to _nested_create_scenario_widgets ?

@DorpsGek DorpsGek temporarily deployed to preview-pr-10058 November 7, 2022 05:48 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-10058 November 7, 2022 05:53 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-10058 November 7, 2022 06:23 Inactive
@Arastais
Copy link
Contributor Author

Arastais commented Nov 7, 2022

Needs a rebase 🙂

Fixed merge conflicts then rebased👏 Should be good now since, despite the failed commit check, the commit message is in the correct format.

Is there a reason to keep the 3 buttons in the title menu? They only affect new games, and now they are in the mapgen gui. So I would remove them from the title menu.

I would think to keep them in the title menu; a couple reasons come off the top of my head. They're convenient for if you just want quick access to ai, game script, or newgrf straight from when you load the game (for example, you load the game just to check what newgrf or ais you have). Not to mention, new players probably wouldn't notice the existence of ai, game script, or newgrf as much if they're underneath having to create a new game.

I am not sure how the scenario editor should work (I never use it myself). Maybe the easiest option would be to also add the three buttons to _nested_create_scenario_widgets ?

The scenario editor has the in-game toolbar which has the settings button with the "Game Script settings" and "AI Settings" options already. Adding to _nested_create_scenario_widgets is thus not necessary because it would be redundant.

Copy link
Member

@2TallTyler 2TallTyler left a comment

Choose a reason for hiding this comment

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

I haven't delved too deep into how the code works, but here's a bunch of code style and documentation items.

The failed commit check is a format thing. You can't have more than one type. Personally I'd call this a Feature.

src/game/game_gui.cpp Outdated Show resolved Hide resolved
src/game/game_gui.cpp Outdated Show resolved Hide resolved
src/game/game_gui.cpp Outdated Show resolved Hide resolved
src/game/game_gui.cpp Outdated Show resolved Hide resolved
src/game/game_gui.cpp Show resolved Hide resolved
src/window_type.h Outdated Show resolved Hide resolved
src/intro_gui.cpp Outdated Show resolved Hide resolved
src/genworld_gui.cpp Outdated Show resolved Hide resolved
src/genworld_gui.cpp Outdated Show resolved Hide resolved
src/genworld_gui.cpp Outdated Show resolved Hide resolved
@DorpsGek DorpsGek temporarily deployed to preview-pr-10058 November 7, 2022 23:11 Inactive
@Arastais Arastais changed the title Feature, Change: [UI] Split AI/Game Script configuration windows and add them to world gen window Feature: [UI] Split AI/Game Script configuration windows and add them to world gen window Nov 7, 2022
@DorpsGek DorpsGek temporarily deployed to preview-pr-10058 November 7, 2022 23:30 Inactive
src/game/game_gui.cpp Show resolved Hide resolved
wi_rect.top = pt.y - rel_y + (this->line_height - SETTING_BUTTON_HEIGHT) / 2;
wi_rect.bottom = wi_rect.top + SETTING_BUTTON_HEIGHT - 1;

/* For dropdowns we also have to check the y position thoroughly, the mouse may not above the just opening dropdown */
Copy link
Member

Choose a reason for hiding this comment

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

It should either be changed to make sense or removed, then. No sense keeping it the way it is. 🙂

@DorpsGek DorpsGek temporarily deployed to preview-pr-10058 November 9, 2022 05:24 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-10058 November 9, 2022 05:53 Inactive
Copy link
Member

@2TallTyler 2TallTyler left a comment

Choose a reason for hiding this comment

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

If you're moving items around in the generate world GUI because of the moved Generate button, I'd suggest the following:

Left side (land generation):

  • Map size
  • Land generator
  • Terrain type
  • Variety distribution
  • Smoothness
  • Map edges

Right side (everything besides land):

  • Date
  • Sea level
  • Rivers
  • Town names
  • No. of towns
  • No. of industries

That gives a nice even 6 items on each side. 🙂

src/genworld_gui.cpp Show resolved Hide resolved
@DorpsGek DorpsGek temporarily deployed to preview-pr-10058 November 9, 2022 22:52 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-10058 November 9, 2022 23:01 Inactive
@Arastais
Copy link
Contributor Author

Arastais commented Nov 9, 2022

This went missing and didn't get re-added elsewhere. 🙁

Good catch; The merge conflict was an absolute mess so I forgot to put that back in. It's there now.

If you're moving items around in the generate world GUI because of the moved Generate button, I'd suggest the following:

Left side (land generation):

* Map size

* Land generator

* Terrain type

* Variety distribution

* Smoothness

* Map edges

Right side (everything besides land):

* Date

* Sea level

* Rivers

* Town names

* No. of towns

* No. of industries

That gives a nice even 6 items on each side. 🙂

You forgot about snow/desert coverage, so I can't do this exactly 😭. I tried to make the new game menu as consistent as possible with the heightmap generation menu, while also trying to keep these suggestions. Thus, I've moved rivers to the left half on both windows, but kept sea level on the right since that's only on the new game menu. I've also added some comments in the source code to help clarify the structure of the GUI tree and cleaned up the structure a bit.

Here's what the windows look like now:
new_game
heightmap

Copy link
Member

@2TallTyler 2TallTyler left a comment

Choose a reason for hiding this comment

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

The new layout looks great. And gives me space to do #10093. 😉

Sorry, I don't know if these are new changes to review or if I missed them before.

src/game/game_gui.cpp Outdated Show resolved Hide resolved
src/game/game_gui.cpp Outdated Show resolved Hide resolved
src/game/game_gui.cpp Show resolved Hide resolved
src/game/game_gui.cpp Outdated Show resolved Hide resolved
@DorpsGek DorpsGek temporarily deployed to preview-pr-10058 November 10, 2022 01:27 Inactive
Copy link
Member

@2TallTyler 2TallTyler left a comment

Choose a reason for hiding this comment

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

Thanks for being patient with all my nitpicks. 🙂

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.

9 participants