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
Conversation
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)); |
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.
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)); |
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.
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.
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.
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.
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.
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.
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.
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.
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.
Alright, so should I add a constant multiplier like ROAD_DEPOT_TRACKBIT_FACTOR, analogously to TUNNELBRIDGE_TRACKBIT_FACTOR, in economy_type.h
?
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.
I've amended the changes
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.
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)?
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.
Sure
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.
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.
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.
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.
8b92988
to
d261993
Compare
…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.
d261993
to
7e548fb
Compare
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.
Looks good, also thanks for the extra work removing some magic numbers.
…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.