[PATCH] No retries with the SimpleGroup placer

[elexis]

On some maps we can notice that the random map placement algorithm just doesn't place the resources we want it to place, even when passing gazillions of retries.

This can be noticed with metal on the map latium or fish on the map corinthian isthmus.

The reason for that being the retry loop used by createObjectGroups and createObjectGroupsByAreas of placer.js being broken if a SimpleGroup or a RandomGroup is passed, because these functions just return true or false while all other place functions return an array or undefined. The retry loop needs the array to decide how often it has to retry.

The bug exists at least since alpha 19, most likely since the introduction of these functions.

Unfortunately this means that many resource amount numbers in random map scripts are wrong as they hid the bug by increasing the amount of res, leaving us to rebalance all maps.

[Imarok]

We agreed on fixing the bug and add a "deprecated" function with the old behaviour to not have to change all the old maps.

[19:02] <Imarok> elexis: wrt 4294 what about adding some additional argument named deprecated to the buged functions?
[19:02] <elexis> #4294
[19:02] <WildfireBot> #4294 ([PATCH] No retries with the SimpleGroup placer) – http://trac.wildfiregames.com/ticket/4294
[19:02] <Imarok> ^^
[19:02] <Imarok> so that we have two functions: one deprecated and one new one
[19:02] <elexis> Imarok: good idea
[19:02] <Imarok> so new maps won't use the old one
[19:03] <elexis> partially addresses the issue
[19:03] <elexis> xd
[19:03] <elexis> yes, still 65 broken maps
[19:03] <Imarok> and we can create a simple ticket for adjusting the old maps
[19:03] <Imarok> the old maps will use the old function
[19:03] <Imarok> and we can adapt the maps one by one
[19:03] <elexis> some maps f.e. never have more than 1 metal mine on them
[19:03] <elexis> latium
[19:03] <elexis> or only 1 or 2 fish spots
[19:04] <Imarok> than we can fix them first
[19:05] <Imarok> and adjusting single maps is much more appealing then adjusting 65 maps at once
[19:05] <elexis> yes
[19:05] <elexis> true that

[elexis]

The deprecated approach is definitely more feasible than rewriting all 65+ random maps in one go.

Would result in something like this ​https://code.wildfiregames.com/P34 but the fixed maps should either use a createObjectGroupsNonDeprecated function or all createObjectGroups calls would have to be renamed to createObjectGroupsDeprecated in one go

[Imarok]

Replying to elexis:

Would result in something like this ​https://code.wildfiregames.com/P34 but the fixed maps should either use a createObjectGroupsNonDeprecated function or all createObjectGroups calls would have to be renamed to createObjectGroupsDeprecated in one go

The renaming was planned(otherwise the patch makes no sense), but I first wanted to hear a opinion about the general approach. I'll do it as soon I have time...

[elexis]

(The bug is not that important, it's just noticeable on few maps like latium, you can do other things too, review queue being annoying f.e.. I had just posted that ticket in the irc discussion as an example of a patch that shouldn't be committed without fixing it's sideeffects)

[Imarok]

It's not like I saw this ticket the first time when you posted it in irc. I've thought about this some months ago and forgot about it...

[elexis]

Patch proposed at ​https://code.wildfiregames.com/D249 by Imarok

[Imarok]

In 19929:

Fix SimpleGroup and RandomGroup placement retries

Reviewed by: FeXoR. Fixes: #4294
Differential Revision: ​https://code.wildfiregames.com/D249

[Imarok]

Followup ticket: #4695

[elexis]

In 20439:

Tidy up rmgen point location randomization and retry loops.

Remove retryPlacing args helperobject by making use of the hoisting effect.
Remove the deprecation warning in retryPlacing and equally and the unused behaveDeprecated argument from createAreas and createAreasInAreas, refs #4294, rP19929, D249.

Rename randomizePlacerCoordinates to randomizeCoordinates and placer to group in the four createObjectGroups functions following rP17728,
because Groups aren't Placers (as established in rP20355), just share the randomizable x/z properties.

Fix randomizeCoordinates on square maps offering entity locations outside of the passable map area that are only rejected later in the codeflow, missed first in rP14183, but also rP18450 and rP20283, refs #4012, #4814.
Clarify randomizeCoordinates by passing a boolean stating whether to include the impassable map border instead of a varying halfMapSize.
Remove the unused halfMapSize argument from the randomizePlacerCoordinatesFromAreas call in createAreasInAreas.