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: Equalise the number of frames needed for road vehicles to traverse different radius curves #8609

Closed
wants to merge 2 commits into from

Conversation

SamuXarick
Copy link
Contributor

@SamuXarick SamuXarick commented Jan 25, 2021

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.

Vehicle Speed Test Large, 2104-02-22

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.

outer-inner frame count

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)

zig-zag counting

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:
Vehicle Speed Test Large, 2077-04-21

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

Detailed changes

_roadveh_drive_data

(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}
};

@SamuXarick SamuXarick marked this pull request as draft January 25, 2021 20:00
@SamuXarick SamuXarick marked this pull request as ready for review January 25, 2021 20:47
@SamuXarick SamuXarick force-pushed the roadveh-movement branch 2 times, most recently from f72f17b to 456bcd4 Compare January 26, 2021 11:19
@SamuXarick SamuXarick marked this pull request as draft January 26, 2021 13:32
@SamuXarick SamuXarick marked this pull request as ready for review January 26, 2021 15:38
@frosch123
Copy link
Member

frosch123 commented Jan 27, 2021

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.

@frosch123 frosch123 added the candidate: probably not This Pull Request will most likely be closed soon label Jan 27, 2021
@SamuXarick
Copy link
Contributor Author

SamuXarick commented Jan 27, 2021

@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
Copy link
Contributor

Eddi-z commented Jan 29, 2021

<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.

@2TallTyler
Copy link
Member

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. 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: probably not This Pull Request will most likely be closed soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants