-
-
Notifications
You must be signed in to change notification settings - Fork 957
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
Change: Allow dock to be constructed in more locations #6926
Conversation
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 don't agree it's a good idea to allow the buoy and one-corner-raised cases. The aqueduct case is fine.
We recently switched from Jenkins as CI to Azure Pipelines as CI. This means you need to rebase before this Pull Request will pass its checks. Sorry for the troubles! |
184efb1
to
bc6799a
Compare
Why not? you try to place a dock on those places and you are denied, even though there's water there |
src/station_cmd.cpp
Outdated
@@ -2506,8 +2507,18 @@ CommandCost CmdBuildDock(TileIndex tile, DoCommandFlag flags, uint32 p1, uint32 | |||
if (ret.Failed()) return ret; | |||
|
|||
tile_cur += TileOffsByDiagDir(direction); | |||
if (!IsTileType(tile_cur, MP_WATER) || !IsTileFlat(tile_cur)) { | |||
return_cmd_error(STR_ERROR_SITE_UNSUITABLE); | |||
if (!IsTileType(tile_cur, MP_WATER)) { |
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'd like to see a comment at the top of this large number of if/else conditions summarising what it's doing
I also suspect it's structure could be improved a bit, using else if
, but unsure
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.
While I agree that it could be nicer structured, I'm not sure another structure would make it better readable... maybe with in-place comments inside the condition of what of the larger idea is currently being checked?
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 the restructuring is a must, as I cannot make heads nor tails from this. Especially the cascading of if/else is difficult to read. And as that often goes, also unneeded.
For example, one could use functions to improve readability. Or one could invert the code (say what is allowed instead of what not). Many choices here. I would prefer we fix it, not giving our future-self a headache ;)
dcc4adf
to
ec99684
Compare
We don't need docks in these cases. On that basis, I'm closing this one. Thanks for contributing! |
You really should reconsider this. |
Sure, it looks odd. But I don't think that it's a new issue really triggered by this patch. |
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.
Anyhow, the situations now enabled could be gotten to by terraforming / building the other stuff after dock building in current master, too - so this is fine and makes ship handling easier.
src/station_cmd.cpp
Outdated
@@ -2506,8 +2507,18 @@ CommandCost CmdBuildDock(TileIndex tile, DoCommandFlag flags, uint32 p1, uint32 | |||
if (ret.Failed()) return ret; | |||
|
|||
tile_cur += TileOffsByDiagDir(direction); | |||
if (!IsTileType(tile_cur, MP_WATER) || !IsTileFlat(tile_cur)) { | |||
return_cmd_error(STR_ERROR_SITE_UNSUITABLE); | |||
if (!IsTileType(tile_cur, MP_WATER)) { |
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.
While I agree that it could be nicer structured, I'm not sure another structure would make it better readable... maybe with in-place comments inside the condition of what of the larger idea is currently being checked?
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.
In general I am not in favour of these kind of patches, as it adds a lot of special cases that easily break. I would think there is a more elegant way to solve this same issue. For example, by checking if a ship can enter the third tile from the direction the dock is in (as I am guessing that is the intended functionality here).
But okay .. I am not that deep in the code to say anything conclusive about that.
src/station_cmd.cpp
Outdated
@@ -2506,8 +2507,18 @@ CommandCost CmdBuildDock(TileIndex tile, DoCommandFlag flags, uint32 p1, uint32 | |||
if (ret.Failed()) return ret; | |||
|
|||
tile_cur += TileOffsByDiagDir(direction); | |||
if (!IsTileType(tile_cur, MP_WATER) || !IsTileFlat(tile_cur)) { | |||
return_cmd_error(STR_ERROR_SITE_UNSUITABLE); | |||
if (!IsTileType(tile_cur, MP_WATER)) { |
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 the restructuring is a must, as I cannot make heads nor tails from this. Especially the cascading of if/else is difficult to read. And as that often goes, also unneeded.
For example, one could use functions to improve readability. Or one could invert the code (say what is allowed instead of what not). Many choices here. I would prefer we fix it, not giving our future-self a headache ;)
src/station_cmd.cpp
Outdated
if (IsTileType(tile_cur, MP_TUNNELBRIDGE)) { | ||
if (GetTunnelBridgeTransportType(tile_cur) != TRANSPORT_WATER) return_cmd_error(STR_ERROR_SITE_UNSUITABLE); | ||
} else if (IsTileType(tile_cur, MP_RAILWAY)) { | ||
if (GetRailGroundType(tile_cur != RAIL_GROUND_WATER)) return_cmd_error(STR_ERROR_SITE_UNSUITABLE); |
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.
Does this work if the rail is on the side of the dock? As that sounds wrong? (I miss a directional check, basically). I could be wrong, hence the questionmark.
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.
The ship doesn't dock at the sides in the original code.
799adb0
to
1975c0f
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.
Can I (strongly) suggest you take more time and effort for your PRs? There are at least 2 obvious things wrong with this PR. At the rate you are creating PRs, you are simply burning us out, as they all need a ton of attention to get up to a decent standard.
Please, take your time creating these PRs, check them, check them twice, think about the content, the patch, what we ask, what we expect, what we have been teaching you for the last few months. I see very little improvement in the quality of your PRs. Please. Take note of this and act on it.
Tnx!
1975c0f
to
5939b10
Compare
src/ship_cmd.cpp
Outdated
@@ -66,7 +66,7 @@ bool IsValidImageIndex<VEH_SHIP>(uint8 image_index) | |||
return image_index < lengthof(_ship_sprites); | |||
} | |||
|
|||
static inline TrackBits GetTileShipTrackStatus(TileIndex 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.
I do not think removing inline
is a good choice here. More commonly, these functions are in a .h
file.
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 followed the example set by WaterClass GetEffectiveWaterClass(TileIndex 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.
Do you understand what inline
does? And with that knowledge, do you still think it is a good idea to 'just remove' that? How is GetEffictiveWaterClass
an example? Did you see a similar change there, where someone removed inline
? And how does it relate to this function? I have so many questions.
src/ship.h
Outdated
@@ -58,6 +58,7 @@ struct Ship FINAL : public SpecializedVehicle<Ship, VEH_SHIP> { | |||
}; | |||
|
|||
static const uint SHIP_MAX_ORDER_DISTANCE = 130; | |||
TrackBits GetTileShipTrackStatus(TileIndex 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.
I do not think this is the right place for this entry. If you look at other classes, you see they are in different files. Did you think about the place of this line, or just used the first that worked?
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 still believe ship.h is the right place for it. I looked at water.h, track_func.h, landscape.h and ship.h. Makes better sense in ship.h.
src/ship.h
Outdated
@@ -58,6 +58,7 @@ struct Ship FINAL : public SpecializedVehicle<Ship, VEH_SHIP> { | |||
}; | |||
|
|||
static const uint SHIP_MAX_ORDER_DISTANCE = 130; | |||
TrackBits GetTileShipTrackStatus(TileIndex 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.
Lacking comment of function (Yes, the old one didn't have it; that is no excuse)
src/station_cmd.cpp
Outdated
} | ||
|
||
/* Ensure the location where ships dock, has water tracks. */ | ||
if (GetTileShipTrackStatus(tile_cur) == TRACK_BIT_NONE) return_cmd_error(STR_ERROR_SITE_UNSUITABLE); |
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.
Should it not be GetTileTrackStatus
, which is already available without needing to uninline things?
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 ended doing it this way. I couldn't grasp the concept of inline
. Sorry, TrueBrain.
src/ship_cmd.cpp
Outdated
@@ -66,7 +66,7 @@ bool IsValidImageIndex<VEH_SHIP>(uint8 image_index) | |||
return image_index < lengthof(_ship_sprites); | |||
} | |||
|
|||
static inline TrackBits GetTileShipTrackStatus(TileIndex 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.
Do you understand what inline
does? And with that knowledge, do you still think it is a good idea to 'just remove' that? How is GetEffictiveWaterClass
an example? Did you see a similar change there, where someone removed inline
? And how does it relate to this function? I have so many questions.
In regards to the 3rd tile: - It allows construction if it is an aqueduct head. - It allows construction if it is a buoy. - It allows construction if it is a water tile with one corner raised and without a rail track on the opposite corner. - It allows construction if it is a water tile with one corner raised and with a rail track on the opposite corner.
cf3b98d
to
b8c48c1
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.
Just a reminder to anyone looking at this: in my opinion is what the patch trying to solve fine, but how it is being solved not. The author couldn't figure out how to move an inline function to the header, so copy/pasted the content. This of course is far from ideal. This most likely just needs someone to fix that up, and it should be done.
Although I personally like the idea of this patch, the code quality is not on-par with what we expect. Despite our best efforts, we still don't see the result we expect (although we have been extensively coaching @SamuXarick on the topic). Given no other dev has a real interest in this, and we are also working towards NewGRF docks, I am going to call this a day. |
In regards to the 3rd tile:
Note: Some AIs are not prepared to deal with water tiles with one corner raised.
https://www.tt-forums.net/viewtopic.php?f=33&t=75221