Fix reliability penalty not being removed for newly designed vehicles #3043
Conversation
|
Running the math by hand, I don't think this fully makes sense. Assume vehObject.designed = 2000, and initial reliability = 100 (for the sake of argument). When current year = 2001 (or lower): When current year = 2002 (or higher): Why check the + 3 case after checking the + 2 case? It is already known that |
|
Idk either. It was already like that in the original game code. I realised after submitting the PR that removing that if statement is something I could've done. The original code is in the issue that I linked to also. |
| @@ -153,11 +153,11 @@ namespace OpenLoco::GameCommands | |||
| newBogie->var_38 = Flags38::none; | |||
|
|
|||
| int32_t reliability = vehObject.reliability * 256; | |||
| if (getCurrentYear() + 2 > vehObject.designed) | |||
| if (vehObject.designed + 2 > getCurrentYear()) | |||
| { | |||
| // Reduce reliability by an eighth after 2 years past design | |||
There was a problem hiding this comment.
It might be nice to update (or remove) these code comments to reflect the fixed behaviour
There was a problem hiding this comment.
Changes appear to be correct by testing in-game 🙂
Displayed reliability of newly purchased Ce 2/2 Tram (in Sandbox Settler scenario):
| Year | OpenLoco v25.02 | Original (Steam) | pr/3043 |
|---|---|---|---|
| 1900 | 72% | 72% | 72% |
| 1901 | 72% | 72% | 72% |
| 1902 | 72% | 93% | 93% |
| 1903 | 72% | 93% | 93% |
And of trucks designed in 1930 (in Boulder Breakers scenario) (All five of them always had the same reliability):
| Year | OpenLoco v25.02 | Original (Steam) | pr/3043 |
|---|---|---|---|
| 1930 | 66% | 66% | 66% |
| 1931 | 66% | 66% | 66% |
| 1932 | 66% | 85% | 85% |
| 1933 | 66% | 85% | 85% |
| 1934 | 66% | 85% | 85% |
| { | ||
| // Reduce reliability by an eighth after 2 years past design | ||
| // Reduce reliability by an eighth twice until 2 years past design year |
There was a problem hiding this comment.
The reduction is more than an eighth, it works out to 15/64ths or approximately 23%
There was a problem hiding this comment.
Yeah. I did mention in the comment that the 1/8th reduction happens twice, but I'm open to changing the comment to make it clearer as to what the code does. Would "reduces reliability by an eighth, then another eighth" sound better, or something else altogether?
There was a problem hiding this comment.
Code comments in general should not describe what code does, because that is obvious from the code itself. Code comments should describe why code does something, because that meaning and context is lost over time, or was never apparent in the first place. In this case I would add a comment saying something to the effect of
"Vanilla had the intention of reducing reliability twice by 1/8 for the first 2 years, then reducing it by 1/8th once after 3 years. However a bug in the assembly meant that the 3-year condition was always true, meaning the twice-applied 1/8th penalty was always applied"
Feel free to rewrite that, but be sure to cover the 'why' this code is like this, as your future self (and others) will thank you
There was a problem hiding this comment.
I wrote a comment. I'm a bit concerned it might be too long or slightly too detailed or specific that it might end up being confusing to read. Also, should I mention that the code in OpenLoco applied the two 1/8th reductions indefinitely instead of removing them after two years or would that be unnecessary/irrelevant info for the future?
(Also, sorry for the review request. Pressing that was a mistake, especially when I hadn't changed anything since.)
There was a problem hiding this comment.
Long comments are not an issue imo as long as it's readable and concise. How about this then?
// Vanilla intended to reduce reliability by (2 * 1/8) for the first two years after year designed, then reduce reliability by (1 * 1/8) in the third year. However, a bug meant that the penalty of (2 * 1/8) was always applied.
|
This will likely need updating the testing data at https://github.com/OpenLoco/TestData/tree/master/Saves. I'll need to run it locally to check. |
38319a4 to
75d6cb9
Compare
Fixes #1689. The reliability penalty on newly designed vehicles will now be removed after two years, matching vanilla behaviour.