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

Feature: Generate lock ready rivers upon world generation #7073

Closed

Conversation

SamuXarick
Copy link
Contributor

@SamuXarick SamuXarick commented Jan 19, 2019

This patch is intended to ease water construction, especially locks on rivers. The changes might be too subtle to notice, but I think they're helpful if you're familiar to the constraints of water-based transportation inland.

With this patch applied, and while on map generation, rivers are generated in a way that facilitate construction of locks on river rapids. It assures that the river source can be connected to its mouth, without the need to terraform land for locks. The player only needs to place locks on rapid tiles to connect each lock to the rest of the stream. In this manner, as long as ships can make 90 degree turns, ships can traverse the entire river.

Other features:
- Towns will not build bridges over rivers if a lock could be built under it, while growing during a game and/or during map generation.
Posted as separate: #7195

https://www.tt-forums.net/viewtopic.php?f=33&t=76075

@SamuXarick
Copy link
Contributor Author

SamuXarick commented Jan 20, 2019

screenshot 87

1 / 2 - A town bridge isn't generated in 1 because 2 is suitable for a lock.
3 / 4 - River tiles 4 are generated to connect to a suitable lock at 3.
5 / 6 - Same as 1 / 2

Copy link
Contributor

@andythenorth andythenorth left a comment

Choose a reason for hiding this comment

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

I have tested this. I really want easier lock placement on rivers, and this is one possible solution.

  • I can't comment on the code style
  • the change to town bridge behaviour should be split from this PR, it combines too many concerns into one PR
  • the patch frequently produces isolated rivers that can't flow to the sea, I suspect this is because it bans rivers from using contiguous slopes. I'll attach screenshots. This will need fixed.
  • the approach of adding extra tiles in corners near a slope is 'interesting', but works much better than might be expected, the resulting rivers are more organic looking

Screenshots of isolated rivers with no path to sea. These happen occasionally in master, but are very much more frequent with this patch.
unnamed 01-01-2000 7
unnamed 01-01-2000 6
unnamed 01-01-2000 5
unnamed 01-01-2000 4

src/landscape.cpp Outdated Show resolved Hide resolved
src/landscape.cpp Outdated Show resolved Hide resolved
src/landscape.cpp Outdated Show resolved Hide resolved
src/landscape.cpp Outdated Show resolved Hide resolved
}
}

TileIndex t_2dm = t_dm + m * delta_mid;
Copy link
Member

Choose a reason for hiding this comment

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

Variable name.

Copy link
Member

Choose a reason for hiding this comment

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

^^ Still

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 don't know what to do about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tile: main tile
t_dm: tile + delta mid
t_dm_ds: tile + delta mid + delta side
t_dm_2ds: tile + delta mid + two times delta side
t_2dm: tile + two times delta mid
t_2dm_ds: tile + two times delta mid + delta side
t_3dm: tile + three times delta mid
t_3dm_ds: tile + three times delta mid + delta side

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just removed all those variables, and only kept one which is reused multiple times to compute the offsets. I called it tile_offset.

int delta = TileOffsByDiagDir(dir_rot);

for (int m = -1; m <= 1; m += 2) {
TileIndex t_dm = tile + m * delta_mid;
Copy link
Member

Choose a reason for hiding this comment

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

Variable name.

src/landscape.cpp Outdated Show resolved Hide resolved
src/town_cmd.cpp Outdated Show resolved Hide resolved
@nielsmh
Copy link
Contributor

nielsmh commented Jan 20, 2019

Agree with @andythenorth it's nice to see some "lakes" being generated as part of rivers.

From what I can tell, right now the code seems to reject certain river flows if there isn't a path that allows it to be lock-ready. I wonder if a better approach would be to let the river flow downgrade regardless, but best-effort place additional river tiles at slopes to permit locks. Or maybe the current approach where it tries to reject rivers that aren't lock-ready, but gives up after a while and just uses the old (simple) approach so the river can flow down.

I hope this makes sense.

@andythenorth
Copy link
Contributor

I am +1 that the river should always ultimately be allowed to flow to the sea.

It is useful however to prefer 'lockable' slopes where possible. If none are available, allowing flow downslope is better than rejecting.

@SamuXarick
Copy link
Contributor Author

fanley transport 1952-02-29

Variable names inside the now renamed to IsPossibleLockLocationRecursively

@SamuXarick SamuXarick force-pushed the generate-lock-friendly-rivers branch 2 times, most recently from b60e0d8 to db0d66c Compare January 22, 2019 14:30
@SamuXarick SamuXarick force-pushed the generate-lock-friendly-rivers branch 2 times, most recently from 1a040d5 to 3af1f0d Compare February 8, 2019 02:56
@SamuXarick
Copy link
Contributor Author

Changes:

  • Don't find a spring near other rivers in a radius of 8 tiles.
  • Only try to generate lakes if the river has already flowed down at least twice.

@SamuXarick SamuXarick force-pushed the generate-lock-friendly-rivers branch from 3af1f0d to bee74c5 Compare March 1, 2019 15:10
@SamuXarick SamuXarick force-pushed the generate-lock-friendly-rivers branch 2 times, most recently from a200bce to 9677ffe Compare March 26, 2019 02:45
@SamuXarick SamuXarick force-pushed the generate-lock-friendly-rivers branch from 9677ffe to 8c8035f Compare April 13, 2019 22:04
@stale
Copy link

stale bot commented May 13, 2019

This pull request has been automatically marked as stale because it has not had any activity in the last month.
Please feel free to give a status update now, ping for review, or re-open when it's ready.
It will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale Stale issues label May 13, 2019
@andythenorth
Copy link
Contributor

Stale. Closing, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Stale issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants