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

Open
wants to merge 1 commit into
base: master
from

Conversation

@SamuXarick
Copy link
Contributor

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

@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

SamuXarick commented Jan 8, 2020

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

SamuXarick commented Jan 9, 2020

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.

@SamuXarick SamuXarick force-pushed the SamuXarick:group-get-profit-last-year branch from 739baa5 to d451471 Feb 9, 2020
…n those displayed in gui

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
@SamuXarick SamuXarick force-pushed the SamuXarick:group-get-profit-last-year branch from d451471 to 992d197 Jul 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.