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: AI/GS settings with the flag SCRIPTCONFIG_RANDOM could be altered after loading from a savegame. #7486

Conversation

SamuXarick
Copy link
Contributor

@SamuXarick SamuXarick commented Apr 8, 2019

Non-Random AI/GS configured in the main menu with their settings left at their defaults, could have their SCRIPTCONFIG_RANDOM flagged settings randomized when loading from a saved game.

This patch anchors them as unchangeable to prevent game load from randomizing them.

@SamuXarick
Copy link
Contributor Author

The second commit is probably better than the first. Need someone to review which to pick.

@SamuXarick SamuXarick force-pushed the also-anchor-SCRIPTCONFIG_RANDOM-as-unchangeable-settings branch from b889e88 to 72a45c4 Compare April 8, 2019 17:30
@SamuXarick
Copy link
Contributor Author

This PR fixes two things now.
1 - the issue with SCRIPTCONFIG_RANDOM when loading from a savegame. Commit 1 marks such settings as unchangeable, but it is only able to do that for AIs that have already started. AIs that are yet to start that start later on would still be randomizing those settings. From what I understand, those settings are only meant to be randomized if the AI was started as Random AI, not when it was specified. So I came with Commit 2 for solving that. I found this _switch mode thing which would help exactly in preventing those settings from being randomized. However, I'm not sure preventing random deviation to occur is okay, I need to investigate this yet.

2 - I found out that random ais were not getting their start_date deviated. It appears to be a constructor issue. ScriptConfig constructor thing doesn't construct the start_date in random AI, so there was no setting at all to deviate. PushExtraConfigList calls the AI version of the function which adds the "start_date". AddRandomDeviation then calls the AI version of the function which already does the right thing, so that's why it doesn't run the 2nd part of the code.

@SamuXarick
Copy link
Contributor Author

The 2nd thing that I fix here with 72a45c4
breaks somewhere else:

https://github.com/OpenTTD/OpenTTD/pull/7486/files#diff-b1ee4283f7522a54d2b5a8c603b301f5R42

Now random deviation is done twice for Random AIs start_date.

@SamuXarick SamuXarick force-pushed the also-anchor-SCRIPTCONFIG_RANDOM-as-unchangeable-settings branch 2 times, most recently from 203e764 to 48408c2 Compare April 9, 2019 01:53
@SamuXarick
Copy link
Contributor Author

SamuXarick commented Apr 9, 2019

Commit 1 of the first thing fixed, was removed. Commit 2 was the correct way to fix it. As it stands now, commit 2 became 1, commit 3 became 2, and 4 became 3.
This last commit fixes the issue of random deviation being done twice.
This PR is now awaiting approval/review.

@SamuXarick SamuXarick force-pushed the also-anchor-SCRIPTCONFIG_RANDOM-as-unchangeable-settings branch from 48408c2 to fd246dd Compare April 10, 2019 23:13
@stale
Copy link

stale bot commented May 14, 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 May 14, 2019
@SamuXarick SamuXarick force-pushed the also-anchor-SCRIPTCONFIG_RANDOM-as-unchangeable-settings branch from fd246dd to 8ba7d38 Compare July 18, 2019 23:58
Copy link
Contributor

@nielsmh nielsmh left a comment

Choose a reason for hiding this comment

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

Am I understanding right that the core of this problem is that an AI has its settings changed (silently) during savegame-load from the settings the AI was running with before the save-load?

src/ai/ai_config.cpp Outdated Show resolved Hide resolved
src/script/script_config.hpp Show resolved Hide resolved
/* If we're in an existing game and the Script is changed, set all settings
* for the Script that have the random flag to a random value. */
for (ScriptConfigItemList::const_iterator it = this->info->GetConfigList()->begin(); it != this->info->GetConfigList()->end(); it++) {
if ((*it).flags & SCRIPTCONFIG_RANDOM) {
this->SetSetting((*it).name, InteractiveRandomRange((*it).max_value + 1 - (*it).min_value) + (*it).min_value);
}
}
this->AddRandomDeviation();
this->AddRandomDeviation(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the entire changeset even this large? Wouldn't it make more sense to just not call AddRandomDeviation at all if the script that's being restarted after load has already been started? So scripts that were active when the save was made have their settings static (all of them), and scripts that were not started yet can get randomized.

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'm not sure how to answer this.

The scripts are not started at the time ScriptConfig::Change is called from ai_sl.cpp / game_sl.cpp.

Fix from commit 1 already prevents AddRandomDeviation from being called after load. So I'm confused as to what you're requesting here.

@SamuXarick SamuXarick force-pushed the also-anchor-SCRIPTCONFIG_RANDOM-as-unchangeable-settings branch from 8ba7d38 to 87229c7 Compare November 3, 2019 17:10
@SamuXarick SamuXarick force-pushed the also-anchor-SCRIPTCONFIG_RANDOM-as-unchangeable-settings branch from 87229c7 to a5d2e75 Compare December 31, 2019 16:57
@SamuXarick
Copy link
Contributor Author

Note: if #7661 is approved, it turns commit 2 and 3 of this PR irrelevant.

@SamuXarick SamuXarick force-pushed the also-anchor-SCRIPTCONFIG_RANDOM-as-unchangeable-settings branch from a5d2e75 to a8988c1 Compare January 1, 2020 14:47
…d after loading from a savegame.

Non-Random AI/GS configured in the main menu with their settings left at their defaults, could have their SCRIPTCONFIG_RANDOM flagged settings randomized when loading from a saved game.

This patch disallows SM_LOAD_GAME switch mode from randomizing them.
@SamuXarick SamuXarick force-pushed the also-anchor-SCRIPTCONFIG_RANDOM-as-unchangeable-settings branch from a8988c1 to 02843e9 Compare February 9, 2020 20:49
@LordAro
Copy link
Member

LordAro commented Feb 29, 2020

Would like to see @nielsmh 's comment addessed before doing anything further with this

@LordAro
Copy link
Member

LordAro commented Mar 31, 2020

Closing in favour of #7661

@LordAro LordAro closed this Mar 31, 2020
SamuXarick added a commit to SamuXarick/OpenTTD that referenced this pull request Jan 7, 2023
This attempts to redo the whole start_date thing to be more sane, understandable and predictable.

- Moves 'start_date' AI parameter away from scripts losing its "per-AI" customization, into a game setting. It is now a "one for all" approach for the feature.
- Retains the random deviation behaviour for AIs with a start date different than zero
- Default value was set to that of Easy Preset.
- Also display Start Delay on AI/Game Script Config window.
- Solves all of the issues reported in OpenTTD#7486, due to the hackish attempts to deal with the 'start_date' parameter and the randomization features upon loading savegames.
SamuXarick added a commit to SamuXarick/OpenTTD that referenced this pull request Jan 7, 2023
This attempts to redo the whole start_date thing to be more sane, understandable and predictable.

- Moves 'start_date' AI parameter away from scripts losing its "per-AI" customization, into a game setting. It is now a "one for all" approach for the feature.
- Retains the random deviation behaviour for AIs with a start date different than zero
- Default value was set to that of Easy Preset.
- Also display Start Delay on AI/Game Script Config window.
- Solves all of the issues reported in OpenTTD#7486, due to the hackish attempts to deal with the 'start_date' parameter and the randomization features upon loading savegames.
SamuXarick added a commit to SamuXarick/OpenTTD that referenced this pull request Jan 7, 2023
This attempts to redo the whole start_date thing to be more sane, understandable and predictable.

- Moves 'start_date' AI parameter away from scripts losing its "per-AI" customization, into a game setting. It is now a "one for all" approach for the feature.
- Retains the random deviation behaviour for AIs with a start date different than zero
- Default value was set to that of Easy Preset.
- Also display Start Delay on AI/Game Script Config window.
- Solves all of the issues reported in OpenTTD#7486, due to the hackish attempts to deal with the 'start_date' parameter and the randomization features upon loading savegames.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Stale issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants