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
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@SamuXarick
Copy link
Contributor

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

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)

@nielsmh nielsmh added good first issue Good for newcomers bug Something isn't working labels Jan 14, 2019
@fibbo
Copy link

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

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

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

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

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

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 as completed in ebc3934 Mar 3, 2019
nielsmh pushed 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
douiwby pushed a commit to douiwby/OpenTTD that referenced this issue Apr 16, 2020
… 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

7 participants