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 3c047b1: AIGroup.GetProfitLastYear could get values different than those displayed in gui #7918

Closed

Conversation

SamuXarick
Copy link
Contributor

@SamuXarick SamuXarick commented Jan 7, 2020

AIGroup.GetProfitLastYear was only accounting profits of vehicles which had an age higher than VEHICLE_PROFIT_MIN_AGE, which is different than what is displayed on group_gui widget WID_GL_INFO

EDIT (31 Aug 2021):
After long deliberation, I went with the solution of: store 2 values, the "all time" and the "since minimum age". For that I had to rename some functions and objects, to reflect what they are actually doing, because the documentation wasn't clear about it. For example, GroupStatistics::profit_last_year becomes GroupStatistics::profit_last_year_min_age because that's what it reflects, vehicles above the minimum age for considered profits. Another one is GetGroupProfitLastYear which becomes GetGroupProfitLastYearMinAge. These are being used for displaying the colour of the profit icons in the vehicle list window.

Then I added GroupStatistics::profit_last_year, the one that is intended to be used by the AI script function GetProfitLastYear. This profit reflects exactly the profit last year value that is displayed in GUI for a selected group. While the implementation was somewhat easy to do, there was an issue that was causing a discrepancy of values between what's shown in GUI and what the script command was getting, and it was caused by Autoreplace code.

The issue happens when trying to copy group and vehicle statistics from the old vehicle to the new vehicle. The group was getting the last year profit of the new vehicle which was still 0, before the profit was updated over from the old vehicle. The end result after autoreplace was completed, was that the group statistics didn't account for the missing profit. It was only copied between vehicles, but not when updating the group.

I solved by cirurgically updating the last year profits of the affected groups in CopyVehicleConfigAndStatistics.

@LordAro
Copy link
Member

LordAro commented Jan 8, 2020

I feel like the correct solution here is to fix what's stored in the Group object - you shouldn't have to iterate over every single vehicle

In fact, the doc-comment for the variable doesn't say anything about VEHICLE_PROFIT_MIN_AGE:

Money profit_last_year; ///< Sum of profits for all vehicles.

@SamuXarick
Copy link
Contributor Author

I found 23a2f23
Money profit_last_year; is used to display profit icons. In that situation it makes sense to consider vehiles above VEHICLE_PROFIT_MIN_AGE.

But then, in the same window, the value it displays is computed in another way:

OpenTTD/src/group_gui.cpp

Lines 572 to 598 in 6e7117e

case WID_GL_INFO: {
Money this_year = 0;
Money last_year = 0;
uint32 occupancy = 0;
size_t vehicle_count = this->vehicles.size();
for (uint i = 0; i < vehicle_count; i++) {
const Vehicle *v = this->vehicles[i];
assert(v->owner == this->owner);
this_year += v->GetDisplayProfitThisYear();
last_year += v->GetDisplayProfitLastYear();
occupancy += v->trip_occupancy;
}
const int left = r.left + WD_FRAMERECT_LEFT + 8;
const int right = r.right - WD_FRAMERECT_RIGHT - 8;
int y = r.top + WD_FRAMERECT_TOP;
DrawString(left, right, y, STR_GROUP_PROFIT_THIS_YEAR, TC_BLACK);
SetDParam(0, this_year);
DrawString(left, right, y, STR_JUST_CURRENCY_LONG, TC_BLACK, SA_RIGHT);
y += FONT_HEIGHT_NORMAL;
DrawString(left, right, y, STR_GROUP_PROFIT_LAST_YEAR, TC_BLACK);
SetDParam(0, last_year);
DrawString(left, right, y, STR_JUST_CURRENCY_LONG, TC_BLACK, SA_RIGHT);

Both methods have their uses, I don't think that one is right and the other is wrong. However, for the AI function, it should follow the 2nd method of calculating profit, because I am getting a number out of it, same way I also get a number displayed in the Group GUI.

@glx22
Copy link
Contributor

glx22 commented Jan 8, 2020

It should be possible to store 2 values, the "all time" and the "since minimum age"

@SamuXarick
Copy link
Contributor Author

It should be possible to store 2 values, the "all time" and the "since minimum age"

I couldn't do this. Everything was fine, except on autoreplace. It was getting the last profit of the new vehicle which was still 0, before the profit was copied over from the old.

@TrueBrain TrueBrain added candidate: probably not This Pull Request will most likely be closed soon size: small This Pull Request is small, and should be relative easy to process labels Dec 14, 2020
@SamuXarick SamuXarick force-pushed the group-get-profit-last-year branch 2 times, most recently from 357f94e to d80a984 Compare December 24, 2020 15:46
@SamuXarick
Copy link
Contributor Author

SamuXarick commented Jan 15, 2021

It should be possible to store 2 values, the "all time" and the "since minimum age"

I couldn't do this. Everything was fine, except on autoreplace. It was getting the last profit of the new vehicle which was still 0, before the profit was copied over from the old.

I was able to re-do this, and found a way to copy the old vehicle's last year profit to the new vehicle, by making sure the group membership copying happens after vehicle statistics copy.

I created a different branch to test it:
master...SamuXarick:group_get_profit_last_year_2
Savegame: autoreplace test.zip
AI: autoreplace test.zip

How to debug test:
1 - Savegame contains the test AI that is constantly calling AIGroup.GetProfitLastYear.
2 - AIGroup.GetProfitLastYear has two methods for retrieving the last year profit. One computes manually by iterating all vehicles in the group, the other retrieves directly from the group statistics last year profit. It compares the result of both if they match with an assert.
3 - Load the savegame and fastforward, wait for the vehicle to become old and trigger the autorenew.
4 - Once that happens, the next call from the AI to AIGroup.GetProfitLastYear should not assert.

How to make the assert fail:
5 - The last commit of that branch can be reverted to test this claim. It changes the order of vehicle statistics/group membership to their original.
6 - Repeat steps 3 and 4, and the assert will trigger when the vehicle is autorenewed.

The question now is... which is the preferred method? Personally I prefer this PR's method, over the alternative branch method (once clear of debug code, of course), because it doesn't touch many parts of the code, just for a feature only an AI would use.

@SamuXarick
Copy link
Contributor Author

SamuXarick commented Aug 31, 2021

The question now is... which is the preferred method? Personally I prefer this PR's method, over the alternative branch method (once clear of debug code, of course), because it doesn't touch many parts of the code, just for a feature only an AI would use.

I have something to say about this quote of mine: now that I first-hand experienced what it is like to experience the slowdown that is iterating all vehicles during my AI development, I come to appreciate the power of cached values.

I'm now more inclined to prefer the 2nd solution presented in my last comment. master...SamuXarick:group_get_profit_last_year_2

if (cost.Succeeded() && old_head != new_head && (flags & DC_EXEC) != 0) {
/* Copy other things which cannot be copied by a command and which shall not stay resetted from the build vehicle command */
new_head->CopyVehicleConfigAndStatistics(old_head);

/* Copy group membership */
cost.AddCost(DoCommand(0, old_head->group_id, new_head->index, DC_EXEC, CMD_ADD_VEHICLE_GROUP));
if (cost.Failed()) return cost;
Copy link
Member

Choose a reason for hiding this comment

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

The code comment over the if says never fail, by moving this here you could now have inconsistencies by skipping the calls below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can move it to below that block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I investigated this further, and there was indeed something else happening behind the scenes that resulted in miscalculated last year profits for the all group and the ungrouped group.

I present a new solution: revert these autoreplace switchings to their origin, and instead, cirurgically update the last year profits of the affected groups in CopyVehicleConfigAndStatistics. After re-testings, all the affected groups, including also a custom group now compute last year profits correctly.

@SamuXarick SamuXarick force-pushed the group-get-profit-last-year branch 2 times, most recently from c05cfca to 74fad05 Compare September 1, 2021 11:06
@2TallTyler
Copy link
Member

I'm a bit confused on what problem this is trying to solve, and how it's being done. The PR description seems to mention things which don't exist in the code (autoreplace) and I don't understand if it's unfinished or if you solved it a different way. The current diff looks fine to me but there's lots of comments discussing what looks like a previous approach.

I'm also confused about how this affects the AI API, since it doesn't seem to add a new API method for getting the profit after the minimum age or change the current API to consider age.

Could you please open a new PR, following the template, which describes the need and how it affects AIs, and then describe your current solution (without describing old solutions; it's confusing to mix the two)? That would make reviewing this possible. 🙂

@2TallTyler 2TallTyler closed this Nov 9, 2022
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: small This Pull Request is small, and should be relative easy to process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants