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

This comment has been minimized.

Copy link
Contributor Author

SamuXarick commented Apr 8, 2019

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

@SamuXarick SamuXarick force-pushed the SamuXarick:also-anchor-SCRIPTCONFIG_RANDOM-as-unchangeable-settings branch from b889e88 to 72a45c4 Apr 8, 2019
@SamuXarick

This comment has been minimized.

Copy link
Contributor Author

SamuXarick commented Apr 8, 2019

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

This comment has been minimized.

Copy link
Contributor Author

SamuXarick commented Apr 8, 2019

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 SamuXarick:also-anchor-SCRIPTCONFIG_RANDOM-as-unchangeable-settings branch 2 times, most recently from 203e764 to 48408c2 Apr 9, 2019
@SamuXarick

This comment has been minimized.

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 SamuXarick:also-anchor-SCRIPTCONFIG_RANDOM-as-unchangeable-settings branch from 48408c2 to fd246dd Apr 10, 2019
@stale

This comment has been minimized.

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 label May 14, 2019
@SamuXarick SamuXarick force-pushed the SamuXarick:also-anchor-SCRIPTCONFIG_RANDOM-as-unchangeable-settings branch from fd246dd to 8ba7d38 Jul 18, 2019
Copy link
Contributor

nielsmh left a comment

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
@@ -139,7 +139,7 @@ class ScriptConfig {
/**
* Randomize all settings the Script requested to be randomized.
*/
virtual void AddRandomDeviation();
virtual void AddRandomDeviation(bool all = true);

This comment has been minimized.

Copy link
@nielsmh

nielsmh Sep 28, 2019

Contributor

Missing documentation comment update.

This comment has been minimized.

Copy link
@LordAro

LordAro Nov 23, 2019

Member

Specifically, it should use the doxygen @param all ... format

/* 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);

This comment has been minimized.

Copy link
@nielsmh

nielsmh Sep 28, 2019

Contributor

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.

@SamuXarick SamuXarick force-pushed the SamuXarick:also-anchor-SCRIPTCONFIG_RANDOM-as-unchangeable-settings branch from 8ba7d38 to 87229c7 Nov 3, 2019
@SamuXarick SamuXarick force-pushed the SamuXarick:also-anchor-SCRIPTCONFIG_RANDOM-as-unchangeable-settings branch from 87229c7 to a5d2e75 Dec 31, 2019
@SamuXarick

This comment has been minimized.

Copy link
Contributor Author

SamuXarick commented Jan 1, 2020

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

@SamuXarick SamuXarick force-pushed the SamuXarick:also-anchor-SCRIPTCONFIG_RANDOM-as-unchangeable-settings branch from a5d2e75 to a8988c1 Jan 1, 2020
SamuXarick added 3 commits Apr 8, 2019
…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 SamuXarick:also-anchor-SCRIPTCONFIG_RANDOM-as-unchangeable-settings branch from a8988c1 to 02843e9 Feb 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.