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 #6468: Load correct version of AI as specified during the time of its save. #7193

Closed

Conversation

SamuXarick
Copy link
Contributor

@SamuXarick SamuXarick commented Feb 7, 2019

For a detailed description of the problem, see #6468 .

src/ai/ai_scanner.cpp Outdated Show resolved Hide resolved
if (strcasecmp(ai_name, i->GetName()) == 0 && i->CanLoadFromVersion(versionParam) && (version == -1 || i->GetVersion() > version)) {
version = (*it).second->GetVersion();
info = i;
if (versionParam != -2) {
Copy link
Member

Choose a reason for hiding this comment

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

This is also basically identical to the GS version, I think differing only in AIInfo & GameInfo types? Can this be split out into a (templated?) function?

@TrueBrain
Copy link
Member

Friendly poke; this is currently waiting on @SamuXarick (the author).

@SamuXarick
Copy link
Contributor Author

#6468

@SamuXarick
Copy link
Contributor Author

I need several versions of the same AI to re-test this. I have my own AI uploaded to bananas which would be useful, but I can only download the latest version. I thought I could download old versions at any time, so I deleted my old versions from disk.

@SamuXarick
Copy link
Contributor Author

This is currently awaiting review, again

@stale
Copy link

stale bot commented Apr 3, 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 Apr 3, 2019
@stale stale bot removed the stale Stale issues label Apr 7, 2019
@SamuXarick SamuXarick force-pushed the load-specific-ai-version branch 2 times, most recently from 87fba74 to 004843f Compare April 13, 2019 22:39
@stale
Copy link

stale bot commented May 13, 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 13, 2019
@nielsmh
Copy link
Contributor

nielsmh commented Apr 7, 2020

I think we need a test case for this. The test case should take the form of two versions of an AI script where the second is an upgrade of the first, a save file which uses the first version, and a save file which uses the second version. The test would involve loading the first save and verifying the script version loaded, loading the second save and verifying the version loaded, then starting a new game with that AI enabled, and verifying it uses the latest version. Possibly also testing that the presence of other AIs does not affect this.

The test AI scripts for a test case should be written specifically for that test case, they should not be based off (or copies of) existing scripts.

@SamuXarick
Copy link
Contributor Author

SamuXarick commented Apr 8, 2020

PR7193 AI version tests savegame.zip
PR 7193 test AIs.zip

There are 4 versions of the same AI.
The savegame contains 5 AIs started. They were started like this:

  • startai pr7193
  • startai pr7193.1
  • startai pr7193.2
  • startai pr7193.3
  • startai pr7193.4

The first AI started as the latest available, which in this case it was version 4.
The other 4 AIs will try to load the exact version as the version they started with (1, 2, 3, 4), or the latest version that is compatible with the saved version.

Version 1 and 2 have a minimum load version 1. Version 3 and 4 have a minimum load version 3.

@TrueBrain TrueBrain added candidate: needs considering This Pull Request needs more opinions size: large This Pull Request is large in size; review will take a while and removed stale Stale issues labels Dec 14, 2020
@TrueBrain
Copy link
Member

This Pull Request puzzles me for several reasons. First of all, the only thing I have to go on is the commit message, as the patch is hard to read and there is (yes @SamuXarick , once again) no description or reasoning.

Second, the AI framework has MinVersionToLoad, which should control what AI version is loaded upon loading a savegame. If this is broken, what this PR kinda suggests (dunno, guessing), I would expect fixes to be done around there. But the lines of code are in a completely different place.

Lastly, I have noticed problems with versions of scripts and libraries being loaded weird and/or wrong. It might be that this PR tries to fix an underlying problem; this needs further investigation, but as I am not sure what this PR tries to fix, it might be completely unrelated.

So that brings me to the big question:

What is this PR trying to fix?

@TrueBrain TrueBrain removed the candidate: needs considering This Pull Request needs more opinions label Dec 24, 2020
@TrueBrain TrueBrain added candidate: probably not This Pull Request will most likely be closed soon work: missing intention This Pull Request is missing "why" it exists labels Dec 24, 2020
@SamuXarick
Copy link
Contributor Author

SamuXarick commented Dec 24, 2020

This PR is trying to fix a problem that originates from the console command 'startai'.

  • 'startai' allows to start different versions of the same AI by using 'startai ai_name.ver', provided they're available on disk (example: starting version 4 of WormAI, we type 'startai WormAI.4')
  • saving a game where an AI was started in this manner, will save the name of the AI as 'ai_name.ver' (saves the name of the AI as 'WormAI.4')

Loading it back is where things go wrong, it will try to load AI data in this order:
1st - will try to load an AI from disk with the name matching exactly 'ai_name.ver' (tries to load 'WormAI.4'), but it doesn't find it.
2nd - it then proceeds to load the latest version of the AI of the same name. In this instance, it strips 'ai_name.ver' into 'ai_name' (tries to load 'WormAI'), and finds it. It proceeds to load the latest version available on disk.

Here's what the documentation says about loading:
1st - try to load the exact same version of the same script.
2nd - if that fails, then try to load the latest version of the same script that supports loading data from the saved version (the version of saved data must be equal or greater than AIInfo::MinVersionToLoad).

The correct behaviour, given that the savegame data contains the 'ai_name' and the 'ver', is that it should not fail on the 1st step. I think my PR addresses the issue. It corrects the loading behaviour of the 1st try:

  • splits 'ai_name.ver' into 'ai_name' for the name, and 'ver' for the version
  • if the version that was entered in the name matches the version that was actually saved as, it'll load the AI 'ai_name' with version 'ver' from disk.

If the version that was entered in the name doesn't match the version that was actually saved as, it proceeds to 2nd step:

  • it'll load the AI 'ai_name' with the latest version available from disk.

@SamuXarick SamuXarick force-pushed the load-specific-ai-version branch 2 times, most recently from b167166 to facce03 Compare December 25, 2020 10:38
src/script/script_config.cpp Outdated Show resolved Hide resolved
@SamuXarick
Copy link
Contributor Author

SamuXarick commented Dec 25, 2020

Another issue I need to talk about is the code that corrects the name of an AI.

Requirements to trigger the issue:

  • Loading, or using 'startai' command, to start a specified version of an AI that doesn't exist in disk where a newer version exists. (example: 'startai wormai.5', where version 6 is the only one installed)
  • Creating a savegame ends up storing two versions, one in the manner 'ai_name.ver', and the other by retrieving it via info->GetVersion() (stores with the name 'wormai.5' and version info '6')

The PR fixes this, by correcting the name of the AI, by striping the version from the name ('wormai.5' becomes 'wormai') at the time of it's configuration.

Saving a game afterwards will store the name 'wormai', with version '6' from GetVersion().

@SamuXarick
Copy link
Contributor Author

There is a problem with the DEBUG messages presenting the version it is trying to load.
Mainly... when it's working with possible 3 different version reports: -1, the version in the name, and the actual version of the script.
Version being -1 is caused by my workaround to pass the correct variables to FindInfo, so there's that being my PR's fault. I've just dealt accordingly and a version of -1 is no longer going to show up in the debug message.

But then there's the "version in the name" vs "actual version saved", which one to report for the DEBUG message.

With this PR, correcting the name of the AI prevents a mismatch between these two versions.
But savegames prior to this PR may exhibit this mismatching in versions.

@TrueBrain
Copy link
Member

TrueBrain commented Dec 25, 2020

The AI loading code is really a true gem. I did some investigation of what is going on here really, and basically, your PR is fixing symptons, not the real issue. What is happening:

There are 2 ways to load AIs:

  1. the normal way, via the GUI or via console, where you just start an AI by its name. Example: myai
  2. via console by stating explicit the version to load. Example: myai.1

After loading an AI, the game finds the most suitable script to use. In case of 1) this is the latest version, in case of 2) is that exact version .. kinda of (I will get back to that in a bit). It now stores the version next to the name of the AI it loaded. Upon save, this information is stored in the savegame.

When loading the savegame, it tries to find back the AIs it was saved with.

For 1), it tries to find the latest version of myai that is compatible with the version stored (via MinVersionToLoad).
For 2), it completely fails in so many ways it is not even funny. The following sequences kicks in:

i) It tries to load an AI with the name myai.1 with version 1, which is very unlikely to exist
ii) It tries to load an AI with the name myai.1 with version -1, where -1 means "latest" in this context
ii.a) Special code kicks in, that if version is -1 and there is a dot in the name where the right value is an integer, it changes the name to myai and the version to 1 instead
ii.b) It does not set the boolean to force an exact match, so it tries to find the latest AI that is compatible with myai version 1.

This means two things:

a) You initially get a warning that the AI could not be found
b) The AI, even the exact version you meant, could be loaded after all

Your PR does somewhat completely work around the whole issue, by adding more exceptions. But the problem really is that starting an AI from console with an exact version is processed really strange in OpenTTD. There are more bugs around this flow than I care to count, and we should try to address them. I have some code that tries to mitigate some of the issues, but it is really difficult :P

Anyway, I am still working on this, but just an in-between analysis what is going on, why the initial bug found what he found, and why I am struggling with this PR :)

@TrueBrain
Copy link
Member

Resolved with #8430 now. Tnx for the savegame, that was really useful :)

@TrueBrain TrueBrain closed this Dec 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: probably not This Pull Request will most likely be closed soon size: large This Pull Request is large in size; review will take a while work: missing intention This Pull Request is missing "why" it exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants