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

Feature: [NewGRF] vehicle var 0x4E, 'date of last station departure' #9093

Closed
wants to merge 1 commit into from

Conversation

andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Apr 24, 2021

Motivation / Problem

Extends NewGRF API with vehicle var 0x4E 'Last Station Departure Date'.
Using a "date" value makes this variable similar to use as "date of last service".
The use-cases are also similar (stuff that is refreshed/refilled/refurbished in stations, instead of in depots).

Description

  • vehicle property for long date (integer days since 1-1-0) of last station departure
  • property will be DATE_INVALID until at least one station has been departed from
  • date is set by LeaveStation()
  • newgrf var 0x4E is provided to read the property
  • var 0x4E provided in newgrf vehicle debug window
  • exceeding max year handled in same ways as "date of last service"

Limitations

  • for trains or articulated RVs, the property is only updated for lead vehicle
    • seems like TMWFTLB to recursively set an identical value on every vehicle in the consist)
    • frosch indicates this is fine
    • newgrf can access the var with PARENT scope, no problems using it
  • nml patch: Feature: support for vehicle var 4E, date_of_last_station_departure nml#211
  • needs newgrf documentation before PR is ready
  • tested with a grf, works as expected

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

@andythenorth andythenorth added the work: still in progress (draft) This Pull Request is a draft label Apr 24, 2021
@frosch123
Copy link
Member

The variable probably needs storing in savegames.

@andythenorth
Copy link
Contributor Author

The variable probably needs storing in savegames.

Done, I hope

@PeterN
Copy link
Member

PeterN commented Apr 24, 2021

Smells like a recipe for more desyncs ;)

@JGRennison
Copy link
Contributor

Wouldn't ticks or days since last departure be more useful than an absolute date?
Presumably the only reasonable thing for a NewGRF to do with this is validate that it isn't DATE_INVALID and then subtract from the current date to get a relative value.

It's probably also worth thinking about the change year cheat and the similar effect when MAX_YEAR is reached.

@frosch123
Copy link
Member

Using a "date" value makes this variable similar to use as "date of last service". The use-cases are also similar. (stuff that is refreshed/refilled/refurbished in stations, instead of in depots)

Using a constantly changing "days/ticks since" variable OTOH, adds yet another variable to be careful about wrt. desyncs. Though "desyncs" are more of a subject for callbacks, than variables. It's easier to document/detect "do not use current date in callbacks" and "do not use these N variables in callbacks".

So I vote for consistent/similar behaving variables, instead of assuming some use case.

@JGRennison
Copy link
Contributor

It's easier to document/detect "do not use current date in callbacks" and "do not use these N variables in callbacks".

It seems odd to offer something which cannot be used safely...

It's probably also worth thinking about the change year cheat and the similar effect when MAX_YEAR is reached.

To expand on this a bit, I had this in mind specifically: https://github.com/OpenTTD/OpenTTD/blob/master/src/date.cpp#L214
I thought that there was an equivalent line in the date cheat path, but apparently not.

@andythenorth
Copy link
Contributor Author

Would comparing var 4E with current date cause desync? Asking for a friend....

@JGRennison
Copy link
Contributor

Would comparing var 4E with current date cause desync? Asking for a friend....

Doing this in the running cost callback should be fine, as that is called daily, and not on load.

Callbacks which are called on load are likely to be problematic, still most of the time you won't be unlucky enough for a desync to occur.

@andythenorth
Copy link
Contributor Author

andythenorth commented Apr 28, 2021

It's probably also worth thinking about the change year cheat and the similar effect when MAX_YEAR is reached.

Now handled by duplicating approach used for "date of last service".

Possibly that should be 2 actions in 1 loop, rather than iterating all vehicles twice, but I'm out of my depth there so eh :)

@andythenorth andythenorth force-pushed the var_4E_test branch 2 times, most recently from 1ceed08 to 33b0db8 Compare June 5, 2021 10:02
src/date.cpp Outdated Show resolved Hide resolved
@@ -354,6 +354,7 @@ Vehicle::Vehicle(VehicleType type)
this->cargo_age_counter = 1;
this->last_station_visited = INVALID_STATION;
this->last_loading_station = INVALID_STATION;
this->last_station_departure_date = INVALID_DATE;
Copy link
Member

@frosch123 frosch123 Jun 5, 2021

Choose a reason for hiding this comment

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

What value is most useful for NewGRF, when the vehicle is newly built and has not yet visited a station?

  • 0 = infinitely in the past
  • INVALID_DATe = infinitely in the future (used now)
  • build-date (used by date_of_last_service)

Same question can be raised when autorenewing/replacing an engine. Is the variable copied or reset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0 or INVALID_DATE are the most useful. Does INVALID_DATE return an actual numeric value to newgrf? I can see it's -1 in src.

@@ -2274,6 +2275,7 @@ void Vehicle::LeaveStation()

HideFillingPercent(&this->fill_percent_te_id);
trip_occupancy = CalcPercentVehicleFilled(this, nullptr);
this->last_station_departure_date = _date;
Copy link
Member

Choose a reason for hiding this comment

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

date_of_last_service is applied to all vehicles in a consist (see VehicleServiceInDepot), which is on the same level as the question above:
What to return when a train has visited a station before, and is then rearranged in the depot to make a different engine the front engine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rearranging should return same value as when newly built. Changing the consist should clear this var IMO.

@michicc
Copy link
Member

michicc commented Dec 21, 2021

There's some overlap with #9693, in case this is not completely dead as a dodo yet.

@andythenorth
Copy link
Contributor Author

Not dead, just lacking insight on how to move it forward :P

@andythenorth
Copy link
Contributor Author

Closing.

The motivation for this was to provide an alternative to cargo aging rate property, which is widely misunderstood and ineffective for differentiating types of vehicle. However I don't think this var will be an effective solution for that case as the “just use cargo aging” meme is too strongly rooted now and dislodging it from the user base will be hard, plus there are diehard advocates for it that are too boring, even though multiple people can demonstrate cleanly that they are wrong 👻

As a sticking plaster I have updated the wiki to indicate that cargo aging may contain surprise: https://newgrf-specs.tt-wiki.net/wiki/Action0/Vehicles/Trains#Custom_cargo_ageing_period_.282B.29

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 work: still in progress (draft) This Pull Request is a draft
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants