Page MenuHomeWildfire Games

Unit death damage

Authored by Mate-86 on May 7 2017, 10:05 PM.



I. Business logic

  1. When an entity (unit or structure) is destroyed - either by damage or by the user hitting DELETE button - the destruction causes damage ("death damage") to nearby entities.
  2. Death damage is configurable in the entity template.
  3. Death damage configuration can be affected by upgrades (ApplyValueModificationsToEntity).

Technical consideration

  1. Proposed structure in template (outside of Attack):
<?xml version="1.0" encoding="utf-8"?>
<Entity parent="template_unit_mechanical_ship">
  1. A new component DeathDamage is introduced to extract death damage logic from attack component.
Test Plan

Unit tests added/updated.

Functional tests:

  • test a fire ship with configured friendly fire and surrounded by allied, gaia and enemy ships. Kill it by hitting DELETE button.
  • Same as above but have the ship killed by enemies
  • Same as above but without friendly fire

Regression test:

  • test a catapult firing at allies/enemies and with/without friendly fire

Diff Detail

rP 0 A.D. Public Repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Vulcan added a subscriber: Vulcan.May 8 2017, 1:54 PM

Build has FAILED

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests).
In TestComponentScripts::test_scripts:
../../../source/test_setup.cpp:134: Error: Test failed: L"Stack trace:\n@simulation/components/tests/test_Attack.js:161:2\nattackComponentTest@simulation/components/tests/test_Attack.js:123:2\n@simulation/components/tests/test_Attack.js:127:1\nExpected equal, got ({hack:0, pierce:15, crush:35, friendlyFire:false, shape:\"Circular\"}) !== ({hack:0, pierce:15, crush:35, friendlyFire:false})"
ERROR: JavaScript error: simulation/components/tests/test_Attack.js line 123
TypeError: test_function is not a function
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_Attack.js"
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content)
Failed 1 and Skipped 0 of 306 tests
Success rate: 99%

Link to build: http://jw:8080/job/phabricator/1070/
See console output for more information: http://jw:8080/job/phabricator/1070/console

Mate-86 updated this revision to Diff 1775.May 8 2017, 9:12 PM
bb added a subscriber: bb.May 8 2017, 9:20 PM
bb added inline comments.
180 ↗(On Diff #1738)

newline before {

42–49 ↗(On Diff #1738)

Incorrect indentation for whole block
while at it add periods in the help entries.

197 ↗(On Diff #1738)


630 ↗(On Diff #1738)

We don't need the 3D position so GetPosition2D

632 ↗(On Diff #1738)

running into an edgy case here: AFAIK corpses doesn't have an owner, so if we give a corpse a Death attack it will crash, but corpses generals don't die and we require an owner below, it seems ok, but as we want mod support we might just give a warning in case !Engine.QueryInterface(this.entity, IID_Ownership). As giving an error is better than continueing silently

638 ↗(On Diff #1738)

I guess it should be allowedto change this value in a tech (nor it should be done) so use the ApplyValueModificationsToEntity function as in the GetRange function

641 ↗(On Diff #1738)

direction only required when linear splash, so nuke

643 ↗(On Diff #1738)

This should be "Death"

71 ↗(On Diff #1738)

list of number not a JsDOc type (afaiks) => array

73 ↗(On Diff #1738)

IMO the order of the argument should be inverted

76 ↗(On Diff #1738)

Adding neutrals here leads to some spurious attack detection messages which might needs to be fixed after, also not convinced of adding them here since neutral is sorta friendly, so i don't expect them to be hit by some splash attack.

81 ↗(On Diff #1738)

It looks tremendously hardcodish to say that Enemy +neutral+allies == all players (also not sure if the player self and gaia are included in this, need to check that), better make some global function to get all playerIds

15 ↗(On Diff #1738)

Wondering why we need this component/object doesn't seem used

198 ↗(On Diff #1738)

Use arrow function

Thanks for the patch.
It seems natural that the stats lies in Attack component.
Perhaps someone has another idea? Shouldn't we have another component for that (like Explosion?)

250 ↗(On Diff #1775)

I already ask that but shouldn't we put that earlier?

Mate-86 marked 10 inline comments as done.May 8 2017, 10:49 PM
Mate-86 added a subscriber: vladislavbelov.

Thanks for the comments! I've addressed them. I'm doing some local tests tomorrow and upload a new patch.

76 ↗(On Diff #1738)

This solution was proposed by @vladislavbelov

I can change it of course if needed.

250 ↗(On Diff #1775)

Yes, you asked and I replied in a comment:

fatherbushido added inline comments.May 9 2017, 7:46 AM
12 ↗(On Diff #1775)

Just nitpicking but it can be confusing (it's when the unit lose all its hp not when it's destroyed by the user for example)

250 ↗(On Diff #1775)

That needs to be dug into.
Perhaps before the first if: "if (can this entity do death damage) then (get the needed data)"
And at that place call directly a Damage method (which is a system component).
That's just something to consider.

I still wonder.

  1. It makes sense to have those stats in the Attack component as they used the splash schema.
  2. And so it makes sense to have that function in the Attack component.

Though we could also have another component managing that kind of things "Explosion" for example. (called on death or even another thing).


Mate-86 updated this revision to Diff 1799.May 9 2017, 10:44 PM
Mate-86 marked an inline comment as done.
Mate-86 added inline comments.
12 ↗(On Diff #1775)

The death damage is inflicted even if the user kills the unit (by hitting "Delete"). Original description says: "When a unit is destroyed the destruction causes damage to nearby units." from

Uploaded a new diff fixing most of the issues raised. These ones are still outstanding:

fatherbushido added inline comments.May 10 2017, 7:12 AM
656 ↗(On Diff #1799)

use cmpOwnership for consistency

658 ↗(On Diff #1799)

I guess you kept that for debugging, else you can remove it as anyway an error message will be displayed

659 ↗(On Diff #1799)

And that's where I am annoyed.
As we have already put the entity in the destroy queue, here we could have a invalid owner (owner == -1)

76 ↗(On Diff #1738)

Perhaps, yes, neutrals could be together with allies, the main point from me is don't forget about neutrals.

81 ↗(On Diff #1738)

GetAllPlayers would be good, also gaia and self are included in diplomacy.

@Mate-86 : well, I thought again about that.

Imo, we need a small component called "Explode" containing that CauseDeathDamage function and perhaps the data schema and also a boolean "ExplodeOnDeath".
Indeed that thing could be called on death or even on other circunstances.

Imo, we need a small component called "Explode" containing that CauseDeathDamage function and perhaps the data schema and also a boolean "ExplodeOnDeath".

I'm not sure if Explode is a good name, but the rough idea of having another (quite minimal, thus freeing us of having to check most things, we just require them to be there) component for just that seems nice. The rough handling of is already in cmpDamage which solves issues of lifetime, etc.

True attack has quite a lot of similar code already, but I wouldn't really consider death damage that much of an attack (now if someone says kamikaze that will get complicated).
Then again if the whole thing works even in edge-cases one could easily move it from cmpAttack to something else later on.

(Completely unrelated, but I suspect that there is some way one could abuse SpawnEntityOnDeath to do something similar to this, but that's most likely an ugly hack.)

250 ↗(On Diff #1775)

It is quite unlikely for the entity to actually be gone by that point, however from a logical standpoint doing this first seems better. (One could also do the actual damage code in here, however that then creates some kind of coupling between Health and damge related things.)

224 ↗(On Diff #1799)

Pointless comment (while we're here).

225 ↗(On Diff #1799)

if ( while we're here.

228 ↗(On Diff #1799)

We could use a switch for these.

And do the Engine.DestroyEntity(this.entity); call afterwards.

245 ↗(On Diff #1799)

If the issue hinted at in the below comment we could just move the hitpoints assignment to an earlier place, which shouldn't result in anything, as long as we leave the message sending part down here (someone actually test and verify this, I'm barely guessing here).

Mate-86 edited the summary of this revision. (Show Details)May 12 2017, 9:36 PM
Mate-86 edited the test plan for this revision. (Show Details)

I've updated the description. Please have a look and challenge it so that requirement related concerns can be identified prior to implementation.

In D451#19546, @Mate-86 wrote:

I've updated the description. Please have a look and challenge it so that requirement related concerns can be identified prior to implementation.

I'd name the component DeathDamage instead of Explosion, since someone might want to deal damage that isn't exactly an explosion (eg spilling some chemicals or something).

Mate-86 updated this revision to Diff 2057.May 20 2017, 8:44 PM
Mate-86 marked 8 inline comments as done.
Mate-86 edited the summary of this revision. (Show Details)
Mate-86 added inline comments.
658 ↗(On Diff #1799)

@bb suggested to throw a warning here instead of failing silently.
I can even abort the execution of this function if the owner is not valid anymore.

I did the following changes as instructed:

  • moved up the calling of DeathDamage in the Health component code (plus making a switch)
  • sorted out the diplomacy handling of FriendlyFire (GetAllPlayers functions added)

I did not do in this patch:
Extracting the death damage related code and schema from Attack component because I either duplicate the splash schema or I have to extract it from Attack which touches too many parts of the game (extra regression testing needed). On top of that it's not clear whether the extracted component should be responsible for solely DeathDamage only or should be more general, eg. handling any kind of explosions.

leper added inline comments.May 20 2017, 9:35 PM
658 ↗(On Diff #1799)

If cmpOwnership is null (or equivalent, but it would be null in this case), then we will get a quite obvious error when executing cmpOwnership.GetOwner(). Also I'd be slightly more worried about owner being -1, though not a lot.

75 ↗(On Diff #2057)


235 ↗(On Diff #2057)

That's not how we format switch statements. See

242 ↗(On Diff #2057)

I'd actually add a case "vanish": break; default: error("Invalid deathtype") break; (with a slightly nicer error message) here, to make this a bit more robust against future extensions.

705 ↗(On Diff #2057)

Missing ;, also this seems quite pointless.

Mate-86 updated this revision to Diff 2074.May 21 2017, 9:41 AM
Mate-86 marked 16 inline comments as done.
Mate-86 marked an inline comment as done.May 21 2017, 9:45 AM
Mate-86 added inline comments.
659 ↗(On Diff #1799)

I've extended the above check to throw a warning for GetOwner = -1.
If you dislike it and either want to abort execution at that point or do not throw any warning at all then please advise!

fatherbushido added inline comments.May 23 2017, 3:40 PM
12 ↗(On Diff #2074)

It seems we have not to add that here (it will be use by CanAttack, a function related to UnitAI).
That's the kind of things why it would be better to have that logic out of Attack.

659 ↗(On Diff #2074)

You call 'two times' GetOwner()

As explained above,
No need to add this one: if (!cmpOwnership)
You'll get an error anyway

Then you can do let owner = cmpOwnership.GetOwner();
and here if you want add the if (owner == -1) check with the warn.

Mate-86 edited the summary of this revision. (Show Details)May 31 2017, 8:45 PM
Mate-86 updated this revision to Diff 2510.Jun 10 2017, 3:04 PM

DeathDamage logic extracted to separate component from Attack component.

fatherbushido requested changes to this revision.Aug 3 2017, 4:42 PM

Design is ok and patch is good.
I have issue to apply it: "(different line endings)"
Could you just adress/comment/discuss the last remarks then I will do a final test and commit it.
(I will tweak numbers for balance, but anyway it will need other tweaks - ultimately when the other damage patch would all be done).

(In another diff, I or you or someone else could add a quick simple test for the new component (mainly CauseDeathDamage method). Something obvious but mainly for prevent future errors. (And ideally writting some tests for the Health component)).

71 ↗(On Diff #2510)

I would use number[] for the array for consistency.

81 ↗(On Diff #2510)

I would instead just move the old block from Damage.js here and use the for loop on the number of players instead of that new GetAllPlayers method which has imo nothing to do in the Player component (or I miss some elements).

20 ↗(On Diff #2510)

We don't use it, do we?

686 ↗(On Diff #2510)

Do we really need that one?

704 ↗(On Diff #2510)

Imo remove that

709 ↗(On Diff #2510)

I would remove that

This revision now requires changes to proceed.Aug 3 2017, 4:42 PM
Mate-86 marked 3 inline comments as done.Aug 6 2017, 5:26 PM
Mate-86 added inline comments.
20 ↗(On Diff #2510)

bonusesSchema is used in L37

686 ↗(On Diff #2510)

Nope, removed.

Mate-86 updated this revision to Diff 3017.Aug 6 2017, 5:27 PM
Mate-86 edited edge metadata.
Mate-86 marked an inline comment as done.

It seems you forget to add the new file :-)

fatherbushido added inline comments.Aug 6 2017, 5:48 PM
20 ↗(On Diff #2510)

Sure, but we doesn't use it anywhere in the code, right? So imo you can remove it for now?

Mate-86 added inline comments.Aug 6 2017, 7:32 PM
20 ↗(On Diff #2510)

I haven't found any occurrence of using bonusesSchema in the code although it's present in Attack component too. Should it be removed from there as well?

Mate-86 updated this revision to Diff 3020.Aug 6 2017, 7:36 PM
Mate-86 edited the summary of this revision. (Show Details)
fatherbushido added inline comments.Aug 6 2017, 9:03 PM
20 ↗(On Diff #2510)

It's used via GetAttackBonus for melee capture and ranged units but it seems it's not used (even if it's in the schema) for the splash. It seems it doesn't really make much sense for splash.
For the moment I guess we can just remove it too.

fatherbushido accepted this revision.EditedAug 7 2017, 2:20 PM
  • Code is well thought and extendable.
  • All is well documented.
  • Patch works fine as expected: in game tests ok.
  • Tests ok.
  • I will let the example templates in the patch.
  • I had issues with line endings, I will fix when commiting.
  • There was the interfaces/DamageDeath.js file missing, I will remove when commiting.

As we discussed, the bonus schema for splash (here and in attack) are not used (and imo would be more or less meaningless). I suggest to remove them in another diff.

I or you can open a trac ticket about:

  • tooltip (you set all the ground in your patch) (and/or editing the description of those 2 units)
  • possibly balancing (and perhaps remove the melee-ranged attack of those units, no clue about that)
  • (perhaps a new death animation for those units.)

Also writing a unit test of that specific new component (really small and trivial but mainly for protect future coding error).

This revision is now accepted and ready to land.Aug 7 2017, 2:20 PM
This revision was automatically updated to reflect the committed changes.