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

Change: AI/GS Config GUI overhaul #7084

Conversation

SamuXarick
Copy link
Contributor

@SamuXarick SamuXarick commented Jan 22, 2019

List of current features for this patch:

  1. Maximum no. competitors cap have been raised from 14 to 15. This is useful for multiplayer and dedicated server games.

  2. All slots are configurable and visible, regardless of the current value of Maximum no. competitors. This means they're all highlighted in orange text in the AI/Game Script Configuration/Settings window.
    2.1. AI slots are identified with its respective Company ID number.
    2.2. Status icons may be displayed next to each slot to reflect the current state of a script.
    2.2.1. A Warning Sign indicates that a Human Company is, or will be in this slot and that this AI configuration won't be used.
    2.2.2. Yellow indicates an eligible AI or GS that is yet to start.
    2.2.3. Green indicates an AI or GS script in this slot that started and is alive.
    2.2.4. Red indicates an AI or GS script in this slot that started but died.
    2.2.5. No Yellow icons are displayed if the value of Maximum no. competitors doesn't allow more AIs to start, or if no GS is configured.
    2.3. AIs that started as random are identifiable with a newspaper icon next to the first icon.

  3. Note: While in the main menu, slot 1 may be configurable, but beware: when starting a single or multiplayer game, the first company is always human and will start on slot 1. Making slot 1 configurable from gui is especialy useful when setting AIs for multiplayer or dedicated server games.

  4. While in a game, depending on the status, some slots in the AI/Game Script Settings window have these additional behaviours:
    4.1. Warning sign - Permits AI slot movement up or down. Permits selection of another AI. Displays the "Configure" button. Permits changes to all AI parameters.
    4.2. Yellow status - Permits AI slot movement up or down. Permits selection of another AI. Displays the "Configure" button. Permits changes to all AI parameters.
    4.3. Green status - Disables AI slot movement up or down. Disables selection of another AI or GS. Displays the "Settings" button. Permits changes to some AI or GS parameters.
    4.4. Red status - Disables AI slot movement up or down. Disables selection of another AI or GS. Displays the "Configure" button. Permits changes to all AI or GS parameters.
    4.5. No status displayed - Permits AI slot movement up or down. Permits selection of another AI. Displays the "Configure" button. Permits changes to all AI parameters.

  5. While a game is running, AIs can be started or stopped from the AI/Game Script Settings window. You may also alter their parameters if you so desire:
    5.1. To stop an AI, select the AI from the list, then click Stop AI. Be aware that by doing so, you are removing the AI Company from the game.
    5.2. If the current number of running AI companies is less than the maximum number of competitors, you may start an AI on any empty slot by selecting it and clicking Start AI.
    5.3. If you are receiving a message telling that you can't start an AI, make sure you are allowing AI competitors in multiplayer games.

6. While hosting a server, AIs or GS are not going to be requested to save their data when a client attempts to join. But should you manually save the game or perform autosaves, AIs or GS are still going to be requested to save their data. This permits scripts that usually take too long to save to be useable while running a server for as long as no autosaves or manual saves are done during the session.

687474703a2f2f692e696d6775722e636f6d2f4a6470466755352e706e67

@SamuXarick SamuXarick force-pushed the max_no_competitors-=-15-v33-r27931.patch branch from 155c5e3 to a02bcaa Compare January 22, 2019 22:39
@SamuXarick
Copy link
Contributor Author

SamuXarick commented Jan 23, 2019

References to other stuff that were combined/fixed/reported with:
#6479 - already fixed in master (nothing to do)
#6478 - rejected (TODO: remove from this PR) removed
#6468 - patch combined in this PR removed
#6464 - GS Settings can be edited in SE with this PR, but nothing done about GS running in SE
#6461 - already fixed in master (nothing to do)
#6460 - already fixed in master (nothing to do)
#6453 - TODO: remove if it has already been fixed in master not fixed in master
#6452 - semi-fixed in this PR (disables Reset button) not fixed in master
#6450 - already fixed in master (nothing to do)
#6445 - nothing to do
#6438 - semi-fixed in this PR (sets the value directly, which may "not" cause desync) removed, fixed in master

@SamuXarick SamuXarick force-pushed the max_no_competitors-=-15-v33-r27931.patch branch from 708776c to 7717cff Compare January 23, 2019 15:17
@SamuXarick
Copy link
Contributor Author

SamuXarick commented Jan 23, 2019

[16:06] TODO: remove all windows closures to test them again and see if they still crash
[16:08] ... because they could have been fixed already without my knowledge

Done.

@SamuXarick SamuXarick force-pushed the max_no_competitors-=-15-v33-r27931.patch branch from 7717cff to 07620d9 Compare January 23, 2019 16:54
@PeterN
Copy link
Member

PeterN commented Jan 24, 2019

I think this is too big with too many changes. Should be split into separate PRs.

@SamuXarick SamuXarick force-pushed the max_no_competitors-=-15-v33-r27931.patch branch 3 times, most recently from 96d41fe to ab89ffe Compare January 27, 2019 23:14
@SamuXarick
Copy link
Contributor Author

SamuXarick commented Jan 27, 2019

An update on how this currently stands:

Windows closures have been re-evaluated again, in recent code, I allowed more windows'es to stay open, and invalidate their data instead, more noticeable the textfile window and the ai settings window.

Debug window now invalidates less data and closes less windowses. I seriously dislike the annoying randomly windows closure due to ai starting, crashing or changing, this was one of the goals I had in mind with the ai redesign, to keep most windows open as possible, and only close when it's strictly necessary.

The Reset button in the AI Settings list has been through some changes too. It now resets only visible and in-game flagged parameters. It was previously simply disabled. Current master behaviour regarding this is still unfixed, (see #6452).

Just now, I fixed a crash that would occur when stopping an AI that started as random while the AI Settings window was open. The problem was caused by the "start_date" parameter not being present in-time for the InvalidateData command call.

@@ -521,6 +521,7 @@ static void ResetLandscapeConfirmationCallback(Window *w, bool confirmed)
/* Now that all vehicles are gone, we can reset the engine pool. Maybe it reduces some NewGRF changing-mess */
EngineOverrideManager::ResetToCurrentNewGRFConfig();

InvalidateWindowData(WC_GAME_OPTIONS, WN_GAME_OPTIONS_AI);
Copy link
Member

Choose a reason for hiding this comment

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

Seems random to have this here. What does this solve and why at this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

screenshot 90

It's for this. the AI's are then removed, so I have to update the window. If I dont update, it's gonna reflect them as being active, with the green smiles for a while, until I drag the window around.

Copy link
Member

Choose a reason for hiding this comment

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

Can this line be moved up to where companies are deleted, as that is kind of a logical grouping for it?

uint text_width = GetStringBoundingBox(text).width;
uint text_left = rtl ? rai_left - 10 - text_width : rai_left + rai.width + 10;
uint text_right = rtl ? text_left + text_width : text_left + text_width;
DrawString(text_left, text_right, y + WD_MATRIX_TOP, text, this->selected == -1 ? TC_WHITE : TC_ORANGE);
Copy link
Member

Choose a reason for hiding this comment

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

What is going on here? Why do you care about the text width?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

screenshot 92

because of right to left text mode. Trying to make it aligned.

Copy link
Member

Choose a reason for hiding this comment

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

Look at how other code does this. You need the width of the images, not the text!

uint text_left = rtl ? rai_left - 10 - text_width : rai_left + rai.width + 10;
uint text_right = rtl ? text_left + text_width : text_left + text_width;
if (this->slot != OWNER_DEITY && static_cast<AIInfo *>((*it).second)->UseAsRandomAI()) DrawSprite(SPR_AICONFIG_RANDOM, PAL_NONE, rai_left, y + rai_y_offset);
DrawString(text_left, text_right, y + WD_MATRIX_TOP, (*it).second->GetName(), (this->selected == i - 1) ? TC_WHITE : TC_ORANGE);
Copy link
Member

Choose a reason for hiding this comment

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

As above.

@@ -223,7 +255,7 @@ struct AIListWindow : public Window {
*/
virtual void OnInvalidateData(int data = 0, bool gui_scope = true)
{
if (_game_mode == GM_NORMAL && Company::IsValidID(this->slot)) {
if (_game_mode == GM_NORMAL && Company::IsValidAiID(this->slot)) {
Copy link
Member

Choose a reason for hiding this comment

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

AI settings won't close on invalidation if the company is not an AI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When a company starts, this is called. It is okay to keep it open if it's a human company. But it can be trouble if it's an ai company, because by leaving it open, I could select another AI, which in turn would change the list of AI settings for the slot. The just started AI would be working with a config that wasn't of itself, a recipe for trouble.

@@ -1751,7 +1751,7 @@ STR_INTRO_HIGHSCORE :{BLACK}Highscor
STR_INTRO_CONFIG_SETTINGS_TREE :{BLACK}Settings
STR_INTRO_NEWGRF_SETTINGS :{BLACK}NewGRF Settings
STR_INTRO_ONLINE_CONTENT :{BLACK}Check Online Content
STR_INTRO_SCRIPT_SETTINGS :{BLACK}AI/Game Script Settings
STR_INTRO_SCRIPT_SETTINGS :{BLACK}AI/Game Script Configuration
Copy link
Member

Choose a reason for hiding this comment

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

Why change settings to configuration?

@@ -1771,7 +1771,7 @@ STR_INTRO_TOOLTIP_HIGHSCORE :{BLACK}Display
STR_INTRO_TOOLTIP_CONFIG_SETTINGS_TREE :{BLACK}Display settings
STR_INTRO_TOOLTIP_NEWGRF_SETTINGS :{BLACK}Display NewGRF settings
STR_INTRO_TOOLTIP_ONLINE_CONTENT :{BLACK}Check for new and updated content to download
STR_INTRO_TOOLTIP_SCRIPT_SETTINGS :{BLACK}Display AI/Game script settings
STR_INTRO_TOOLTIP_SCRIPT_SETTINGS :{BLACK}Display AI/Game script configuration
Copy link
Member

Choose a reason for hiding this comment

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

As above.

@@ -4057,10 +4057,11 @@ STR_ERROR_AI_DEBUG_SERVER_ONLY :{YELLOW}AI/Game

# AI configuration window
STR_AI_CONFIG_CAPTION :{WHITE}AI/Game Script Configuration
STR_AI_CONFIG_GAMELIST_TOOLTIP :{BLACK}The Game Script that will be loaded in the next game
STR_AI_CONFIG_AILIST_TOOLTIP :{BLACK}The AIs that will be loaded in the next game
STR_AI_CONFIG_CAPTION_INGAME :{WHITE}AI/Game Script Settings
Copy link
Member

Choose a reason for hiding this comment

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

But still "settings" in game?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to somehow distiguish them, main menu and in-game. I suppose I can revert these, they're really not necessary. "Configure" for when it was not started yet, "Settings" for when it has started.

@SamuXarick SamuXarick force-pushed the max_no_competitors-=-15-v33-r27931.patch branch 5 times, most recently from 92470c6 to 6b26431 Compare January 30, 2019 04:29
@SamuXarick
Copy link
Contributor Author

SamuXarick commented Jan 30, 2019

TODO: some more ideas came up in IRC.
- start as spectator, (as a dev setting for single player, and always in multiplayer) implemented removed, posted as separate
- start_date = 0 to force all AIs to start at the same time. implemented removed, due to implemented in master

@SamuXarick SamuXarick force-pushed the max_no_competitors-=-15-v33-r27931.patch branch 4 times, most recently from 752a69f to b50c185 Compare February 3, 2019 15:51
@SamuXarick SamuXarick force-pushed the max_no_competitors-=-15-v33-r27931.patch branch from b50c185 to b72bac6 Compare February 8, 2019 03:19
@SamuXarick SamuXarick force-pushed the max_no_competitors-=-15-v33-r27931.patch branch from b72bac6 to 734a0ac Compare February 17, 2019 01:22
@stale
Copy link

stale bot commented Mar 19, 2019

This pull request has been automatically marked as stale because it has not had any activity in the last month.
Please feel free to give a status update now, ping for review, or re-open when it's ready.
It will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale Stale issues label Mar 19, 2019
@SamuXarick SamuXarick force-pushed the max_no_competitors-=-15-v33-r27931.patch branch from 734a0ac to d2b374f Compare March 24, 2019 23:54
@stale stale bot removed the stale Stale issues label Mar 24, 2019
@TrueBrain
Copy link
Member

Given as most of the comments were not picked up, given stalebot already tried to close it once, but a simple rebase was given to avoid that, given I have little overview what is really happening in this PR, given it feels massive instead of doing one thing at the time, given for 2 months now no developer gave this any interest, I am going to call it a day.

Thank you for your contribution. It really is appreciated, but I am not sure these kind of PRs are the way forward. Possibly begin with smaller chunks till you have more grasp on both coding itself, as the OpenTTD code.

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

3 participants