Page MenuHomeWildfire Games

Remove civ-specific hardcoding in Random Map Generation Wall Placement script.
ClosedPublic

Authored by s0600204 on Sep 11 2017, 2:34 AM.

Details

Summary

The code in this revision has the primary purpose of stripping out the civ-specific hardcoding inside the Random Map Generation wall placement script.

At present, this hardcoding contains:

  • For each civ:
    • A wall scale multiplier (used to calculate wall piece length)
  • For each wall element of each civ:
    • The name is derived from a pattern, feeding in a civ code,
    • The length is derived from a constant number multiplied by the above mentioned civ's "wall scale" multiplier.
  • And for some "special" pieces, a hardcoded indent, orientation and length is set.

This hardcoding has been removed, with any specific indent or orientation being moved to the respective template files; and a list of wallsets that a civ can build is now read from that civ's {civ}.json file.

This change has the dual advantage of:

  • Being easier for modders to add new civs/wallsets to the game, or edit existing ones, and getting them to work with the rms-wall-placement; and
  • The rms-wall-placement code now reads the same wall-length values as the wall-placement code used by players placing walls in game. (There is the caveat that they do handle them slightly differently.)

This is a fairly large patch, so I'm suggesting it be committed across three commits:

  1. Editing the templates to add the new indent and orientation values, (Done in rP20589)
  2. Committing of the wall-placement code,
  3. Committing of changes to all random maps that currently use the rms-wall-placement script.

Whilst not all of @FeXoR's concerns about standardising how the two different wall-placement codebases handle and calculate wall lengths, end points, and overlapping are addressed; the hope is that these concerns can be addressed in a later patch that can focus entirely on that problem. (And be much smaller and concise than this one.)

(Patch originally started by FeXoR, edited by me (s0600204), passed back and forth, before sitting in limbo until rebase'd and uploaded here, with FeXoR's permission.)

Test Plan

Merge patch locally.

Run/generate the "Random" > "Demos" > "Wall Demo" map (on "Large", "Very large", or "Giant" size) (either in game or in the editor, it shouldn't matter)

Make sure the walls are drawn ok.

  • The walls should be drawn with the correct orientation and positioning,
  • There shouldn't be any large (unintentional) gaps between wall segments (tiny gaps are okay, as this just indicates a small tweaking of wall lengths are needed, and that can be addressed later)
  • Forts, towers, and other such buildings should be drawn with appropriate indents.

As the patch modifies some wall lengths to remove gaps when drawn by the rms-wall-placement, make sure that these new values work with the players'-wall-placement for those wallsets that were altered (athen, mace, maur, ptol, sele, spart). (The updated wall lengths have been committed.)

Run/Generate the following random maps to make sure that the walls, fences (and in the case of Danubius, ritual circles) built/placed by the map generator are present and correct:

  • Caledonian Meadows
  • Danubius
  • Fortress
  • Wild Lake

Diff Detail

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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/globalscripts/Templates.js
|  92| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 417| function·placeWall(startX,·startY,·wall·=·[],·style,·playerId·=·0,·orientation·=·0)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 471| function·placeFortress(centerX,·centerY,·type·=·"medium",·style,·playerId·=·0,·orientation·=·0)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 491| function·placeLinearWall(startX,·startY,·targetX,·targetY,·wallPart·=·undefined,·style,·playerId·=·0,·endWithFirst·=·true)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 583| function·placeCircularWall(centerX,·centerY,·radius,·wallPart,·style,·playerId·=·0,·orientation·=·0,·maxAngle·=·Math.PI·*·2,·endWithFirst,·maxBendOff·=·0)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 674| function·placePolygonalWall(centerX,·centerY,·radius,·wallPart,·cornerWallElement·=·"tower",·style,·playerId·=·0,·orientation·=·0,·numCorners·=·8,·skipFirstWall·=·true)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 735| function·placeIrregularPolygonalWall(centerX,·centerY,·radius,·cornerWallElement·=·"tower",·style,·playerId·=·0,·orientation·=·0,·numCorners,·irregularity·=·0.5,·skipFirstWall·=·false,·wallPartsAssortment)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 735| function·placeIrregularPolygonalWall(centerX,·centerY,·radius,·cornerWallElement·=·"tower",·style,·playerId·=·0,·orientation·=·0,·numCorners,·irregularity·=·0.5,·skipFirstWall·=·false,·wallPartsAssortment)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 735| function·placeIrregularPolygonalWall(centerX,·centerY,·radius,·cornerWallElement·=·"tower",·style,·playerId·=·0,·orientation·=·0,·numCorners,·irregularity·=·0.5,·skipFirstWall·=·false,·wallPartsAssortment)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 862| function·placeGenericFortress(centerX,·centerY,·radius·=·20,·playerId·=·0,·style,·irregularity·=·0.5,·gateOccurence·=·3,·maxTrys·=·100)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/caledonian_meadows.js
| 429| »   »   let·actor·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'actor' to 'undefined'.
Imarok added a subscriber: Imarok.Nov 28 2017, 12:50 PM

You may want to look at the linting results from vulcan

FeXoR added a comment.EditedDec 3 2017, 12:41 PM

Thanks for the feedback!

@s0600204
The purpose of this function I had in mind when writing it was to provide a helper tool to place stuff in an orderly manner in a line that can bend.
If it can be made useful otherwise even better.
The first things that come to my mind is use this for walls/fences, roads/bridges, fortresses (closed walls), parks or entire cities (though orderly more in a 2D way than a 1D like a line), hence the name of the fist use case "wall_builder".
So there is no "misuse" or "abuse" happening.

However, with the provided patch the cases in which this functionality can be used are reduced to predetermined cases.
This can be avoided by building the data structure from the template data at import of this library.
This should be easy to achieve since everything needed is there and just the time when what is done is slightly different.
That way we can have both IMO positive things:

  • Modifications don't have to change random map lib code in order to make use of this lib.
  • Map designers still have a general purpose line placing tool

As was the roads style, on the basis that neither others nor roads are walls.

Would renaming the file to "linePlacementTool.js" make you more comfortable?

The "wallElement" class was removed. I don't like that at all since it makes adding map specific wall elements unnecessarily complicated.

To be fair, the elements you're attempting to add with that class in Caledonian Meadows aren't really wall elements - that map (and Danubius) (ab)uses the wall-placement code for its own purposes. So it's no wonder it's a little unpleasant.

I have no idea where you take that from. I wrote this to be useful for random maps to place stuff in an orderly manner in a line.

@ Ritual place: I agree that there are other ways to do it but there is no abuse happening in the eyes of the author of this tool ;)

[...] but after it is loaded getWallElement must not be used any more.

Completely disagree, sorry. The entire purpose of the function is so that the wall placement functions can ask "I want this element from this style" and the function returns it. To only use it during init not only defeats the purpose of having such a function in the first place, but you'd find you'd need a function that does that anyway, as every wall placement function needs some way to fetch the wall elements it needs (and provide a suitable fallback if necessary).

That's why I introduced the wallPiece class.

I'll let @elexis argue his case about the deepfreezeing.c

Agreed ;)
Continueing to read so this will be a double post again ^^

FeXoR added a comment.EditedDec 3 2017, 12:58 PM

To change the indentation of e.g. a defense tower would require to copy the wall set, change the indentation parameter and rewrite the wall alignment function.
Without the freeze and with the wall stiles constructed at import only the indent parameter would need to be changed.

Another case would be if a map prefers the towers at Pi/2 corners to have their faces aligned with the adjacent walls: Only the rotation parameter would need to be changed.

Both are IMO valid things to do so they should be allowed.

@s0600204 I can imagine that after you worked quite a bit on this you have your own concept of what this can/should be used for. And that's absolutely OK and good. But please don't enforce stuff on the random map authors that is unneeded. Instead provide the usual usecase and allow the unusual ;)

elexis added a comment.Dec 3 2017, 1:14 PM

The ritual place on danubius should be done with vector algebra ideally. I have this distributePointsOnCircle function now, so it should be even easier now to replace.

I don't have a strong opinion about the deepfreeze, it's just good practice to mark things as read-only in many cases.
As s0600204 said, if someone wants to modify a civ wall, he can still clone it to a new style and modify that. That might be cleaner. For instance someone might want to create some iberian fortress with custom wall styles and then forget that the same wall theme is afterwards used for the iberian starting walls. Or at least the code might not show it immediately.

Patch looking much nicer already, perhaps you can find a way to commit the template changes separately?

Itms added a subscriber: Itms.Dec 3 2017, 8:14 PM
FeXoR added a comment.EditedDec 4 2017, 3:24 PM

I wanted to clarify that my proposal is only a simple solution, not the definite way to go.

However, being able to change/add wall elements and sets from a RMS without minding the template structure or the wall element/set coming from the templates or the map is not optional IMO.

elexis added a comment.Dec 4 2017, 3:38 PM

FeXoR the discomfort you mentioned is what you experience when trying to optimize some values for caledonian meadows? If so perhaps I can reproduce your discomfort and post some opinion on how to address it.

binaries/data/mods/public/maps/random/rmgen/library.js
397 ↗(On Diff #4420)

entPath is templateName?

Can we avoid that return and error hard if someone ordered somthing that couldn't be answered (or is that useful somewhere?)
Does that happen to be return Engine.GetTemplate(templateName).filter(key => key_list.indexOf(key) != -1)?
Otherwise for (let key of key_list) (unless key is a reserved keyword).
Is that function unused or my phabricator bugging? If the former, add it in the patch that needs it (if its only one line used once, perhaps we dont need the function)

FeXoR accepted this revision.EditedDec 4 2017, 10:02 PM

@elexis No, there isn't an actual issue right now. I mentioned those two cases that came to mind above. I don't think this will help reducing debug time consumption and is IMO a pure limitation.

Sine everything else seems to work now I would agree to a commit since all besides the deepfreeze is nitpicking like:

  • Do we reall need that "_stone" suffix for the wall sets? I'd remove them and only have a suffix for the unusual cases like rome_siege
  • EDIT: Consider the following as scrapped(Is getWallElement really the right place for the case checks? I don't really have a good alternative but the function is called quite some times.) It returns the element before the checks if already present x)
  • IMO "entPath" isn't needed as a wall element's property. Instead, if needed, read the template data and add them to g_WallStyles. After that the template is irrelevant.

Over all great job!

Please check if everything works cleanly with the current revision before commit, plz.

@s0600204 If you change your mind and agree to remove the deepfreeze I'd appreciate that allot.
But before this needs another major rebase I'd say: Go for it ;)

This revision is now accepted and ready to land.Dec 4 2017, 10:02 PM

These three things must be fixed before committing and you can do that without uploading a new version unless FeXoR wants to see it:

  • Procedural code is terrible for libraries. rmgen/ files may only contain globals and functions, but no series of statements! So put this into a function and call it in some existing init function or do const g_Foo = initFoo();
  • The numeric template changes that are correct independent of the rmgen feature should be committed separately.
  • Remove that unused function, everyone can call GetTemplateas wished and do custom processing with it.

(I still think the files would be much better off with much less comments, but that can certainly be done whenever by whomever.)

(I will try to test later, my danubius gaia fellows need to be able to deliver that damage)

binaries/data/mods/public/maps/random/rmgen/library.js
397 ↗(On Diff #4420)

nuke nuke

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
803 ↗(On Diff #4420)

Infinity. if MAX_VALUE actually makes it into the engine in some coordinate it would fail equally.

s0600204 planned changes to this revision.Dec 4 2017, 11:33 PM
s0600204 updated this revision to Diff 4575.Dec 5 2017, 9:39 PM
s0600204 marked 6 inline comments as done.
s0600204 edited the summary of this revision. (Show Details)
s0600204 edited the test plan for this revision. (Show Details)

Update/Rebase

  • Rebase post-commit of wall length updates,
  • Move procedural code into functions as requested by elexis,
  • Remove an unused function,
  • Revert back to using a trigger-script instead of a map-setting flag to reveal entire map on Wall Demo map,
  • Apply fix to Wild Lake which had same problem as Caledonian Meadows wrt farmsteads and fences. (Code shared between those two maps can and should be dealt with in a later clean-up revision.)
This revision is now accepted and ready to land.Dec 5 2017, 9:39 PM
s0600204 requested review of this revision.Dec 5 2017, 9:39 PM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/globalscripts/Templates.js
|  91| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 429| function·placeWall(startX,·startY,·wall·=·[],·style,·playerId·=·0,·orientation·=·0)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 483| function·placeFortress(centerX,·centerY,·type·=·"medium",·style,·playerId·=·0,·orientation·=·0)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 503| function·placeLinearWall(startX,·startY,·targetX,·targetY,·wallPart·=·undefined,·style,·playerId·=·0,·endWithFirst·=·true)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 595| function·placeCircularWall(centerX,·centerY,·radius,·wallPart,·style,·playerId·=·0,·orientation·=·0,·maxAngle·=·Math.PI·*·2,·endWithFirst,·maxBendOff·=·0)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 686| function·placePolygonalWall(centerX,·centerY,·radius,·wallPart,·cornerWallElement·=·"tower",·style,·playerId·=·0,·orientation·=·0,·numCorners·=·8,·skipFirstWall·=·true)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 747| function·placeIrregularPolygonalWall(centerX,·centerY,·radius,·cornerWallElement·=·"tower",·style,·playerId·=·0,·orientation·=·0,·numCorners,·irregularity·=·0.5,·skipFirstWall·=·false,·wallPartsAssortment)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 747| function·placeIrregularPolygonalWall(centerX,·centerY,·radius,·cornerWallElement·=·"tower",·style,·playerId·=·0,·orientation·=·0,·numCorners,·irregularity·=·0.5,·skipFirstWall·=·false,·wallPartsAssortment)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 747| function·placeIrregularPolygonalWall(centerX,·centerY,·radius,·cornerWallElement·=·"tower",·style,·playerId·=·0,·orientation·=·0,·numCorners,·irregularity·=·0.5,·skipFirstWall·=·false,·wallPartsAssortment)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 874| function·placeGenericFortress(centerX,·centerY,·radius·=·20,·playerId·=·0,·style,·irregularity·=·0.5,·gateOccurence·=·3,·maxTrys·=·100)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/caledonian_meadows.js
| 429| »   »   let·actor·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'actor' to 'undefined'.

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

Thanks for adding the functions, they were inevitable if we want to maintain this.

entPath should be templateName. Names starting with ent should be about actual instances of entities.

binaries/data/mods/public/maps/random/rmgen/library.js
26 ↗(On Diff #4575)

perhaps it's nicer to inline this, we do it in the simulation too (I wish to minimize the set of globals). Perhaps we could alternatively have it in globalscripts/ some day (unification thing)

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
662 ↗(On Diff #3897)

(Would be nice to find out if this TODO and the epsilon can be killed)

803 ↗(On Diff #4420)

Thanks

2 ↗(On Diff #4575)

@file if you want, we have it in some other rmgen/ files

8 ↗(On Diff #4575)

Probably ok to keep it here. (Just wondering we might want to move the declaration to the maps like we do with other constants and then pass this around all the time... maybe better to keep it here.)

9 ↗(On Diff #4575)

Same comment as g_FortressTypeKeys. I'd be happy to get rid of all calls to this one.

11 ↗(On Diff #4575)

FortressKeys unused and don't really see the need to keep it in a second global (https://en.wikipedia.org/wiki/Single_source_of_truth thing)

30 ↗(On Diff #4575)

(would be less maintenance to have it agnostic of filenames somehow)

102 ↗(On Diff #4575)

for (let fortressType in defaultFortresses)

235 ↗(On Diff #4575)

element.substr("turn_".length)? (same above) (bit odd to parse a number from a name rather than having it in a template property)

241 ↗(On Diff #4575)

Would be nice to avoid fallbacks

369 ↗(On Diff #4575)

When already fixing the whitespace for (let align of alginment)

399 ↗(On Diff #4575)

Probably better to make playerID mandatory.
Why is the playerID irrelevant when called from getWallElement?

401 ↗(On Diff #4575)

!g_WallStyle[style]? same for similar checks

481 ↗(On Diff #4575)

I don't see an information in the 6 lines above that isn't in the one line below

510 ↗(On Diff #4575)

I saw bend !== 0 above, so this one should also have to check explicitly or both should use non-explicit checks

553 ↗(On Diff #4575)

if (entPath)? same below

665 ↗(On Diff #4575)

(The addition of Math and whitespace fixes could be committed separately too, but sounds like a lot of rebase work, no need to)

758 ↗(On Diff #4575)

I expect that the reverse shouldn't break the iterator if we use a for of loop here

binaries/data/mods/public/maps/random/wall_demo.js
74 ↗(On Diff #4575)

let wallStyles = Object.keys(thing).length; + for (let wallStyle in g_WallStyles)?

binaries/data/mods/public/maps/random/wild_lake.js
274 ↗(On Diff #4575)

Oh yeah and then we also don't have to add code to keep things in sync

binaries/data/mods/public/simulation/components/GuiInterface.js
1257 ↗(On Diff #4420)

(Some prefer moving the civ query out of this loop, some prefer to inline if its not performance critical, others don't care)

FeXoR added inline comments.Dec 6 2017, 1:47 AM
binaries/data/mods/public/maps/random/rmgen/wall_builder.js
662 ↗(On Diff #3897)

Sadly not. The float errors might sum up and if they do so that the sum us smaller than 2Pi there will be wall elements placed twice.

235 ↗(On Diff #4575)

I thought the same ^^

399 ↗(On Diff #4575)

Because getWallElement just returns the data of an element, it doesn't place it (for any player).
We seem to have very different understanding of this but IMO it's not good to force others...

elexis added inline comments.Dec 6 2017, 1:39 PM
binaries/data/mods/public/maps/random/rmgen/wall_builder.js
399 ↗(On Diff #4575)

A function called validate should test if something is correct.
A function that gets something should start with get.
Perhaps playerID = undefined would be more accurate.
Anyway, I don't want to block this and I feel it might be much easier to do further cleanup in separate commits.
(Also I have a plan to move this file soonish)

FeXoR accepted this revision.Dec 6 2017, 3:08 PM

I'm not convinced that validateStyle is needed in the first pace (and that _stone suffix) but OK.

I'd say go for it.

This revision is now accepted and ready to land.Dec 6 2017, 3:08 PM
s0600204 updated this revision to Diff 4614.Dec 6 2017, 9:15 PM
s0600204 marked 14 inline comments as done.

Update in response to elexis' comments.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
90 ↗(On Diff #3632)

(That said, providing a centerToFirstElement argument (which doesn't happen anywhere, at the time of writing this) could ostensibly offset the fortress so it's not actually centred around the [x, z] spot it's placed. Although there are probably better ways of achieving the same goal.)

241 ↗(On Diff #4575)

And yet we need this one to support incorporating structures (such as houses) with the palisade wallset.

(Or we could emit a warning that you can't use structures/{civ}_{structure} with wallsets (like palisades) that don't have an intrinsic or native civ. That would cause the wall demo map to display said warnings, however.)

(Or we could return a blank wall element, which FeXoR might not be happy about.)

binaries/data/mods/public/maps/random/wall_demo.js
74 ↗(On Diff #4575)

You're going to have to provide a bit more of an explanation as to what you're asking me to do, please.

binaries/data/mods/public/simulation/components/GuiInterface.js
1257 ↗(On Diff #4420)

(To be honest, I think a discussion should be had as to whether we want to support {civ} replacement in the wallset component (or {native} even, wrt D1084). Then this replacement could be done properly and in the correct location.)

Vulcan added a comment.Dec 6 2017, 9:55 PM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/maps/random/caledonian_meadows.js
| 428| »   »   let·actor·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'actor' to 'undefined'.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 425| function·placeWall(startX,·startY,·wall·=·[],·style,·playerId·=·0,·orientation·=·0)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 472| function·placeFortress(centerX,·centerY,·type·=·"medium",·style,·playerId·=·0,·orientation·=·0)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 492| function·placeLinearWall(startX,·startY,·targetX,·targetY,·wallPart·=·undefined,·style,·playerId·=·0,·endWithFirst·=·true)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 582| function·placeCircularWall(centerX,·centerY,·radius,·wallPart,·style,·playerId·=·0,·orientation·=·0,·maxAngle·=·Math.PI·*·2,·endWithFirst,·maxBendOff·=·0)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 672| function·placePolygonalWall(centerX,·centerY,·radius,·wallPart,·cornerWallElement·=·"tower",·style,·playerId·=·0,·orientation·=·0,·numCorners·=·8,·skipFirstWall·=·true)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 733| function·placeIrregularPolygonalWall(centerX,·centerY,·radius,·cornerWallElement·=·"tower",·style,·playerId·=·0,·orientation·=·0,·numCorners,·irregularity·=·0.5,·skipFirstWall·=·false,·wallPartsAssortment)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 733| function·placeIrregularPolygonalWall(centerX,·centerY,·radius,·cornerWallElement·=·"tower",·style,·playerId·=·0,·orientation·=·0,·numCorners,·irregularity·=·0.5,·skipFirstWall·=·false,·wallPartsAssortment)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 733| function·placeIrregularPolygonalWall(centerX,·centerY,·radius,·cornerWallElement·=·"tower",·style,·playerId·=·0,·orientation·=·0,·numCorners,·irregularity·=·0.5,·skipFirstWall·=·false,·wallPartsAssortment)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 860| function·placeGenericFortress(centerX,·centerY,·radius·=·20,·playerId·=·0,·style,·irregularity·=·0.5,·gateOccurence·=·3,·maxTrys·=·100)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/globalscripts/Templates.js
|  91| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

Very grateful about the civ and template hardcodings in responses to the yelling about code style.

Besides that entPath should be templateName (which should be fixed before the commit) I don't see any other blatant issues.

Phabricator also needs 30 seconds or something to add a new inline commit which I see as a sign for moving on ;-)
Tested on danubius, still works.

binaries/data/mods/public/maps/random/wall_demo.js
74 ↗(On Diff #4575)

Just that you can use a for ... in in this and some but not all loops, since the index is not used.
And wallStyleList can be replaced with a wallStyleCount afaics.

binaries/data/mods/public/maps/random/wall_demo.json
11 ↗(On Diff #3897)

Thanks for the removal!
(There is still an open concern on that one commit where we had added a trigger script to reveal the map in order not to change the gamesettings. Will be hard to find a solution I'm afraid.)

12 ↗(On Diff #4614)

whitespace (1 space rest tabs)

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
FeXoR added inline comments.Dec 7 2017, 12:30 AM
binaries/data/mods/public/maps/random/rmgen/wall_builder.js
90 ↗(On Diff #3632)

The function used when centerToFirstElement is just ONE of many possible ways to do it (basically the center of mass if all wall elements "weigh" the same) but definitely not always a very good one.
And there's no problem if the "center" is not even insight the fortress.
So leave PPL the chance to determine this manually in case the function is to far off (like "L" shaped fortresses or square fortresses with only small wall parts on one and long wall parts on the other side).

241 ↗(On Diff #4575)

Absolutely correct, I won't like not being able to place civ dependent structures within a palisade wall.
(And I don't think Gaia should have a civ. AFAIK the previous fallback was Athen - before that Hele - , but that doesn't really matter. Would be nice if the fallback civ was always the same but it's good that civ comes from g_CivData)

FeXoR added a comment.Dec 7 2017, 12:43 AM

Offtopic:
@elexis "Phabricator also needs 30 seconds or something to add a new inline commit which I see as a sign for moving on ;-)"
For me that is a clear sign of a ill chosen web framework ^^
(...and a reason I like trac somewhat more.
But I also like phabricators ToDo list so, OK ^^)

elexis added inline comments.Dec 7 2017, 2:38 PM
binaries/data/mods/public/maps/random/rmgen/wall_builder.js
241 ↗(On Diff #4575)

s0600204 so you're saying if we fix the palisade template and give each civ a civ-specific palisade (like fatherbushido planned with Nesico) we can remove this fallback?

s0600204 updated this revision to Diff 4636.Dec 7 2017, 9:02 PM
s0600204 marked 3 inline comments as done.

More tweaks:

  • for...in instead of basic for
  • templateName instead of entPath
elexis added a comment.Dec 7 2017, 9:12 PM

I meant the for loop I quoted there, for (let wallStyle in g_WallStyles), this way you don't need the helper variable let style = g_WallStyles[styleIndex]. As far as I see the only thing that really needs Object.keys(g_WallStyles) is when we need to get the length.

let wallStylesCount = Object.keys(g_WallStyles).length;
for (let wallStyle in g_WallStyles)

Unless I'm not seeing any use.
This way the global wallStyleList = Object.keys(g_WallStyles) could be avoided.
(Anyway, not a blocker at all.)

Vulcan added a comment.Dec 7 2017, 9:23 PM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/globalscripts/Templates.js
|  91| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/maps/random/caledonian_meadows.js
| 428| »   »   let·actor·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'actor' to 'undefined'.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 425| function·placeWall(startX,·startY,·wall·=·[],·style,·playerId·=·0,·orientation·=·0)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 472| function·placeFortress(centerX,·centerY,·type·=·"medium",·style,·playerId·=·0,·orientation·=·0)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 492| function·placeLinearWall(startX,·startY,·targetX,·targetY,·wallPart·=·undefined,·style,·playerId·=·0,·endWithFirst·=·true)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 582| function·placeCircularWall(centerX,·centerY,·radius,·wallPart,·style,·playerId·=·0,·orientation·=·0,·maxAngle·=·Math.PI·*·2,·endWithFirst,·maxBendOff·=·0)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 672| function·placePolygonalWall(centerX,·centerY,·radius,·wallPart,·cornerWallElement·=·"tower",·style,·playerId·=·0,·orientation·=·0,·numCorners·=·8,·skipFirstWall·=·true)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 733| function·placeIrregularPolygonalWall(centerX,·centerY,·radius,·cornerWallElement·=·"tower",·style,·playerId·=·0,·orientation·=·0,·numCorners,·irregularity·=·0.5,·skipFirstWall·=·false,·wallPartsAssortment)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 733| function·placeIrregularPolygonalWall(centerX,·centerY,·radius,·cornerWallElement·=·"tower",·style,·playerId·=·0,·orientation·=·0,·numCorners,·irregularity·=·0.5,·skipFirstWall·=·false,·wallPartsAssortment)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 733| function·placeIrregularPolygonalWall(centerX,·centerY,·radius,·cornerWallElement·=·"tower",·style,·playerId·=·0,·orientation·=·0,·numCorners,·irregularity·=·0.5,·skipFirstWall·=·false,·wallPartsAssortment)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
| 860| function·placeGenericFortress(centerX,·centerY,·radius·=·20,·playerId·=·0,·style,·irregularity·=·0.5,·gateOccurence·=·3,·maxTrys·=·100)
|    | [NORMAL] JSHintBear:
|    | Regular parameters should not come after default parameters.
Vulcan added a comment.Dec 7 2017, 9:26 PM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
In D900#44956, @elexis wrote:

I meant the for loop I quoted there, [...]

We need an incrementing count in each loop, which is used as a multiplier to help space the various wall arrangements along the x-axis. If not provided by the for statement in some way, then another global/helper variable would need to be added.

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
241 ↗(On Diff #4575)

[...] planned with Nescio [...]

(Can't seem to find that discussion)

If every civ had a structures/{civ}_wallset_palisade, then yes.

structures/athen_wallset_palisade.xml
<?xml version="1.0" encoding="utf-8"?>
<Entity parent="other/wallset_palisade">
  <Identity>
    <Civ>athen</Civ>
  </Identity>
</Entity>

It would require any map that wanted to use it to specify something like brit_palisade or maur_palisade rather than just palisade. (Looking at Danubius, there.) Unless that map did

g_WallStyles["palisade"] = loadWallset("other/palisade_wallset");

manually itself, which kinda puts us back to square one, esp. if that map then tried including arbitrary structures through this function (rather than pre-defining them).

elexis added a comment.Dec 7 2017, 9:45 PM

loop

So I'm just too stupid to read, sorry.

brit_palisade.xml

Yes, that. Danubius would use the gaul one since it uses gaul templates everywhere else.
There were discussions about that proposal scattered over random trac tickets and irc discussions I believe (in response to rP19694 IIRC).
Nescio just has tons of template cleanup patches, but not one for this yet (but I wouldn't be surprised if he would be interested in writing one if we don't want to do it ourselves. We should review one of his in that case however)

(We would need the spikes too for at least gauls)
(Same goes for that wooden tower on Gallic Fields which should also use gauls according to its name)
Good.

(I would click on accept if I had read it completely, but I'm satisfied so far. Code comments still a bit TLDR, but that had been the least concern)

FeXoR accepted this revision.Dec 9 2017, 11:45 PM

Everything seems to be in order.

@s0600204 Thanks again for your effort!

This revision was automatically updated to reflect the committed changes.