Comments.

Off by one error.

Who'd have thought that everything that is not a wall _is_treated as such,,,

(GCC 7 specific.)

I totally missed your comments here ^^'

In D2685#139996, @Nescio wrote:

The same could be said about civ bonuses: in principle those changes could be done in the specific templates too.

Not really, since one can capture buildings and the civ-bonuses wouldn't be applied then.

In D2685#139996, @Nescio wrote:

Also unit_elephant_african.json and unit_elephant_indian.json, or unit_advanced.json and unit_elite.json.

Yep, not a fan of those either :) At least the latter two are considered a hack and work is in progress to be replaced by a different system. As to the former two, I guess I missed D2864.

With this replay

Thread 1 "main" received signal SIGSEGV, Segmentation fault.
std::__uninitialized_copy_a<std::move_iterator<void**>, void**, ProxyAllocator<void*, Allocators::DynamicArena> > (
    __alloc=..., __result=<optimised out>, __last=..., __first=...)
    at /usr/include/c++/7/bits/stl_uninitialized.h:275
275		    __traits::construct(__alloc, std::__addressof(*__cur), *__first);
(gdb) bt
#0  std::__uninitialized_copy_a<std::move_iterator<void**>, void**, ProxyAllocator<void*, Allocators::DynamicArena> >
    (__alloc=..., __result=<optimised out>, __last=..., __first=...)
    at /usr/include/c++/7/bits/stl_uninitialized.h:275
#1  std::__uninitialized_move_if_noexcept_a<void**, void**, ProxyAllocator<void*, Allocators::DynamicArena> > (
    __alloc=..., __result=0x55556d1802cc, __last=0x55556d17f54c, __first=<optimised out>)
    at /usr/include/c++/7/bits/stl_uninitialized.h:312
#2  std::vector<void*, ProxyAllocator<void*, Allocators::DynamicArena> >::_M_realloc_insert<void*> (
    __position=0xffffcde000005555, this=0x55556d17e5b8) at /usr/include/c++/7/bits/vector.tcc:424
#3  std::vector<void*, ProxyAllocator<void*, Allocators::DynamicArena> >::emplace_back<void*> (
    this=this@entry=0x55556d17e5b8) at /usr/include/c++/7/bits/vector.tcc:105
#4  0x0000555555994f41 in std::vector<void*, ProxyAllocator<void*, Allocators::DynamicArena> >::push_back (
    __x=@0x7fffffffd0d0: 0x1437c, this=0x55556d17e5b8) at /usr/include/c++/7/bits/stl_vector.h:954
#5  CPatchRData::RenderBlends (patches=std::vector of length 38, capacity 64 = {...}, context=..., 
    shadow=shadow@entry=0x0, isDummyShader=isDummyShader@entry=false, 
    dummy=std::shared_ptr<CShaderProgram> (empty) = {...}) at ../../../source/renderer/PatchRData.cpp:948
#6  0x000055555591a698 in TerrainRenderer::RenderTerrainShader (this=<optimised out>, context=..., 
    cullGroup=cullGroup@entry=0, shadow=0x0) at ../../../source/renderer/TerrainRenderer.cpp:529
#7  0x00005555558f82b1 in CRenderer::RenderPatches (this=this@entry=0x5555572bb150, context=..., 
    cullGroup=cullGroup@entry=0) at ../../../source/renderer/Renderer.cpp:771
#8  0x00005555558fbe77 in CRenderer::RenderSubmissions (this=this@entry=0x5555572bb150, waterScissor=...)
    at ../../../source/renderer/Renderer.cpp:1355
#9  0x00005555558fc5fc in CRenderer::RenderScene (this=0x5555572bb150, scene=...)
    at ../../../source/renderer/Renderer.cpp:1718
#10 0x000055555587f464 in CGameView::Render (this=<optimised out>) at ../../../source/graphics/GameView.cpp:240
#11 0x00005555557b5419 in Render () at ../../../source/ps/GameSetup/GameSetup.cpp:234
#12 0x00005555555b19b8 in Frame () at ../../../source/main.cpp:424
#13 0x00005555555b49f8 in RunGameOrAtlas (argc=<optimised out>, argv=<optimised out>) at ../../../source/main.cpp:713
#14 0x000055555559e117 in main (argc=1, argv=0x7fffffffdff8) at ../../../source/main.cpp:764

Did you manage to set up your dev-environment?

I agree that it hides template values.

(Also would it be nice to add some unit tests, but I can try to take care of that when the patch is nigh completion.)

In D3155#139110, @mammadori wrote:

it shows total gatherers under population icon too, which could be useful.

(It confused me what that number meant, at first I thought it was the number of houses.)

(I'll do a more extensive review later.)
The amount of traders is already shown in the trade dialog window.
To fix entities in a return cycle counting towards the gatherers one could copy (UnitAI style) the return resource state into the gather-state, adding the gatherer upon entering gather and removing it upon leave. I'm not sure how that works out with retasking, but entity death is accounted for then.

Looks good, works as advertised.

Empty tag.

Perhaps quote some more variables? And add . to comments?
Works.

There seems to be some magic here that I don't understand. Need to look more into it,,,

Works as advertised :) From the main menu, from the match setup menu and from within a match.

Removed unneeded change in test.

Works fine for me! (Lubuntu 18.04; libfmt-dev 4.0.0+ds-2 amd64.)

Try to fix UnitAI only one issue at the time.

Don't go after the chickens.

I assume the rest of the ” \n -> ”\n is not done to not cause needless retranslations.

That is exactly what I meant also :) But I understand my wording was wrong.

Working fine here (GCC 7.5, Lubuntu 18.04).

(They _are_ in the game already.)

(Elephantine map was broken hereafter: D3122.)

Successfully builds on my end :)

Saving some work.

Sure, lets wait then ^^

Can I start tinkering with template values now?

If you make sure the values don't change, I can commit the addition of the templates and you can make a separate diff for the values to be reviewed by the balancing department.

You could also make the sound in Attacking.js to avoid duplication?

I missed rebasing after the attack name addition: rP24213.

Pointed out by: @Sundiata.

Move.

the game is 0 A.D., and what's meant is a match.

Would be nice to get this consistent then :)

The filename doesn't matter :)

In D2737#136900, @Stan wrote:

Is it normal that if I stack three fire arrows on a unit it displays three time the icon while if it extends only the duration is updated accordingly?

Sorry, it was late ^^'
But, then the balancing department needs to take a look :) (rP23890)

Tested by: Stan, Freagarach

I recall not being able to actually apply let alone test the patch ;)

Would you mind moving to watchtower? (You can read some discussion on IRC today.)

Can you please make sure the values don't change?

Correct, split is nice.

• Nice feature.
• Code looks good (do need to use EjectOrKill).
• Works as advertised.
• Properly tested.

Could you add a test to check whether entities get ejected when requirements change? (Would also unearth a bug in the current code.)

(I would actually call the left one a lookout and the right one a watchtower ^^ )

Complete, I assume correctness.

Is it?

Missed one.

Yeah, I don't like it either, but I keep telling myself this is only done on execution and thus not so often (compared to other GUI functions).

So is the dashing enough visual feedback that the path is uncertain?

In D3081#136008, @bb wrote:

While at it, might want to make it modifiable

Is that worth the perf loss for something purely visual? I mean, yeah the entity is in a different state, but it will still behave like it's idle (except responding to the idle-unit button -- which is annoying).

• Checkrefs passes.
• UnitsDemo loads fine.
• Change is good (entities are structures, although one can argue the tables are not).

Well, we also need to keep in mind the understanding of the function to players. I think adding "tower" makes it much more clear. Also, you proposed watchtower earlier (D2551).

Sorry to ask for another rebase ^^

Isn't the idea that the path is uncertain and hence reddish?

What do observers hear?

This needs a rebase. Also, why introduce templates which are not used? I mean a mod that needs to introduce the template probably also needs different values? And the templates can be added if units that are to use them are added, right?

(Still working here.)

(Still working.)

General comments on the build script:

Please quote variables.

And use the same style as used in the rest, i.e. ${varname}.

(Still working here.)

Indeed. Although this commit wil be _huge_ it will actually encompass one change: moving templates from folder/{civcode}_* to folder/{civcode}/*. But nothing unrelated. E.g. renaming aura's to a similar schema is unrelated and can be done in a separate commit (if ever).

In D2995#135881, @Nescio wrote:

the values are chosen such that it would result in an instant kill

(Annoyingly, that is quite wrong.)

Please quote variables, especialy paths.
Not sure how to fix this (properly):

Or cmpTargetable and use it also for healing (to inform the healer that we have for example) instead of listening to global ownership changes.