-
-
Notifications
You must be signed in to change notification settings - Fork 945
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
Conversation
src/ai/ai_scanner.cpp
Outdated
if (strcasecmp(ai_name, i->GetName()) == 0 && i->CanLoadFromVersion(versionParam) && (version == -1 || i->GetVersion() > version)) { | ||
version = (*it).second->GetVersion(); | ||
info = i; | ||
if (versionParam != -2) { |
There was a problem hiding this comment.
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?
2e3f65c
to
5b40c05
Compare
Friendly poke; this is currently waiting on @SamuXarick (the 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. |
007f3aa
to
f932cd5
Compare
This is currently awaiting review, again |
This pull request has been automatically marked as stale because it has not had any activity in the last month. |
f932cd5
to
e6ca553
Compare
87fba74
to
004843f
Compare
This pull request has been automatically marked as stale because it has not had any activity in the last month. |
004843f
to
e7fac8c
Compare
e7fac8c
to
3edf435
Compare
3edf435
to
425c863
Compare
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. |
PR7193 AI version tests savegame.zip There are 4 versions of the same AI.
The first AI started as the latest available, which in this case it was version 4. Version 1 and 2 have a minimum load version 1. Version 3 and 4 have a minimum load version 3. |
425c863
to
e2d1cd9
Compare
e2d1cd9
to
7af98bf
Compare
7af98bf
to
c4388af
Compare
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 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? |
This PR is trying to fix a problem that originates from the console command 'startai'.
Loading it back is where things go wrong, it will try to load AI data in this order: Here's what the documentation says about loading: 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:
If the version that was entered in the name doesn't match the version that was actually saved as, it proceeds to 2nd step:
|
b167166
to
facce03
Compare
Another issue I need to talk about is the code that corrects the name of an AI. Requirements to trigger the issue:
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(). |
facce03
to
5b6a5bb
Compare
There is a problem with the DEBUG messages presenting the version it is trying to load. 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. |
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:
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 i) It tries to load an AI with the name This means two things: a) You initially get a warning that the AI could not be found 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 :) |
Resolved with #8430 now. Tnx for the savegame, that was really useful :) |
For a detailed description of the problem, see #6468 .