Yexo wrote:

A few small problems in this patch:

* The do/while to for change in DoCreateTown is unneeded as it doesn't change the behavior.
* CmdBuildTown doesn't check the found_town bit so modified clients can send the command without that bit set

Is the following line in CmdBuildTown needed? What happens if you just use "return CommandCost(EXPENSES_OTHER, GetNewTownConstructionCost());"?
+ return (found_town) ? CommandCost(EXPENSES_OTHER, GetNewTownConstructionCost()) : CommandCost();

* You add a line with a single tab in CmdRenameTown
* Not sure on this one, but in UpdateWidgetSize most of the switch seems unneeded. If you don't change the size just return. A simple "if (widget != TSEW_FOUND_COST) return;" should also work. And you need to fix the implementation in the TSEW+_FOUND_COST case.
* I think it's better to get the name of the company in DoCreateTown, not in the gui. Currently it allows a modified client to sent an arbitrary string as company name.
* You introduce a tab on an otherwise empty line in saveload/town_sl.cpp

This comment was imported from FlySpray: https://bugs.openttd.org/task/3142#comment6532

Terkhen wrote:

Here's a new version that solves the posted issues:

- The do/while to for change in DoCreateTown has been removed.

- CmdBuildTown now checks the found_town bit before starting. CmdRenameTown had a similar problem, now it checks if the current player can rename the town.

- Using "return CommandCost(EXPENSES_OTHER, GetNewTownConstructionCost());" in CmdBuildTown does the same than the previously implemented return. It has been changed to the simpler line.

- Lines with tabs removed at CmdRenameTown and town_sl.cpp.

- About UpdateWidgetSize code: I need to change size in two cases. One is if the widget is TSEW_FOUND_COST and the game is at the scenario editor (it must be hidden). The other is if the widget is TSEW_RANDOMTOWN, TSEW_MANYRANDOMTOWNS, TSEW_TOWNSIZE, TSEW_SIZE_SMALL, TSEW_SIZE_MEDIUM, TSEW_SIZE_LARGE, TSEW_SIZE_RANDOM or TSEW_CITY and the game is not at the scenario editor (they must be hidden). I have rewritten the function to be clearer and faster.

- Now the name of the founding company is obtained at DoCreateTown.

Attachments

• found_town_v10_r17285.diff (21.21 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3142#comment6533

Yexo wrote:

+ if (found_town) x = 8; /* Towns founded ingame are slightly bigger than small sized towns. /
This should be either:
if (..) ..; // Towns founded....
Or:
/ Towns founded ... */
if (..) ..;

The found_town bit is always set during the game and never in the scenario editor, why is it a parameter at all? DoCreateTown can check if you're in game or in the scenario editor and act accordingly.

For the check in CmdRenameTown I think it's more clear to leave the old check (t == NULL) and add the new check on a new line. If you want to keep it as one if-block, then use parentheses around the new check.

This comment was imported from FlySpray: https://bugs.openttd.org/task/3142#comment6534

Terkhen wrote:

Testing all possibilities I found a bug caused by the new CmdRenameTown check. If the player acting as server tries to rename a town founded by another company, the name is only updated on his game and it does not get changed on the rest of the clients. It is possible that the _network_server variable is not safe to be used into a Cmd function?

I will post a new version once I correct this bug and I make the new suggested corrections.

This comment was imported from FlySpray: https://bugs.openttd.org/task/3142#comment6535

Terkhen wrote:

I have been checking the bug. The _is_network_server and _network_server variables are not network safe as they don't get sent when making a command in online games. This means that, no matter what kind of check is implemented at the CmdRenameTown function, we can't check if the command was made by the server or not. If the found_town is active, I can just implement a behaviour that don't depends on checking if the command was made by the server at all. The problem is that, since the command is not CMD_SERVER anymore, we can't implement the old behaviour when the setting is not active (game server can rename all towns, the rest of clients can't) while preventing a modified client from renaming towns as he wants. I have thought of three ways of solving this problem:

- Alter the functions at network_command.cpp to allow sending _is_network_sender in the same way as the company of the command user is already sent. This would solve the issue, but I doubt that sending an additional variable that will not get much use (commands shouldn't give special privileges to the server anyways) with every command is useful at all.

- Drop the renaming feature completely, and show a dialog that allows to set the name of the town when it is founded. The problem is that only the game server can change the name of the town after it is founded.

- Create two different commands: a CMD_SERVER command that allows renaming all towns, and a normal command that can only be used if the found_town setting is active and the renamer company founded the town that it's trying to rename. To avoid duplication of code, both of them would call the same function after these checks. This is my preferred option.

Here is a version that implements Yexo's suggestions (the bug is still present).

- Checking if the game is at the scenario editor in the DoCreateTown made the game crash at scenario generation (probably because it is also called at CreateRandomTown). The check is made at the CmdBuildFunction and passed to DoCreateTown. CreateRandomTown always passes it as false.

- Comment style corrected.

- At CmdRenameTown, the old check has been left as it was to be more clear.

Attachments

• found_town_v11_r17288.diff (20.75 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3142#comment6538

Terkhen wrote:

I have been talking about this feature with SmatZ, and I have checked his implementation (http://devs.openttd.org/~smatz/found_town_17288.diff). Some issues that need discussion arised:

1. Should it let players create towns with different sizes? --> As long as big towns and cities are not allowed and correct prices are set for each size, I don't think that allowing this is a problem.

2. Should it let players decide the town layout? --> I don't know what kind of gameplay problems could arise, besides someone creating towns with chaotic layouts in a game where they are not wanted. I think the best option would be what SmatZ uses (use a setting to enable/disable custom layouts).

3. How should prices be implemented? --> Not much to say about this, as I don't know about price implementation.

4. How should renaming be handled to allow the founder company to (re)name his town?

==============================================================================================================

Another version is here: nothing new, just code changes and removing of the renaming feature.

- Simplify code for the toolbar GUI (SmatZ code).

- Rename the setting and place it on its correct type.

- Remove the implementation of renaming, as it changed a lot of parts of the code in a feature that should be discussed.

Attachments

• found_town_v12_r17295.diff (17.07 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3142#comment6547

Rubidium wrote:

What about a user having "town builds road" disabled? In that case only the town 'flag' will be created, unless it's build on top of an existing road. Is that behaviour wanted or not? Probably not, but spawning a town that can't do anything seems wrong too.

This comment was imported from FlySpray: https://bugs.openttd.org/task/3142#comment6601

Terkhen wrote:

Right now, founding towns ignores the allow towns to build roads setting, since _generating_world is activated while the town is being created. This behaviour is a possible option. Another option could be enforcing the setting, and make the command return an error and delete the generated town if its population is zero. I prefer the first option since it is way simpler and even with the allow towns to build road setting off it makes sense that a newly founded town creates some roads. I have been checking the code and to implement the second option the CmdBuildTown command would need the CMD_NO_TEST flag, as the population of the generated town can only be known after creating it. If the second option is preferred I'll check this more thoroughly.

I have attached another version of the patch, with no changes besides adapting the code to the recently commited economy changes.

Attachments

• found_town_v13_r17460.diff (15.87 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3142#comment6607

Terkhen wrote:

I have coded a new version that checks if the town can be built when the "town build roads" setting is off. To avoid the complicated solution I posted in my last comment, now CmdBuildTown checks if there is a road nearby. If there is no roads available, the town will not be built (see the first screenshot).

This solution creates a smaller problem: If there are not enough roads, the created town will be smaller (see the second screenshot). As a result, the player will be paying for a town size and getting a smaller one. The only solution I have found while thinking on this problem was implementing a custom price that would depend in the final size of the town and not on the selected size. Some way of building the town and then deleting it to get the expected size of the town when founded in a tile would be needed in order to get the estimated cost of building a town in a certain tile. In my opinion this would complicate the code too much for a single case.

Finally, now towns are not populated when they are built: only the foundations of the town buildings are placed, and the town will slowly get population when each building is finished (see the third screenshot).

Attachments

• screenshot (1).png (30.11 KiB)
• screenshot (2).png (41.51 KiB)
• screenshot (3).png (92.98 KiB)
• found_town_v14_r17558.diff (17.45 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3142#comment6662

Terkhen wrote:

New version: there are no changes besides some code reorganization and documentation and an update to current trunk. Founding towns has been tested with the "choose town name before its creation" feature in multiplayer games.

Attachments

• found_town_v15_r17654.diff (17.95 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3142#comment6766

SmatZ closed the ticket.

Reason for closing: Implemented

In r18281 (in a bit different way). Thank you for patch

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