Opened 6 years ago

Closed 2 years ago

#5092 closed defect (fixed)

ClumpPlacer seems to use incorrect formula to determine if placement failed for a given failFraction allowed.

Reported by: Inari Owned by: Stan
Priority: Should Have Milestone: Alpha 26
Component: Maps Keywords:
Cc: Patch: Phab:D1414

Description (last modified by Inari)

To check if placement has failed, ClumpPlacer allows to pass a failFraction to allow partial placement to still count as success. (ps/trunk/binaries/data/mods/public/maps/random/rmgen/placer/centered/ClumpPlacer.js#L103)

The formula to check if placement has failed seems a bit off though. It keeps track of a failed variable, increasing that by 1 each time a point fails to put.

In a quick test for an area with size 1200 there seem to be roughly 13500~15500 points that it attempts to put. It varies a bit because of the randomness involved in the placement.

So it can't just check that failed variable against the size multiplied by failFraction. It should instead keep track of how many points it tried to put and multiply that count by failFraction to check against failed.

ChainPlacer seems to do this already (ps/trunk/binaries/data/mods/public/maps/random/rmgen/placer/centered/ChainPlacer.js#L110).

This is also why #4810 seemed to be an issue, originally it passed a failFraction of 1, allowing for placement to proceed in any case. But that seems to not have worked as intended.

Change History (7)

comment:1 by Inari, 6 years ago

Description: modified (diff)

comment:2 by Inari, 6 years ago

Patch: D1414

I created a patch for this as I ran into it again while trying to work on another ticket.

comment:3 by Itms, 5 years ago

Milestone: BacklogAlpha 24
Patch: D1414Phab:D1414

Thanks for the patch! And sorry for our clogged review queue. We shouldn't take much longer in committing it.

comment:4 by wraitii, 3 years ago

Milestone: Alpha 24Alpha 25

Famous last words™

comment:5 by wraitii, 3 years ago

Milestone: Alpha 25Alpha 26

comment:6 by fsincos, 2 years ago

The patch is correct in that it actually uses a fraction of the positions that were checked, but it still doesn't check the "ineligible" fraction of the area:

At the center, the point density is (a lot) greater than at the perimeter. For instance, the center point is counted at least intPerim times (if it is "invalid").

In total, every point in the circle is checked (on average) eight times(*), so that explains some part of the discrepancy (1200 -> 9600). The rest is caused by the "random part" (if I understood the code correctly, this part yields an expected factor of 2 - coherence, something like 9600 -> ~14000, I guess).

To fix these issues, the "gotRet" part should also be used for the "else" branch.

We still need the actual count of tiles as the "size" doesn't accurately reflect it because of randomness (and also parts being cut off by the map's edges - should those tiles count as invalid or should they just be ignored?).


perim * radius = 8 * PI * radius * radius = 8 * size

comment:7 by Stan, 2 years ago

Owner: set to Stan
Resolution: fixed
Status: newclosed

In 26503:

Make ClumpPlacer determine the correct fraction of failed attempts to place points
Comments by: @smiley, @elexis
Patch by: @Inari
Fixes #5092
Differential Revision:

Note: See TracTickets for help on using tickets.