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

expense type passed in CommandCost form Cmds #1114

Closed
DorpsGek opened this issue Aug 8, 2007 · 15 comments
Closed

expense type passed in CommandCost form Cmds #1114

DorpsGek opened this issue Aug 8, 2007 · 15 comments
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay

Comments

@DorpsGek
Copy link
Member

DorpsGek commented Aug 8, 2007

Noldo opened the ticket and wrote:

This patch adds a member to class CommandCost for purpose of using that to return expense type from the command functions. Currently this is done via global variable _yearly_expenses_type and SET_EXPENSES_TYPE macro. AddCost function is not changed which leads to this having its expense type unchanged in the operation.

-New ExpenseType EXPENSES_INVALID is added.
-2 new constructors and a get method for the new member are added for CommandCost. The default value for expense type in costructors is EXPENSES_INVALID, which makes debuging easier. No set method is added due to personal preference and a gut feeling that this way is cleaner.
-SubtractMoneyFromAnyPlayer function is edited to fit the change. It also asserts that all costs that have money effect have a valid expense type.

-All Cmd-functions and some functions that are called from those functions and return CommadCost objects are chaged to the new system.
-Some comments are updated to reflect the changes.

So why all the fuzz?
-Getting rid of a global _yearly_expenses_type
-Making a cleaner and more trackable way to connect expense type with the relevant cost.

Attachments

Reported version: trunk
Operating system: All


This issue was imported from FlySpray: https://bugs.openttd.org/task/1114
@DorpsGek
Copy link
Member Author

Noldo wrote:

So it was buggy as hell, a lot of asserts which is a good thing too because thats why there is the EXPENSES_INVALID default. Fixed the ones I run into excluding one that really puzzles me. To find the reason for that I added an assert to the CommandCost::AddCost function to make sure that all CommandCosts that are added something have a valid ExpensesType. I had to make some changes here and there to comply with the assertion. Didn't find the original problem though so there is still atleast one assert there. I used AIs and fast-forward to get backtraces as they will step on it given enough time.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/1114#comment1869

@DorpsGek
Copy link
Member Author

Rubidium wrote:

AIs do not test all commands. Especially the onces related to ships are not tested. Another example of totally untested could would be adding/removing drive through stations.


This comment was imported from FlySpray: https://bugs.openttd.org/task/1114#comment1873

@DorpsGek
Copy link
Member Author

Noldo wrote:

Ok, thanks for the pointer I tested those myself.


This comment was imported from FlySpray: https://bugs.openttd.org/task/1114#comment1874

@DorpsGek
Copy link
Member Author

Noldo wrote:

There was a problem with rendering currencies, but I got that sorted. Also the mysterious assert hasn't appeared for a while. I'm starting to be happy with the results. Have to test more though.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/1114#comment1936

@DorpsGek
Copy link
Member Author

Rubidium wrote:

Coding-style wise:
- do not add trailing whitespace
- conform to the coding style (as on the wiki)
- I think the expense type should be before the cost
- do not "just" change CommandCost to Money as those were put there to safeguard the function from overflowing. Either make a subclass of CommandCost that is overflow safe and show that as a different patch or don't change it to Money.


This comment was imported from FlySpray: https://bugs.openttd.org/task/1114#comment2107

@DorpsGek
Copy link
Member Author

DorpsGek commented Nov 4, 2007

TrueBrain wrote:

Will there be any update? I hope so...


This comment was imported from FlySpray: https://bugs.openttd.org/task/1114#comment2597

@DorpsGek
Copy link
Member Author

DorpsGek commented Nov 4, 2007

Noldo wrote:

Ok, I updated the patch to the current trunk, as expected the new Money was the most work. Coding style is propably as bad as earlier. I'll try to find some time to make it better.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/1114#comment2601

@DorpsGek
Copy link
Member Author

DorpsGek commented Nov 4, 2007

TrueBrain wrote:

As Rubidium said, it would be nice if the first param would be the type, the second the cost. I understand this means a lot of search/replace, but it really would be for the best :)


This comment was imported from FlySpray: https://bugs.openttd.org/task/1114#comment2610

@DorpsGek
Copy link
Member Author

Noldo wrote:

Ok, finally found the time to do something about this.

I switched the order of parameters. Specific comments about the coding style would be welcome now.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/1114#comment3135

@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 3, 2008

Belugas wrote:

pretty cool result, even if it is a VERY big patch :)


This comment was imported from FlySpray: https://bugs.openttd.org/task/1114#comment3156

@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 8, 2008

Noldo wrote:

EXPENSES_INVALID -> INVALID_EXPENSES (That was the convention in the wiki)

Normal updating.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/1114#comment3188

@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 8, 2008

Rubidium wrote:

See the annotated diff.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/1114#comment3189

@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 9, 2008

Noldo wrote:

Changed the things Rubidium pointed out.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/1114#comment3195

@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 9, 2008

Rubidium wrote:

Still a lot of trailing whitespaces :(


This comment was imported from FlySpray: https://bugs.openttd.org/task/1114#comment3197

@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 9, 2008

Rubidium closed the ticket.

Reason for closing: Implemented

In r11793.


This comment was imported from FlySpray: https://bugs.openttd.org/task/1114

@DorpsGek DorpsGek closed this as completed Jan 9, 2008
@DorpsGek DorpsGek added Core flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay labels Apr 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay
Projects
None yet
Development

No branches or pull requests

1 participant