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

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

Closed
wants to merge 1 commit into from

Conversation

SamuXarick
Copy link
Contributor

@SamuXarick SamuXarick commented Oct 1, 2018

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

Copy link
Contributor

@nielsmh nielsmh left a comment

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

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 5, 2019

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
Copy link
Contributor Author

@SamuXarick SamuXarick commented Jan 19, 2019

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
Copy link
Contributor Author

@SamuXarick SamuXarick commented Jan 19, 2019

prondingville transport 1950-08-31

@SamuXarick
Copy link
Contributor Author

@SamuXarick SamuXarick commented Jan 19, 2019

prondingville transport 1950-12-20

LordAro
LordAro previously requested changes Jan 20, 2019
@@ -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)) {
Copy link
Member

@LordAro LordAro Jan 20, 2019

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

Copy link
Contributor

@planetmaker planetmaker Feb 10, 2019

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?

Copy link
Member

@TrueBrain TrueBrain Mar 29, 2019

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

@SamuXarick SamuXarick force-pushed the easier-dock-placement branch 2 times, most recently from dcc4adf to ec99684 Compare Jan 22, 2019
@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Jan 24, 2019

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

@SamuXarick
Copy link
Contributor Author

@SamuXarick SamuXarick commented Jan 25, 2019

You really should reconsider this.

@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Feb 10, 2019

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

6926-turning

@planetmaker
Copy link
Contributor

@planetmaker planetmaker commented Feb 10, 2019

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

@planetmaker planetmaker reopened this Feb 10, 2019
planetmaker
planetmaker previously approved these changes Feb 10, 2019
Copy link
Contributor

@planetmaker planetmaker left a comment

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.

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

@planetmaker planetmaker Feb 10, 2019

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?

@LordAro LordAro dismissed their stale review Mar 10, 2019

resolved, probably

Copy link
Member

@TrueBrain TrueBrain left a comment

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.

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

@TrueBrain TrueBrain Mar 29, 2019

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

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);
Copy link
Member

@TrueBrain TrueBrain Mar 29, 2019

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.

Copy link
Contributor Author

@SamuXarick SamuXarick Mar 29, 2019

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.

@SamuXarick SamuXarick force-pushed the easier-dock-placement branch 2 times, most recently from 799adb0 to 1975c0f Compare Mar 29, 2019
Copy link
Member

@TrueBrain TrueBrain left a comment

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!

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)
Copy link
Member

@TrueBrain TrueBrain Mar 30, 2019

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.

Copy link
Contributor Author

@SamuXarick SamuXarick Mar 30, 2019

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

Copy link
Member

@TrueBrain TrueBrain Mar 30, 2019

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);
Copy link
Member

@TrueBrain TrueBrain Mar 30, 2019

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?

Copy link
Contributor Author

@SamuXarick SamuXarick Mar 30, 2019

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);
Copy link
Member

@TrueBrain TrueBrain Mar 30, 2019

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)

}

/* Ensure the location where ships dock, has water tracks. */
if (GetTileShipTrackStatus(tile_cur) == TRACK_BIT_NONE) return_cmd_error(STR_ERROR_SITE_UNSUITABLE);
Copy link
Member

@PeterN PeterN Mar 30, 2019

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?

Copy link
Contributor Author

@SamuXarick SamuXarick Mar 30, 2019

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)
Copy link
Member

@TrueBrain TrueBrain Mar 30, 2019

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.
Copy link
Member

@TrueBrain TrueBrain left a comment

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
Copy link
Member

@TrueBrain TrueBrain commented Apr 13, 2019

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.

@TrueBrain TrueBrain closed this Apr 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants