HomeWildfire Games

Generalise Attack effects. All attacks, including death damage and splash, can…
Concern RaisedrP22754

Description

Generalise Attack effects. All attacks, including death damage and splash, can deal any number of attack effects (damaging, capture, giving status effects.)

This moves most of what was in the Damage system component to a helper, and renames that component DelayedDamage.
It also introduces a new global script with all possible attack effects.

Comments Taken From: Freagarach, Stan, bb

Differential Revision: https://code.wildfiregames.com/D2092

Event Timeline

elexis raised a concern with this commit.Aug 25 2019, 1:04 AM
elexis added a subscriber: elexis.

A commit before r22766, I guess this one as bisecting took too long, made it so that the production queue (civic center) displays the correct attack stats after researching attack upgrades, but the individual units still display their old value. Or perhaps the units still have their old value and it's displayed correctly.

This commit now has outstanding concerns.Aug 25 2019, 1:04 AM
Freagarach added a subscriber: Freagarach.

I've looked into this: the stats itself do not change also. Both can easily be fixed (see below).
D2224.

/ps/trunk/binaries/data/mods/public/simulation/components/Armour.js
56

The armour is not split in the different attack types (that was not deemed necessary at the time of generalising damages).

/ps/trunk/binaries/data/mods/public/simulation/components/Attack.js
347

This passes just "/Splash" always.
Can be fixed by outlining (is that proper english?) the splash check.
splash = splash ? "/Splash" : "";

wraitii added a comment.EditedAug 25 2019, 8:03 AM

Thanks for reporting, indeed it appears to be this diff.
At a glance two issues:

  • for Attack I forgot that the '?' had such low precedence so it's passing the wrong string - no modifications.
  • for Armour I forgot to change techs as we now change "Armor/Damage/XXX" instead of "Armor/XXX" directly.

Should be fixable tonight.

(edit: as noted by freagarach above)

  • for Armour I forgot to change techs as we now change "Armor/Damage/XXX" instead of "Armor/XXX" directly.

Armour/Damage/XXX is not changed, since the armours in the templates are still Armour/XXX, right?

wraitii added a comment.EditedAug 25 2019, 8:08 AM
  • for Armour I forgot to change techs as we now change "Armor/Damage/XXX" instead of "Armor/XXX" directly.

Armour/Damage/XXX is not changed, since the armours in the templates are still Armour/XXX, right?

Yes. Given that Armour will (soon?) be extended to protect against other effects type, the correct fix imo is to change templates to "Armour/Damage/XXX".

Edit: er, finally understood what you meant. Scrap the above, and change it back to "Armour/XXX" :p

  • for Armour I forgot to change techs as we now change "Armor/Damage/XXX" instead of "Armor/XXX" directly.

Armour/Damage/XXX is not changed, since the armours in the templates are still Armour/XXX, right?

Yes. Given that Armour will (soon?) be extended to protect against other effects type, the correct fix imo is to change templates to "Armour/Damage/XXX".
Edit: er, finally understood what you meant. Scrap the above, and change it back to "Armour/XXX" :p

Well actually it would not be the worst idea to add nodes to the Armour ;) Because we do support a damage type called "Capture" (yeah, strange) in the attack, but not in the armour. But anyone who wants to use that name may either change things her-/himself or choose another name for the damage type, IMHO.

Short-term it should be changed to "Armour/XXX", but then yes:

Well actually it would not be the worst idea to add nodes to the Armour ;)

Tt could be a diff ;)

Because we do support a damage type called "Capture" (yeah, strange) in the attack, but not in the armour.

Tis' not a damage type, actually. Though one could also call a damage type "Capture" if one wanted.

Short-term it should be changed to "Armour/XXX", but then yes:

Well actually it would not be the worst idea to add nodes to the Armour ;)

Tt could be a diff ;)

I hate to say it but in D1950 I proposed that xD

Because we do support a damage type called "Capture" (yeah, strange) in the attack, but not in the armour.

Tis' not a damage type, actually. Though one could also call a damage type "Capture" if one wanted.

Exactly what I said/meant :D Don't worry, it's still early.

I hate to say it but in D1950 I proposed that xD

Yeah, I wanted to be conservative then, but we'll probably have to do it anyways.

wraitii requested verification of this commit.Aug 25 2019, 8:05 PM
This commit now requires verification by auditors.Aug 25 2019, 8:05 PM
elexis removed an auditor: elexis.Aug 25 2019, 8:13 PM

Thanks for the fix and review of the fix.

This commit no longer requires audit.Aug 25 2019, 8:13 PM
Freagarach raised a concern with this commit.Aug 26 2019, 11:52 AM
Freagarach added inline comments.
/ps/trunk/binaries/data/mods/public/simulation/helpers/Attacking.js
239–242

This will early return when the target does not have *one* of IIDs, which means that the message that one is attacked is never sent even though it migh be hurt by any other IID.
Instead it should continue, I guess.

This commit now has outstanding concerns.Aug 26 2019, 11:52 AM
Freagarach added inline comments.Aug 28 2019, 9:28 AM
/ps/trunk/binaries/data/mods/public/maps/random/polar_sea_triggers.js
54

Attacking

bb added a subscriber: bb.Aug 30 2019, 5:03 PM
In D368#91976, @wraitii wrote:

@bb: I've committed rP22754, making "Damage" and "Capture" not based on the attack names, allowing you to drop that from this diff.
I think you'll find my final decision OK with regards to how templates get written.
If not, feel free to discuss this by PM or on a forum thread or through the audit feature or something.

I am actually quite positive about this patch, thanks for the endurance, I think it ended up in better code after all. However the above appears to give me the right to be picky ;)

/ps/trunk/binaries/data/mods/public/globalscripts/AttackEffects.js
2

Object.keys(g_EffectReceiver) and yes JSON. Also maybe use a prototype like we are doing for resources

/ps/trunk/binaries/data/mods/public/globalscripts/Templates.js
201

trailing comma

/ps/trunk/binaries/data/mods/public/simulation/components/Health.js
216

we shouldn't use HP in code, use health instead (meaning we should get healthChanged)

/ps/trunk/binaries/data/mods/public/simulation/helpers/Attacking.js
13

good that this stuff got moved, the duplication started to annoy me, only maybe AttackHelper would be a better name, Attacking sounds like it would be directly linked to the UnitAI state, while it is more directly linked with the attack component imo. Not a real strong opinion though.

In rP22754#37054, @bb wrote:

I am actually quite positive about this patch, thanks for the endurance, I think it ended up in better code after all.

I'm glad to hear that :)

/ps/trunk/binaries/data/mods/public/globalscripts/AttackEffects.js
2

Yes could be done now.

I mostly didn't do it because adding a new effect means adding some code anyways, so it's not that easy to mod in compared to resources, but I guess for extensibility it would still be nice.

/ps/trunk/binaries/data/mods/public/simulation/helpers/Attacking.js
13

Hm, fair. That name didn't really occur to me, I would probably have liked it more.

I'll see if a straight rename is easy enough to do.

Freagarach accepted this commit.Sep 1 2019, 11:52 AM

My concerns were fixed in rP22814.

All concerns with this commit have now been addressed.Sep 1 2019, 11:52 AM
Freagarach added inline comments.Sep 5 2019, 10:26 AM
/ps/trunk/binaries/data/mods/public/simulation/helpers/Attacking.js
237

Will this multiply the bonus with every iteration? E.g. bonus is 3x vs Buildings, the entity has both a capture effect (value 5) and a crush effect (value 4), first iteration: It does 3x5=15 capture, second iteration, it does (3x3)x4=36 crush damage?

wraitii added inline comments.Sep 5 2019, 10:32 AM
/ps/trunk/binaries/data/mods/public/simulation/helpers/Attacking.js
237

Ah, I think you're right, that's broken too then.

Freagarach raised a concern with this commit.Sep 5 2019, 10:34 AM
This commit now has outstanding concerns.Sep 5 2019, 10:34 AM
Freagarach added inline comments.Sep 9 2019, 11:58 AM
/ps/trunk/binaries/data/mods/public/simulation/helpers/Attacking.js
278

This is not true anymore. As in: we can damage an entity without resistance (for it will default to 0).
I do not know what a proper solution would be, but I guess we should query for all effect-taking IIDs?

wraitii added inline comments.Sep 9 2019, 12:02 PM
/ps/trunk/binaries/data/mods/public/simulation/helpers/Attacking.js
278

Mh, sounds like we could just check for all entities, since basically all player-owned entities can take damage right now, and I don't think that function accepts multiple interfaces.
Technically, we could pass IID_Health right now and it would be equivalent.

Freagarach added inline comments.Sep 9 2019, 12:05 PM
/ps/trunk/binaries/data/mods/public/simulation/helpers/Attacking.js
278

Yeah, but probably the point is that e.g. trees are not considered to be hit by a stray missile.
There are plans to remove IID_Health from catafalques right? So that would "break" it (not really for we do not have capturing missiles, but still).

wraitii added inline comments.Sep 9 2019, 1:33 PM
/ps/trunk/binaries/data/mods/public/simulation/helpers/Attacking.js
278

I don't think trees have health.

Freagarach added inline comments.Sep 9 2019, 1:36 PM
/ps/trunk/binaries/data/mods/public/simulation/helpers/Attacking.js
278

Mh, sounds like we could just check for all entities, since basically all player-owned entities can take damage right now, and I don't think that function accepts multiple interfaces.

Yeah, but probably the point is that e.g. trees are not considered to be hit by a stray missile.

Technically, we could pass IID_Health right now and it would be equivalent.

There are plans to remove IID_Health from catafalques right? So that would "break" it (not really for we do not have capturing missiles, but still).

Better? ;)

Multiplication concern is fixed.

EntitiesNearPoint concern not yet.