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
Conversation
The variable probably needs storing in savegames. |
f5b18ad
to
ec8a9a4
Compare
Done, I hope |
Smells like a recipe for more desyncs ;) |
Wouldn't ticks or days since last departure be more useful than an absolute date? It's probably also worth thinking about the change year cheat and the similar effect when MAX_YEAR is reached. |
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. |
It seems odd to offer something which cannot be used safely...
To expand on this a bit, I had this in mind specifically: https://github.com/OpenTTD/OpenTTD/blob/master/src/date.cpp#L214 |
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. |
ec8a9a4
to
25bf4e1
Compare
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 :) |
1ceed08
to
33b0db8
Compare
@@ -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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There's some overlap with #9693, in case this is not completely dead as a dodo yet. |
Not dead, just lacking insight on how to move it forward :P |
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 |
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
Limitations
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.