Rebased.

Doesn't this close D843 also?

This revision now requires review to proceed.

Not sure why and how I triggered that.

In D2016#84821, @Freagarach wrote:

In D2016#84269, @wraitii wrote:

I don't see why melee units would have longer range when they are higher than their opponent. I can be overturned on that.
I think the important thing is specifying that this is a range bonus, not an attack damage bonus (unlike in AOE 2).

Not a argument for melee range bonus, but for melee damage bonus: forum.

Ref: D781.

• Rebase.
• return true after execute.
• Hotkey.

In D1868#87412, @Angen wrote:

I just tested it and does not work. They do not even move to that location / unit I order them to attack with this command.

That is interesting, for me it does work (at least yesterday it did). Do you get an error or something?

Wait for result of D2070 discussion. Especially regarding curly braces.

If restricted classes would be also applicable to damage (also splash/death/trample) immunity would not be necessary, I guess.

This merge-conflicts a bunch with D1938

Not sure where it conflicts with D1938, yes some rebasing is needed when either is committed but no major changes needed as far as I can see.

Thanks for committing this @bb!

I've been testing this, UM looks so much better now than a few weeks ago! Kudos @wraitii.
Regarding this specific diff, no strange things found.

Perhaps you can add a reference to the source? For future reference and may help to persuade people ;)

In D2081#87107, @wraitii wrote:

I might give a shot at commandeering this in a more general way tonight.

Feel free!

• Test.
• Proper checking wheter an entity is moving.

Rebased.

I agree with @wraitii that using capture as a damage type is weird ;) But also agree that a modder should have the option to do both damage and capture at the same time.
The armour for capture is interesting, but isn't there the CP-regenrate for already?
Wouldn't beams be just ranged attack with a stream of projectiles? Or am I thinking to limited now.

Rebased.

Make Jenkins/Vulcan a tad happier.

Rebased.

In D1955#86530, @wraitii wrote:

Also you have no phenotype for, for example, gold mines. I'm not sure any values makes sense except "default".

Well, the function GetPhenotype *is* called for them (i.e. mines, trees, buildings etc. (not fish; don't ask me why)), so I'm not sure that "default" would not throw errors, but I'll test.
Open questions:

I wonder if all this shouldn't be handled in cmpidentity. To make the other components agnostic. That would simplify the code a lot here and in cmpvisual actor I think you wanted to do it

CauseAreaDamage => CauseDamageOverArea.

• Moved cmpDamage-function to CauseSplashDamage.
• Removed support for status effects and restrictedClasses which are to be added in a seperete diff (D2081)

I can't get the test working properly, too little experience with that I guess, but I do not really want to leave it out. I'll try harder soon.

Split functions.

In D1938#86525, @wraitii wrote:

Except for the above typo, this works correctly and it looks rather clean. I'd appreciate some basic tests for at least the case that "unknown damage type in armour = 0 armour", but I wouldn't call it necessary.

The armour is used nowhere in tests, should a new test (test_armour) be added?

In D1938#86542, @elexis wrote:

It is important to analyze why the code was the way it was written - g_DamageTypes was introduced in rP20203 rP20625 (I can't read through everything now)

From what I read rP20625 included the variable g_DamageTypes to the wall_builder-script because getting the template required that.
Regarding rP20203, this seems indeed an extension of that commit. If I read it correctly, it was desired to have it more generic, but no one did it yet.

Hi! I got a segfault when testing this:

Thanks @Nescio!

• Inlines.
• One var => let.

and always discourage non-core members from doing wholesale commits like this for obvious reasons..

Sorry, didn't know,,,

In D2069#86271, @Nescio wrote:

@Nescio, you are my superior in this

Am I?
Because it involves javascript, you actually need someone else; perhaps @bb is willing to.

I did not really change any code (except one var -> let) and, as far as I can tell, you are a very exact person. So yeah, I think you are ;)

In D2065#86263, @Nescio wrote:

Actually “splash” implies a liquid (the word itself is an onomatopoeia), so I'm not sure it is the most appropiate term for anything we use in 0 A.D.; anyway, that's probably outside the scope of this patch.

Thanks for the info! It might be in scope to change it then, but I guess that needs a team decision.

Reuploading because the test doesn't fail locally.

Some more fixes (last?).

In D2065#86153, @Nescio wrote:

If it is to be changed, shouldn't *all* occurrences of “splash” then be replaced with “area”?

Well, I think that the term "splash" is still correct in many places. It is splash damage that a projectile causes, right? You're more proficient with language ;)
I agree though that regarding DeathDamage it should probably be replaced.

• Added tooltip.
• Many fixes from inlines.

Test not yet done.

!

Some more checks in PetraAI.

In D1903#85881, @Freagarach wrote:

Whilst testing this I got:

*** stack smashing detected ***: <unknown> terminated
Thread 1 "pyrogenesis" received signal SIGABRT, Aborted.
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
51	../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007ffff3029801 in __GI_abort () at abort.c:79
Backtrace stopped: Cannot access memory at address 0x7fffffffb928

a couple of times when starting an attack after move, but I cannot reliably reproduce it. Not sure whether it is related, but I haven't seen this earlier.

In D1977#82718, @Angen wrote:

Units that get fired upon by archers might still cheer if they kill an enemy

If you mean that when unit gets hit by arrow should keep cheering, do not think so

I think what he meant is that when an entity kills its direct opponent it will cheer, but it might be the case that while cheering (s)he is under arrow fire from afar.

Whilst testing this I got:

*** stack smashing detected ***: <unknown> terminated

Not going to work on this any time soon.

Is fixed.

In D1846#85787, @elexis wrote:

Having dedicated functions would mean the JS complains about it when parsing the code

Actually not true, only happens when the code is run, and the only code that is run when the file is loaded is the creation of the prototype and registering of global functions.
So catching typos can happen in both cases only when the affected code is run during the simulation.
Actually the generic approach prevents testing for typos by design since it is undecidable whether there is just no resource with the tag in the current set of mods or whether there is a typo.

I'll wait untill you find an agreement ;)

Improved tooltip to also show extra garrison size.

• Tooltip.
• slots -> size
• Extended test.

• Good check.
• Also disables trade window when there are neither resources to trade nor to barter.

If everything went okay now, PetraAI can handle situations where there are no resources tradable, tributable and/or barterable.

In D1846#85757, @Angen wrote:

I would not see a problem with petra as looping through 0 resources takes 0 time

Yeah, but there is happening a lot before it loops through the resources ;)

Even more fixes.

In D1846#85749, @Freagarach wrote:

Question: Should we also try to account for the cases that no resources have a specific property?

I tested these situations just now:

• No Bartering resources reveals no errors,
• No Trading resources gives only errors when trying to trade (obviously).
• No Tributing resources gives an error when opening the diplomacy screen (which I think can easily be fixed by setting a default for widthOffset).

Performance-wise it would be good if PetraAI would not even try to initialise the different components when they are not possible.

Question: Should we also try to account for the cases that no resources have a specific property?

Should do the job, @Angen.

Many small fixes.

You beat me to it @Angen ;)

In D1846#85636, @Nescio wrote:

Occassionally I get the following warning and error:

WARNING: JavaScript warning: simulation/ai/petra/tradeManager.js line 193 reference to undefined property tradingGoods[res]
ERROR: Invalid trading goods: ({silver:NaN, metal:NaN})

Any ideas how to fix it?

So glad you're testing it already! I'm starting to get a love-hate relationship with PetraAI ;) (The implementation of the garrison slots took me about half an hour, fixing Petra took the rest of the day,,,)

Fixed AI.

Duplicated in D1268. Sorry people.

• Some fixes regarding PetraAI.

In D2044#85582, @Angen wrote:

i ll check another things later

Thanks! I'm currently running many test games AI vs AI and already found lots of errors :( Mainly when I use *.id() where it should not be used and vice versa.

So here is a problem: I cannot find any code to give a (bombard) circle overlay at the pointer position. (Range visualisation is a component of an entity.) Currently a tooltip is shown with the bombard radius, but that seems kind of odd. Moreover, distances/radii are quite difficult to estimate in-game, IMHO.
Hence the question what to do with it now?
Perhaps:

• Nothing extra, we can just use it as is. (Not likely because of reasons above.)
• Split the radius-changing part as requested earlier. (Probably, but I'm no fan of that for reasons mentioned earlier (here & here ).)
• There *is* a way of showing the radius! (Please point me in that direction then ;) )
• We will make a way of showing the radius. (My last encounter with C++ was not very comfortable, but I still am willing to learn.)

Or something else?

• Improved function canAttackTarget in entity.js in common-api.
• More checks whether the AI can attack units.

Also, what to do with attackMove?

In D2044#85441, @Angen wrote:

This has to be fixed with this change of armour.

Thought so.

Created a canAttackTarget-function in "entity.js" of the "common-api".

In D2044#85423, @Angen wrote:

yes that would be better place :)

I'm not sure, but is seems that I cannot check the target there, is that correct? target.anyFunction() gives an error.

In D2044#85396, @Angen wrote:

You would need to write one at extendedentity.js in petra directory. I suggest to not call canAttack from cmpAttack but rather to check attack types and armour using functions petra has (attackStrengths,armourStrenghts), because that would use only template values and will be faster as petra would call canAttack very often.

Shouldn't the check be in entity.js of the common-api? There is also a function canAttackClass already in that file.

In D2044#85131, @Angen wrote:

and you need to edit AttackPlan.js in Petra AI function "update" as well

Can I somehow use the function CanAttack from Attack.js for that?

Added unit test.

In D2044#85350, @FeXoR wrote:

This seems to be a duplication of D1965

No, I split it into instant-kill (D1965) and immunity (D2044).