Page MenuHomeWildfire Games

Extending rmgen lib's SimpleGroup's place method to avoid collision of included SimpleObjects

Authored by elexis on Mar 4 2017, 10:13 PM.



Extends the rmgen lib's placer.js file to check for collision between several SimpleObjects included in a SimpleGroup. A SimpleGroup's center point is now used to randomly generate new center points for every SimpleObject included. Generated center points are checked against constraints and a given minimum object distance.
In Addition two bugs are fixed to properly generate and check these center points:

  1. Random points are now correctly generated around the given center point in several placing methods
  2. Engine coordinates "element.position.x" and "element.position.z" are divided by CELL_SIZE prior to comparing them to rmgen lib's coordinates to calculate correct distances
Test Plan

Generated several random maps for template mainland and checked results of generated stone mines in Atlas.

Diff Detail

rP 0 A.D. Public Repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

rapidelectron created this revision.Mar 4 2017, 10:13 PM
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Mar 4 2017, 10:13 PM
elexis added a subscriber: elexis.
elexis added inline comments.
242 ↗(On Diff #713)

This sounds like it should be changed in every map then. This should likely wait before the random map script part of D121 is committed before running into merge conflicts. Or even better make a separate proposal for the division fix if you need to change every file then (or write a ticket and let someone else do it)

elexis requested changes to this revision.Oct 29 2017, 9:59 PM

Sorry for noone posting a review in the past 6 months, but our one random map guy was unavailable and the other random map guy just got educated on how all these weird rmgen classes fit together.

From reading the code, the CELLSIZE and 0.5 + -> 0.5 * change seem to be unwanted and also change all maps.
But the new distance argument and retry loop seems to be preferable way to address this issue and we might have to commit some cleaned (as in rP20357) code equivalent to this retry loop taken from the SimpleObject as proposed by you.

So 2/3 request changes, 1/3 acceptance of the patch.

(It seems the only reason why we only noticed this bug with the stone mines is because the SimpleGroup is otherwise only used for fish, berries & acotrs (i.e. collision box disabled), animals (issue self correcting due to the animals moving and testing for collisions after the game), initial starting resources (only one object placed, so collision impossible).)

220 ↗(On Diff #713)

alphabetic order

242 ↗(On Diff #713)

Ah, because of the CELL_SIZE change below this is changed from 1 to 4.

That's not good, because it invalidates every number in every random map script!

360 ↗(On Diff #713)

I think the idea behind that 0.5 is to center the entity on the tile coordinate cx, cz.
The distance argument should be the radius, not the diameter, so I wouldn't really agree with the + to * change.

375 ↗(On Diff #713)

I'm dubious about doing the CELL_SIZE division in this place as well as the comparison with 1 here.
The number 1 seems to mean the footprint size in the rmgen coordinate grid (which is the tile grid, i.e. the numbers in map_sizes.json).
The rmgen library should use only one coordinate system where possible, so why would we do the CELL_SIZE division here only to test against 1 and nowhere else?

I suspect this is the part that fixes the reported "small stone mine inside a big stone mine bug". But this is due to the fact that the footprint of the mines isn't taken into account to begin with and just hapens to be solved with 4 (cellsize).

511 ↗(On Diff #713)

Given that we only seem to have issues with the stone mine use-case, just adding these arguments at the end of the constructor seems to work, even though avoidSelf seems to become unneeded / redundant with the distances.

519 ↗(On Diff #713)

Adding these new min/max distance arguments seems like the only way to address the issue if we want to keep using a SimpleGroup where each Object is able to avoid the other objects within the group.

522 ↗(On Diff #713)

(I wonder if this the right place for maxFailCount. I doubt the callers to place are happy with having it in place, not the constructor. But same is true for the SimpleObject)

529 ↗(On Diff #713)

(This while loop was copied from the SimpleObject and has the same issue that it needlessly nests if-else statements instead of just using &&, which allows removal of the fail variable)

546 ↗(On Diff #713)

This loop is actually correct, because it considers all SimpleObjects placed by the SimpleGroup and every Entity placed by the SimpleObject, i.e. everything placed.

552 ↗(On Diff #713)

I appreciate that there are distances here instead of the magic 1. I think the meaning of avoidSelf is redundant with the distances though.

557 ↗(On Diff #713)

This condition is always satisfied (usual issues with copying code. Perhaps we can unify those now 10 lines)

594 ↗(On Diff #713)

(Here the tiles with the placed objects are marked. A thought would be to use the avoidClasses mechanism to avoid resource collisions. But that conflicts with the fact that first all locations should be found, then all or none being placed.)

This revision now requires changes to proceed.Oct 29 2017, 9:59 PM

Ah, now that I read temples comment in #4338, I understand where that CELLSIZE is coming from. So ack with that.
From a quick grep Group and Object classes are the only ones constructing an Entity, so not unlikely to be a complete addressing of that bug. Nice find!

-> 50% acceptance

elexis commandeered this revision.Feb 20 2018, 3:36 AM
elexis edited reviewers, added: rapidelectron; removed: elexis.

I'm surprised this revision isn't a year old yet. Before I started rewriting maps/random/ it was too long in the review queue already, but then I had perfect knowledge of this file and still left it here.
But today is day where this bug will come to an end.

I will give you the credit for finding both essential issues in this patch, the different coordinate system issue addressed in rP20396 and the missing avoidance of previously placed SimpleObjects within the SimpleGroup (which temple also described in #4600 if I'm not mistaken).
So abandoning is not an option, accepting not really either as the patch comes out differently. Commandeering and closing will allow people to see your code in the history tab and my committed code fixing this issue here.

So thanks again for revealing the issues in these two prototypes!

546 ↗(On Diff #713)

Actually the center of the group should remain fixed, only the objects around that should be randomized.

Final word, we should have an obstruction testing mechanism (which would also be reused by the ObstructionConstraint in #4955) that could be used by Objects.
This would make it unnecessar to change every single call and remove the fact that it's error prone.
But the new avoidDistance argument is still useful as we can set a greater distance than the obstruction size.

This revision was not accepted when it landed; it landed in state Needs Revision.Feb 20 2018, 4:44 AM
This revision was automatically updated to reflect the committed changes.

Hi I think I found a problem I'm not sure, I couldn't select any appropriate action since the the ticket is closed.


I'm getting this error, I don't know if it's related to this change set

ERROR: JavaScript error: maps/random/rmgen/group.js line 44 TypeError: this.objects is undefined createObjectGroup@maps/random/rmgen/library.js:214:9 createObjectGroups/placeFunc@maps/random/rmgen/library.js:140:10 retryPlacing@maps/random/rmgen/library.js:78:16 createObjectGroups@maps/random/rmgen/library.js:143:9 createObjectGroupsDeprecated@maps/random/rmgen/library.js:96:9 createMines@maps/random/rmgen/gaia_entities.js:74:1 @maps/random/mainland.js:119:1

ERROR: CMapGeneratorWorker::Run: Failed to load RMS 'maps/random/mainland.js'

The issue seems to be in the mainland map file.

The issue seems to be in the mainland map file.

That's what the log says indeed.


missing comma