[PATCH] Unit death damage

[Pureon]

When a unit is destroyed the destruction causes damage to nearby units.

This will be configured in the unit template.

[Josh]

Diff of simulation/components/PlayerManager.js

[Josh]

Diff of all changes including test functions

[Josh]

For this ticket I've been trying to set up a new "Damage" component with more flexible functions for both splash and normal damage. I'm hoping to get this into alpha 14 and I'm bumping the priority also because as things are now, splash damage is hardcoded to only work with projectiles.

[zoot]

Not a full review or anything, but from a quick glance I'm a bit confused as to why you choose to implement this as a system component? Why not make it a regular component that can be used by templates that require the splash damage effect? Gameplay components should ideally never be implemented as system components, since they need to be hardcoded into the engine, reducing moddability.

I would also suggest giving your component a more descriptive name, like DeathBlast or something, or people will confuse it with other components concerned with damage.

[Josh]

Well my idea with this was to entirely abstract damage so that it was independent of various attack types. This would facilitate modders who would be able to discard the functions/logic in attack.js but keep these functions in Damage.js.

For example, lets say a modder is building a game where natural disasters can happen. They would be able to construct new functions that call, for example, SplashDamage if an earthquake was triggered. This would be entirely independent of templates and (most) other gameplay components.

This also helps with future damage-related functions which would be able to be consolidated into one component instead of being scattered among many components as they are now.

Another more concrete problem with moving the template data from Attack.js is that Attack.js still needs much of that template data so if it was moved it would effectively be a copy. That could become very problematic to maintain and synchronize.

[Josh]

The new component with all essential functions working (At the moment this is more relevant to #1496, but I'm posting it here for consistency)

[Josh]

Diff of all changes to hopefully fix #1496 including test functions.

[Josh]

I'm not going to finish this for awhile, the attached files are primarily for #1496.

[Josh]

As I've moved on to mostly UI/network programming in 0AD, this ticket would serve better as a starter task for someone new to the simulation. The changes should be mostly contained in Attack.js and Damage.js. My patches can all be ignored as irrelevant for any future work on this ticket (much more refined versions were committed to fix #1496 a few months ago).

[Diamond-Warrior]

I just had a look at this issue. I think it is obvious that the splash damage should depend on the unit type. Is there any suggestion which unit properties should define the strength and the radius of the splash damage? I don't think the damage caused upon death of the unit should be the same as the damage caused by a regular attack. In my opinion this would mean to define a fourth type of attack beside hack, pierce and crush.

[sanderd17]

Yes, you need a new attack type, but besides "ranged", "melee", "charge", "slaughter" and maybe others. Not anew attack strength type.

[Mate-86]

I wanted to provide a similar solution like the splash damage of ranged attack (eg. onager) but the existing solution for that does not work for when I created a test map. I've checked the code and found these:

1) simulation/components/Damage.js:90 says let playersToDamage = !data.friendlyFire ? QueryPlayerIDInterface(data.attackerOwner).GetEnemies() : null; but data.friendlyFire is always true because the string "false" is assigned to it from the template and non-empty strings are interpreted as boolean "true" 2) The same line says if the friendlyFire == true then playersToDamage = null but this is wrong because Damage.js:241-242 says if (!origin || !radius || !players) return [];. Which means the players should not be null. Could you please confirm if I understood the problem correctly? Shall I fix this issue in my patch #1910?

[leper]

Nice find! I should have checked the patch at #3610 a bit better. The second issue was an oversight in r18645 which indeed breaks splash damage for non-friendly-fire cases.

CCing fatherbushido since we should fix that before the release, else I'd have suggested that you fix this, but in a separate patch. (fatherbushido: Keep this ticket open, I just don't want to open a new ticket just for this fix.)

The playersToDamage fix is a bit long, so in case someone wants to make that shorter, please do. Also some tests that help catch this issue would be nice (as in friendly-fire splash damage).

EDIT: Not tested, but should probably work.

[Josh]

I do still actually monitor all this. Re-adding myself as CC'ed.

[fatherbushido]

Thanks Mate-86!

leper: I should be blamed too.

(It was hard to noticed as in the current game only the quinquereme has the friendly fire. It was removed some years ago for the catapult.)

I tested the patch wich works as expected. I will look at writing a test for that. We can reduce the L94-L98 with something like

playersToDamage = [...Array(Engine.QueryInterface(SYSTEM_ENTITY, IID_PlayerManager).GetNumPlayers()).keys()];

but I don't know if it's the good way to do.

I wonder too if we should not swap the if and the else (though it makes sense to use the generic case before).

[Mate-86]

Thx for the feedback and for the fix! Reusing this splash damage fix I've created a patch for the original problem (dying unit causing damage). Some questions:

• Is "Death" the proper name for this attack type?
• Are there any example units intended to have death attack? Currently the slinger is used for an example.
• Does directed (linear) death damage make sense or only circular?
• Shall I update the tooltips? (tech tree + session)

Unit tests coming soon ...

[fatherbushido]

mate-86: if you could first make a test for the leper's fix, it would be nice!

[Mate-86]

Manual test in the game was successful for splash damage. I can create the unit tests as well.

[fatherbushido]

Yes that's what I was speaking about.

[leper]

Replying to fatherbushido:

We can reduce the L94-L98 with something like

playersToDamage = [...Array(Engine.QueryInterface(SYSTEM_ENTITY, IID_PlayerManager).GetNumPlayers()).keys()];

but I don't know if it's the good way to do.

Benchmark both versions, likely decide that the performance impact is minimal, likely use the shorter version.

Also using a function like in Mate-86's last patch is a nice idea, though that could be made shorter by just returning instead of keeping the full if-else; also passing the data object to it directly might be worth considering.

[fatherbushido]

I think I will commit it this evening, then in the Mate-86's patch, he could use that new function (as it will be use 2 times).

[Mate-86]

I'm struggling with the unit test and I won't be available in the weekend. I can finish that next Monday-Tuesday the earliest. Feel free to take it over if it's urgent for the release.

[fatherbushido]

In 18810:

Fix two issues related to splash damage: fix a mismatch between the string false and the false boolean, fix an issue with an omitted array of players. Noticed by Mate-86. Patch by leper. Refs #3610, #1910, r18645.

[fatherbushido]

I moved my unrelated comments to #4276

[leper]

First one looks good, that bug seems to have been there for ages, nice find.

The second one is the result of me trying to fix something else, then not doing that and leaving that in. But for actual splash damage the break should be removed too.

I didn't test anything, so better ask someone else. Also someone should push this ticket to A22 at some point and possibly open a new ticket for more bugs that don't really have anything to do with this ticket.

[Mate-86]

I've uploaded a patch for the death damage with unit test. There are still some open questions:

1. Is "Death" the proper name for this attack type?
2. Are there any example units intended to have death attack? Currently the slinger is used for an example.
3. Does directed (linear) death damage make sense or only circular?
4. Shall I update the tooltips? (tech tree + session)

[fatherbushido]

1. No clue.
2. As an example, I would have choose a structure like a fortress or a cc.
3. You just have to use the spash damage so both could be done in the same move ? (but circular makes more sense)
4. I don't know what we want to be displayed, but it's indeed better to have that too.

I have not yet look at the patch in details, perhaps someone have a clearer view of that than me, but:

• I don't know if that must be a part of attack (is it an attack ?)
• if it's in attack, perhaps you can use a splash schema to avoid duplication
• Take care to not call that too late in Health.js

Thanks for working on that part of the Bermuda Triangle #1910 #1911 #1912 (leper © expression).

[Mate-86]

2. I've added example for fortress and cc but values need to be revised
3. It's a bit complicated because the linear damage uses the vector pointing from the attacker to the defender. In MissileHit it's given but don't how to calculate for the DeathDamage because the defender is not specified or ambiguous then.
4. Added in the patch. It looks like this: ​http://imgur.com/a/6k2U9
5. is it an attack ? => Same for me but then please suggest another component then.
6. splashSchema created to avoid duplication
7. too late in Health.js => I don't see the problem here. Please clarify!

[fatherbushido]

• all that seems nice.
• i forgot, as leper said in a previous comment, you can save an identation level in getplayerstodamzge by doing return at l76 after the if and removing else
• for 7, i mean that you call a method of the attack component of an entity for wich you have call the destroy order (it wouldn t be a matter for the damage component wich is a system component), but as the destroying of an entity is slow, perhaps it (luckily) works.

[Mate-86]

• I fixed getplayerstodamage to save an indentation level.
• for 7, I tried to move CauseDeathDamage upper in the function but it resulted run-time error. Looks like it must come after this.hitpoints = 0; otherwise it would call the DestroyEntity function again. (CauseDeathDamage kills the originator entity itself again.) CauseSplashDamage does not get any info whether it shoue attacker or not.er or not.

[elexis]

Moving to the new WIP milestone.

[bb]

Partial review:

Templates L147 not sure if that comment is very useful, probably just leave it there because of consistence in the file.

Attack L194 destroyed L632 test for "cmpPosition" too. L646, I don't think this attack should have a direction now, but that is a design decision, which is not up to me.

Let the splash attack also use the new function GetPlayersToDamage instead of checking that in damage.js

The UnitAI will treat this new attack type as a new way of attacking, so it will perform this attack when the unit is not even close to dying. To prevent this "abuse" the CanAttack and GetBestAttackAgainst function in attack.js should exclude this type of attack.

Damage L77 var --> let

Test_attack L146, L205, L208 use { "foo": bar } not {foo: bar} L192 var --> let

[Mate-86]

@bb: Thx for checking! Most of the issues fixed in the new patch. Others:

1. "Let the splash attack also use the new function GetPlayersToDamage instead of checking that in damage.js" => Sorry I don't understand this one. Could you please tell me the lines in the code which need to be changed?
2. In Attack.js both CanAttack and GetBestAttackAgainst use GetAttackTypes which returns a hard-coded list not including death damage (L207).

[bb]

1. "Let the splash attack also use the new function GetPlayersToDamage instead of checking that in damage.js" => Sorry I don't understand this one. Could you please tell me the lines in the code which need to be changed?

Actually I was mistaking here too, anyway L112 of damage should be merged with L121(?) (current L110), then the code is correct imho. (So nuke the playersToDamage var).

besides that some more: Test_attack L85 "false" --> false "L146, L205, L208 use { "foo": bar } not {foo: bar}", look at whitespace too...

I don't know if balance is alright since nearby citizen skrims just get killed when destroying a cc, but at least the range needs an increase since the attack range is inside foorprint thus there is no attack.

[Mate-86]

Ok, another patch should fix those. I tweaked the range a bit: ​http://imgur.com/a/KmjpJ

[Vladislav Belov]

I think, the GetPlayersToDamage is wrong enough. Because, @param {boolean} friendlyFire - a flag indicating if allied entities are also damaged and you return GetEnemies() if !friendlyFire, but there are neutral players except allies.

So you should return GetEnemies() + GetNeutrals(), but GetNeutrals() isn't defined, so you could use GetEnemies().concat(GetPlayersByDiplomacy("IsNeutral")) (or define this function and use GetEnemies().concat(GetNeutrals())).

Also, I think, instead of the loop you could use the same thing:

return GetPlayersByDiplomacy("IsNeutral").concat(GetAllies(), GetEnemies());

I'm not sure that we should have this radius of damage, buildings don't have barrels of gunpowder. Probably, make the radius smaller and damage too?

The rest of the patch looks good.

[Mate-86]

@vladislavbelov thx for checking the patch! I've adressed your comments in 1910-death-damage-7.patch.

[fatherbushido]

Don't focus on the example. I don't even know if we will use it in the 'main' game, but for mods sure it's nice. I am not able to review the patch atm but the part in Health.js needs to be carefully checked.

Meanwhile, you can perhaps works on the other ticket ?

[Vladislav Belov]

You could write this:

if (!friendlyFire) 
    return nonAllies; 
return nonAllies.concat(player.GetAllies()); 

Shorter:

return friendlyFire ? nonAllies.concat(player.GetAllies()) : nonAllies;

[Mate-86]

• I've adressed the latest comment from vladislavbelov in patch-8
• I've put some questions in 1911 (seems to depend on 1909 which has some additional questions to clarify)
• for 1912 I can look at the patch submitted by Leander

[Vladislav Belov]

1. We're using { on the next line instead of same if (type == "Death") {.
2. Do we need this comment here: "friendlyFire": template.Attack.Death.FriendlyFire != "false" // true if undefined, isn't obvious?

[Mate-86]

1.fixed

2. the // true if undefined is already used in trunk, shall I remove that too? ​http://trac.wildfiregames.com/browser/ps/trunk/binaries/data/mods/public/globalscripts/Templates.js#L138

[fatherbushido]

Sorry Mate-86 for the delay, are you still interested on that one? If so you can create an account on code.wildfiregames.com and upload the patch here. I read some stuff.

From a gameplay point of view that things could be used in main mod for the iberian ship or the fireraiser.

Concerning the current patch, I guess we could get rid of the Attack.prototype.CauseDeathDamage function. Moreover, even if it makes sense to have the stats in the Attack component as it's nothing else than a splash damage, I would be more for putting them in a small component like "Explosion" or "ExplodeOnDeath" or something like that which call directly CauseSplashDamage. Both (having that in Attack or not) have pros and cons. That can then listen to death message (bad) or directly call from the Health component like you did (better). EDIT: keeping like in the patch is perhaps better :p

[Mate-86]

Ok, np. I can have a look and register in the other page.

[Mate-86]

​Phab:D451

[fatherbushido]

In 19950:

Splash damage on death. When an entity dies, it can do a splash damage. Fire ship and fire raiser templates provided as example. Fix #1910.
Patch by Mate-86.
Advices from leper.
Reviewed by fatherbushido.
Differential Revision: ​https://code.wildfiregames.com/D451

[fatherbushido]

Thanks for that work started one year ago.