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
Conversation
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: Line 32 in 190e074
|
I found 23a2f23 But then, in the same window, the value it displays is computed in another way: Lines 572 to 598 in 6e7117e
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. |
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. |
739baa5
to
d451471
Compare
d451471
to
992d197
Compare
357f94e
to
d80a984
Compare
d80a984
to
46e5c0c
Compare
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: How to debug test: How to make the assert fail: 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. |
46e5c0c
to
cf63a2b
Compare
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 |
cf63a2b
to
9c69597
Compare
src/autoreplace_cmd.cpp
Outdated
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; |
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.
The code comment over the if says never fail
, by moving this here you could now have inconsistencies by skipping the calls below.
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.
I can move it to below that block
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.
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.
c05cfca
to
74fad05
Compare
c940df3
to
baee12f
Compare
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. 🙂 |
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
becomesGroupStatistics::profit_last_year_min_age
because that's what it reflects, vehicles above the minimum age for considered profits. Another one isGetGroupProfitLastYear
which becomesGetGroupProfitLastYearMinAge
. 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 functionGetProfitLastYear
. 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
.