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

…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')

@LordAro LordAro added the backport requested This PR should be backport to current release (RC / stable) label Nov 5, 2021
LordAro
LordAro previously approved these changes Nov 5, 2021
@2TallTyler
Copy link
Member Author

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

@James103
Copy link
Contributor

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 commented Nov 5, 2021

If you run regression locally it should keep the output.

@LordAro
Copy link
Member

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 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

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

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 merged commit e9cb9c1 into OpenTTD:master Nov 8, 2021
LordAro pushed a commit to LordAro/OpenTTD that referenced this pull request Nov 8, 2021
@LordAro LordAro mentioned this pull request Nov 8, 2021
LordAro pushed a commit that referenced this pull request Nov 8, 2021
@2TallTyler 2TallTyler deleted the object_cost branch November 24, 2021 12:39
@LordAro LordAro added backported This PR is backported to a current release (RC / stable) and removed backport requested This PR should be backport to current release (RC / stable) labels Dec 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR is backported to a current release (RC / stable)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improperly assigned objects construction and HQ relocation costs
5 participants