Monthly rating / statistics issues

[Player701]

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:

Spoiler:

The last two lines in this piece of code do not seem to have any purpose (Source: MonthlyReportState.cpp, line 335):

Code: [Select]

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

Code: [Select]

if ((*k)->getActivityXcom().size() > 2)
_lastMonthsRating += (*k)->getActivityXcom().at(lastMonthOffset)-(*k)->getActivityAlien().at(lastMonthOffset);

When adding the research score (Source: MonthlyReportState.cpp, line 357):

Code: [Select]

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:

Spoiler:

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:

Code: [Select]

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:

Spoiler:

There is a comment in MonthlyReportState::calculateChanges which says (Source: MonthlyReportState, line 348):

Code: [Select]

// 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):

Code: [Select]

(*k)->newMonth(xcomTotal, alienTotal, pactScore);

And note how xcomTotal is calculated (Source: MonthlyReportState, line 355):

Code: [Select]

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:

Spoiler:

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

Spoiler:

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:

Spoiler:

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.

#5. Unused values in StatisticsState.cpp
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):

Code: [Select]

if ((*i)->isAlienBase() && (*i)->success)
{
alienBasesDestroyed++;
}
if ((*i)->isBaseDefense() && !(*i)->success)
{
xcomBasesLost++;
}

Proposed solution: pull request.

[kevL]

didn't want to leave your post hanging in the cold.

I maintain (and heavily adapt) a personal build of OxC, and rewrote the monthly turn-over code. For me, the vectors became much easier to think through by NOT pushing (0) values onto their backs until *after* doing the monthly calculations.

This required re-thinking and re-writing the entire cycle... starting at

https://github.com/kevL/0xC_kL/blob/master/src/Geoscape/GeoscapeState.cpp#L3159 (GeoscapeState::time1Month())

note that

https://github.com/kevL/0xC_kL/blob/master/src/Geoscape/MonthlyReportState.cpp#L347 (MonthlyReportState::calculateReport())

happens before the 'global' months-elapsed counter increments.

In MonthlyReportState, only Regions are tallied to get a TOTAL activity-score (since Countries are always inside regions). Their activity-counters are reset only *after* their activity-values are added to the total. After the regions are iterated over, the Countries do their thing in the countries-loop.

https://github.com/kevL/0xC_kL/blob/master/src/Savegame/Country.cpp#L183 (Country::newMonth())

But countries need special care since their newMonth() call calculates, and pushes, new values that are required for the MonthlyReport (ie, satisfaction and funding-change). Only *after* that has been done is the research-score added to the XCOM Total.

finally, the budget gets balanced:

https://github.com/kevL/0xC_kL/blob/master/src/Savegame/SavedGame.cpp#L1016 (SavedGame::balanceBudget())

----
re. tabulating totals for Statistics
decided to ignore current month and use only previous months in cumulative totals. Cash-income and expenditures are tallied when the budget gets balanced, while total-score is tallied in the MonthlyReport. I decided to show these both raw and divided by months-elapsed (for averages).


PS. The line #'s above are likely to change since i'm often tinkering with the files....

/anyway, just wanted to vote-up your analysis above. I'd think it should be adopted in one way or another

[Player701]

Thank you for you input.

I can see your code is heavily changed compared to the original version, so I'm not sure if it would help me much with analyzing the master branch. I can clearly see, however, that in your version the research bonus is indeed applied after all the countries' data has been updated, so perhaps issue #2 is indeed valid.

I'm not so fond of proposing more fundamental changes, because I usually try to follow the pricinple: "if it ain't broke, don't fix it". I usually try to take note of any suspicious behavior I observe during my playthroughs and attempt to reproduce it reliably, then try to find the part in the code which causes it and then make a report / PR (though my interest in the game is usually seasonal / periodic, and I may not always be active).

In this case, I initially noticed that the "average monthly rating" value displayed when I won the game was clearly wrong, and further investigation revealed several other potential bugs / inconsistencies which I thought I would report in one post instead of filing separate issues on the bug tracker, because: a) the forums seem to be more popular than the bug tracker, and b) the affected parts in the code are more or less closely related to each other.

Regarding totals for statistics: ignoring the current month is possible, but then what should the game display if the campaign is won / lost in less than a month? Technically, it is possible to do (perhaps not without the use of mods / cheating), so this should also be accounted for. 0 could be used as a fallback value, but this will probably look a little weird (especially since other kinds of data for the current month is not discarded).

[kevL]

i hear what you're saying and agree. my purpose was/is simply to let you take a look at the problems through a different camera, so to speak - then ofc go back to your own focus/priority.

I've seen several of your posts and think you're pretty much on the ball code-wise. But, uh, yeah what you do with it isn't really up to me;

--
ps. i suppose, re Statistics, that a tally could be kept whenever points are scored +/-, and income and expenditures done similarly - I just didn't want to try to calculate accurate days (since OxC does use real time-keeping, with all its quirks). Or perhaps, to fudge things a bit, ignore the current month, but only if a game lasts longer than a month (or perhaps a year eg.)

pps. hm, a tally could be kept of total-days also, incremented in time1Day() ... but this starts chasing a dragon's tail: how fine should the granularity be?