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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow the center tile to always get a house when playing with 3x3/Better #6986

Closed
wants to merge 1 commit into from

Conversation

@msikma
Copy link

msikma commented Dec 7, 2018

This small change was made after a topic on the TT-Forums about an issue I noticed during gameplay. Basically, if you play with "towns are allowed to build roads" turned off, the algorithm that determines where houses are built misses a branch somewhere. This results in occasional gaps showing up, especially when using the 3x3 road layout:

screen shot 2018-12-07 at 17 01 33 2

If you do give towns the ability to construct their own roads, this does not happen and the gaps get filled up with houses like normal.

It seems to me that removing the whole check for both these variables fixes the problem, allowing the houses to be built even with the setting turned off.

screen shot 2018-12-07 at 17 01 33

Since I'm new to contributing, I'm not sure if it's the right way to go. I can also understand that maybe we need a setting for this? Please let me know 馃檪

@nielsmh

This comment has been minimized.

Copy link
Contributor

nielsmh commented Dec 7, 2018

I haven't tested the patch yet, I'm not sure if this is really the right thing to do. From my understanding, the intention is that towns only build houses adjacent to roads. The only case a house can appears two tiles from a road is when a large house (e.g. 2x2 stadium) north-east/north-west of a road is replaced with a small house, which always appears in the north corner of the 2x2 building's footprint. I think that case might actually be the bug.

If this is actually considered a bug that should be fixed, I don't think it should have a new setting.

On technicalities, you need to change your commit message to conform to the standard, with a keyword in front. Do this by making a amend-commit on your local branch, then force-pushing that to GitHub.
My suggestion would be: Fix: Allow towns to fill the center square of 3x3 blocks
(You are removing code, but the purpose of the removal is to fix a bug, so the appropriate keyword is Fix.)

@msikma

This comment has been minimized.

Copy link
Author

msikma commented Dec 7, 2018

Well, I'm not sure if this is the right thing to do either. But basically this just makes the game behave the same with regard to house placement regardless if you have the "towns are allowed to build roads" setting on or off.

There is an inconsistency at the moment: unless you have that setting setting turned on, that whole code block never executes (except when generating a map). Since the map generation also runs this code, generated towns at the start of a game will also have houses in those places, but then if you play with that setting disabled they will slowly disappear over time.

Since the setting is called "towns are allowed to build roads", I think it's weird that it would change where houses are chosen to be built, so that's why I think this is probably a bug. Removing these conditions makes house placement identical regardless of the value of that setting. So if you play with road building enabled you won't notice any difference.

There are a few other uses of the _settings_game.economy.allow_town_roads and _generating_world bools but to me it seems like they all properly have to do with road building. Just this one doesn't, but then I'm only looking into the code for the first time so I can't be sure

Edit: modified commit message

@msikma msikma force-pushed the msikma:CenterTile3x3 branch from e0bf6a4 to c1920bb Dec 7, 2018
@msikma

This comment has been minimized.

Copy link
Author

msikma commented Dec 8, 2018

Someone in the thread said I should test this to make sure this change doesn't let towns build roads all of a sudden. I didn't take screenshots (will do tomorrow) but I did test it, perhaps a more robust test should be done though.

Basically it seems to work as expected: with allow_town_roads on, everything works as it always has, towns refuse to build houses on tiles where roads will go, and eventually roads show up. With allow_town_roads off, towns freely builds houses on tiles where the roads should go, doesn't make roads, and it also fills in the "gaps" in 3x3 blocks as with the setting on.

@ldpl

This comment has been minimized.

Copy link
Contributor

ldpl commented Dec 9, 2018

Town growth algorithm isn't a good place to start patching tbh. It's quite complex and there're lots of subtleties. Especially you don't wan't to mess up allow_town_roads=false behavior or there will be a mob of citybuilder players coming after you xD
As for your patch how did you even check it? Because for me it's completely broken. Towns build roads when not allowed and reserve spots for the grid. And that's exactly how it should be as you just allowed that code to run with disabled setting.

@msikma

This comment has been minimized.

Copy link
Author

msikma commented Dec 10, 2018

As for your patch how did you even check it? Because for me it's completely broken. Towns build roads when not allowed and reserve spots for the grid. And that's exactly how it should be as you just allowed that code to run with disabled setting.

That is weird, when I tested it I didn't see new roads being made. All I saw was new houses being made on road tiles, and the gaps being filled up. I clearly saw different behavior based on the roads setting. Are you sure your test was with just this patch and nothing else? I don't even see a code path that leads to roads being built with this change. Just GrowTownWithExtraHouse(), and if allow_house gets turned on it runs LevelTownLand() and BuildTownHouse(). Maybe I'm missing something.

I'm new to the code so I understand that I'm not the most suitable for making a change like this, but I'm determined to try harder and figure out if I can get this working somehow. I will also do another test to see if I can reproduce that behavior, because it seems incompatible with what I saw.

@ldpl

This comment has been minimized.

Copy link
Contributor

ldpl commented Dec 10, 2018

It may not be building all the roads when disabled but it still builds some roadbits.
allow_house there does grid reservation and rcmd is passed to GrowTownWithRoad later in this function. So you'll need to separate GrowTownWithExtraHouse from those.

@msikma

This comment has been minimized.

Copy link
Author

msikma commented Dec 10, 2018

Thanks for clarifying, my test wasn't good enough. Going back to the code.

@TrueBrain

This comment has been minimized.

Copy link
Member

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!

@msikma

This comment has been minimized.

Copy link
Author

msikma commented Jan 6, 2019

Sorry, I've been really busy the past few weeks, but I'll get back to this as soon as I'm able to. I made one last change to my local code, I'll rebase and push that when I've got some time this week. I'll then post a test that displays clearly what the before/after consequences are, to ensure there aren't any weird unintended changes.

@stale

This comment has been minimized.

Copy link

stale bot commented Feb 16, 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 label Feb 16, 2019
@stale stale bot closed this Feb 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can鈥檛 perform that action at this time.