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
Details
Details
- Reviewers
Stan - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP26503: Make ClumpPlacer determine the correct fraction of failed attempts to place…
- Trac Tickets
- #5092
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
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
| binaries/data/mods/public/maps/random/rmgen/placer/centered/ClumpPlacer.js | ||
|---|---|---|
| 88 | += maxRadiusSteps outside of the loop would cost less performance | |
Comment Actions
Added the suggested change of elexis for better performance. Added myself to programming.json.
Comment Actions
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?
Comment Actions
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. | |
Comment Actions
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.
Comment Actions
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)
Comment Actions
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.