Fix: Vehicle list windows did not update when this year's profit changed #8715
Conversation
Remove caching from vehicle group object. and recalculate it whenever required instead.
|
Thanks for this fix! I should be able to try it out maybe tomorrow. However, it appears that we're doing O(n log n) recalculations (because the comparator for std::sort is called that many times) when we could have done it in O(n) (by saving the calculations on each vehicle group into an auxilliary array just before doing the sort), I wonder if it is worth doing that instead? |
|
Up to you. Doing less is always better, but it doesn't appear to have any great impact on an average game. Might be diminishing returns |
|
Nevermind, it shouldn't matter at all, since each vehicle can only be in one vehicle group, so it works out to O(n log n), which is what we used to have when not using vehicle groups at all. |
|
I tried some saves with around 200-500 buses and 20-70 groups, and it doesn't appear to be any slower than normal. Unfortunately, I don't have a save with 65535 vehicles to push it to the limit... but I would be fairly surprised if it was any slower because the time complexity works out to be the same as not grouping the vehicles at all. |
| const Vehicle *oldest = *std::max_element(this->vehicles_begin, this->vehicles_end, [](const Vehicle *v_a, const Vehicle *v_b) { | ||
| return v_a->age < v_b->age; | ||
| }); | ||
| return oldest->age; |
frosch123
Feb 26, 2021
Member
Can the list be empty? Then "oldest" is an invalid iterator.
Can the list be empty? Then "oldest" is an invalid iterator.
btzy
Feb 27, 2021
Contributor
No, each vehicle group must have at least one vehicle, since they are formed by partitioning the list of vehicles into groups.
No, each vehicle group must have at least one vehicle, since they are formed by partitioning the list of vehicles into groups.
Motivation / Problem
#8575 (update commit message when merging)
584df54 introduced Vehicle groups, which cached age, this-year-profit & last-year-profit of the groups. These groups were not (and could not be) updated when the profit of the group changed, meaning that
Description
Remove caching from vehicle group object and recalculate it whenever required instead.
Was an awful lot less effort than finding all the places where vehicle profit changes and updating the relevant group object.
Limitations
Might slow things down for group-heavy games. Needs further testing with a game with such properties. Perhaps @btzy would like to test?
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.