[PATCH] In RMS: Fix iber civ bonus wall. Fix celt split civs errors. Add roads.

[fabio]

The walls generated by the walls random map library are not properly aligned. When towers have lower terrain walls "graphically penetrate" towers:

[FeXoR]

Is this the wall the Iberians are starting with? If this is a modified RMS please tell me which method you used.

I'll check this on Monday.

[fabio]

Replying to FeXoR:

Is this the wall the Iberians are starting with?

Yes, it is the standard "Corsica vs Sardinia" random map (just reproduced now on current SVN).

[FeXoR]

Walls overlapping with towers beyond the mid of the towers could occur in two cases:

• The 'safety width reduction' to enforce no gaps are there in the random wall parts. Fixed by removing 'safety width'.
• The gap from the first random wall parts wall element and the last is smaller then a medium wall (which is used to close the random wall parts). Fixed by checking if the gap is big enough (otherwise nothing is placed). This has to be redone when gates are in to ensure the wall is closed.

Both are fixed in the file attached. AFAIK this works fine for Iberians (with their large tower, 'large' in comparison to half the length of a short wall, see http://www.wildfiregames.com/forum/index.php?showtopic=16012) but might cause gaps for other civilizations with the same placement method.

However, it is simply impossible to fit Mythos_Ruler's request for 'generic' looking walls without something like that on maps with high hight differences near the start positions (perhaps obstructions that close to the start locations should be avoided or on such maps only towers should be created for Iberians like on some other RMSs).

Here's a screen shot of what still happens:

The linear wall (with which the random wall is closed) has similar problems but I think not much more than the in-game method.

I will add a wall creation like the in-game drag method though it would look much less 'organic' and it will not be as generic as the other placement methods (because in the in-game method only the civ can be determined, not the used wall elements). Guess I will get to work on Monday.

[FeXoR]

Here's the new Iberian wall with gates replaced with gaps. The maximum overlapping I noticed was about 0.5 rms tiles (~0.1 engine units). I guess that should be OK. Now only the corners are randomly placed and a well fitting corner derivation is picked out of 100 tries. Long walls are just set in between while every 3rd wall is a gate (gap until gates work, to try just replace wall_builder.js line 802 "entry" with "gate"). Due to the longer walls there are less towers now. Thx to quantumstate I got rid of a bug that made me hit the ceiling ;) THX!

[FeXoR]

Historicbruno asked me to enlarge the wall to grand more space but it's only possible on very few maps without placing walls in water or on mountains. I think there should be a convention for random maps that the players should have a radius of 20 rms tiles to build his base. More is not possible on tiny maps and even on small or medium maps with many players. Less would restrict the wall size further.

Some testing on small maps with 2 players for obstructions in 20 rms tiles radius of the start positions:

• Aegean Sea: Woods are to close
• Alpine Lakes: Mountains are a bit to close, but OK IMO
• Alpine Valley: Mountains are to close (player might be trapped inside his fortress), Water is to close as well
• Anatolian Plateau: OK but map border prevent larger walls
• Archipelago: OK but map border and water prevent larger walls
• Ardennes Forest: OK but holes and woods prevent larger walls
• Atlas Mountains: OK but mountains, woods, map border and expansion resources prevent larger walls
• Cantabrian Highlands: Towers only, towers radius might be a little to wide
• Canyon: OK but mountains, map border and woods prevent larger walls
• Continent: OK but water and woods prevent larger walls
• Corsica vs Sardinia: Cliffs are MUCH to close. I can live with that though. Woods prevent larger walls
• Cycladic Archipelago: Towers only, OK
• Deep Forest: Perfect (Of cause, I made it ), grants a bit more space but not much.
• Fortress: Irrelevant, custom fortresses not the Iberian default civ bonus one.
• Guadalquivir River: Map border to close but OK IMO
• Gulf of Bothnia: OK, Water and map border prevent larger walls
• Hyrcanian Shores: OK, Mountains prevent larger walls
• Islands: Towers only, OK (Just noticed that there is not enough wood to advance to II and build ships )
• Lake: Mountains and woods are to close, but OK IMO
• Latium: Cliffs are to close, woods are a bit close too
• Migration: Towers only, OK (Same problem as Islands: Not enough wood)
• Neareastern Badlands: Cliffs are to close
• Persian Highlands: OK, Mountains and maybe woods prevent larger walls
• Phoenician Levant: OK, Mountains pervent larger walls (The map is exported twice at the end of the RMS which leads to a wrong entity placement angle! This is an export issue as well IMO (caused by the coordinate swap from 2D to 3D for the engine), may file a bug report...)
• Rhine Marshlands: Water is to close (Player may be trapped inside his wall), Woods are a bit close too
• Rivers: Woods are a bit close but OK IMO, Map border prevents larger walls
• Saharan Oases: Oases are to close
• Snowflake Searocks: Towers only, OK
• The Nile: Woods are to close or better start positions should be placed more towards the map border
• Volcanic Lands: OK, Walls could be slightly enlarged

So as can be seen it's not really possible to enlarge walls.

I think it'd be a good idea to add a rms design convention part at the beginning of rmgen documentation at ​http://trac.wildfiregames.com/wiki/Rmgen_Library.

[FeXoR]

misc.js (changed the default Iberian starting entity placement)

[FeXoR]

wall_builder.js (added the new wall placement)

[FeXoR]

Diff for that patch

[FeXoR]

Sorry for that misc.2.js file, I missed to hit replace...

[FeXoR]

• Added the wall styles for the new civilizations split by the Celts. That also fixes a bug in RMS Fortress though it didn't need changes (needed changes in wall_builder.js, wall_demo.js)
• Cleaned up/unified/documented the code (all, mainly wall_builder.js)
• Added special "wall style" "roads" (needed changes in wall_builder.js)

NOTE: rms_wall_pack2012-6-9.zip and/or rms_wall_pack2012-6-9.diff replaces all other files (they are obsolete now but I don't know how to remove them)

I noticed that the same lines are removed and added again directly afterwards in the diff. Why is that??? EDIT: Ah, those are the change from playerID to playerId.

I set the priority to "Release Blocker" since the recent additions of brit/gaul causes errors on random map Fortress.

[FeXoR]

SVN diff file for all changes

[FeXoR]

Just saw that brits/gauls had less pop cap on RMS Fortress so I adjusted that as well. So fortress.js had to be added to the zip as well...

[FeXoR]

All changed files in a zip

[FeXoR]

All changed files in a zip

[FeXoR]

The changes as an SVN diff

[FeXoR]

Adjusted the center concept of fortresses as discussed with vtsj yesterday: Changed Fortress.center to Fortress.centerToFirstElement (and switched direction appropriate). Renamed getWallCenter to getCenterToFirstElement.

Fixed the issue that if the center was defined manually (which actually never happened before) the fortress is placed correct. Center is now only calculated once for each fortress type (with undefined centerToFirstElement) and set it.

Removed old/obsolete concepts for Iberian civ default starting walls.

rms_wall_pack2012-6-11.zip/diff replaces/contain all previous changes.

Documentation:

• RMGEN scipt wall_builder: Most basic documentation
• ​http://trac.wildfiregames.com/wiki/Rmgen_Library#Placingwalls: General concept description
• RMS wall_demo.js: How to actually use it as an RMS designer

[FeXoR]

Center has to be calculated every time a fortress is placed because the same Fortress instance with different styles (e.g. "athen" and "palisades") need differing centerToFirstElement vectors. It still is only calculated if not set manually.

As always rms_wall_pack2012-6-12.zip/diff replaces all earlier files.

@historic_bruno: I don't know exactly what priority to choose but releasing for example a new alpha with errors when a patch is available that would fix that would seam strange to me...

[FeXoR]

ZIP containing all changed files

[FeXoR]

SVN patch diff

[historic_bruno]

Replying to FeXoR:

@historic_bruno: I don't know exactly what priority to choose but releasing for example a new alpha with errors when a patch is available that would fix that would seam strange to me...

If every error was considered a release blocker, than lots of trivial problems would be release blockers :) Since we're in alpha, the odd error is expected especially in the middle of a release cycle, whereas a release blocker is something serious enough to push back the release deadline if it's not fixed. While every patch is important in some degree. The best thing is to leave priority on its default or ask a developer if unsure.

[FeXoR]

@historic_bruno: Ok, makes sense, thx.

[vts]

FYI; I was reviewing this patch, but I got sidetracked briefly. I still intend to finish the review, sorry about the delay :(

I should note, the most important issues I found up to where I got were discussed on IRC and have since been addressed by FeXoR in rms_wall_pack2012-6-11.diff​.

[vts]

Some WIP comments on the patch, as well as on the RMS's wall builder system in general. I went into quite a bit of detail as I also wanted to understand how the RMS wall builder works myself, so I took advantage of the opportunity to try and understand the code in detail.

Most of these are because I found the documentation confusing or unhelpful to understand the code, so don't be discouraged by the number of these; I aim only to provide constructive criticism.

• The improvements you've made seem to work, but I'm still seeing some towers that are located "below" their neighbouring wall segments, as you've shown on your screenshots above. The in-game wall tool had the same problem, but there it was acceptably fixed simply by ensuring that towers are always at a Y position that is the maximum of their direct neighbouring wall segments. Both the wall segment and tower models were intentionally designed to support this, so you needn't worry about them floating above the ground.

• In WallElement's documentation, it's probably worth to draw the parallel between your concept of 'outside'/'inside' and (2D) face normals to help the reader to better understand what you're explaining.

• In WallElement's documentation, the 'indent' parameter doesn't mention relative to what the wall segment is being pushed inwards or outwards, nor along which axis. From what I understand from reading the code, the indentation moves the wall element along its lateral axis, relative to the anchor position of where the previous wall segment (if any) ends.

• In WallElement, I had a hard time understanding what you were trying to explain in the documentation for the angle property. I don't know what you mean by

  Placement angle so that 'outside' is 'right' (towards positive X like a unit placed with angle 0).

  From reading the code, I get the impression that this is a correction term for the models' rotation when placed ingame so that their correct side corresponds to the 'outside' face normal. I.e., it would be a corrective angle to make a model's desired side face towards the right when it is placed at an angle of 0. Is this correct? If so, I would rename this to something like modelAngle, to emphasize that this is angle that serves only to make the models line up visually, and that is not otherwise involved in any placement calculations.

• On line 292, you have:

  fortressTypes[newKey] = new Fortress(newKey, []); // [] just to make sure it's an array though it's an array by default
  

  Your constructor already takes care of defaulting the second argument to the empty array, so this shouldn't be needed. If you're worried that at some time in the future this might change causing the .push() on line 279 to fail, then good OOP practices dictate that you should disallow public access to the 'wall' property and encapsulate the operation into a method.

• Could you add some documentation to the Fortress class as to what an instance of Fortress represents? This is to prevent confusion between the 'fortress' entities and the concept of a Fortress in the RMS wall system. If I understand correctly, the latter is a definition of a type of closed wall, where a wall is a sequence of wall element types independently of the particular style.

• In getWallAlignment, you do not mention where on the shape of a wall element startX and startY should be located. Presumably the center? You also mention that you're returning only a subset of the information required to place a sequence of wall segments. Could you document exactly which subset that is, and what those values represent?

• In getWallAlignment, the orientation argument is documented as being the placement angle of the first wall element. From reading the code, it looks like this value is in fact the rotation of the 'outside' vector, i.e. the angle of the wall element's face normal (CCW relative to the X axis, as usual). (Other places refer to the face normals as placement angles as well I believe, e.g. placeLinearWall).

I somewhat object to this naming, as I consider "placement angle" to implicitly mean the final angle that you place models at in the simulation, which in the case of wall segments is shifted -90 degrees CCW from the face normal. What I'm saying is: working with the face normal angle is fine, but you shouldn't document it as being the placement angle; or if you do, explicitly state near the beginning of the file what the conventions are for what you call "placement angles" and "wall angles".

• In getWallAlignment, I find that instead of doing something like this:

  if (<expression> === whatever) {
      ...
  }
  var x = <expression>;
  

  it's generally preferable to do it like this instead:

  var x = <expression>;
  if (x === whatever) {
      ...
  }
  

  This avoids duplicating <expression> and is hence more robust to changes to the expression that constitutes x's value.

• getWallAlignment's algorithm only seems to work because you're using separate WallElements to represent rotations along the wall. According to WallElement's definition, any wall element can have an arbitrary bending angle. If you use the RMS wall builder system to place two wall segments right after one another, where the first one has a bend of, say, 57 degrees CCW, getWallAlignment will compute the wrong position for the next wall segment. This is because it modifies orientation after 'placing' the first wall segment, and then goes on to place the next wall element at a distance of (element.width + nextElement.width)/2 along that direction, which does not take into account that you first need to travel element.width/2 along the current orientation, and only then nextElement.width/2 along the new, modified orientation.

• Why does getWallAlignment return an array of arrays instead of an array of objects with descriptive keys? Currently, the X and Y positions returned by getWallAlignment for the i-th wall element need to be accessed by alignment[i][0] and alignment[i][1], versus alignment[i].x and alignment[i].y if you used objects. Similarly, the template name is currently stored in AM[iWall][2] as opposed to something like AM[iWall].templateName. In both cases, the latter is much clearer to the reader. A similar argument goes for getWallCenter.

• In placeCustomFortress, the comment in the function body that says "Place fortress with the first wall element placed at centerX/centerY" looks incorrect. It's the center of mass of the fortress that will be at centerX/Y, not the first element.

• In placeCustomFortress, if fortress.centerToFirstElement is defined, then the variable centerToFirstElement is undefined and the calculations for startX and startY will fail and throw errors.

• Can you add some documentation about what the various parameters to placeCircularWall do, how they control its behaviour, and what their legal/expected values are?

• In placeCircularWall, you need to make it more clear that orientation is in fact the direction of the bisector of the "open" bit of the circle; i.e., that the remaining 2PI - maxAngle worth of "empty" arc length is centralized around the direction given by orientation. This is entirely unclear at first glance; I had originally assumed that it would be the angle at which the first wall segment would be placed.

• In placeCircularWall, near the end of the nested for loop, it looks like you can reuse the computed values of targetX to targetY for the values of x and y. No need to do the computation again. Also, shouldn't it cap maxAngle at 2PI? Do you intentionally support angles that are bigger than a complete revolution? If so, then I'm a bit confused what that if (maxAngle >= 2*PI - 0.001) is all about.

• Some statements appear to have missing semicolons at the end. JS does not require semicolons at the end of statements, but we try to be consistent in using them.

Nitpicking: These are comparatively minor things that I personally would change, but that aren't a huge deal.

• It was initially not very clear to me from the name alone that a WallElement describes a type of wall elements, as opposed to a single wall element entity as it exists in the game simulation. The entity property added to this by giving the impression that it contains an entity ID (as is common in other parts of the codebase), while in actuality it contains a template name. In an ideal world, I'd prefer for WallElement to be named something like WallElementType, and its entity property something like templateName.

• In WallElement, I'd prefer for the property bending to be called something like bendAngleAfter to clarify that it applies only to whatever wall element comes afterwards, as well as to clarify that it is an angle ('bending' brings to mind the mathematical concept of curvature, which is not quite the same). The former is documented, but it's good practice to make your variable names as clear as possible.

• In placeLinearWall, would be nice to explicitly state that the start and ending coordinates are the endpoints of the wall as opposed to the centers of the first and last wall segments. That's a bit clearer than "not the place of the first wall element".

• I'd prefer for the documentation of Fortress's centerToFirstElement property to mention that it's the vector at orientation 0, and also that it's the vector from the center of mass to the center of the first wall element.

• The 'ground' is on the XZ plane, so the comments in WallElement and placeWall need to read 'towards positive Z'.

• In getWallAlignment, the checks for valid wallStyles are mostly redundant. You only need to do the first check for element [0], and from there on just the ones for i+1 inside the loop.

• In the Fortress constructor, can you be more clear about where exactly on the first wall element the vector points to? Presumably this is the center, but it would be nice to mention this explicitly, because it might also reasonably be the center-left of the a wall segment (considering that this vector would be used to find out where to start placing wall elements).

[FeXoR]

I'll comment on this one by one but first a general thing:

Coordinates: We swapped the coordinates appropriate to 2D behavior (X is the same as in simulation, Y is the same as Z in simulation) in the rmgen area. This is documented here: ​http://trac.wildfiregames.com/wiki/Rmgen_Library#MapCoordinateSystem and the code that does that is in the file map.js, function Map.getMapData, line 330. This was mainly done because height is not supported in rmgen (yet, but not meant to be AFAIK, ask quantumstate) and rotation was done in 3D (X/Z plane: growing angle -> clockwise rotation) not as usual in 2D, which me and quantumstate found confusing (Discussion: http://www.wildfiregames.com/forum/index.php?showtopic=15817). I don't really know why anyone should use Y for the hight though the "action" takes place in the 2D plane then called X/Z (leading to clockwise rotation and growing Y means burrow things). It seams to be widely used practice but I don't like it (mainly because I don't like to have 2 positive rotation directions in 2D that differ, clockwise and mathematical right handed positive).

• There is no way meant to change/adjust the Z coordinate (hight, Y in the engine) in RMGEN area at all (yet).

• I'll look into this. Adding axis and what means "normal" orientation of buildings mainly. Though I'm not sure if the art team really mean what I found was true in most cases: Unit drop point of buildings/"noses" of units) faces right (+X) when placed with angle 0 in rmgen (top/+Z in the engine). So this is where the first wall element of a closed wall like a fortress should face (with it's "outside") when the wall is placed with an angle of 0 as well. So "outside" will be right then (+X).

• You got it right in general. Indent 0 means the lateral (with placement angle 0 its +X/-X) center of a short/medium/long wall (for all existing wall styles). "Inside" is left/-X (positive indent) then and "outwards" is right/+X (negative indent). I had it swapped for mathematical correlation (positive indent raises the X coordinate) but found that the "in" in "indent" corresponded more with the "in" in "inside". Not sure if thats perfect.

• Your totally right about WallElement.angle. Just for placement. Going to rename it.

• I think that can be removed indeed. It should always be an array.

• Oh. It's documented but before defining all the default fortresses in the fortressTypes array (line 243 and following). Better to put it to the Fortress class definition, you're right.

• For getWallAlignment startX/Y is the coordinate where the first wall elements "center" would be placed if it where a wall. This has to be corrected with the indent (so an outpost outside the wall as a first element wouldn't even overlap with startX/Y). "Center" is the same as anchor for e.g. short/medium/long walls or any other wall element without indent that has a symmetry relative to the transversal axis (inside is the same as outside) that should be placed in the same lateral position as a short/medium/long wall. The returned values are directly used to place the entities with placeObject (library.js line 334). The only argument needed by placeObject but not relevant for getWallAlignment is the players ID.

• You're right! I always think of "wall elements" as the entities themselves (which is less confusing to me) but of cause the placement angle of an entity and the placement angle of a wall element differ. I'll correct this.

• OK... though this will enforce another var not needed in some cases. I'll do that anyways.

• It works perfectly for 90° angles, just generate the demo RMS "Wall Demo", bottom is the placeWall example (first method in the wall_demo.js line 69 and following called "Custom wall placement" here which may be a better name) with 2 bending wall elements. I'm not sure about other angles. But there may be a problem when the first wall element has bending (but I don't think so). The shift in the other direction is made at the end of the loop. I will test that after completing the response.

• I'm more familiar with arrays but you're right. I'll change this.

• Well, it's the center of mass if the instanceOfFortress.center === undefined. Otherwise it's the center defined there. I'll correct this appropriate.

• Fixed (uploading later)

• placeCircularWall, placePolygonalWall and placeIrregularPolygonalWall documentation is poor, yes, I'll add this.

• OK

• I'm not sure about the reuse of targetX/Y, I will check that. The angle is not supported to be > 2*PI with much sense, yet, though it's not explicitly forbidden. The idea was to allow/support spirals as well which then could have more than 2*PI angle. Concerning endWithFirst: It should (and is) set to false by default if the circle is not closed. To ensure float >= to return true if maxAngle = 2*PI I reduced the 2*PI by a small number (this small gap would have no use at all). If the wall is not closed, the wall should be finalized with the first wall element (endWithFirst = true by default) to have an axial symmetry towards the axis through the center in the angle of orientation between it and the positive X axis. If closed the wall should have rotation symmetry towards its center (so endWithFirst = false by default).

• I'll search for those and add the semicolons.

Nitpicking:

I will look into the other things and mainly agree. Variable names should be clear but it's not always that simple to find good ones so THX. Documentation is meant for others, you're not me, so you can tell better what is clear and what is not, so THX here too.

The perfect world: I'll try to make everything as perfect as possible and are grateful for all comment helping me to do so. There's one BIG thing that is far from perfect though: I define all the properties of the entities again instead of accessing the entity definition. That's mainly because there's no (easy) way to access the data and not all data needed for building composed walls are in the entity definition. If you have anything in mind to do so that would be great! This would still be a hack though. Because the main problem is that rmgen is not part of simulation and simulation is not based on triggers (since triggers are chosen to be delayed to part 2). IF there where triggers implemented everything (including AI and rmgen) could build on them and just additional libraries with easy to use functions for a specific purpose that themselves use the more general trigger functions had to be written (instead of hole APIs more or less reproducing thing again and again as it is now). That's a major drawback of the concept IMO since everything has to be rewritten again if triggers are in to make it clean and usable! So more or less everything in the RMGEN part written now is in vain. I'm thinking about adding triggers myself but wouldn't like to do so without any support by more experienced programmers in the team (because it has a huge impact and should be as feature complete and clean as possible before using it for everything else). I mentioned and tried to explain it before in the forum (http://www.wildfiregames.com/forum/index.php?showtopic=16096) but no programmer answered. The gain of this change would be extraordinary! Everything would be much more easy to change because there would only be one place in code to change to change a feature. Implementing new features would be about as simple as now and every part of the code could automatically benefit from/use it (so in the end less work). I'd love to have your support there but ofc. it should be the teams decision (or at least have some support there). But that's a matter on it's own so one step after the other...

Expect the new files Wednesday 20.6.2012

Work in progress...

[FeXoR]

ZIP with all changed files

[FeXoR]

SVN patch as .diff

[FeXoR]

That should more or less include everything you asked. I'm not that sure about variable names. Perhaps some ideas? The WallElement.entity should be named entityType/templateType perhaps but that is confusing as well considering there are 2 types then (entity and wall element type). I don't know where's the difference between entity and template. For roads I also use actors (which work as well) so placeObject can place more then one specific object type (entities as well as actors). But objectType would be even less descriptive. Perhaps "representation"?

I don't know though why the patch is not already added. It fixes 3 bugs and is better then the versions before! That doesn't mean whe have to stop the discussion ofc.

I will listen to advises (and always have) and try to make changes appropriate to them, but I didn't add this patch to discuss everything (though I'm grateful for that happening) but to fix bugs (that would be fixed after my second upload already). IMO the important thing about patches is that they fix the issue while discussions and advises can be made in the forum (in this case http://www.wildfiregames.com/forum/index.php?showtopic=15750) and I will upload new versions there until a bug is found in the SVN version (which most of the time happens if new content was added that doesn't take into account RMGEN like the celt split civs here).

So is the idea just to, while you're at it, go through the whole stuff? Then we can do it in the forum and if you see fit you add a patch. But if errors occur and a patch is available that fixes that, it should be added ASAP IMO despite other things that should be changed in that area of code.

I don't mean to force anyone how to do things, but that is how I see fit. And I'm quite incapable of silently accepting things done "strange" for me ;)

[vts]

Replying to FeXoR:

The WallElement.entity should be named entityType/templateType perhaps but that is confusing as well considering there are 2 types then (entity and wall element type). I don't know where's the difference between entity and template.

Templates define what components an entity is made up of, and how those components are defined. Entities are instances of those templates. It's somewhat comparable to instances and classes in object-oriented programming.

I will listen to advises (and always have) and try to make changes appropriate to them, but I didn't add this patch to discuss everything (though I'm grateful for that happening) but to fix bugs (that would be fixed after my second upload already). IMO the important thing about patches is that they fix the issue

Agreed; I'll work more towards getting this patch added in rather than arguing about details.

[FeXoR]

THX much!

ATM I'm working on a pdf to make the concept clear because I find it hard to explain. Pictures may say more than a thousand words. I hope to find bad design issues and better names for the variables in the process.

Here's an early version: ​http://fexor.dyndns.org/files/wall_builder_concept/wall_builder%20concept.pdf

[O.Davoodi]

Is the patch ready? It works well and seems sane. I think we should commit it.

BTW, FeXoR: did you change the name of createStartingPlayerEntities to placeCivDefaultEntities? The new name is good but it can be confused with "placers". Also the reason I put that "entities" in createStartingPlayerEntities was that we may want to add some new entities to the player starting entities. But I don't think we'd need to change the functions. We'll use the previous function where it was necessary.

[FeXoR]

Replying to Spahbod:

Is the patch ready? It works well and seems sane. I think we should commit it.

I totally agree! It may not be perfect but I'm constantly working on it anyways.

BTW, FeXoR: did you change the name of createStartingPlayerEntities to placeCivDefaultEntities? The new name is good but it can be confused with "placers". Also the reason I put that "entities" in createStartingPlayerEntities was that we may want to add some new entities to the player starting entities. But I don't think we'd need to change the functions. We'll use the previous function where it was necessary.

No. I wanted to but didn't want to change the function without confirmation. I started a thread for that but none reacted, perhaps I didn't make my point clear enough: http://www.wildfiregames.com/forum/index.php?showtopic=16019. So I added another similar function with support of keyword arguments (don't know if that's a usual practice in javascript) with a reasonable name to support the Iberian civ bonus wall. The old function is still there as it was e.g. still used by RMS Fortress.

I don't really mind the naming. IMO it's not very confusing since placers are called xyzPlacer and are placed with xyzPlacer.place AFAIK so it doesn't begin with "place" anyway.

The new function doesn't take the starting entities but gets them by the players ID in the function. I guess that's what you mean?

I think best would be to use the new function, rename it to the old functions name (removing the old one) and add a keyword argument for startingEntities. That way the common case would be easy to use but changed starting entities and the Iberian civ bonus walls are supported as well.

[O.Davoodi]

Replying to FeXoR:

Replying to Spahbod:

BTW, FeXoR: did you change the name of createStartingPlayerEntities to placeCivDefaultEntities? The new name is good but it can be confused with "placers". Also the reason I put that "entities" in createStartingPlayerEntities was that we may want to add some new entities to the player starting entities. But I don't think we'd need to change the functions. We'll use the previous function where it was necessary.

No. I wanted to but didn't want to change the function without confirmation. I started a thread for that but none reacted, perhaps I didn't make my point clear enough: http://www.wildfiregames.com/forum/index.php?showtopic=16019. So I added another similar function with support of keyword arguments (don't know if that's a usual practice in javascript) with a reasonable name to support the Iberian civ bonus wall. The old function is still there as it was e.g. still used by RMS Fortress.

I don't really mind the naming. IMO it's not very confusing since placers are called xyzPlacer and are placed with xyzPlacer.place AFAIK so it doesn't begin with "place" anyway.

The new function doesn't take the starting entities but gets them by the players ID in the function. I guess that's what you mean?

I think best would be to use the new function, rename it to the old functions name (removing the old one) and add a keyword argument for startingEntities. That way the common case would be easy to use but changed starting entities and the Iberian civ bonus walls are supported as well.

No. No need to change the name. The new function exactly creates faction based player entities but the old one didn't (e.g. you could make a completely unusual starting entities list) and it corresponds it's name "createStartingPlayerEntities" instead of "createStartingCivEntities".

Although I think that we can use createStartingPlayerEntities inside placeCivDefaultEntities to decrease the amount of duplicated code.

[FeXoR]

Replying to Spahbod:

[...]

No. No need to change the name. The new function exactly creates faction based player entities but the old one didn't (e.g. you could make a completely unusual starting entities list) and it corresponds it's name "createStartingPlayerEntities" instead of "createStartingCivEntities".

Although I think that we can use createStartingPlayerEntities inside placeCivDefaultEntities to decrease the amount of duplicated code.

OK. I'll do this.

[vts]

Can we keep that in a separate patch though to be applied after this one is done? To prepare for the commit I've been adding some comments of my own that I think will further help clarify some things (if that's ok with you?), and it's gonna get hard to keep track of all these different patches.

FeXoR, some more things I noticed in your new patch (June 20th):

• The Iberians bonus fortresses don't seem to have walls in between their towers on the Cycladic Archipelago map, regardless of map size or how many players are in the game. They're there on most other maps though. This is not new; it's also the case currently, but is this intended behaviour? (I'm not very familiar with the bonus default entity rules).

• In getWallAlignment, you have the following:

  if (element === undefined && i == 0)
  

  I wonder why the extra i == 0 is in there. It wasn't there before, and if i happens to be greater than zero, then the code following it will fail.

[FeXoR]

Replying to vts:

Can we keep that in a separate patch though to be applied after this one is done?

Yes

To prepare for the commit I've been adding some comments of my own that I think will further help clarify some things (if that's ok with you?)

Yes. It's good to have different points of view for documentation IMO.

, and it's gonna get hard to keep track of all these different patches.

Agreed

FeXoR, some more things I noticed in your new patch (June 20th):

• The Iberians bonus fortresses don't seem to have walls in between their towers on the Cycladic Archipelago map, regardless of map size or how many players are in the game. They're there on most other maps though. This is not new; it's also the case currently, but is this intended behaviour?

Yes. Starting entities are placed with:

placeCivDefaultEntities(fx, fz, id, BUILDING_ANGlE, {'iberWall' : 'towers'});

here like in other maps without enough space for a fortress. It's documented in misc.js function placeCivDefaultEntities about line 163 "keyword arguments". Supported choices for "iberWall" are false, "towers" and "walls". If false nothing is placed in addition to the normal starting entities for Iberians. If "walls" it's the default fortress (and default if no keyword argument given). If "towers" there will just be some towers added without walls in between and in a smaller radius.

(I'm not very familiar with the bonus default entity rules).

• In getWallAlignment, you have the following:

  if (element === undefined && i == 0)
  

  I wonder why the extra i == 0 is in there. It wasn't there before, and if i happens to be greater than zero, then the code following it will fail.

Yes. That is to fix what you said before:

Replying to vts:

In getWallAlignment, the checks for valid wallStyles are mostly redundant. You only need to do the first check for element [0], and from there on just the ones for i+1 inside the loop.

So now the first wallElement is checked with:

if (element === undefined && i == 0)
    warn("No valid wall element: " + wall[i]);

while all others are checked some lines below by:

if (nextElement === undefined)
    warn("No valid wall element: " + wall[i+1]);

But you are right! The first check would be better before the for loop and checked with:

var firstWallElement = wallStyles[style][wall[0]];
if (firstWallElement === undefined)
    warn("No valid wall element: " + wall[0]);

The var here is not that helpful IMO though (like in some other cases) because firstWallElement is only exactly used in the if. But I don't really mind.

I will not add another diff/zip to not conflict with your changes.

[vts]

In 12034:

In RMS: Fix misaligned iberian bonus starting walls. Fix celt split civs errors. Add roads. Closes #1449.

[vts]

Some comments on the RMS wall builder

[vts]

Attached some more detailed documentation I intend to add. FeXoR, please take a look and see if there's anything you disagree with; if not, I'll add it :)

[FeXoR]

You changed "if (element === undefined && i == 0)" to "if (element === undefined/* && i == 0*/)" though I thought you agreed that the first was correct?! I haven't read everything but it looks fine. I'd say commit it (after checking the "&& i == 0").

[vts]

Indeed I did, ignore that, that was a test comment. Not sure how it ended up in that patch, my bad :)

[fabio]

See also #1565.

[O.Davoodi]

Unfortunately there seems to be no way to fix this except by using constraints. A tileclass named clWall should be defined in wall_builder.js and used to prevent cases such as #1565.

[FeXoR]

Replying to Spahbod:

Unfortunately there seems to be no way to fix this except by using constraints. A tileclass named clWall should be defined in wall_builder.js and used to prevent cases such as #1565.

I'm sorry but I don't understand what you mean. As far as I can see this ticket is fixed. Can you please describe what exactly you want to fix? A screen-shot would help...

[fabio]

Replying to FeXoR:

Replying to Spahbod:

Unfortunately there seems to be no way to fix this except by using constraints. A tileclass named clWall should be defined in wall_builder.js and used to prevent cases such as #1565.

I'm sorry but I don't understand what you mean. As far as I can see this ticket is fixed. Can you please describe what exactly you want to fix? A screen-shot would help...

The proper description was reported as #1565.

[historic_bruno]

I think new tickets would be better than reusing this one as it's quite long and difficult to follow now :)

[FeXoR]

Replying to fabio:

Replying to FeXoR:

Replying to Spahbod:

Unfortunately there seems to be no way to fix this except by using constraints. A tileclass named clWall should be defined in wall_builder.js and used to prevent cases such as #1565.

I'm sorry but I don't understand what you mean. As far as I can see this ticket is fixed. Can you please describe what exactly you want to fix? A screen-shot would help...

The proper description was reported as #1565.

Ah, I see, THX!

Replying to historic_bruno:

I think new tickets would be better than reusing this one as it's quite long and difficult to follow now :)

I agree.

IMO this is not really a wall_builder.js issue btw. It's the result of 2 general rms design issues:

• In rms entities can be placed anywhere, even on impassible terrain. In addition there is no way to check if an entity is placed that way AFAIK. This is (I'm sorry to say this again) a result of splitting rmgen from simulations and there's no way (yet) to get the entity properties like obstruction/footprint/... to check this sane.
• There is no way (yet) to remove entities in rmgen. If a new entity is placed there should be a method to remove all obstructing other entities so for example walls remove trees inside them when placed.

EDIT: To get more constructive I opened a new ticket #1589. I don't have any idea how to do this well so I'm not doing it ATM. If anyone has ideas please let us know. I'd implement it then and try to better wall placement afterwards.