• rebased and updated, smaller patch, as requested by @wraitii

• <MaxGatherers>

• simplify fauna_shark.xml by changing parent to the very similar template_unit_fauna_hunt_whale.xml

• also include <Attack> node

This patch is outdated, incomplete, and superfluous because of the more recent D2721/rP23930.

I don't understand why this is an absolute number instead of a fraction of the max pop cap, e.g. 25% (for 200 pop baseline) or 16.6% (for 300 pop baseline)

The "add": 50 could be replaced with e.g. "multiply": 1.2 (i.e. 20%), if you think that's better. However, the effect would be exponential, i.e. with a population cap of 250 the first wonder would add 50, the second 60, the third 72, andsoforth, so I guess that's why a flat addition rather than a multification was chosen in the past. I have no strong opinion on this specifically.

I don't understand why this is an absolute number instead of a fraction of the max pop cap, e.g. 25% (for 200 pop baseline) or 16.6% (for 300 pop baseline)

• fix oversights spotted by @Freagarach

I have nothing against it either, if "stackable" is taken out, hardly anyone converts it into a wonder.

What do you mean exactly? The wonder auras are actually stackable, this patch does not change that. As are the library, theatre, and kush temple of amun auras. Or are you of the opinion structure auras shouldn't be stackable?

I like the idea of the patch, but I'm not sure about Iber / Brit / Gaul.
I think fortress and tower should be removed for iber.
PopulationBonus op="add">2, seems useless, a higher value like 5-10 is preferable.

That (i.e. iber +5, gaul +2) is basically what they have right now.
Personally I wouldn't mind removing all those <PopulationBonus> from the brit, gaul, iber structures; I didn't do it here, to limit overlap with D2841.

Interesting that I didn't encounter that when playtesting ^^' (A closer inspection suggests techs need no change.)
Any suggestion how the auras can be handled in the script? To me it seems they have to be fixed manually. (One could, of course, put the aura files also in a civ-dir ^^ )

There are not that many, just six in the public mod, so I suppose you could restore them manually. Luckily hero aura lines are unaffected, I guess because they have an additional folder step, e.g. units/heroes/athen_hero_pericles_1. However, this is something to be aware of when fixing other mods.

Palisades could go into a structures/common folder, at least until palisade models can be created on a per civ basis (if ever; largely a palisade is a palisade, not much culturally happening there, so I imagine the Palisades objects being common amonfg multiple civs for quite a while longer).

It's possible to introduce structures/gaia/ and units/gaia/ subfolders for all entities belonging to <Civ>gaia</Civ> (e.g. common, merc, other, shared, special are less suitable). For palisades, it's not impossible someone at some point decides to introduce {civ} palisade files, which are identical to and inherit from the current palisades, except that they have a different <Civ> and <SpecificName>.

Palisades could go into a structures/common folder, at least until palisade models can be created on a per civ basis (if ever; largely a palisade is a palisade, not much culturally happening there, so I imagine the Palisades objects being common amonfg multiple civs for quite a while longer).

Pinging @FeXoR over wall_builder.js.

I like the idea of the patch, but I'm not sure about Iber / Brit / Gaul.

I can agree with that. It's a bit confusing as it is now and really + 10 pop makes almost no difference. I have nothing against it either, if "stackable" is taken out, hardly anyone converts it into a wonder.

Sure, no problem.

(Also, there are just three merc* units and only one merc* structure.)

Run P225 and apply this patch on top.

To make it work I had to insert a line at the beginning:

cd binaries/data/mods/public/

and another at the end:

cd ../../../../

I suppose the test plan meant to say to run it inside the public/ folder?

The script works, is, as far as I currently know, correct and complete.

Neither technology nor aura lines in templates should be altered:

ERROR: File 'simulation/data/auras/structures/cart/super_dock_repair.json' does not exist

Build failure - The Moirai have given mortals hearts that can endure.

Build failure - The Moirai have given mortals hearts that can endure.

Build failure - The Moirai have given mortals hearts that can endure.

Fix mapgen.

The script works, is, as far as I currently know, correct and complete.

Template folders after the script and this patch:

athen
brit
cart
gaul
iber
kush
mace
maur
merc
noldor_ship_bireme.xml
pers
plane.xml
ptol
rome
samnite_skirmisher.xml
samnite_spearman.xml
samnite_swordsman.xml
sele
spart
thebes_sacred_band_hoplitai.xml
theb_siege_fireraiser.xml
thespian_melanochitones.xml
viking_longship.xml

In D2646#128677, @Nescio wrote:

Why the rename?

For I can image there are mods that like to be able to bring elephants into musth periodically.

To me “natural behaviour” implies something that can't really be changed, whereas “default stance” implies something that's optional and can be set differently. Why the rename?

I'd say the position in the template does matter actually, since the code finds the first free spot and occupies that.

If that's the case then I failed to notice it in game; to me it seemed units merely occupied the nearest free spot; I guess I was mistaken then.
Anyway, the nodes are listed from centre outwards (also in D2783), so it shouldn't really matter in practice.

It seems like one initial idea for the celts would have been that they could upgrade their houses to other structures, which would explain their pop-bonus.

The initial idea behind this patch is to differentiate the Britons from the Gauls by giving each only one of their civ bonuses, rather than both both. That it would probably improve gameplay balance is a nice extra.
If the consensus is it would be better to simply delete the extra structure population bonus for both civs, then that could be done instead.
Suggestions for an entirely new civ bonus for either Britons or Gauls are certainly welcome too.

I'd say the position in the template does matter actually, since the code finds the first free spot and occupies that.

I mean they are binaries and I would guess renaming them would perhaps duplicate them in the version history.

That has never been an objection in the past.

I'm not sure how SVN (or any SCM for that matters) handles binaries and renaming thereof ^^

I don't know either, but I guess if images are replaced, both versions are preserved in the version history, but if they're simply moved, then the files themselves aren't changed, only their location is, so there is no real need to duplicate them?

"I don't like them."

Could you elaborate?

Do the symmetric ones fill up one tower first then the other? Or are entities added evenly? If the former, perhaps do the latter. Also perhaps you can use more descriptive names for the turret points of the symmetric ones (e.g. Tower1Pos1 or something).

No, there is no fixed loading order. It seems units occupy the nearest open spot. Neither the node name nor the position they're listed in the templates matters. That's also why I preferred the node names One, Two, etc., which are generic and applicable to basically anything, rather than First, Second, etc., which imply a fixed order.

(Well, this is an overhaul after all ;) That being said, I don't agree with the changes here, with the only argument: "I don't like them." so I won't mix myself further into this patch.)

I mean they are binaries and I would guess renaming them would perhaps duplicate them in the version history. I'm not sure how SVN (or any SCM for that matters) handles binaries and renaming thereof ^^

(One *could* introduce a "Fire" damage type for the fireraiser and fireship.)

Perhaps something for a future patch.

Do we want to rename actors as well?

It would be more consistent, so I'd be in favour; it's up to the art team to decide, though.

(I guess Icons not due to them being art?)

What do you mean by that?

Personally I would say: "pack 'm full" ;)

The gates of different civilizations have different designs, which means some have space for only 4 or 5 spots, others for 8 or 10, unlike long walls, which all are fundamentally equivalent, and can easily support 8 (see D2783). So the question is whether it's desirable for gates of different civilizations to have different numbers.

• re-enlarge player colour band:

In D2948#128595, @Angen wrote:

Above would not work because there is Population element inside Cost currently.

I'm not sure where this would fail code-wise? Because that is indeed *inside* the Cost.

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

Tested and feedback by @Nescio.

• Remove redundant variable.

In D2945#128522, @Freagarach wrote:

That I cannot place the theatre to the south-east or north-west facings of the wonder (small sides) because it snaps and I guess the obstructions overlap.

E.g.

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

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

Fixes notes.

Ok I found discussion:

20:50 <@k776> Why is <PopulationBonus> inside <Cost>
20:50 < Pureon> Mythos_Ruler: I'm moving back into photoshop, vector icon didn't look too good
20:51 <@k776> We don't require a certain population to build something, so <PopulationBonus> should probably be it's own element under <Entity>
20:56 < leper> yay alpha 9 bugs http://trac.wildfiregames.com/ticket/1346
20:57 < Mythos_Ruler> k776: I agree with you re: cost/populationbonus
20:57 < Mythos_Ruler> I didnt design the schema though. :)
21:00 <@k776> leper: You want to try tackle that one? Pull <PopulationBonus> out of <Cost>
21:03 < leper> k776: into Identity?
21:03 <@Philip`> Being in Cost makes more sense from an implementation perspective, since it's precisely symmetric to population cost
21:03 <@Philip`> (Effectively it's just a negative cost)
21:03 <@k776> Philip`: Um, it's the bonus that the building increases population by
21:04 <@k776> i.e.upgrades max pop
21:04 <@k776> Not a Cost...
21:04 < Mythos_Ruler> k776:  is right.
21:04 <@k776> Not an Identity either
21:04 <@k776> It seems suited to a <Bonuses>  element, like  Bonuses/PopulationLimit
21:04 <@k776> I think we have a <Bonuses> somewhere already?
21:05 < quantumstate> add 5 to the pop cap, remove 5 from the current population, what difference does it make in the end?
21:05 < Mythos_Ruler> There's <Population> and then there's <PopulationBonus>
21:05 <@Philip`> If it's going to be moved anywhere, it should be a whole new component like <PopulationBonus><Amount>5</...>

Thank you for working on this. I hope I do not create much trouble with this comment :) .

• Don't pretend footprints are universal: move to child-templates is good.
• Move shared GarrisonHolder to parent is good.
  • Dogs are able to garrison fishing ships now, which seems not strange and deemed not a (large) gameplay change.
• Cleans up nicely.

• Useful (use mercenaries whenever possible).
• Compleat (cannot be extended).
• Correct (no errors).

(One *could* introduce a "Fire" damage type for the fireraiser and fireship.)

@Feldfeld, @ValihrAnt, @badosu opinions?

In D1905#102075, @Angen wrote:

but realistically, if you see someone runs on top of hill and then down, you dont see him anymore but you know where he went :)

Not exactly, if I turn you can assume me going straight, but I didn't go there ;)

Negative damage (or positive, depending on how one looks at it). Splash increasing the health of a target. Around turn 9k ;)

You can wait with these inlines until there has been a comment about the population cost (or don't wait ;) ). In the meantime, feel free to get your hands dirty on some other issue ^^

Refactors the code to follow conventions and also to improve performance

A data validator within the game actually sounds nice, that way modders can use it as well. But I agree it ought to have the same functionality.

@badosu, @Feldfeld, @genava55 any time to test this and provide us with feedback?

@Freagarach The sorter did sort all the files, but it sorted all the comments in some of those files as well. I wasn't sure how to proceed with those, so I discarded them alone. Around 4-5 files I believe. No worries, I'll run the script again and push them with the rest of the changes.

In D2948#128541, @lonehawk wrote:

Sorts template contents using the templatessorter script

It seems that not all templates were sorted -_- This means that your diff now includes unrelated changes. And that means that when this is committed, it makes the so called blaming harder (see what commit caused a change in the code). I won't mind that, but there are people who do (and they're probably right).

In D2948#128534, @Freagarach wrote:

General comments about style (see also @wiki CC):

• We use let as much as possible as opposed to var to limit the scope of a variable.
• Comments start with a capital and end in a dot.
• We like to keep the templates in alphabetical order (there is a script for that in source/tools/templatessorter).

Regarding the name of the component, to me it seems that PopulationBonus would be more appropriate, then you can give it an Amount node in the template. PopulationHolder does not seem to cover what it does.

Another thought: Maybe all of the population ought to be moved to this new component, also the population "cost". Perhaps you would need for someone more experienced to answer this question. Then one can have the Population component with two possible entries Cost and Bonus (or Used and Provided).

Sorts template contents using the templatessorter script

@vladislavbelov would you commandeer it in near future or is it obsolete by now?