Opened 6 years ago

Closed 3 years ago

Last modified 3 months ago

#2944 closed enhancement (fixed)

[PATCH] Make RMGen wall placement more generic so no/less tweaking is needed if new civs are added

Reported by: FeXoR Owned by: FeXoR
Priority: Nice to Have Milestone: Alpha 23
Component: Maps Keywords: patch
Cc: Patch: D900

Description (last modified by FeXoR)

This patch includes:

  • Adding function getCivList() to RMGen lib library.js that works even before civ data are fully loaded (needed for RMGen lib wall_builder.js, used by RMS wall_demo.js)
  • Avoiding explicitly naming the civ strings in RMGen lib wall_builder.js and RMS wall_demo.js so no patching is needed for them if a new civ is added. If a mod sticks to the entity template naming syntax ("structures/"+[civ string]+"_"+[building type]) and has all needed wall elements (wall_tower, wall_short, wall_medium, wall_long, wall_gate) the wall builder now will work for all "default" placement methods (there are advanced functions/arguments that still will cause failure in some cases).
  • Tweaking RMS wall_demo.js to use the full map width and due to the added civ recommending map size "Giant" for it.
  • Spellcheck and explanations: Fixed some explaining in RMGen lib misc.js and some typos in the changed files

Some questions that arose while writing the patch:

  • Wouldn't it be better to load the civ data during RMGen import RMS.LoadLibrary("rmgen"); instead of the map objects initialization InitMap(); so RMGen libs can use them? (This would remove the need of getCivList())
  • Civ theb is not a full faction AFAIK. This is still explicitly named and avoided (which is bad on it's own). Will this cause problems? Is there a way to avoid the explicit naming?
  • Some wall styles (like rome_siege and palisades) are not a civ. I have no idea how to avoid explicit naming in such cases (and patching will be required if further cases like this are added). Is this OK?

Attachments (9)

generic_wall_builder_rmgen.patch (8.4 KB ) - added by FeXoR 6 years ago.
Patch for r16007
generic_wall_builder_rmgen2015-4-12.patch (21.6 KB ) - added by FeXoR 5 years ago.
Patch for r16533
gen_wall_builder2015-8-3.patch (30.3 KB ) - added by FeXoR 5 years ago.
gen_all_builder2015-8-13.zip (75.5 KB ) - added by FeXoR 5 years ago.
All changed files
gen_wall_builder2015-8-16.patch (56.2 KB ) - added by FeXoR 5 years ago.
wallgen_additional_20150816.patch (81.4 KB ) - added by s0600204 5 years ago.
Follows on from gen_wall_builder2015-8-16.patch
gen_wall_builder2015-8-16b.patch (133.6 KB ) - added by FeXoR 5 years ago.
Full patch for r16920, also changes increments and some warnings
wallgen_full_2015-08-16c.diff (170.3 KB ) - added by s0600204 5 years ago.
Full patch this time. FeXoR's changes and mine. It's big.
gen_wall_builder2015-08-23.diff (225.4 KB ) - added by FeXoR 5 years ago.
Some functions added and changed coment style

Download all attachments as: .zip

Change History (40)

by FeXoR, 6 years ago

Patch for r16007

comment:1 by FeXoR, 6 years ago

Addition to the first question: ...because the list could be directly stored.

comment:2 by sanderd17, 6 years ago

It's nice that the civ list isn't hardcoded anymore, but the wallScaleByType values are still hard-coded.

It's 1.8 for carthage, nothing for thebe and 1.5 for the rest.

So if a mod wants some different value, it needs to copy the entire wallbuilder to its mod, which shouldn't be the case.

Instead, the values probably should be stored somewhere else (like in the civ.json), so it can easily be overwritten by mods.

Maybe it's good to have it default to 1.5, and find a way to add a wallset for the Romans, and disable it for the Thebans.

comment:3 by FeXoR, 6 years ago

The hard-coded wall elements dimensions will be removed, too. I'll use the (now available) template data for this.

However, this might take some time.

Concerning Thebans, Roman siege walls and palisades: I don't have any idea how to do this. I'll check if I can use the "wall-set" entities to determine whether one faction has a wall set or maybe more than one.

Last edited 6 years ago by FeXoR (previous) (diff)

comment:4 by sanderd17, 6 years ago

You can require civs to have a list of wallsets in their civ.json (and take a default value for it when not present, so you don't have to modify all jsons).

I'd like to wait with comitting the patch, until the issue of hard-coded wall dimensions is removed too. Since that might bring up additional requirements for the code.

comment:5 by stanislas69, 6 years ago

Keywords: random review patch added; mandom removed

comment:6 by historic_bruno, 6 years ago

Keywords: RMGen RMS random maps walls wall wall_builder civilizations civs civ removed
Milestone: BacklogAlpha 18
Priority: Must HaveNice to Have

comment:7 by FeXoR, 6 years ago

Is inheritance possible for .JSON? Do the civ .JASON use this? I want to add "wallSets" to the civ .json but only want to add "palisade" once.

Additionally IMO the wallSet entities should also be fused with this. Any information if this would be possible/wanted highly appreciated! With "wallSet entities" I mean e.g. athen_wallset_stone that are allready in - and AFAIK meant for in-game wall placemet. Fusion of rmgen and in-game wall placement would be nice for many reasons: less code duplication, compatibility, consistent behavior of both methods - e.g. same target values for overlapping, ...

comment:8 by leper, 6 years ago

Milestone: Alpha 18Alpha 19

comment:9 by FeXoR, 5 years ago

Adding simple patch for r16533 generic_wall_builder_rmgen2015-4-12.patch

1) Wall styles added to civ .JSON files including

  • name as specified in the wall_set entities (duplicated naming with potential inconsistency that might lead to failure, bad for moding)
  • wall_set entity
  • Scale (later to be replaced with template footprint width so might get obsolete)

2) Wall scale in wall_builder now taken from civ .JSON (civ specific wall set type "Stone" still hardcoded) 3) Alongside fixed some terrain height range increase issues

Bad things here:

  • Duplicated naming of wall sets in civ .JSON and wall_set entity .XML
  • Naming inconsistent with wall sets in RMGEN lib wall_builder.js
  • When scale is removed the syntax might be better changed (wall set definition no longer needs the inner object. Even one list of entity files would suffice then - and also would avoid the double name occurance)

So the options are now IMO:

  • Add this (or similar) patch
  • Wait for a much bigger patch including entity footprint usage (and take the naming from the wall_set templates only)
  • Restructure to a clean yet generic civ .JSON, wall_set entity .XML and RMGEN wallStyle concept (This would potentially also fix several bugs with the in-game wall placement not being able to close pre-build walls in any of the 3 map types as well as getting rid of the triple naming)
Last edited 5 years ago by FeXoR (previous) (diff)

by FeXoR, 5 years ago

Patch for r16533

comment:10 by FeXoR, 5 years ago

Getting a templates width by angle works now (only 0, pi/2, pi, 3*pi/2 for now).

However, there is no way (but guessing again e.g. by the template name) if a "wall element" should be indented outwards, inwards or not at all (If you're not sure what I'm talking about pls. see the bottom part of the wall_demo random map).

I'd really appreciate if someone would (roughly) look into wall_builder functionality and the wall_demo map (to see how easy and generic to use wall_builder is) and then help me figure out what to do with it and/or the ingame wall placement (and likely some parts of the xml/json structure).

Any suggestions very welcome ;)

comment:11 by FeXoR, 5 years ago

Reminder to do list:

  • Get wall_set entities from RMS.GetCivData()
  • Add a wall style for each corresponding to the keys in the template
  • Add "Defensive" tags in the wall_sets entity templates to specify that they are neither a real wall part nor civic and to be placed in front of the wall (like defense_tower). All non-specified buildings will be considered civic and be placed at the inside of the wall.
  • Break the wall_builder into two parts: wall_builder exclusively for default wall placement and another for fortresses/cities

After collision detection: http://wildfiregames.com/forum/index.php?showtopic=16242&page=10#entry297569 and heightmap manipulation lib (for incline map, also fixes Schwarzwald) are added adjust wall placement to avoid collision and rough terrain.

Last edited 5 years ago by FeXoR (previous) (diff)

comment:12 by FeXoR, 5 years ago

Now there's a problem with using the Footprint property of entities for the determination of the actor's size (see http://wildfiregames.com/forum/index.php?showtopic=16242&page=11#entry308725)

by FeXoR, 5 years ago

by FeXoR, 5 years ago

All changed files

comment:13 by FeXoR, 5 years ago

ToDo/WIP:

  • Adjusting the entity templates WallPiece.Length values to the space the given entities can actually fill without leaving gaps. As a result the "scale" property of wall sets will no longer be needed (this may lead to certain (short) wall lengths that are unplacable if the towers are to small - read as not fitting the original idea of "wall scale"). It's likely that the wall sets properties min/max overlap can then be removed as well when the WallPiece.Length property is set correctly. This will also enable closing walls placed in RMS or Atlas (if placed without visual gaps/to much overlapping) with the ingame placement function.
  • Add a function that returns the wall set entities to remove the changes from the civ .xml files.
  • Remove all special cases from wall_builder.js so mods no longer need to have it.
  • Adjust the wall placement methods so they are more tollerant for poorly chosen WallPiece.Length values.

by FeXoR, 5 years ago

by s0600204, 5 years ago

Follows on from gen_wall_builder2015-8-16.patch

comment:14 by s0600204, 5 years ago

FeXoR on #0ad-dev:

Feel free to change things as you see fit.

Heh, I may have gone a little overboard. Although to be honest, this is about 2 days work... I got a little carried away.

Use as much or as little as you need. Or simply use as a reference of what's possible. The patch sits on top of your last posted one.

There's a branch with further WallPiece.Length modifications over on GitHub, and here's an in-game screenshot (just to prove it works - note that palisade is on the other side of the map, rome_seige is straight after rome_stone and the seleucids are included).

Last edited 5 years ago by s0600204 (previous) (diff)

comment:15 by FeXoR, 5 years ago

@s0600204: Nicely done!

Sadly tortoise SVN did reject to patch wall_builder.js (Trying to apply it by hand) It would be nice if you could add a complete patch (so not on top of mine).

Some comments:

  • Good: Replace endLeft / Right with Start / End, remove WallElement class
  • Very good: Replace optional argiments directly if possible (I didn't know that was possible in JS). We might want to change some functions argument order to push the optional arguments to the end.
  • Some documentation of the concept should be present (removed with WallElement class) though maybe rephrased by a native Anglican ;)
  • What is "ret" standing for? Guess return value? Quite unspecific but OK ;)
  • Is "element.slice(0.3)" and "element.slice(0,4)", so "." and ",", intendet? (Don't know what they do ATM)
  • If something is unlikely to be fatal we should just log, not warn IMO
  • Not sure if the "g_" indicator is appropriate here. It's for sure better than just massing variable names without it. For my taste we should pack all wall placement functionality in an object e.g. g_walls or something (like I did for collision http://wildfiregames.com/forum/index.php?app=core&module=attach&section=attach&attach_id=8780).
  • getWallElement() has to be tested well to not brake things
  • Did you test e.g. the quarterCurve? I never tested them extensively and I'm not entirely sure if the maths is correct in all placement methods supporting bending pieces.
  • How do you choose the indentation of non wall set entities? (Without seeing the patched file that's hard to figure out)

Also I agree renaming TWO_PI to TAU (Though maybe in another patch. This one is allready quite big)

EDIT After manually patching:

  • placeWall() should log if called without startX / startY and both being 0 should not be rejected IMO
  • Thanks for showing off by showing me the "of" operator! x)
  • I'm sceptical about the need / usefullness of the "@overlap" property. If the WallPiece.length values are chosen properly the min value would always be 0. The maximum value can differ though (when the wall "pierces" e.g. the tower and shows up on the other side). There should either be the WallPiece.length property OR the "@overlap" property (and than the obstruction value used for length / width) not both of them IMO.
  • Shouldn't the gates be the other way arround?
  • Tower's drop points have to be checked (The Helenic ones look good but the droppoint didn't always match the visual entrance IIRC).
  • There is the "other/palisades_rocks_fort" entity. We may want to use that as the fortress for palisade type walls (not sure if it is supposed to stay in SVN).
  • The WallPiece.length (or the @overlap property) has to be adjusted. See the gaps in http://wildfiregames.com/forum/index.php?showtopic=16242&page=11#entry308755

Otherwise it works fine. Good job!

Though a bit ugly we could remove the wall sets from the civ .json if we add a function to get them from the template data (That's where the changes in library.js where for). Not sure if that's worth the hastle.

How to continue: I would continue from your patch if applying cleanly to SVN so please add a complete and working patch. If you have more changes in the git branch and think that should also be added please let me know (reading through it now).

It also might be good if you yourself read through the file after a nap.

Some nitpicking:

  • For my taste the indentation level shouldn't change within a block even if it's just a double newline (maybe just pythonic obsession on my part)
  • If things are changed according to a ToDo the corresponding ToDo lines should be removed (e.g. renaming endLeft, line 32)
Last edited 5 years ago by FeXoR (previous) (diff)

by FeXoR, 5 years ago

Full patch for r16920, also changes increments and some warnings

comment:16 by s0600204, 5 years ago

...though maybe rephrased by a native Anglican?

An "Anglican" is someone who belongs to the CoE (Church of England) faith, an offshoot of Protestant Christianity. "native English speaker" would have been better. ;)

Is "element.slice(0.3)" and "element.slice(0,4)", so "." and ",", intendet? (Don't know what they do ATM)

Whoops. Fixed (should have both been ", "). They return the first three and four characters from the value of element respectively.

Not sure if the "g_" indicator is appropriate here.

It makes it easier when glancing over function code to tell which is a globally scoped var and which is local.

If something is unlikely to be fatal we should just log, not warn IMO

We have error() for serious errors. Also log() doesn't appear in interestinglog.html, which is an easier file to check post-incident.

For my taste we should pack all wall placement functionality in an object

That should be achievable, did you want to try to do this within this patch, or wait? It'll break any maps that currently use the wallgen.

How do you choose the indentation of non wall set entities?

Chosen here. Adapted from Lines 134-146 of the original file. It's a little arbitrary.

Shouldn't the gates be the other way arround?

Uh, no...? They're the same way round as before my changes, so...

I'm sceptical about the need / usefullness of the "@overlap" property.

I get where you're coming from. I think a design/implementation decision is required as to how wallsets should work: Option A: Keep as is. Wallpieces have a length and wallsets have both a min and a max overlap. Option B: Get rid of the min overlap, and just have a max overlap. Reduce all wallpiece lengths by the old min overlap. This would mean modifying the wall-placement code used by players in-game (for the sake of consistency), which is outside the purview of this ticket.

Note: Prepping a patch. "Please hold" ;)

Last edited 5 years ago by s0600204 (previous) (diff)

by s0600204, 5 years ago

Full patch this time. FeXoR's changes and mine. It's big.

comment:17 by leper, 5 years ago

Since this patch already does quite a lot of cleanup, could we also change comment style to something more doxygen-like? (Not that we generate docs for js at all, but having a consistent style would help.)

Also there are some for (...; ... <= ...; ...) loops in there. <= in for loops is something that is very likely to be wrong (it might not be an issue in js, but in other languages that might lead to an out-of-bounds read/write) so can this be changed?

Reordering the parameters for some functions to actually make the default parameters work would be nice too. Apart from that (and ignoring the behaviour changes) the patch looks good.

by FeXoR, 5 years ago

Some functions added and changed coment style

comment:18 by FeXoR, 5 years ago

Keywords: review removed

comment:19 by stanislas69, 5 years ago

Milestone: Alpha 19Alpha 20

Since feature freeze is ahead and you don't seem to be finished + huge patch, pushing it to A20

comment:20 by FeXoR, 5 years ago

After another long thought period I think it will be best to continue like this:

  • Adjust the templates WallPiece.Length property to actually fit the space that entity covers within a wall (when placed in a straight wall)
  • Adjust the wall sets Min/MaxTowerOverlap to ensure it works for all angles (Not sure if that's what it was meant for in the first place, a comment on that would be great)

Sorry for the time I need to fix this but I want to make it right preserving most of the functionality present while not introducing further problems.

comment:21 by elexis, 5 years ago

Milestone: Alpha 20Backlog

Backlogging due to lack of progress.

comment:22 by Palaxin, 4 years ago

Summary: [Patch] Make RMGen wall placement more generic so no/less tweaking is needed if new civs are added[PATCH] Make RMGen wall placement more generic so no/less tweaking is needed if new civs are added

comment:23 by FeXoR, 4 years ago

Component: UI & SimulationMaps

comment:24 by FeXoR, 4 years ago

Might be related to #2631

comment:25 by FeXoR, 4 years ago

Owner: set to FeXoR
Status: newassigned

comment:26 by FeXoR, 3 years ago

Description: modified (diff)

Reopened at https://code.wildfiregames.com/D900 by s0600204, thanks ;)

comment:27 by s0600204, 3 years ago

In 20625:

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

comment:28 by s0600204, 3 years ago

In 20626:

Update random maps that use rmgen wall-placement.

Reviewed By: FeXoR
Commented On By: elexis
Refs: #2944, D900

comment:29 by s0600204, 3 years ago

Milestone: BacklogAlpha 23
Patch: D900
Resolution: fixed
Status: assignedclosed

Closed with r20625.

Other pertinent commits: r20589

comment:30 by s0600204, 3 months ago

In 23684:

Add help comments to WallPiece component schema

Also resolves whitespace issue in WallSet component breaking structree and rmgen
wallbuilder when wall curve pieces are listed separated with newlines (& tabs)
instead of single spaces.

Requested by: Nescio
Refs: #2944, D900

comment:31 by s0600204, 3 months ago

In 23687:

Correct WallPiece schema help comments

(Fairly sure I've got them right this time.)

Refs: #2944, D900, rP23684

Note: See TracTickets for help on using tickets.