Opened 3 years ago

Closed 2 years ago

Last modified 20 months ago

#4294 closed defect (fixed)

[PATCH] No retries with the SimpleGroup placer

Reported by: elexis Owned by: Imarok
Priority: Must Have Milestone: Alpha 23
Component: Maps Keywords:
Cc: Patch: Phab:D249

Description (last modified by Imarok)

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.

Attachments (1)

retry_simplegroups_v1.patch (3.6 KB) - added by elexis 3 years ago.

Download all attachments as: .zip

Change History (12)

Changed 3 years ago by elexis

Attachment: retry_simplegroups_v1.patch added

comment:1 Changed 2 years ago by 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

comment:2 Changed 2 years ago by 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

comment:3 in reply to:  2 Changed 2 years ago by 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...

comment:4 Changed 2 years ago by 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)

comment:5 Changed 2 years ago by 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...

comment:6 Changed 2 years ago by elexis

Milestone: BacklogWork In Progress

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

comment:7 Changed 2 years ago by Imarok

Description: modified (diff)
Patch: Phab:D249

comment:8 Changed 2 years ago by Imarok

Owner: set to Imarok
Resolution: fixed
Status: newclosed

In 19929:

Fix SimpleGroup? and RandomGroup? placement retries

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

comment:9 Changed 2 years ago by stanislas69

Keywords: patch removed
Milestone: Work In ProgressAlpha 23

comment:10 Changed 2 years ago by Imarok

Followup ticket: #4695

comment:11 Changed 20 months ago by 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.

Note: See TracTickets for help on using tickets.