Just wondering: train seems like D1245 and recruit more like what is done when clicking the button? I think this can be comitted as is and we can discuss "train" vs "recruit" elsewhere, if you want?

• Remove if in addBuilder.
• AddSingleBuilder -> AddBuilderHelper.
• Don't say orders have changed when they might have not been.

Added undefined type.

Rebased.

In D2496#104876, @Nescio wrote:

Unit templates don't have a static obstruction defined in their templates, so I assumed their effective target size is based upon their footprint.

The related code (Attacking.js-helper; L137 onwards) shows that footprints are indeed used.

I agree that the CC ought to be placed first. Not sure though that the crannog, which is a Briton-specific structure, should be placed in the general build-list.
+1 on the Military Colony.

Fix loading a map which has an entity with a turret on init.

Thanks for the report @Nescio! I have a hunch where it could come from, but I'll investigate.
(The rebuilding of the game is not necessary for quick tests on this patch, one only needs it when testing (de)serialisation (i.e. saving/loading and MP).)

(Agreed, just a reference.)

Thanks :)

For territory root see also D2005.

• Handle builders changed function in Foundation.js.
• Comment for @param.

Please revert the ordering.

Yes :)
So if you have any feedback regarding the GUI part (which seems to be the last hurdle for now), feel free to comment on that :)

Yeah, there is a concern raised IIRC. And moreover: a fix proposed somewhere.

Revert template change.

I thought the convention still is Hack, Pierce, Crush?

Thanks @Angen for the review and commit :)

Thanks for the review and commit :)

• SetBuilders.
• Send only one message when adding multiple builders.

Remove GUI-icons when an entity dies.

Comments.

Aye, something of a pun around here (in real space) ;)

• Compleat.
• Improves consistency -> Is an improvement.
• Breaks nothing.

Revert template change.

Add new property for StatusEffects to disallow XP gain by SE.

Added misses check.

Remove accidentally added extra space.

Revert IID_Vision change.

Test fails (OnGlobalEntityRenamed but also then.`).
And one extra suggestion for a test: when garrisoned visibly and ownership changes, just to be compleat.

• Reuse the entire GARRISON-state.

I meant you can write a test that checks that a unit that is garrisoned visibly promotes to a class that is not allowed to garrison visibly that it is actually not visibly garrisoned anymore?

In D2308#100065, @Stan wrote:

Should I add a test for promotion ?

More tests is more better, I would say :)

Thanks for the review and commit @Angen :)

Isn't that out of scope for this patch (although it seems to be partially solved), since that wasn't correct before this patch also?

It would indeed be good to add that to the tooltips, I have a patch locally waiting for my diff list to shrink xD However, the GUI/tooltips are getting clumsy, certainly when Status Effects are added someday. Hence I think it would be good to do that in a different patch.

Copy member position.

You may (sounds cleaner) close this instead of abandoning?

You can (sounds cleaner anyway) close this instead of abandoning?

Patch by @Nescio.

Patch by @Nescio .

In D2281#106137, @Stan wrote:

As I said on IRC it should keep status effects if it makes sense, as in disease, curse when turning to a resource or whatever. So it's okay.

Dead stuff doesn't keep StatusEffects.

My concern of the main menu tooltip has been fixed.

• Removed (wrong) formation check.
• Split ownership check for readability.

Ah, so the GUI is not properly updater perhaps? If you select something else and then come back to the corpse, does it still have the Status Effect signs?
Or better: if does the selection state show status effects?

See "discussion" at D2160 ;)

• No XP from Status Effects.
• Less object creation when applying multiple Status Effects at once.

In D2281#106005, @Angen wrote:

Unit gets experiences through status effect timer. Unit should get experiences only when it hits with attack not just through status effect.
Here is why: Hit target with poisoned arrow, you get exp because you landed hit. But you do not get exp, you do not improve your fighting skills sitting somewhere safe (chopping trees, ...) and do not attacking while poison is doing its job.

Okay, but edge case: what if a unit only inflicts status effects (and damage with that), than it cannot gain XP, although it does hit and kill stuff?

Or just that units in the vicinity of an allied CC have a larger search area, because that is "safe"?

Fix test.

We have to sent messages to update other stuff, but in UnitAI we can ignore "attacks" from StatusEffects?

I thought the difference lay in the setup of their armies: quality over quantity for the Spartans and vice versa for the Persians.
The Mauryas don't seem to follow this logic and it seems to be that they indeed have the bonus due to the sheer size of their former empire (forum post).

Why not train them at a house? I like that idea on the forum.
(I've got not really a clue about balancing, so please ignore me as appropriate.)

• Rebased.

Still good to go?

I'm not sure whether it is intended or not, but when switching from a Random map to a Skirmish map, the win settings change back to Conquest. (Also when changing the map in Skirmish.)

• Cache SearchRange of ResourceGatherer. (Not of Builder for that value is requested less often but the ValueModification-message is sent often.)

• SearchRange.
• More informative description.
• Inline range.

Rebase and probably the inline is what this needs.

It partly fixes r22526, this would only fix the chasing, not other orders which are affected.

In D2285#105187, @Angen wrote:

I think it would be reasonable to handle it by clicking at origin market again so that looks for another patch.

Agreed (twice) ;)

Give credit to attacker and its owner.

No issues found during testing, other than the very specific case described below.
When I have three markets, one in the center, two next besides those. And I accidentally set the middle one as the origin I cannot revert that (the outer two are too close to the middle, but far enough from eachother). The only way I found of freeing the trader is by deleting the middle market.

I discussed that earlier with @wraitii, it may have some side effects, I need to investigate.
(Besides, without an owner a capturing status effect would not work.)

See also D506?

So it doesn't need a Builder component anymore?

Updated test to use code style update (, at end of object property list spanning multiple lines).

In D2113#103693, @Angen wrote:

Another note to think is that getBestAttackAgainst should probably not return attack without needed resources

It check whether one can attack the target, so that is covered.

In D2113#103693, @Angen wrote:

Petra will probably screw up her resources usage and planing with having this.

Aye, badly.

• Try to take resources before attack.
• Fix typo.
• Proper tooltip.

Builds on (L)Ubuntu 18.04 with gcc.

Thanks @Angen :D

Based on a patch by @causative.

Thanks @Angen for the review and commit :)

After the rewrite of gamesetup.

• Return undefined instead of false.
• Don't check for this.template[type].Splash when launching a projectile since that is checked in the GetSplashData.