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

Disconnected towns during world generation #7043

Closed
SamuXarick opened this issue Jan 12, 2019 · 7 comments
Closed

Disconnected towns during world generation #7043

SamuXarick opened this issue Jan 12, 2019 · 7 comments

Comments

@SamuXarick
Copy link
Contributor

@SamuXarick SamuXarick commented Jan 12, 2019

[14:59] Samu> bt, I found a bug regarding town bridges, during world generation the code deletes towns that are generated with 0 population, but by doing so, nearby bridges are also removed
[14:59] Samu> btw*
[14:59] Eddi|zuHause> then maybe the code that assigns bridge ownership is wrong?
[15:00] Samu> those bridges didn't belong to the town being deleted
[15:00] Gabda> it is a world generated bridge
[15:00] Samu> it results in disconnected towns

https://imgur.com/a/hqCAgsO

dbbzcfq
i7epssm

@J0anJosep

This comment has been minimized.

Copy link
Contributor

@J0anJosep J0anJosep commented Jan 13, 2019

I think that storing the TownID of the town that builds the bridge on the map array instead of using CalcClosestTownFromTile(*) would solve the problem. The needed space in the map array is free.

Lots of pieces of code will need to be changed, but changes are easy and mostly obvious. I think it is a good first issue. Feel free to investigate if my suggestion is right.

See these functions (for a starting point):
GetTownIndex
SetTownIndex
CmdBuildTunnel
CmdBuildBridge
GrowTownInTile
(update also doc landscape*.html)

@fibbo

This comment has been minimized.

Copy link

@fibbo fibbo commented Jan 25, 2019

Is there a way to reproduce this so it's testable? I don't know the game very well but I'm interested in contributing to this project because I think it's amazing.

@TrueBrain

This comment has been minimized.

Copy link
Member

@TrueBrain TrueBrain commented Jan 25, 2019

It might be a bit trial and error at first, to find a map seed that shows this on the generation screen (so you can spot it going wrong). Once you have found such map, you could regenerate the same map with the same seed. getseed in the console gives the seed of the map. Possibly that helps out? (I am a bit guessing here; possibly there are easier ways).

@SamuXarick

This comment has been minimized.

Copy link
Contributor Author

@SamuXarick SamuXarick commented Jan 25, 2019

A town is created here:
https://github.com/OpenTTD/OpenTTD/blob/master/src/town_cmd.cpp#L1957

If it's generated with 0 population, it is deleted right away. The deletion code takes bridges of nearby towns away by mistake, here:

https://github.com/OpenTTD/OpenTTD/blob/master/src/town_cmd.cpp#L93

SamuXarick added a commit to SamuXarick/OpenTTD that referenced this issue Feb 26, 2019
SamuXarick added a commit to SamuXarick/OpenTTD that referenced this issue Feb 26, 2019
SamuXarick added a commit to SamuXarick/OpenTTD that referenced this issue Feb 26, 2019
SamuXarick added a commit to SamuXarick/OpenTTD that referenced this issue Feb 26, 2019
@andythenorth

This comment has been minimized.

Copy link
Contributor

@andythenorth andythenorth commented Feb 26, 2019

Why does OpenTTD even generate towns with popn. 0? It's odd to permit this then remove the result if 0 popn.

Is this simply to handle the case of pathological sites where no buildings can be located, and/or pathological placement rules for newgrf houses?

@PeterN

This comment has been minimized.

Copy link
Member

@PeterN PeterN commented Feb 26, 2019

To make a town, a town is created and then it grows it for several iterations. During this iteration is when it discovers a site is suitable. If it ends up with no population, the town is then removed as unviable. It is very rare, though.

@andythenorth

This comment has been minimized.

Copy link
Contributor

@andythenorth andythenorth commented Feb 26, 2019

If this was my solo project, I'd probably just have done "popn = 8 + sum(popn buildings)". Nobody would ever notice 😈 Might have a problem producing pax for transport with no buildings though.

PeterN added a commit to PeterN/OpenTTD that referenced this issue Feb 26, 2019
…acent tile ownership, not nearest town.

The existing test performed badly with a large number of towns due to having to calculate the
nearest town, and also by its nature assumed a bridge was built by the nearest town, leading
to bridges of nearby large towns be removed incorrectly.
PeterN added a commit to PeterN/OpenTTD that referenced this issue Feb 26, 2019
… adjacent tile ownership, not nearest town.

The existing test performed badly with a large number of towns due to having to calculate the
nearest town, and also by its nature assumed a bridge was built by the nearest town, leading
to bridges of nearby large towns be removed incorrectly.
SamuXarick added a commit to SamuXarick/OpenTTD that referenced this issue Mar 3, 2019
@PeterN PeterN closed this in ebc3934 Mar 3, 2019
nielsmh added a commit to nielsmh/OpenTTD that referenced this issue Mar 11, 2019
… adjacent tile ownership, not nearest town. (OpenTTD#7284)

This only affects failed town generation, as towns do not delete bridges under any other circumstances.

The existing test performed badly with a large number of towns due to having to calculate the
nearest town, and also by its nature assumed a bridge was built by the nearest town, leading
to bridges of nearby large towns be removed incorrectly.

If we gain the ability to quickly retrieve the correct town (which is _not_ the nearest town) from the bridge, this change should be reviewed.
SamuXarick added a commit to SamuXarick/OpenTTD that referenced this issue Apr 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.