Change: Allow dock to be constructed in more locations #6926

SamuXarick · 2018-10-01T16:12:03Z

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.

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

nielsmh

I don't agree it's a good idea to allow the buoy and one-corner-raised cases. The aqueduct case is fine.

TrueBrain · 2019-01-05T21:23:55Z

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!

SamuXarick · 2019-01-19T16:41:32Z

I don't agree it's a good idea to allow the buoy and one-corner-raised cases. The aqueduct case is fine.

Why not?

you try to place a dock on those places and you are denied, even though there's water there

SamuXarick · 2019-01-19T16:57:18Z

SamuXarick · 2019-01-19T16:58:37Z

LordAro · 2019-01-20T18:28:14Z

src/station_cmd.cpp

@@ -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)) {


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

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?

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

andythenorth · 2019-01-24T21:58:17Z

We don't need docks in these cases. On that basis, I'm closing this one. Thanks for contributing!

SamuXarick · 2019-01-25T03:03:33Z

You really should reconsider this.

andythenorth · 2019-02-10T18:51:21Z

Testing result. I have no opinion on whether this is a problem or not.

planetmaker · 2019-02-10T19:31:11Z

Sure, it looks odd. But I don't think that it's a new issue really triggered by this patch.

planetmaker

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.

planetmaker · 2019-02-10T19:45:08Z

src/station_cmd.cpp

@@ -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)) {


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?

resolved, probably

TrueBrain

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.

TrueBrain · 2019-03-29T16:04:33Z

src/station_cmd.cpp

@@ -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)) {


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

TrueBrain · 2019-03-29T16:05:07Z

src/station_cmd.cpp

+		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);


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.

The ship doesn't dock at the sides in the original code.

TrueBrain

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!

TrueBrain · 2019-03-30T09:16:24Z

src/ship_cmd.cpp

@@ -66,7 +66,7 @@ bool IsValidImageIndex<VEH_SHIP>(uint8 image_index)
 	return image_index < lengthof(_ship_sprites);
 }

-static inline TrackBits GetTileShipTrackStatus(TileIndex tile)


I do not think removing inline is a good choice here. More commonly, these functions are in a .h file.

I followed the example set by WaterClass GetEffectiveWaterClass(TileIndex tile);

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.

TrueBrain · 2019-03-30T09:21:17Z

src/ship.h

@@ -58,6 +58,7 @@ struct Ship FINAL : public SpecializedVehicle<Ship, VEH_SHIP> {
 };

 static const uint SHIP_MAX_ORDER_DISTANCE = 130;
+TrackBits GetTileShipTrackStatus(TileIndex tile);


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?

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.

TrueBrain · 2019-03-30T09:21:32Z

src/ship.h

@@ -58,6 +58,7 @@ struct Ship FINAL : public SpecializedVehicle<Ship, VEH_SHIP> {
 };

 static const uint SHIP_MAX_ORDER_DISTANCE = 130;
+TrackBits GetTileShipTrackStatus(TileIndex tile);


Lacking comment of function (Yes, the old one didn't have it; that is no excuse)

PeterN · 2019-03-30T10:55:38Z

src/station_cmd.cpp

-	}
+
+	/* Ensure the location where ships dock, has water tracks. */
+	if (GetTileShipTrackStatus(tile_cur) == TRACK_BIT_NONE) return_cmd_error(STR_ERROR_SITE_UNSUITABLE);


Should it not be GetTileTrackStatus, which is already available without needing to uninline things?

I ended doing it this way. I couldn't grasp the concept of inline. Sorry, TrueBrain.

TrueBrain · 2019-03-30T18:09:45Z

src/ship_cmd.cpp

@@ -66,7 +66,7 @@ bool IsValidImageIndex<VEH_SHIP>(uint8 image_index)
 	return image_index < lengthof(_ship_sprites);
 }

-static inline TrackBits GetTileShipTrackStatus(TileIndex tile)


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.

TrueBrain

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.

TrueBrain · 2019-04-13T06:24:25Z

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.

nielsmh reviewed Nov 1, 2018

View reviewed changes

andythenorth added waiting on author stale labels Jan 11, 2019

SamuXarick force-pushed the easier-dock-placement branch from 184efb1 to bc6799a Compare January 12, 2019 15:34

TrueBrain removed stale waiting on author labels Jan 12, 2019

LordAro previously requested changes Jan 20, 2019

View reviewed changes

SamuXarick force-pushed the easier-dock-placement branch 2 times, most recently from dcc4adf to ec99684 Compare January 22, 2019 01:16

andythenorth closed this Jan 24, 2019

planetmaker reopened this Feb 10, 2019

planetmaker previously approved these changes Feb 10, 2019

View reviewed changes

TrueBrain requested changes Mar 29, 2019

View reviewed changes

SamuXarick dismissed planetmaker’s stale review via 799adb0 March 29, 2019 17:10

SamuXarick force-pushed the easier-dock-placement branch 2 times, most recently from 799adb0 to 1975c0f Compare March 29, 2019 17:21

TrueBrain requested changes Mar 29, 2019

View reviewed changes

SamuXarick force-pushed the easier-dock-placement branch from 1975c0f to 5939b10 Compare March 29, 2019 18:30

TrueBrain requested changes Mar 30, 2019

View reviewed changes

PeterN reviewed Mar 30, 2019

View reviewed changes

TrueBrain requested changes Mar 30, 2019

View reviewed changes

SamuXarick force-pushed the easier-dock-placement branch from cf3b98d to b8c48c1 Compare March 30, 2019 20:00

TrueBrain requested changes Apr 7, 2019

View reviewed changes

TrueBrain closed this Apr 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change: Allow dock to be constructed in more locations #6926

Change: Allow dock to be constructed in more locations #6926

SamuXarick commented Oct 1, 2018

nielsmh left a comment

TrueBrain commented Jan 5, 2019

SamuXarick commented Jan 19, 2019 •

edited by LordAro

Loading

SamuXarick commented Jan 19, 2019

SamuXarick commented Jan 19, 2019

LordAro Jan 20, 2019

planetmaker Feb 10, 2019

TrueBrain Mar 29, 2019

andythenorth commented Jan 24, 2019

SamuXarick commented Jan 25, 2019

andythenorth commented Feb 10, 2019 •

edited

Loading

planetmaker commented Feb 10, 2019

planetmaker left a comment

planetmaker Feb 10, 2019

TrueBrain left a comment

TrueBrain Mar 29, 2019

TrueBrain Mar 29, 2019

SamuXarick Mar 29, 2019

TrueBrain left a comment

TrueBrain Mar 30, 2019

SamuXarick Mar 30, 2019

TrueBrain Mar 30, 2019

TrueBrain Mar 30, 2019

SamuXarick Mar 30, 2019

TrueBrain Mar 30, 2019

PeterN Mar 30, 2019

SamuXarick Mar 30, 2019

TrueBrain Mar 30, 2019

TrueBrain left a comment

TrueBrain commented Apr 13, 2019

Change: Allow dock to be constructed in more locations #6926

Change: Allow dock to be constructed in more locations #6926

Conversation

SamuXarick commented Oct 1, 2018

nielsmh left a comment

Choose a reason for hiding this comment

TrueBrain commented Jan 5, 2019

SamuXarick commented Jan 19, 2019 • edited by LordAro Loading

SamuXarick commented Jan 19, 2019

SamuXarick commented Jan 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andythenorth commented Jan 24, 2019

SamuXarick commented Jan 25, 2019

andythenorth commented Feb 10, 2019 • edited Loading

planetmaker commented Feb 10, 2019

planetmaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TrueBrain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TrueBrain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TrueBrain left a comment

Choose a reason for hiding this comment

TrueBrain commented Apr 13, 2019

SamuXarick commented Jan 19, 2019 •

edited by LordAro

Loading

andythenorth commented Feb 10, 2019 •

edited

Loading