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: Don't rerandomise a script's settings when loading from a save #8051

Closed
wants to merge 2 commits into from

Conversation

LordAro
Copy link
Member

@LordAro LordAro commented Mar 30, 2020

ScriptConfig stuff is all pretty hairy, but I'm pretty sure this is what's wanted. Besides, Samu's AfterFix AI is literally the only one on BaNaNaS that makes use of the random settings anyway.

Closes #7486

@LordAro LordAro requested a review from nielsmh March 30, 2020 22:14
@SamuXarick
Copy link
Contributor

This is making random deviation for start_date to not occur anymore

@LordAro
Copy link
Member Author

LordAro commented Mar 31, 2020

That's not what I'm seeing. Where are you expecting start_date to be randomised?

Some debug prints from when I started the AI:

FOO ScriptConfig::Change
FOO ludiai afterfix virtual void AIConfig::AddRandomDeviation()
FOO ludiai afterfix virtual void ScriptConfig::AddRandomDeviation()
AddRandomDeviation changing start_date to 690
AddRandomDeviation changing road_days_in_transit to 32

This no longer occurs on load, but I have no idea why you'd want it to change then?

@SamuXarick
Copy link
Contributor

SamuXarick commented Mar 31, 2020

I mean, random deviation for start_date, has to happen on constructor on new game.
You set a start date of 182 days in main menu, then you start a new game, and the deviation doesn't occur. It will occur when the script is starting, which is pointless by then, it has already waited 182 days.

@SamuXarick
Copy link
Contributor

SamuXarick commented Mar 31, 2020

Note that a start_date of 0 means no deviation should occur to start_date. The following table assumes start_date != 0

on new game before starting when starting
Random AI apply random deviation to start_date apply random deviation to non-start_date parameters, apply random range to parameters
Non-Random AI apply random deviation to any parameters, don't apply random range to parameters don't apply random deviation to any parameters, don't apply random range to parameters
on game load saved before it started saved after it started
Random AI don't apply random deviation to start_date don't apply random deviation nor random range to any parameters
Non-Random AI don't apply random deviation nor random range to any parameters don't apply random deviation nor random range to any parameters

These are the expected behaviours (unless I'm mistaken somewhere, this is quite confusing indeed)

@LordAro
Copy link
Member Author

LordAro commented Mar 31, 2020

I think the issue here is that start_date needs to be treated separately to all the other (random) settings.

Currently there are just a load of hacks that either call AddRandomDeviation too early, just for the purposes of setting start_date, or a load of code that undoes what AddRandomDeviation does to start_date just so it can be maintained properly.

@nielsmh
Copy link
Contributor

nielsmh commented Mar 31, 2020

Yeah I see the core issue as start_date being a parameter that logically belongs to OpenTTD rather than the AI script. It shouldn't be part of the same list of configuration settings when it doesn't belong to the AI script and follows different rules.

@glx22
Copy link
Contributor

glx22 commented Mar 31, 2020

I agree with @LordAro and @nielsmh, start_date should not be handled as a script parameter.

@SamuXarick
Copy link
Contributor

In that case, #7661 is a rework of start_date.

@LordAro
Copy link
Member Author

LordAro commented Mar 31, 2020

Alright, going to close this and #7486 in favour of #7661. I'm not very happy about the current state of that PR, but we'll get to that later...

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

4 participants