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

Lifetime profit #7919

Closed
wants to merge 3 commits into from
Closed

Conversation

SamuXarick
Copy link
Contributor

There is now a lifetime profit value against vehicles which can be viewed alongside the profit this year and profit last year.

burty - Stores lifetime profit (gets updated at end of year)
burty - Displays on the vehicle info window where the other profits are shown.
burty - Saves the lifetime profit in to the save game and loads it.
burty - Sort by lifetime profit in vehicle lists.
JGR - Display the current lifetime profit, instead of the lifetime profit at the start of the current year
JGR - Reset overall lifetime profit to 0 when renewing vehicle.
JGR - Sort by length was incorrectly available, and crashed for non ground vehicles.
Samu - Displays on the vehicle group info panel where the other profits are shown.
Samu - Displays on the vehicle list window where the other profits are shown.
Samu - Add AI/GS functions to get lifetime profits of groups and vehicles.

Copy link
Contributor

@nielsmh nielsmh left a comment

Choose a reason for hiding this comment

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

Most of these comments are just thoughts and opinions.

Before reading this code I kind of expected the lifetime profit to include the purchase cost of the vehicle/train. On the other hand, that might be difficult to keep track of with trains composed of multiple vehicles, i.e. what happens if you add or remove cars from a train, how does that update the lifetime profit? (I think that can be solved, but I'm not sure if it's worth solving.)

src/lang/english.txt Outdated Show resolved Hide resolved
src/saveload/saveload.h Outdated Show resolved Hide resolved
src/saveload/vehicle_sl.cpp Outdated Show resolved Hide resolved
src/script/api/script_group.hpp Outdated Show resolved Hide resolved
@SamuXarick SamuXarick force-pushed the lifetime-profit branch 2 times, most recently from 1c85e0c to 9ee5102 Compare January 13, 2020 13:38
@SamuXarick
Copy link
Contributor Author

SamuXarick commented Jan 13, 2020

I have a dilema - what should be the conversion approach for old savegames?

Renewed vehicles reset lifetime profit to 0, but not last year profit. It remains whatever it is.
This makes it so that vehicles before 1 year old of age from old savegames to have uncertain last year profit.

It could be a real new vehicle, or a renewed vehicle. If it's a real new vehicle, then getting its last year profit as best guess for lifetime profit is correct, no matter its age. But if the vehicle was renewed, then its last year profit was inherited from its past vehicle, and using its last year profit to determine lifetime profit, falls into the wrong side, unless the vehicle is already 1 year old at the time of conversion.

How should I do the lifetime conversion?

  • As burty had in his patch, profit_lifetime equals to 0 for every vehicle.
  • As it currently stands, best guess by using last years profit, no matter the age.
  • Using last years profit for vehicles that are already 1 year old, but 0 for the others.

@SamuXarick
Copy link
Contributor Author

Another problem: Too large profits don't fit.
screenshot113 (8)

@SamuXarick SamuXarick force-pushed the lifetime-profit branch 2 times, most recently from 3e797d9 to df1a225 Compare January 16, 2020 18:39
@SamuXarick
Copy link
Contributor Author

Better now.
Unnamed, 2036-10-18

@stormcone
Copy link
Contributor

Wouldn't it be better to make the window resizable horizontally too or to put a horizontal scrollbar at the bottom of the vehicle list? Instead of making wider the whole window by default.

@TrueBrain TrueBrain added candidate: needs considering This Pull Request needs more opinions size: extra large This Pull Request is very large; it properly needs to be split up before it can be properly reviewed labels Dec 14, 2020
There is now a lifetime profit value against vehicles which can be viewed alongside the profit this year and profit last year.

burty - Stores lifetime profit (gets updated at end of year)
burty - Displays on the vehicle info window where the other profits are shown.
burty - Saves the lifetime profit in to the save game and loads it.
burty - Sort by lifetime profit in vehicle lists.
JGR   - Display the current lifetime profit, instead of the lifetime profit at the start of the current year
JGR   - Reset overall lifetime profit to 0 when renewing vehicle.
JGR   - Sort by length was incorrectly available, and crashed for non ground vehicles.
Samu  - Displays on the vehicle group info panel where the other profits are shown.
Samu  - Displays on the vehicle list window where the other profits are shown.
Samu  - Add AI/GS functions to get lifetime profits of groups and vehicles.
@TrueBrain
Copy link
Member

This is once again a Pull Request which is impossible for us to review. It contains thoughts, it contains experiments, it contains lines of code that do not appear to belong in this PR. I am so confused, I am not even willing to look at this anymore.

Please Samu, start with small PRs, of like 1 function, 1 tiny change. Look how other people are doing it, and how much more smooth we can guide them through this process. This PR is all over the place, making it impossible to guide you in any way that will lead to a result. And this is not the first time we are telling you this .. in fact, your recent PRs show that you are not learning at all (and there is a year between those two!).

So again, I am not against this idea, but we do not have the time to go through this implementation and ensure it does not contain any game-breaking side-effects.

@TrueBrain TrueBrain closed this Dec 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: needs considering This Pull Request needs more opinions size: extra large This Pull Request is very large; it properly needs to be split up before it can be properly reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants