HomeWildfire Games

Remove civ-specific hardcoding in rmgen wall-placement script.

Description

Remove civ-specific hardcoding in rmgen wall-placement script.

Original Patch By: FeXoR
Reviewed By: FeXoR
Commented On By: elexis
Refs #2944
Differential Revision: https://code.wildfiregames.com/D900

Event Timeline

elexis raised a concern with this commit.Jan 10 2018, 6:22 PM
elexis added a subscriber: elexis.
elexis added inline comments.
/ps/trunk/binaries/data/mods/public/maps/random/rmgen/wall_builder.js
449

@s0600204 was the orientation intentionally changed from 45° to 0°? It bugs at the fortress map (ground texture and clPlayer class wrong)

If maps are mandated to specify all values used, the default values aren't forgotton as easily.

(Guess one might want to check the other calls for equally forgotton values)

This commit now has outstanding concerns.Jan 10 2018, 6:22 PM
elexis added inline comments.Jan 10 2018, 6:29 PM
/ps/trunk/binaries/data/mods/public/maps/random/rmgen/wall_builder.js
449

Also the ground floor is a bit too wide for the carthaginians on that map. (Would be nice to remove some civ hardcodings on that map, but out of scope)

s0600204 added inline comments.
/ps/trunk/binaries/data/mods/public/maps/random/rmgen/wall_builder.js
449

Not an intentional action on my part.

At the point in time when the changes that would ultimately become this commit were first made and issued as a patch, the default value for orientation was 0. This can be seen by looking at the earliest version of this patch that alters that line, which would be this one: https://trac.wildfiregames.com/attachment/ticket/2944/wallgen_additional_20150816.patch

Looking at the blame for this file, the default value for orientation for this function was changed in rP18198 by @FeXoR to fix #3988.

Looking through this commit at the default value of the orientation argument in the other functions in this file (where they have such an argument) shows that this is the only function where orientation was, or is, anything other than 0, before or after this commit.

So how would you like to proceed?

  1. Change the default value here, knowing that it would make it different to the default value of every other instance of that argument on all the other functions in this file,
  2. Alter any affected map files to send the appropriate orientation angle for that map,
  3. Something else?

Maps that use this function, and the value they pass as orientation:

  • Wild Lake: randFloat(0, 2 * PI)
  • Caledonian Meadows: randFloat(0, 2 * PI)
  • Fortress: doesn't pass a value
  • Danubius:
    • 5 x 0
    • 2 x Math.PI or PI

In addition, this function is used inside another function in this library, specifically PlaceFortress(). This passes through the value passed to it, defaulting to 0 if one isn't passed. The only map that uses this function is the Wall Demo (which does pass a value).

elexis added inline comments.Jan 12 2018, 12:05 AM
/ps/trunk/binaries/data/mods/public/maps/random/rmgen/wall_builder.js
449

In general if it's possible that the default value is wrong, it's better to not have a default and let every call specify the value IMO.
Another exception would be if the code becomes thousands of lines longer (see that starting base commit).
Maps should also really get the playerID right, so if it was up to me, I'd just not have defaults in this function.
But I'm ok with any solution you prefer as long as it fixes the issue.
(If I recall correctly we also had a rule on Phabricator that one can commit simple fixes like this without a review.)
A 45° angle on Fortress seems to be nicer than 0° for the same reason the CC uses it.

Also it is contradictory that placeCustomFortress places the templates of the civ of the passed playerID despite having a civ argument.

How about this, I'll fix the map and you update the template viewer patch for a23 instead?

s0600204 requested verification of this commit.Jan 14 2018, 12:29 AM
@elexis said:

But I'm ok with any solution you prefer as long as it fixes the issue.

I hope that rP20865 is satisfactory.

This commit now requires verification by auditors.Jan 14 2018, 12:29 AM
elexis accepted this commit.Jan 14 2018, 12:40 AM

Thanks for the fix. Guess one could have used BUILDING_ORIENTATION and pass it for the placeStartingEntities call too. Also it would be most failsafe and add some variability to the map to use an arbitrary angle per player, but that would be some work (but on the other hand civic centers are supposed to be 45° rotated everyhwere). But we have bigger fish to fry. :-)

All concerns with this commit have now been addressed.Jan 14 2018, 12:40 AM
elexis removed an auditor: elexis.Jan 14 2018, 12:41 AM

(Actually I didn't review the entire thing, at least yet)

This commit no longer requires audit.Jan 14 2018, 12:41 AM

While changing the wall_builder code to use vectors for positions, I noticed something off. I suspect it was one of the D900 commits:

a22:

rP21017 (i.e. before the vector commit):

See the carthaginian palisade gate and the briton walls being a degenerated triangle.
I made sure that the map looks exactly the same before and after the vector commit.

The "degenerated triangles" is a bug with that particular wall placement function, that is made more apparent as we now demonstrate it on the wall demo map. (FWIW, the wall_demo map is the only map that uses that particular wall placement function.)

The small cart wallset was using palisade gates long before D900 came into existence.

And the houses overlapping are because of the small radius passed to the wall placement functions. Possible solutions:

  • Replace the houses with something smaller (ie obelisks),
  • Make the walls used in each side longer (currently is [tower, medium, house], so [tower, long, long, house] might work, although you'd then get different civs' walls overlapping),
  • Tell 0ad to use a larger than giant sized map,
  • Disable some of the civs (set the SelectableInGameSetup attribute in the respective {civ}.json file to false) so fewer wallsets are ingested and thus drawn, permitting the map to not have to scale things down to get it to all fit on one map,

None of this was introduced by a D900-related commit, just made more obvious.

ping @s0600204, doesn't this satisfy the problem described in #2631 and provides more flexibility to the random map author than the skirmish replacer, i.e. can be marked as fixed or won't fix?

FeXoR added a comment.Nov 19 2018, 1:40 PM

I think this is about the second line from below ind it's placeGenericFortress()?
So I guess placement function isn't able to create any sane fortress because the radius is small compared to the long wall sizes.
If that's the case it could be fixed by:

  • Change placeGenericFortress() to use smaller wall parts if the radius is smaller than the length of the "long" wall
  • Change placeGenericFortress() to have a minimum radius dependent on the "long" wall's length
  • Change the map to have a minimum radius for placeGenericFortress() (overlapping is not that big of a deal here since one still can determine if everything works in general IMO)
  • Use larger than giant maps (not so sure about this)

Likely all of the above (or similar) should be done in the end

I don't get the thing with the houses or, if any, other issues.

ping @s0600204, doesn't this satisfy the problem described in #2631 and provides more flexibility to the random map author than the skirmish replacer, i.e. can be marked as fixed or won't fix?

Resolved. Thank you for bringing that ticket to my attention.

About the unintended wallsets:

The purpose of the demo maps is to show mappers and other curious folks what is possible with the library, i.e. the more realistic or re-usable the use-case, the better.
Also maps that map generation according to the given user settings is better than maps that don't.
So I suppose the map could use a fixed radius for every fortress rather than trying to scale it down and just stop placing stuff that is outside of the map area. Would that fix it?
(Ideally the buildings would also be something that could be seen in a map, rather than placeholders)

FeXoR added a comment.EditedNov 21 2018, 10:00 PM

(The obelisks are just meant to show the center e.g. to show where it is on fortress (the "center of mass") compared to e.g. regular polygons/circular where it's just the center of symmetry)

So I suppose the map could use a fixed radius for every fortress rather than trying to scale it down and just stop placing stuff that is outside of the map area. Would that fix it?

Not in every case sine mod's wall sets can contain walls of any length so no fixed value can solve this for mods. For the public mod, yes, e.g. two times the length of a long wall segment at least should do fine.

(The obelisks are just meant to show the center e.g. to show where it is on fortress (the "center of mass") compared to e.g. regular polygons/circular where it's just the center of symmetry)

The obelisk in the center is good, just saying walls should not be replaced with placeholders to work around.

So I suppose the map could use a fixed radius for every fortress rather than trying to scale it down and just stop placing stuff that is outside of the map area. Would that fix it?

Not in every case sine mod's wall sets can contain walls of any length so no fixed value can solve this for mods. For the public mod, yes, e.g. two times the length of a long wall segment at least should do fine.

It depends on the wall placement type that is called, right? For the regular walls we can predict the needed area for a given civ, for irregular walls one can estimate (presumably more than 2 long wallpieces I suppose).