Fix: Equalise the number of frames needed for road vehicles to traverse different radius curves #8609

SamuXarick · 2021-01-25T18:50:14Z

Motivation / Problem

There is a story behind all this, before I started working on the PR.
It all began when I was interested in knowing why travelling vertically/horizontally was more rewarding than in a diagonal. It led me to create a testing scenario where I would run speed tests for ships, trains and road vehicles, traversing 1999 tiles in different straight directions, and using the timetable to count the number of days needed to traverse those tiles, with all vehicles running at the same max speed of 94 km/h.

Savegame: vehicle speed test large.zip

For directions along the axis (diagonal), between the 3 vehicle types, I expected the results to match. Alas, they don't. Train took 594 days, ship took 588 days and bus took 592 days. Travelling on the way back, the results maintained consistent within each vehicle type: 594 days for train, 588 days for ship, 592 days for bus.

For the other directions (horizontal and vertical), I expected train and ship times to match, but not the bus, due to it not really having a horizontal/vertical road. I tested the bus with a long curvy zig-zag road from start to finish. Between train and ship, train took 396 days and ship took 392 days. On the way back, the results were the same: 396 for train, 392 for ship.
It is now when I look at the bus times that I find an inconsistency between itself:

Going vertically, from North to South, it took 567 days, but on the way back, from South to North, it took 592 days.
Going horizontally, from West to East, it took 579 days, the same on the way back, from East to West, 579 days.

This discrepancy is what led me to investigate deep into the issue, and I find the problem is due to the different number of frames road vehicles need to go through to cross curved tiles.

Each road piece has two curves: the inner curve and the outer curve. Inner curves have fewer frames that outer curves, that's to be expected. However, different inner curves have less frames than other inner curves. The same with the outer curves, having less frames than other outer curves.

Going back to the zig-zag curves of the speed test, and reducing the number of tiles to just 2, we get:

Going vertically, from North to South, 27 frames total (17 for the outer curve + 10 for the inner curve)
Going vertically, from South to North, 29 frames total (18 for the outer curve + 11 for the inner curve)
Going horizontally, from West to East, 28 frames total (17 for the outer curve + 11 for the inner curve)
Going horizontally, from East to West, 28 frames total (18 for the outer curve + 10 for the inner curve)

I wanted to work on a solution to this issue, and what came to mind immediately was to equalise the number of frames needed to traverse the different curves. I had 4 options: "17/10", "18/11", "17/11" or "18/10".

I went on with "18 outer /11 inner" as the solution for all 4 curves, as I thought it would be easier to do savegame conversion based on adding missing frames than removing frames.

Description

To make all curves "18/11", I ended up having to invent values for the missing frames. These values are respectively the x_pos and y_pos of the vehicle. I used my best judgement to make the shift from a frame to another not to break IndividualRoadVehicleController, and the best approach that I went with, is also the one that makes the most sense, which was changing the position values by 1, and accomodate the rest of the frames for this addition/subtraction.

The most complicated part is the savegame conversion. It now has to, not only, update the current positions to match the new values for the movement data, it also has to, in the case of articulated vehicles, play catch-up. In essence, all of the affected curves gain 1 more frame, which means 1 more tick that needs to be catch-up for each trailing part. Luckily this was already done in the past, so I only had to borrow that part of the code, and adapt it to this situation.

The end result:

Looking at the bus again:

Going vertically, from North to South, it took 592 days, the same on the way back, from South to North, 592 days.
Going horizontally, from West to East, it took 592 days, the same on the way back, from East to West, 592 days.

Coincidentally, it is also identical to the diagonal directions, also with 592 days.

Everything equalised!

Limitations

I heard in the chat that changing x_pos and y_pos might break NewGRFs, at least in theory, so that might be a limitation?
Will this break SLV_188 save conversion?

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')
- newgrf_debug_data.h may need updating.
- PR must be added to API tracker

Detailed changes

(This is pseudo-code.)
original data on the left column, altered data on the middle, frame number on the right.

static const RoadDriveEntry _roadveh_drive_data_2[] = {
    {5, 0},     {5, 0},         /*  1 */
    {5, 1},     {5, 1},         /*  2 */
    {5, 2},     {5, 2},         /*  3 */
    /* turn */  {5, 3},         /*  4 */
    {4, 3},     /* turn */      /*  5 */
    {3, 4},     {4, 4},         /*  6 */
    {2, 5},     {3, 5},         /*  7 */
    /* turn */  /* turn */      /*  8 */
    {1, 5},     {2, 5},         /*  9 */
    {0, 5},     {1, 5},         /* 10 */
                {0, 5},         /* 11 */
    {RDE_NEXT_TILE | DIAGDIR_NE, 0}
};

static const RoadDriveEntry _roadveh_drive_data_10[] = {
    {0, 9},     {0, 9},         /*  1 */
    {1, 9},     {1, 9},         /*  2 */
    {2, 9},     {2, 9},         /*  3 */
    {3, 9},     {3, 9},         /*  4 */
    {4, 9},     {4, 9},         /*  5 */
    {5, 9},     {5, 9},         /*  6 */
    /* turn */  {6, 9},         /*  7 */
    {6, 8},     /* turn */      /*  8 */
    {7, 7},     {7, 8},         /*  9 */
    {8, 6},     {8, 7},         /* 10 */
    {9, 5},     {9, 6},         /* 11 */
    /* turn */  /* turn */      /* 12 */
    {9, 4},     {9, 5},         /* 13 */
    {9, 3},     {9, 4},         /* 14 */
    {9, 2},     {9, 3},         /* 15 */
    {9, 1},     {9, 2},         /* 16 */
    {9, 0},     {9, 1},         /* 17 */
                {9, 0},         /* 18 */
    {RDE_NEXT_TILE | DIAGDIR_NW, 0}
};

static const RoadDriveEntry _roadveh_drive_data_12[] = {
    {15, 5},    {15, 5},        /*  1 */
    {14, 5},    {14, 5},        /*  2 */
    {13, 5},    {13, 5},        /*  3 */
    /* turn */  {12, 5},        /*  4 */
    {12, 4},    /* turn */      /*  5 */
    {11, 3},    {11, 4},        /*  6 */
    {10, 2},    {10, 3},        /*  7 */
    { 9, 1},    { 9, 2},        /*  8 */
    /* turn */  /* turn */      /*  9 */
    { 9, 0},    { 9, 1},        /* 10 */
                { 9, 0},        /* 11 */
    {RDE_NEXT_TILE | DIAGDIR_NW, 0}
};

static const RoadDriveEntry _roadveh_drive_data_13[] = {
    {9, 15},    {9, 15},        /*  1 */
    {9, 14},    {9, 14},        /*  2 */
    {9, 13},    {9, 13},        /*  3 */
    {9, 12},    {9, 12},        /*  4 */
    {9, 11},    {9, 11},        /*  5 */
    {9, 10},    {9, 10},        /*  6 */
    /* turn */  {9,  9},        /*  7 */
    {8,  9},    /* turn */      /*  8 */
    {7,  8},    {8,  8},        /*  9 */
    {6,  7},    {7,  7},        /* 10 */
    {5,  6},    {6,  6},        /* 11 */
    {4,  5},    {5,  5},        /* 12 */
    /* turn */  /* turn */      /* 13 */
    {3,  5},    {4,  5},        /* 14 */
    {2,  5},    {3,  5},        /* 15 */
    {1,  5},    {2,  5},        /* 16 */
    {0,  5},    {1,  5},        /* 17 */
                {0,  5},        /* 18 */
    {RDE_NEXT_TILE | DIAGDIR_NE, 0}
};

static const RoadDriveEntry _roadveh_drive_data_18[] = {
    {9, 0},     {9, 0},         /*  1 */
    {9, 1},     {9, 1},         /*  2 */
    {9, 2},     {9, 2},         /*  3 */
    {9, 3},     {9, 3},         /*  4 */
    {9, 4},     {9, 4},         /*  5 */
    {9, 5},     {9, 5},         /*  6 */
    /* turn     {9, 6},         /*  7 */
    {8, 6},     /* turn */      /*  8 */
    {7, 7},     {8, 7},         /*  9 */
    {6, 8},     {7, 8},         /* 10 */
    {5, 9},     {6, 9},         /* 11 */
    /* turn */  /* turn */      /* 12 */
    {4, 9},     {5, 9},         /* 13 */
    {3, 9},     {4, 9},         /* 14 */
    {2, 9},     {3, 9},         /* 15 */
    {1, 9},     {2, 9},         /* 16 */
    {0, 9},     {1, 9},         /* 17 */
                {0, 9},         /* 18 */
    {RDE_NEXT_TILE | DIAGDIR_NE, 0}
};

static const RoadDriveEntry _roadveh_drive_data_20[] = {
    { 9, 0},    { 9, 0},        /*  1 */
    { 9, 1},    { 9, 1},        /*  2 */
    /* turn */  { 9, 2},        /*  3 */
    {10, 2},    /* turn */      /*  4 */
    {11, 3},    {10, 3},        /*  5 */
    {12, 4},    {11, 4},        /*  6 */
    {13, 5},    {12, 5},        /*  7 */
    /* turn */  /* turn */      /*  8 */
    {14, 5},    {13, 5},        /*  9 */
    {15, 5},    {14, 5},        /* 10 */
                {15, 5},        /* 11 */
    {RDE_NEXT_TILE | DIAGDIR_SW, 0}
};

static const RoadDriveEntry _roadveh_drive_data_21[] = {
    {0,  5},    {0,  5},        /*  1 */
    {1,  5},    {1,  5},        /*  2 */
    {2,  5},    {2,  5},        /*  3 */
    {3,  5},    {3,  5},        /*  4 */
    {4,  5},    {4,  5},        /*  5 */
    /* turn */  {5,  5},        /*  6 */
    {5,  6},    /* turn */      /*  7 */
    {6,  7},    {6,  6},        /*  8 */
    {7,  8},    {7,  7},        /*  9 */
    {8,  9},    {8,  8},        /* 10 */
    {9, 10},    {9,  9},        /* 11 */
    /* turn */  /* turn */      /* 12 */
    {9, 11},    {9, 10},        /* 13 */
    {9, 12},    {9, 11},        /* 14 */
    {9, 13},    {9, 12},        /* 15 */
    {9, 14},    {9, 13},        /* 16 */
    {9, 15},    {9, 14},        /* 17 */
                {9, 15},        /* 18 */
    {RDE_NEXT_TILE | DIAGDIR_SE, 0}
};

static const RoadDriveEntry _roadveh_drive_data_26[] = {
    {0, 5},     {0, 5}          /*  1 */
    {1, 5},     {1, 5}          /*  2 */
    {2, 5},     {2, 5}          /*  3 */
    /* turn */  {3, 5}          /*  4 */
    {3, 4},     /* turn */      /*  5 */
    {4, 3},     {4, 4}          /*  6 */
    {5, 2},     {5, 3}          /*  7 */
    /* turn */  /* turn */      /*  8 */
    {5, 1},     {5, 2}          /*  9 */
    {5, 0},     {5, 1}          /* 10 */
                {5, 0}          /* 11 */
    {RDE_NEXT_TILE | DIAGDIR_NW, 0}
};

frosch123 · 2021-01-27T19:03:43Z

The samegame conversion here touches all 4 of "frame", "direction", "x_pos" and "y_pos". On top of that it also calls IndividualRoadVehicleController(), and UpdateInclination().

Pretty sure changing "frame" outside of IndividualRoadVehicleController() is invalid in general, and probably breaks articulated vehicles pretty hard.
Changing "direction", "x_pos" and "y_pos" is probably unnecessary. Aren't they derived from "frame" in all cases?
Calling UpdateInclination() is either unnecessary, or it breaks even more.

In general, this PR is way too complex, and impossible to review: road vehicles have very special movement in curves, when reversing, in depots, in wormholes, ... and the consistency of articulated vehicles must be preserved in all cases.

SamuXarick · 2021-01-27T20:00:23Z

@frosch123

UpdateInclination calls UpdateViewport, which was the main intention. When the diagonal direction is modified, it is not yet reflected in the viewport, only x_pos and y_pos were. There are cases IndividualRoadVehicleController isn't called during conversion, but the viewports have to still be updated outside of it.
As for using UpdateInclination, that was because I didn't find a NewGRF with longer vehicle parts. I wanted to be on the safe side, and used UpdateInclination.
"frame", "direction", "x_pos" and "y_pos" are stored in the savegame, and the loaded data may now be incompatible with the new roadveh movement data. These changes have to be done before IndividualRoadVehicleController is called, otherwise, the vehicle can go into wrong directions, make turns where it shouldn't... it would fail and lead to crashes.

Eddi-z · 2021-01-29T02:24:34Z

<Samu> my suspicion: #8609 will break smooth turning

my professional opinion is that the chance of this happening is fairly low. they would have to have set the length of the middle part to a value that exactly matches the distance between two 45° turns, and since you say this is currently inconsistent, that sounds very unlikely.

if they simply copied the CETS approach, that should not be affected by this change.

…se different radius curves

… IndividualRoadVehicleController

2TallTyler · 2022-11-07T20:58:43Z

In general, this PR is way too complex, and impossible to review

Given this statement from @frosch123; the two years of silence on reviewing this PR; and the fact that nobody even noticed this behavior for over 15 years, much less considered it worth fixing... I'm going to close this PR. Nothing personal. If someone else disagrees, feel free to reopen or start a new PR. 😃

SamuXarick marked this pull request as draft January 25, 2021 20:00

SamuXarick force-pushed the roadveh-movement branch from bc0aadc to 8396535 Compare January 25, 2021 20:47

SamuXarick marked this pull request as ready for review January 25, 2021 20:47

SamuXarick force-pushed the roadveh-movement branch 2 times, most recently from f72f17b to 456bcd4 Compare January 26, 2021 11:19

SamuXarick marked this pull request as draft January 26, 2021 13:32

SamuXarick force-pushed the roadveh-movement branch from 456bcd4 to b8610df Compare January 26, 2021 15:34

SamuXarick marked this pull request as ready for review January 26, 2021 15:38

SamuXarick force-pushed the roadveh-movement branch from 135623b to d2e2838 Compare January 27, 2021 14:16

frosch123 added the candidate: probably not label Jan 27, 2021

SamuXarick force-pushed the roadveh-movement branch from d2e2838 to eddeff3 Compare June 6, 2021 15:59

SamuXarick force-pushed the roadveh-movement branch from eddeff3 to d254f90 Compare August 3, 2021 17:09

SamuXarick force-pushed the roadveh-movement branch from d254f90 to d6cbd4c Compare August 30, 2021 20:18

SamuXarick force-pushed the roadveh-movement branch 2 times, most recently from 058b07a to 75499ef Compare October 3, 2021 12:05

SamuXarick added 2 commits June 12, 2022 16:50

Fix: Equalise the number of frames needed for road vehicles to traver…

48d5a8d

…se different radius curves

Fix: SLV_188 conversion needs updated vehicle position before running…

0eb4927

… IndividualRoadVehicleController

SamuXarick force-pushed the roadveh-movement branch from 75499ef to 0eb4927 Compare June 12, 2022 15:56

2TallTyler closed this Nov 7, 2022

ldpl mentioned this pull request Apr 8, 2023

Fix #10493: Fix with station stop location adjusted. #10597

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Equalise the number of frames needed for road vehicles to traverse different radius curves #8609

Fix: Equalise the number of frames needed for road vehicles to traverse different radius curves #8609

SamuXarick commented Jan 25, 2021 •

edited

Loading

frosch123 commented Jan 27, 2021 •

edited

Loading

SamuXarick commented Jan 27, 2021 •

edited

Loading

Eddi-z commented Jan 29, 2021 •

edited

Loading

2TallTyler commented Nov 7, 2022

Fix: Equalise the number of frames needed for road vehicles to traverse different radius curves #8609

Fix: Equalise the number of frames needed for road vehicles to traverse different radius curves #8609

Conversation

SamuXarick commented Jan 25, 2021 • edited Loading

Motivation / Problem

Description

Limitations

Checklist for review

Detailed changes

frosch123 commented Jan 27, 2021 • edited Loading

SamuXarick commented Jan 27, 2021 • edited Loading

Eddi-z commented Jan 29, 2021 • edited Loading

2TallTyler commented Nov 7, 2022

SamuXarick commented Jan 25, 2021 •

edited

Loading

frosch123 commented Jan 27, 2021 •

edited

Loading

SamuXarick commented Jan 27, 2021 •

edited

Loading

Eddi-z commented Jan 29, 2021 •

edited

Loading