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 #10334: Store separate newgrf-safe version of date_of_last_service #11124

Merged
merged 1 commit into from Aug 6, 2023

Conversation

2TallTyler
Copy link
Member

Motivation / Problem

When a player uses the date cheat, it shifts the date_of_last_service for all vehicles. This variable is exposed to NewGRF authors, who can cause crashes by doing creative things with said variable, like changing #10334.

Also, my ongoing NotDayLength work (#10700) will require NewGRFs to read the calendar date while servicing is based on the economy date.

Description

If we provide a separate date_of_last_service_newgrf to the NewGRF API, we can solve both needs. This was started by @PeterN in #10721 — thanks for that! 😄 I rebased, made a few small changes, and (unlike #10721) actually tested to make sure it works! It does indeed fix #10334. 😉

I don't know if this is the right approach, but it's time to figure it out so I can move forward with #10700.

Note: NotDayLength will need a similar split with the build_year property of vehicles, since aging and renewal is based on economy time while NewGRFs need a stable calendar year for their mischef. That would be orphaned now, so I'll save it for when we actually split off economy time.

Closes #10334.
Supersedes and closes #10721.

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 2TallTyler added preview This PR is receiving preview builds size: small This Pull Request is small, and should be relative easy to process needs review: NewGRF Review requested from a NewGRF expert labels Jul 9, 2023
src/vehicle.cpp Outdated Show resolved Hide resolved
@2TallTyler 2TallTyler temporarily deployed to preview July 9, 2023 19:25 — with GitHub Actions Inactive
@2TallTyler 2TallTyler temporarily deployed to preview July 14, 2023 14:34 — with GitHub Actions Inactive
…_service.

This value is not changed when the date cheat is used, which caused issues with changing properties based on service date.

Co-authored-by: Peter Nelson <peter1138@openttd.org>
Copy link
Member

@michicc michicc left a comment

Choose a reason for hiding this comment

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

Looks sound to me, but my NewGRF patch track record is spotty, so take this with a big spoon of salt.

@2TallTyler
Copy link
Member Author

No dissenters in the month since I opened this (or in #10721, or in Discord), so I guess I'll merge it since I'm in a tidying-up sort of mood. 😃

@2TallTyler 2TallTyler merged commit 9a602ff into OpenTTD:master Aug 6, 2023
20 checks passed
@2TallTyler 2TallTyler deleted the fix-10334 branch August 6, 2023 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review: NewGRF Review requested from a NewGRF expert preview This PR is receiving preview builds size: small This Pull Request is small, and should be relative easy to process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Crash]: Unsafe NewGRF use of date_of_last_service crashes after using date cheat
3 participants