Page MenuHomeWildfire Games

Make ClumpPlacer determine the correct fraction of failed attempts to place points
AcceptedPublic

Authored by Inari on Mar 24 2018, 3:12 PM.

Details

Reviewers
smiley
Trac Tickets
#5092
Summary

ClumpPlacer appears to use a wrong formula to determine the current failed placement percentage. Similar to ChainPlacer it should count the points it attempted to place and compare that with the amount of points it failed to place. See #5092

Test Plan

Set the failFraction for the Harbors in [source:ps/trunk/binaries/data/mods/public/maps/random/harbor.js#L318] addHarbors function to 1 instead of Infinity (should be functionally equivalent).

Generate a harbor map (medium size, 4 players, seed: 1928564068), notice that usually 1-2 Harbors do not get placed, causing the map to look asymmetrical (see #4810). With the patch this does not happen.

In my tests, I had also added some logging, telling me that the attempted number of placed points is around 13000 to 15000. For a "size" parameter of 1200. So it seems to make no sense to check against size to see what fraction of points failed to place.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 5685
Build 9553: arc lint + arc unit

Event Timeline

Inari created this revision.Mar 24 2018, 3:12 PM
Owners added a subscriber: Restricted Owners Package.Mar 24 2018, 3:12 PM
elexis added a subscriber: elexis.Mar 24 2018, 3:15 PM
elexis added inline comments.
binaries/data/mods/public/maps/random/rmgen/placer/centered/ClumpPlacer.js
88

+= maxRadiusSteps outside of the loop would cost less performance

Inari updated this revision to Diff 6268.Mar 24 2018, 3:22 PM

Added the suggested change of elexis for better performance. Added myself to programming.json.

Owners added a subscriber: Restricted Owners Package.Mar 24 2018, 3:22 PM
Inari marked an inline comment as done.Mar 24 2018, 3:45 PM

One might argue that the current code is correct. Who's to say the author didn't meant to make failfraction be dependent upon the clump size rather than the number of attempts?

It's also unnecessary to keep on going when failed exceeds the threshold only to discard the points later. But that is out-of-scope.

binaries/data/mods/public/maps/random/rmgen/placer/centered/ClumpPlacer.js
7

However, this tells that either this is or the current is wrong. If this one is considered correct, then this should be committed as this patch is also correct.

smiley accepted this revision.Jan 8 2019, 2:44 PM

As mentioned in the ticket, this is how the other placer does it and it’s nice to have some consistency. Also, the comment seems to suggest this is what the original author intended. Finally, as you have mentioned, it doesn’t really make sense to compare failed attempts with the size as opposed to the number of attempts.
The patch is correct.
At least now it would be stuck in some other queue.

This revision is now accepted and ready to land.Jan 8 2019, 2:44 PM
elexis added a comment.Jan 8 2019, 3:17 PM

Usually when we pass a failFraction, we mean the amount of the intended area, so that the shape generated by the placer remains intact to the specified degree IIRC.
Making it consistent for all placers sounds good.
A downside of that model is if one passes 80% failFraction and there is one successful circle that already covers 80% of the area, the placer will indicate success albeit the map author having intended more than 1 circle by asking for 80%

The comment might not actually be the one from the original author but what the one adding the comments thought it did.

(I didn't compare enough with the rest of the code to decide what's best)

smiley added a comment.Jan 8 2019, 5:04 PM

Usually when we pass a failFraction, we mean the amount of the intended area, so that the shape generated by the placer remains intact to the specified degree IIRC.

Not exactly how I had thought of it. I assumed it was more of tolerance. That's the closest word I could think of.