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 c02ef3e4: [NewGRF] Variable 0x44 is always HZB_TOWN_EDGE for roadstops #8400
Conversation
47d49aa
to
43a7dd1
Compare
This method now checks 3 cases and then falls through to a default HZB_TOWN_EDGE. Are these 3 cases all cases? Or is there more to follow? |
I checked again and you're right, we also have cases for bridges and tunnels which don't respect the town zone. Will add those later and update the PR. |
6397820
to
dc716d2
Compare
src/tunnelbridge_map.h
Outdated
* @pre IsTileType(t, MP_TUNNELBRIDGE) | ||
* @return true if and only if the tunnel/bridge is a road tunnel/bridge | ||
*/ | ||
static inline bool IsRoadTunnelBridge(TileIndex t) |
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 feel less confident messing about in here but it made the code cleaner than replicating this check in newgrf_roadtype.cpp
, even though IsRoadTunnelBridge is currently only used there.
Should we assert in this method? I'm guessing that helps better with stack traces but not certain given the same check is made in GetTunnelBridgeTransportType()
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 think assert is not needed here. But you need to also add IsRoadTunnelBridgeTile()
(you can check how other XXXTile functions are done.
ClosestTownFromTile() itself seems to have a switch case. I wonder whether the cases here are needed at all, or whether ClosestTownFromTile() should be called unconditionally. For some reason ClosestTownFromTile() does not know the shortcut for road depots, not sure what that is about. |
Interesting - I think that latter case may be that it's old, my blame suggests the line of code for depots in ClosestTownFromTile() is from 2009. If you think the cases here are being unnecessarily fiddly about something which will work whatever the tile type, maybe it makes more sense to keep this clean and then optimise ClosestTownFromTile(). I'll give that a try locally to see if it's still happy and we can clean this up. |
I think this boils down to:
|
That makes sense. I put the optimisation for depots into |
src/newgrf_roadtype.cpp
Outdated
@@ -45,7 +46,7 @@ | |||
const Town *t = nullptr; | |||
if (IsRoadDepotTile(this->tile)) { | |||
t = Depot::GetByTile(this->tile)->town; | |||
} else if (IsTileType(this->tile, MP_ROAD)) { | |||
} else if (IsTileType(this->tile, MP_ROAD) || IsRoadStopTile(this->tile) || IsRoadTunnelBridge(this->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.
You need to check tile type before calling IsRoadTunnelBridge(this->tile)
, or just add IsRoadTunnelBridgeTile()
src/tunnelbridge_map.h
Outdated
* @pre IsTileType(t, MP_TUNNELBRIDGE) | ||
* @return true if and only if the tunnel/bridge is a road tunnel/bridge | ||
*/ | ||
static inline bool IsRoadTunnelBridge(TileIndex t) |
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 think assert is not needed here. But you need to also add IsRoadTunnelBridgeTile()
(you can check how other XXXTile functions are done.
It looks like ClosestTownFromTile() doesn't use the depot optimisation as it's called by town.h MakeDefaultName() before the depot cache has been fully set up, meaning you end up with an empty name (and this will then crash when opening the depot) I think working around that is outside the scope of this PR, I want to avoid making too many fundamental changes to things outside of the immediate problem. Based on the conversation, looks like the strategy is:
I'll put in a new version based on that. This also removes the need for adding the IsRoadTunnelBridge() and IsRoadTunnelBridgeTile() methods. |
dc716d2
to
7725a13
Compare
…stops. Fix c02ef3e: [NewGRF] Variable 0x44 is always HZB_TOWN_EDGE for road stops.
src/newgrf_roadtype.cpp
Outdated
@@ -13,6 +13,7 @@ | |||
#include "date_func.h" | |||
#include "depot_base.h" | |||
#include "town.h" | |||
#include "tunnelbridge_map.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.
Left-over ;)
7725a13
to
e9361ce
Compare
When inspecting roadtype variable 0x44 in a newgrf, it would always return HZB_TOWN_EDGE no matter where the roadstop is located. This causes inconsistent behaviour for NewGRFs which use the town zone to select different road underlays.
I think this is because the road stops have tile type MP_STATION and the check is only for MP_ROAD. This fixes the test to also try to find the closest town when the tile has type MP_STATION. Works well in testing and doesn't seem to introduce any unexpected side effects. (Although my set of GRF files to test with is limited, to my knowledge only Timberwolf's Roads inspects 0x44 to decide which type of road underlay and roadstop to display)