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 #8297: Infrastructure counters for road tunnels, bridges, depots … #8454

Merged
merged 1 commit into from Dec 28, 2020

Conversation

UnsuspiciousGooball
Copy link
Contributor

…and bus and truck stops after road type conversion are now correct

The previous fix 887e948 only worked for roads and failed to consider a multiplier used for the infrastructure totals for tunnels/bridges.
Also, depots and bus/truck stops are counted as 2 road pieces on creation but were only counted as 1 road piece on conversion because the function DiagDirToRoadBits() was used, which only ever returns single-piece road segments.

Motivation / Problem

The code for converting between different road types available in NewGRFs such as RattRoads led to incorrect infrastructure totals for tunnels, bridges, depots and stops, enabling exploits such as described in #8297.

Description

For tunnels and bridges their road piece numbers are now multiplied by TUNNELBRIDGE_TRACKBIT_FACTOR and the infrastructure total isn't updated twice as was the case before.
For stops a conditional was removed which meant that they weren't registered in the infrastructure counter at all.
For bay stops and depots the function DiagDirToRoadBits(), which incorrectly reported their number of road pieces on conversion, was replaced with DiagDirToStraightRoadBits().

Limitations

The code was tested in-game as fixes were developed, and when I felt I met the goal of this issue I tested all types of road infrastructure again, so I think this issue is ultimately fixed.
It's not clear though when road type conversion should be possible, for example in towns with bad company ratings, and if road ownership should be changed when converting previously unowned roads.

Checklist for review

The fix only affects in-game logic.

@michicc
Copy link
Member

michicc commented Dec 27, 2020

Code itself looks fine on first glance, but I haven't done any tests yet.

src/road_map.cpp Outdated
}

case MP_STATION:
if (!IsRoadStopTile(tile)) return ROAD_NONE;
if (IsDriveThroughStopTile(tile)) return (GetRoadStopDir(tile) == DIAGDIR_NE) ? ROAD_X : ROAD_Y;
return DiagDirToRoadBits(GetRoadStopDir(tile));
return DiagDirToStraightRoadBits(GetRoadStopDir(tile));
Copy link
Member

@michicc michicc Dec 28, 2020

Choose a reason for hiding this comment

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

See below.

src/road_map.cpp Outdated
@@ -40,13 +40,13 @@ RoadBits GetAnyRoadBits(TileIndex tile, RoadTramType rtt, bool straight_tunnel_b
default:
case ROAD_TILE_NORMAL: return GetRoadBits(tile, rtt);
case ROAD_TILE_CROSSING: return GetCrossingRoadBits(tile);
case ROAD_TILE_DEPOT: return DiagDirToRoadBits(GetRoadDepotDirection(tile));
case ROAD_TILE_DEPOT: return DiagDirToStraightRoadBits(GetRoadDepotDirection(tile));
Copy link
Member

Choose a reason for hiding this comment

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

GetAnyRoadBits is used in various places to check for multiple things. Some of them are functions like ScriptRoad::AreRoadTilesConnected. Any place that calls this function without limiting the tile type might get a different result now. E.g. for an AI, a road would suddenly be connected to the back of a depot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how this should be dealt with. In-game, it sure looks like road depots and bay stops have 2 road bits judging by the number of vehicles that fit on their tiles, and the infrastructure counter is incremented by 2 when placing either of them; so it would make sense to go with 2 road tiles per bay stop/depot across the board.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As for an AI thinking that the back of a road depot is connected to adjacent road tiles, this doesn't seem all that different to an AI thinking that the back of a rail depot is connected to adjacent rails; and clearly this doesn't happen. So perhaps the fix used for that is applicable to road depots and bay stops as well.

Copy link
Member

Choose a reason for hiding this comment

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

Of course two road bits is the correct amount for the infrastructure count, but it is not the correct value for GetAnyRoadBits. Road bits are half-tile bits, rail track bits are full-tile, thus internally simply different.

Don't just modify GetAnyRoadBits, modify the conversion command where the value ends up at.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, so should I add a constant multiplier like ROAD_DEPOT_TRACKBIT_FACTOR, analogously to TUNNELBRIDGE_TRACKBIT_FACTOR, in economy_type.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've amended the changes

Copy link
Member

Choose a reason for hiding this comment

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

Looks good.
Can I bother you to use the two new constants instead of the hardcoded 2 in CmdBuildRoadDepot, RemoveRoadDepot (both road_cmd.cpp), CmdBuildRoadStop and RemoveRoadStop (both station_cmd.cpp)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looking at those commands now, I think it makes more sense to change ROAD_BAY_STOP_TRACKBIT_FACTOR to just ROAD_STOP_TRACKBIT_FACTOR and use it for both drive-through and bay stops, I hope that's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, that ought to have done it.
I apologize for getting a bit caught up on the details, I'm fairly new to all of this. Looking at the code, though, I think it would be nice to homogenise the code a bit since at times the same data seems to be extracted in a bunch of different ways, which sometimes don't actually agree with each other.

@TrueBrain TrueBrain added candidate: yes This Pull Request is a candidate for being merged size: small This Pull Request is small, and should be relative easy to process labels Dec 28, 2020
…depots and bus and truck stops after road type conversion are now correct

The previous fix 887e948 only worked for roads and failed to consider a multiplier used for the infrastructure totals for tunnels/bridges.
Also, depots and bus/truck stops are counted as 2 road pieces on creation but were only counted as 1 road piece on conversion because the function DiagDirToRoadBits() was used, which only ever returns single-piece road segments.
Copy link
Member

@michicc michicc left a comment

Choose a reason for hiding this comment

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

Looks good, also thanks for the extra work removing some magic numbers.

@michicc michicc merged commit 0125892 into OpenTTD:master Dec 28, 2020
@UnsuspiciousGooball UnsuspiciousGooball deleted the issue#8297 branch December 28, 2020 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: yes This Pull Request is a candidate for being merged size: small This Pull Request is small, and should be relative easy to process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants