Paths

Table of Contentst

Diffusion 0 A.D. Public Repository rP22754

Generalise Attack effects. All attacks, including death damage and splash, can…
rP22754
Actions

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

Details

Auditors

Freagarach

Committed

wraitii

Aug 22 2019, 8:00 PM

Differential Revision

D2092: Generalise Attack effects - any attack can do damage, capture, inflict status effects, including splash. Also damage bonuses.

Parents

rP22753: Make Promotion.js use the common Transform helper, add resource gatherer and…

Branches

Unknown

Tags

Unknown

Build Status

Buildable 9009
Build 14758: Post-Commit Build	Jenkins

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

wraitii mentioned this in D2218: Status Effects v1 - GUI support and fix following rP22754.Aug 24 2019, 10:40 AM

Freagarach mentioned this in D2000: aura style corrections.Aug 24 2019, 8:44 PM

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

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" : "";`

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)

In rP22754#36830, @wraitii wrote:

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?

In rP22754#36831, @Freagarach wrote:

In rP22754#36830, @wraitii wrote:

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

In rP22754#36832, @wraitii wrote:

In rP22754#36831, @Freagarach wrote:

In rP22754#36830, @wraitii wrote:

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.

In rP22754#36834, @wraitii wrote:

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

In rP22754#36834, @wraitii wrote:

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 mentioned this in rP22786: Fix attack/armor techs not applying after rP22754, and fix auras/techs….Aug 25 2019, 8:04 PM

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

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`

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.

elexis mentioned this in D2235: Further fixes for Generalized Attack D2092/rP22754 : don't early-return but continue if an entity does not have a receiver, and polar sea triggers fix.Aug 31 2019, 2:24 PM

wraitii mentioned this in rP22814: Further fixes for Generalized Attack D2092/rP22754.Sep 1 2019, 9:29 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

wraitii mentioned this in rP22866: Generalise attack tooltips after D2092/rP22754..Sep 7 2019, 4:47 PM

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

wraitii mentioned this in rP22901: GUI support for Status Effects and extend their functionality to all attack….Sep 15 2019, 11:26 AM

wraitii mentioned this in D2322: Fix bonus multiplier issue in rP22754 (multiple attack effects) and consider all entities with Health instead of Resistance.Sep 23 2019, 9:16 AM

wraitii mentioned this in rP23012: Fix bonus multiplier issue in rP22754 (multiple attack effects) and consider….Sep 29 2019, 8:58 AM

Multiplication concern is fixed.

EntitiesNearPoint concern not yet.

wraitii mentioned this in D2738: Fix EntitiesNearPoint range query..May 17 2020, 10:40 AM

wraitii mentioned this in rP23680: Fix EntitiesNearPoint range query.May 20 2020, 7:04 PM

wraitii requested verification of this commit.May 20 2020, 7:08 PM

wraitii marked 2 inline comments as done.

This commit now requires verification by auditors.May 20 2020, 7:08 PM

All entities are now considered, which is an improvement and enough to lift my concern :)

All concerns with this commit have now been addressed.May 21 2020, 3:41 PM

wraitii mentioned this in D2758: Do not send Attacked message on entities that have no attack effects receiver.May 22 2020, 1:11 PM

wraitii mentioned this in rP23698: Do not send Attacked message on entities that have no attack effects receiver.May 25 2020, 9:15 PM

Freagarach added inline comments.Jun 5 2020, 8:48 AM

/ps/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
285	Ought to be `IID_Health`.

Freagarach mentioned this in D2787: Use IID_Health in test_Damage after rP22754..Jun 5 2020, 9:15 AM

wraitii mentioned this in rP23738: Use IID_Health in test_Damage after rP22754..Jun 5 2020, 7:14 PM

Freagarach raised a concern with this commit.Jun 21 2020, 8:39 PM

Freagarach added inline comments.

/ps/trunk/binaries/data/mods/public/simulation/components/UnitAI.js
3469	Shouldn't be limited to `IID_Resistance`.

This commit now has outstanding concerns.Jun 21 2020, 8:39 PM

Freagarach mentioned this in D2824: Split vision range from going-to-attack range in UnitAI..Jun 21 2020, 8:44 PM

Freagarach mentioned this in D2831: Remove redundant bonus schema from DeathDamage.js..Jun 22 2020, 7:25 PM

bb mentioned this in rP23785: Remove unused bonuses schema from deathDamage.Jun 22 2020, 10:12 PM

wraitii added inline comments.Jul 17 2020, 12:22 PM

/ps/trunk/binaries/data/mods/public/simulation/components/UnitAI.js
3469	@Freagarach Up to you but I don't think this is worth of a 'concern', rather a ticket/diff. It's not really a bug with this diff. If anything, you could raise the concern on the original diff ;)

Freagarach added inline comments.Jul 17 2020, 12:42 PM

/ps/trunk/binaries/data/mods/public/simulation/components/UnitAI.js
3469	I don't fully agree. Prior to this commit, to be able to damage an entity one needed the IID_DamageReceiver, thus the query was correct. After this commit IID_Resistance is not needed to be able to damage an entity, thus it is not correct anymore.