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 #9579: Object and HQ construction is Construction cost, not Property Ma… #9673

Merged
merged 1 commit into from Nov 8, 2021

Conversation

@2TallTyler
Copy link
Member

@2TallTyler 2TallTyler commented Nov 5, 2021

…intenance

Motivation / Problem

Per #9579:

Expected result

The costs of building objects and moving the headquarters are included in the "Construction" or "Other" sections, which do not affect the company's operating profit.

Actual result

The cost of building objects and relocating the company's seat are included in the property maintenance costs.

The cost of clearing objects is already counted under Construction.

Description

Building objects and moving the company HQ is now considered under Construction, not Property Maintenance.

Closes #9579

Limitations

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')
@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Nov 5, 2021

Hmm, this is failing the regression test. Any idea how I'd fix that?

@James103
Copy link
Contributor

@James103 James103 commented Nov 5, 2021

The failure occurs in GetQuarterlyExpenses(), which takes into account property maintenance but not construction costs, both of which this PR touches. Therefore, the regressions will need to be recorded again.

@glx22
Copy link
Contributor

@glx22 glx22 commented Nov 5, 2021

If you run regression locally it should keep the output.

@LordAro
Copy link
Member

@LordAro LordAro commented Nov 6, 2021

I know it's documented as such, but GetQuarterlyExpenses not including construction costs seems unexpected to me.

Also, I question the use of that regression test, now that it's (almost) all 0s for all 24 quarters...

@2TallTyler
Copy link
Member Author

@2TallTyler 2TallTyler commented Nov 6, 2021

I don't disagree, but I think improving regression tests would be out of scope for this PR, no?

@@ -204,7 +204,7 @@ static CommandCost ClearTile_Object(TileIndex tile, DoCommandFlag flags);
*/
CommandCost CmdBuildObject(TileIndex tile, DoCommandFlag flags, uint32 p1, uint32 p2, const std::string &text)
{
CommandCost cost(EXPENSES_PROPERTY);
CommandCost cost(EXPENSES_CONSTRUCTION);
Copy link
Member

@frosch123 frosch123 Nov 6, 2021

Choose a reason for hiding this comment

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

This also puts "purchase land" under "construction". Intended?

Copy link
Member Author

@2TallTyler 2TallTyler Nov 6, 2021

Choose a reason for hiding this comment

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

Not intended because I forgot that's an object, but I would also consider this a bug which is fixed by this PR. Currently purchasing land falls under Property Maintenance, then selling that land counts as income under Construction. Changing it to all be Construction makes better sense to me.

LordAro
LordAro approved these changes Nov 8, 2021
@LordAro LordAro merged commit e9cb9c1 into OpenTTD:master Nov 8, 2021
16 checks passed
LordAro added a commit to LordAro/OpenTTD that referenced this issue Nov 8, 2021
@LordAro LordAro mentioned this pull request Nov 8, 2021
LordAro added a commit that referenced this issue Nov 8, 2021
@2TallTyler 2TallTyler deleted the object_cost branch Nov 24, 2021
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.

5 participants