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 c02ef3e4: [NewGRF] Variable 0x44 is always HZB_TOWN_EDGE for roadstops #8400

Merged
merged 1 commit into from Dec 21, 2020

Conversation

mattkimber
Copy link
Contributor

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)

src/newgrf_roadtype.cpp Outdated Show resolved Hide resolved
@TrueBrain TrueBrain added candidate: needs considering This Pull Request needs more opinions needs review: NewGRF Review requested from a NewGRF expert size: trivial This Pull Request is trivial labels Dec 20, 2020
@frosch123
Copy link
Member

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?

@mattkimber
Copy link
Contributor Author

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.

@mattkimber mattkimber force-pushed the roadstop-town-zone branch 2 times, most recently from 6397820 to dc716d2 Compare December 21, 2020 20:39
* @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)
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 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()

Copy link
Contributor

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.

@frosch123
Copy link
Member

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.

@mattkimber
Copy link
Contributor Author

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.

@frosch123
Copy link
Member

I think this boils down to:

  • Some tiletypes "cache" an index to the nearest town, while for other tiles an expensive tile search is needed.
  • The code for roadtype variable 0x44 was copied from railtypes.
  • The common case for railtypes is a plain rail tile, which does not cache the nearest town. So for performance reasons it was decided to not give access to the town zone for rail tiles, except for those cases where it is cheap (level crossing + depot).
  • The common case for roadtypes is a plain road tile, which does cache the nearest town. So there is no need to restrict access to the town zone for road tiles.

@mattkimber
Copy link
Contributor Author

That makes sense. I put the optimisation for depots into town_cmd.cpp, but it appears to crash shortly after a depot is built. Looking to see why that is.

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

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

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

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.

@mattkimber
Copy link
Contributor Author

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:

  • Keep the depot tile check in newgrf_roadtype.cpp, to take advantage of the cache if possible.
  • For all other tiles, don't try to check they are in a list of particular types, just call ClosestTownFromTile() and let it sort things out. (I'm assuming the performance hit of finding the nearest town is okay for tunnels, bridges and roadstops?)

I'll put in a new version based on that. This also removes the need for adding the IsRoadTunnelBridge() and IsRoadTunnelBridgeTile() methods.

…stops.

Fix c02ef3e: [NewGRF] Variable 0x44 is always HZB_TOWN_EDGE for road stops.
@@ -13,6 +13,7 @@
#include "date_func.h"
#include "depot_base.h"
#include "town.h"
#include "tunnelbridge_map.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Left-over ;)

@frosch123 frosch123 merged commit 6c3a5b5 into OpenTTD:master Dec 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: needs considering This Pull Request needs more opinions needs review: NewGRF Review requested from a NewGRF expert size: trivial This Pull Request is trivial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants