krinn wrote:

From my code understanding i don't read it like you :)
1/ it setup _grow_town_result as a loop counter to do X trys
2/ it loop it
3/ in the loop, it always call GrowTownTile (line 1316)
-> if it have no road (line 1321), it return the value of GrowTownTile (that itself return GROWTH_SUCCEED(-1) if house was built or GROWTH_SEARCH_STOPPED(0) but should also keep it like it is if nothing was done.
4/ line1325 if we are here, we have a road and _grow_town_result remain unchange (still a counter so)
5/ line1337 if we have road, not a depot and of type ROADTYPE_ROAD and a town own the road :
-> if the road is own by a town, and it's not the town to grow that own it, it will force _grown_town_result = -1 (GROWTH_SUCCEED) to force next loop test to fail and endup (so if a road is good to use, but not own by the town to grow, this will endup the loop). So: at first not own by town road found, give up all search.
-> if road is not own by a town, and own by none and GM_EDITOR... because of this check line 1338 force _grown_town_result = -1 to endup the loop instead of just "return -2;" else this check couldn't be made.

So basically it will only return 0, -1 or -2

Now your changes:
- return _grow_town_result;
+ return (_grow_town_result == GROWTH_SUCCEED);
instead of returning -1 (GROWTH_SUCCED) or (0 GROWTH_SEARCH_STOPPED), you return 1 if GROWTH_SUCCEED and 0 if GROWTH_SEARCH_STOPPED, because the GrowTown test is "return value != 0", this will work. But this just change the return value from -1 to 1 in case of GROWTH_SUCCEED, nothing worth any value.
So you change it to return 0, 1 or -2

- _grow_town_result = GROWTH_SUCCEED;
+ _grow_town_result = GROWTH__SEARCH_STOPPED;
Now this is very bad, instead of setting _grown_town_result = -1 (GROWTH_SUCCED), you force it to GROWTH_SEARCH_STOPPED (that is of value 0)
And because the loop test is (line1348) "while (--_grown_town_result >=0); your change will now not exit the loop (because before decreasing grown_town_result, its value is 0 and 0 match the >=0 test), making the loop iter one next try even if a road not own by the town was already founded. It's even worst, as you could get into a loop, that remain always at 0 as long as a road own by a town is found and the road isn't own by the town to grow, making all neightbors roads own by another town to be tests (voiding the iter counter effect base on prior setting on TL settings and voiding the "end all tests at first not own by town road found").
You have change all trys to trys+(number of neighbor roads own by another town found that succeed condition (a road + not a depot + of type ROADTYPE_ROAD + own by a town) ; because as long as this condition remain true, your change keep _gown_town_result to a value of 0.

I'm unsure what was the issue you were trying to fix, but i'm pretty sure you didn't fix it, and makes things worst in fact.

This comment was imported from FlySpray: https://bugs.openttd.org/task/6240#comment13797

dp wrote:

instead of returning -1 (GROWTH_SUCCED) or (0 GROWTH_SEARCH_STOPPED), you return 1 if GROWTH_SUCCEED and 0 if GROWTH_SEARCH_STOPPED, because the GrowTown test is "return value != 0", this will work. But this just change the return value from -1 to 1 in case of GROWTH_SUCCEED, nothing worth any value.
So you change it to return 0, 1 or -2

Before this change it returned whatever value counter had if GrowTownTile haven't done anything (not only 0 or -1). That lead GrowTown to think that it built something since it is non-zero. My change makes it return non-zero only when _grown_town_result was set to GROWTH_SUCCEED, in other words, something was really built.

your change will now not exit the loop
It will exit since prefix -- decrements before returning value. If you look at other town growing functions they all works the same way setting _grown_town_result to either GROWTH_SUCCED or GROWTH_SEARCH_STOPPED when search is finished.

This comment was imported from FlySpray: https://bugs.openttd.org/task/6240#comment13799

krinn wrote:

Ah thanks, got it now that i get how the prefix works.
Your change makes it return now true (1) only if an house was built, so if i get it right, possible return values are now limited only to -2, 0 and 1

I don't get what it change since the GrowTown test still is aimain at (line 1403) return r != 0;
So GrownTown was previously always return true except if it get GROWTH_SEARCH_STOPPED (so -2, -1 or whatever the value of the iter was)
With your changes, GrownTown will still return true if it don't get GROWTH_SEARCH_STOPPED (so -2 or 1)
Expect loosing the ability to see number of trys remain (that is not use in GrownTown anyway), the changes doesn't change GrownTown return value. It will still do like before only return true if GROWTH_SEARCH_STOPPED is not the result, and false in other case.

The only fix i see is the comment on GrownTown that is now reflecting this with "+ * @return true iff something was built (house, road or bridge)"
While previously it was doing the same but pretend it was only return true if an house was built.
In order to fix this, GrownTown should had been change to "return (r == GROWTH_SUCCEED); and it was then returning only true if an house was really built.

While i get the fix to the comment of GrownTown to reflect it return true not only if an house was built, i still don't get what it will fix, either should the comment remain like before (return true if house was built) and the return value should be (r == GROWTH_SUCCEED);
or the comment like you did change to says "return true if something was built", but i don't then get why changing GrowTownAtRoad code as it will not change the behavior of GrownTown function itself. Are the fixes for GrownTownAtRoad done to fix some other function that use it?

Shouldn't the change to GrownTownRoad you should also change its comment from " * Returns "growth" if a house was built, or no if the build failed." -> " * Returns "true" if a house was built, or no if the build failed.", because true is not the value of "growth". As i think "growth" imply GROWTH_SUCCEED value.

Hehe if you think i'm a bit lost, it's because i'm learning c++ :)

This comment was imported from FlySpray: https://bugs.openttd.org/task/6240#comment13800

dp wrote:

possible return values are now limited only to -2, 0 and 1

It is not returning -2 and never was. Before my change it returned
line1342 return _grow_town_result; which can be anything >= -1
line1370 return (_grow_town_result == -2) which is 1 or 0

So it can return value >= -1
I changed line1342 to make it return 0 (false for r!=0 in GrowTown) when previously it returned >0 (true). So now it is returning only 1 or 0.

As for comment fix it had to be done regardless of this bug, it was simply wrong and didn't reflect properly what code is doing. GrowTown could return true if road (not house) was built or even nothing happened at all.
I didn't notice that 'Returns "growth"' comment, that's a valid point. And I don't know what it was supposed to mean since there is already a proper @return section.

I attached savegame that makes it easy to see this bug even with unmodded client. Despite being in same starting conditions Lendstone Bay grows way faster than Chonningwell or Lontbourne.

P.S. No wonders, I didn't report this bug for almost a year coz I did'n want to dig so deep into town growing code :)

Attachments

• ttd_bug_6240_hs.sav (113.51 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/6240#comment13801

krinn wrote:

Don't feel anything other than just that, i'm practicing on a real case (as patches are generally smaller and "more easy" to understand) ; as i'm fully aware my questions might be taken to bug you, well, pretty sure they do, but it's a side effect and not my goal.
Might be also good to change "return (_grow_town_result == -2);" to a "return (_grown_town_result == GROWTH_SUCCEED);" to makes code clearer in GrownTownAtRoad end (after loop exit)?

This comment was imported from FlySpray: https://bugs.openttd.org/task/6240#comment13802

dp wrote:

It's ok, don't worry. Sorry if I sound rude, I'm just not that good with English to explain everything properly. Also this patch may look much simpler than it actually is since it is a piece of bigger puzzle and I put quite some thought into it to make sure that it will fix this particular nuisance, but don't break anything else.
As for changing "return (_grow_town_result == -2);" you're partially right, but that would mean writing (_grown_town_result == GROWTH_SUCCEED - 1) which is almost as ugly imo. Honestly, I think all that town growing code craves for refactoring like making functions to return proper values instead of using global _grown_town_result, getting rid of weird logic and comparisons like the one you mentioned and so on. It just doesn't feel right for me to do all that stuff for such a small reason like this bug. That's why I designed this patch to fit with the current logic and do only minimum changes required.

This comment was imported from FlySpray: https://bugs.openttd.org/task/6240#comment13803

frosch closed the ticket.

Reason for closing: Fixed

in r27249. Thanks for the patch!

This comment was imported from FlySpray: https://bugs.openttd.org/task/6240