Page MenuHomeWildfire Games

Unit death damage
ClosedPublic

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

Details

Summary

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">
  <Attack>
    <Melee>
      <Hack>10.0</Hack>
	  ...
    </Melee>
  </Attack>
  <DeathDamage>
    <Shape>Circular</Shape>
    <Range>30</Range>
    <FriendlyFire>true</FriendlyFire>
    <Hack>300.0</Hack>
    <Pierce>300.0</Pierce>
    <Crush>300.0</Crush>
  </DeathDamage>
  <Cost>
    ...
  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

Lint
Lint Skipped
Unit
Unit Tests Skipped

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
  attackComponentTest@simulation/components/tests/test_Attack.js:123:2
  @simulation/components/tests/test_Attack.js:247:1
/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.
binaries/data/mods/public/gui/common/tooltips.js
180

newline before {

binaries/data/mods/public/simulation/components/Attack.js
42–49

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

197

s/then/,

630

We don't need the 3D position so GetPosition2D

632

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

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

direction only required when linear splash, so nuke

643

This should be "Death"

binaries/data/mods/public/simulation/components/Damage.js
71

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

73

IMO the order of the argument should be inverted

76

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

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

binaries/data/mods/public/simulation/components/tests/test_Attack.js
15

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

198

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?)

binaries/data/mods/public/simulation/components/Health.js
250

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.

binaries/data/mods/public/simulation/components/Damage.js
76

This solution was proposed by @vladislavbelov
https://trac.wildfiregames.com/ticket/1910#comment:40

I can change it of course if needed.

binaries/data/mods/public/simulation/components/Health.js
250

Yes, you asked and I replied in a comment: https://trac.wildfiregames.com/ticket/1910#comment:33

fatherbushido added inline comments.May 9 2017, 7:46 AM
binaries/data/mods/public/gui/common/tooltips.js
12

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)

binaries/data/mods/public/simulation/components/Health.js
250

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).

@leper?

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.
binaries/data/mods/public/gui/common/tooltips.js
12

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 https://trac.wildfiregames.com/ticket/1910

Uploaded a new diff fixing most of the issues raised. These ones are still outstanding:
https://code.wildfiregames.com/D451#inline-8878
https://code.wildfiregames.com/D451#inline-8839

fatherbushido added inline comments.May 10 2017, 7:12 AM
binaries/data/mods/public/simulation/components/Attack.js
631

use cmpOwnership for consistency

633

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

634

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)

binaries/data/mods/public/simulation/components/Damage.js
76

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

81

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.)

binaries/data/mods/public/simulation/components/Health.js
224

Pointless comment (while we're here).

225

if ( while we're here.

228

We could use a switch for these.

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

245

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).

250

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.)

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.
binaries/data/mods/public/simulation/components/Attack.js
633

@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
binaries/data/mods/public/simulation/components/Attack.js
633

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.

binaries/data/mods/public/simulation/components/Damage.js
75

cmpPlayer

binaries/data/mods/public/simulation/components/Health.js
242

That's not how we format switch statements. See https://trac.wildfiregames.com/wiki/Coding_Conventions.

243

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.

binaries/data/mods/public/simulation/components/Player.js
689

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.
binaries/data/mods/public/simulation/components/Attack.js
634

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
binaries/data/mods/public/gui/common/tooltips.js
12

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.

binaries/data/mods/public/simulation/components/Attack.js
634

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)).

binaries/data/mods/public/simulation/components/Damage.js
71

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

81

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).

binaries/data/mods/public/simulation/components/DeathDamage.js
20 ↗(On Diff #2510)

We don't use it, do we?

binaries/data/mods/public/simulation/components/Player.js
667

Do we really need that one?

685

Imo remove that

690

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.
binaries/data/mods/public/simulation/components/DeathDamage.js
20 ↗(On Diff #2510)

bonusesSchema is used in L37

binaries/data/mods/public/simulation/components/Player.js
667

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
binaries/data/mods/public/simulation/components/DeathDamage.js
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
binaries/data/mods/public/simulation/components/DeathDamage.js
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
binaries/data/mods/public/simulation/components/DeathDamage.js
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.

Expansions:
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.