UPD: this should probably be moved to "Contributions->Programming". Sorry, I was lazy to scroll the forums list down a little. :-\
I have found several issues with how some of the values in the statistics window (displayed at the end of the campaign) are calculated, as well as with the monthly rating calculation code. I would like to provide a breakdown of all issues discovered as well as to propose and discuss solutions to them. Please correct me if some of the assumptions I make here are wrong; since I have only worked with the source code for a short time, it is difficult to maintain a consistent picture of all things in my head.
I'm sorry making for such a long post; I thought it would be better to collect everything in one place because the affected parts of the code are closely related. I've added some spoiler tags to reduce the visible length of the post. I hope the developers can look into it when they have the time. I can also provide pull requests to implement the solutions proposed here. In the end, I hope we all can make OpenXcom better together :)
The first group of issues is related to MonthlyReportState.cpp, in particular, to MonthlyReportState::calculateChanges.
#1. Questionable piece of code in MonthlyReportState::calculateChanges
Description:
The last two lines in this piece of code do not seem to have any purpose (Source: MonthlyReportState.cpp, line 335 (https://github.com/SupSuper/OpenXcom/blob/master/src/Geoscape/MonthlyReportState.cpp#L335)):
int lastMonthOffset = _game->getSavedGame()->getFundsList().size() - 3;
if (lastMonthOffset < 0)
lastMonthOffset += 2;
Here the game calculates the index to use in vectors which store the values necessary to compute the last month's rating. If this index ends up being negative, then 2 is added to the resulting value. Since the value returned by getFundsList() contains at least 2 values at this time (it is populated by one value at the start of the game and then with another value just before MonthlyReportState::calculateChanges is called), the check "lastMonthOffset < 0" is practically equivalent to "lastMonthOffset == -1". Therefore, lastMonthOffset will be equal to -1 + 2 = 1 if getFundsList() is 2 values long, which happens only at the end of the first game month.
However, the actual computation logic for the last month's rating is guarded by additional checks on the corresponding vectors' sizes:
When accounting for XCOM and alien activities (Source: MonthlyReportState.cpp, line 343 (https://github.com/SupSuper/OpenXcom/blob/master/src/Geoscape/MonthlyReportState.cpp#L343)):
if ((*k)->getActivityXcom().size() > 2)
_lastMonthsRating += (*k)->getActivityXcom().at(lastMonthOffset)-(*k)->getActivityAlien().at(lastMonthOffset);
When adding the research score (Source: MonthlyReportState.cpp, line 357 (https://github.com/SupSuper/OpenXcom/blob/master/src/Geoscape/MonthlyReportState.cpp#L357)):
if (_game->getSavedGame()->getResearchScores().size() > 2)
_lastMonthsRating += _game->getSavedGame()->getResearchScores().at(lastMonthOffset);
All these vectors - funds, XCOM activities, alien activities, and research scores - have the same size, because they are updated more or less at the same time (funds and research scores are updated in SavedGame::monthlyFunding, the other two are updated in MonthlyReportState::calculateChanges just before the rating is calculated). Therefore, adding 2 to lastMonthOffset if it ends up being negative does not serve any purpose at all - it will only end up negative if the size is 2, and if that is true, the "if" branches in the last two snippets will not be executed, effectively leaving the last month's rating at 0.
Proposed solution:
Depends on what exactly should the last month's rating be equal to after the first month. In both cases, the last two lines in the first code snippet should be removed.
If the last month's rating at the end of the first month should be the same as the current month's rating, then calculate lastMonthOffset as follows:
int lastMonthOffset = std::max(0, _game->getSavedGame()->getFundsList().size() - 3);
Also, both "if" checks in the last two snippets above should be removed.
If the last month's rating at the end of the first month should be 0 (current behavior), then replace both "if" conditions in the last two snippets above with "lastMonthOffset >= 0".
#2. Possible oversight in MonthlyReportState::calculateChanges
Description:
There is a comment in MonthlyReportState::calculateChanges which says (Source: MonthlyReportState, line 348 (https://github.com/SupSuper/OpenXcom/blob/master/src/Geoscape/MonthlyReportState.cpp#L348)):
// apply research bonus AFTER calculating our total, because this bonus applies to the council ONLY,
// and shouldn't influence each country's decision.
However, what the comment says seem to contradict what the code below actually does. Specifically, it passes the value of xcomTotal in a loop to Country::newMonth when sending the rating information to all countries (Source: MonthlyReportState, line 380 (https://github.com/SupSuper/OpenXcom/blob/master/src/Geoscape/MonthlyReportState.cpp#L380)):
(*k)->newMonth(xcomTotal, alienTotal, pactScore);
And note how xcomTotal is calculated (Source: MonthlyReportState, line 355 (https://github.com/SupSuper/OpenXcom/blob/master/src/Geoscape/MonthlyReportState.cpp#L355)):
xcomTotal = _game->getSavedGame()->getResearchScores().at(monthOffset) + xcomSubTotal;
xcomTotal includes the research score, which is in turn passed to all countries. However, the comment above says that the research bonus applies to the council only, and shouldn't influence each country's decision. This is probably an oversight.
Proposed solution:
If what the code does is actually wrong, then pass the value of xcomSubTotal instead of xcomTotal when updating the data for the countries.
The next set of issues is related to the statistics window, which shows various statistical information to the player after the game has ended.
#3. Average monthly rating is incorrectly calculated based on research scores only
Description: See issue #1409 (https://openxcom.org/bugs/openxcom/issues/1409) on the bug tracker.
Proposed solution: depends on the solution to the following issue #4. See below.
#4. Consequences of the limit on the number of values stored in certain vectors in the save file
Description:
The total income and expenditure values are based on the data for the last 12 months only. This is because of how many individual values are stored in the save file, which is at most 12. This behavior is not a bug per se (it seems to be very much intended according to the comments in the code), but if the campaign has lasted for more than 12 months, then summing these values up will not result in the correct totals for the whole campaign. The same issue applies to various other values, including but not limited to: funds, research scores and XCOM and alien activity in regions and countries.
Proposed solution:
There are at least two possible ways to solve this as well as #3.
- Store cumulative values for income, expenditure and monthly score in the save file. These values can be updated in MonthlyReportState::calculateChanges (monthly score) and / or SavedGame::monthlyFunding (income and expenditure). It is probably most consistent to update the corresponding cumulative value at the same time when calculating / pushing the normal value. Then, when calculating total income and total expenditure, the resulting cumulative values can be used. The current month should also be accounted for, since the cumulative values are only updated at the end of a game month. This means that for incomes / expenditures we need to also add the last values from the corresponding vectors to the respective cumulative values, and for the monthly score we have to calculate it based on the current research score as well as XCOM and alien activities in the current month.
- Lift the limit of 12 values to store in the corresponding vectors. For consistency, it would probably be best to lift this limit on all vectors where it is currently present. This will also necessitate some changes in GraphState.cpp so that only the last 12 values are used when calculating graph data. Then, when calculating total income and expenditure, no change of the current code is required. However, the monthly rating calculation will still need fixing to account for XCOM an alien activities in addition to research scores.
Description: There are at least two values that are calculated but not used when displaying the statistics window. These are, respectively, the number of alien bases destroyed and the number of XCOM bases lost. They are calculated when iterating over the mission statistics vector (Source: StatisticsState.cpp, line 147 (https://github.com/SupSuper/OpenXcom/blob/master/src/Menu/StatisticsState.cpp#L147)):
if ((*i)->isAlienBase() && (*i)->success)
{
alienBasesDestroyed++;
}
if ((*i)->isBaseDefense() && !(*i)->success)
{
xcomBasesLost++;
}
Proposed solution: pull request (https://github.com/SupSuper/OpenXcom/pull/1197).