Page MenuHomeWildfire Games

Allow instant-kill for attacks and use it for "Slaughter".
Needs RevisionPublic

Authored by Freagarach on Mon, Jun 10, 7:53 AM.

Details

Reviewers
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Trac Tickets
#1652
Summary

When slaughtering a domestic animal it should be insta-kill.
It was changed from instakill in rP12792 (female citizens) and rP13400 (cavalry and infantry) but I do not know why. If anyone has an idea on this, please enlighten me.

This patch allows an "instantKill"-parameter for attacks that immediately kills an entity.

Test Plan

Kill a domesticated animal. Most visible effect is when killing a sheep with a female citizen.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7923
Build 12894: Vulcan BuildJenkins
Build 12893: arc lint + arc unit

Event Timeline

Freagarach created this revision.Mon, Jun 10, 7:53 AM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 510| 510| 
| 511| 511| 		let horizSpeed = +this.template[type].Projectile.Speed;
| 512| 512| 		let gravity = +this.template[type].Projectile.Gravity;
| 513|    |-		//horizSpeed /= 2; gravity /= 2; // slow it down for testing
|    | 513|+		// horizSpeed /= 2; gravity /= 2; // slow it down for testing
| 514| 514| 
| 515| 515| 		let cmpPosition = Engine.QueryInterface(this.entity, IID_Position);
| 516| 516| 		if (!cmpPosition || !cmpPosition.IsInWorld())
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 557| 557| 		// TODO: Use unit rotation to implement x/z offsets.
| 558| 558| 		let deltaLaunchPoint = new Vector3D(0, this.template[type].Projectile.LaunchPoint["@y"], 0.0);
| 559| 559| 		let launchPoint = Vector3D.add(selfPosition, deltaLaunchPoint);
| 560|    |-		
|    | 560|+
| 561| 561| 		let cmpVisual = Engine.QueryInterface(this.entity, IID_Visual);
| 562| 562| 		if (cmpVisual)
| 563| 563| 		{
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 629| 629| 			});
| 630| 630| 	}
| 631| 631| 	else if (type == "Melee")
| 632|    |-	{
|    | 632|+	
| 633| 633| 		// Melee attack - hurt the target immediately.
| 634| 634| 		cmpDamage.CauseDamage({
| 635| 635| 			"strengths": this.GetAttackStrengths(type),
| 639| 639| 			"type": type,
| 640| 640| 			"attackerOwner": attackerOwner
| 641| 641| 		});
| 642|    |-	}
|    | 642|+	
| 643| 643| 	else if (type == "Slaughter")
| 644| 644| 	{
| 645| 645| 		// Special attack to instantly kill domestic animals.

binaries/data/mods/public/simulation/components/Attack.js
| 500| ·»   let·cmpDamage·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_Damage);
|    | [NORMAL] ESLintBear (no-mixed-spaces-and-tabs):
|    | Mixed spaces and tabs.

binaries/data/mods/public/simulation/components/Attack.js
| 604| »   »   cmpTimer.SetTimeout(SYSTEM_ENTITY,·IID_Damage,·"MissileHit",·timeToTarget·*·1000·+·+this.template[type].Delay,·data);
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1675/display/redirect

Freagarach added a subscriber: Restricted Owners Package.Mon, Jun 10, 6:42 PM
Freagarach retitled this revision from Let slaughter actually be slaughter. to Let slaughter actually be slaughter (i.e. insta-kill)..Tue, Jun 11, 6:52 PM
Freagarach edited the summary of this revision. (Show Details)
Freagarach added a reviewer: Restricted Owners Package.
wraitii requested changes to this revision.Wed, Jun 12, 10:10 PM
wraitii added a subscriber: wraitii.

Don't make "Slaughter" a special case. Instead, make the template accept an <InstantKill/> parameter which gives this property to any attack.

The attack names will be made generic in the future, so it's better to not hardcode anything.

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

Should probably remove the CauseDamage then ;)

This revision now requires changes to proceed.Wed, Jun 12, 10:10 PM

Don't make "Slaughter" a special case. Instead, make the template accept an <InstantKill/> parameter which gives this property to any attack.
The attack names will be made generic in the future, so it's better to not hardcode anything.

Will do, but isn't capture a special case as well?

Stan added a subscriber: Stan.Thu, Jun 13, 5:24 PM

Yeah capture is a particularly annoying case.

Will do, but isn't capture a special case as well?

Capture isn't an attack, per se, so yes, but no :p.

(Honestly, I'm sometimes wondering if we shouldn't remove capturing entirely given how it's implemented right now.)

Capture isn't an attack, per se, so yes, but no :p.

Well Slaughter follows that rule as well xD

(Honestly, I'm sometimes wondering if we shouldn't remove capturing entirely given how it's implemented right now.)

Well, I hope it stays at "wondering" I think it is an awesome feature :D May I ask what your problem with it is?

Stan added a comment.Thu, Jun 13, 5:56 PM

Not easy to disable, confusing as people expect to attack and not to capture by default :) But yeah I'm not for removing it either.

Well Slaughter follows that rule as well xD

It's still affecting HP, whereas capture doesn't.

Well, I hope it stays at "wondering" I think it is an awesome feature :D May I ask what your problem with it is?

I think it's too similar to attacking, gameplay-wise, to be really that interesting. I also dislike that it kinda leads to "garrison all your buildings" instead of actually fighting with units, which I find kind of boring.
I'm not saying that capturing as a concept is bad, but I think our implementation (gameplay wise) could have been better. Lessons learned, I guess.
For example I've read about a concept for AoE 2 where buildings below 25% HP would become neutral and capturable, just by repairing them above that 25%. I think that's much more interesting.

In D1965#82176, @Stan wrote:

(...) confusing as people expect to attack and not to capture by default :) (...)

Well, one could create an option for that can we?

It's still affecting HP, whereas capture doesn't

True that.

I think it's too similar to attacking, gameplay-wise, to be really that interesting. I also dislike that it kinda leads to "garrison all your buildings" instead of actually fighting with units, which I find kind of boring.
I'm not saying that capturing as a concept is bad, but I think our implementation (gameplay wise) could have been better. Lessons learned, I guess.
For example I've read about a concept for AoE 2 where buildings below 25% HP would become neutral and capturable, just by repairing them above that 25%. I think that's much more interesting.

Needs some more discussion and ideas I guess ;) It is indeed an interesting concept but now this is partly the case, because buildings are more easily captured with less hitpoints ;)

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

Hence the question in the summary ;)

Freagarach updated this revision to Diff 8455.Thu, Jun 13, 8:58 PM
Freagarach marked an inline comment as done.
Freagarach edited the summary of this revision. (Show Details)

Moved to a more generic position.
This could be made into a choice when D1950 lands?

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 515| 515| 
| 516| 516| 		let horizSpeed = +this.template[type].Projectile.Speed;
| 517| 517| 		let gravity = +this.template[type].Projectile.Gravity;
| 518|    |-		//horizSpeed /= 2; gravity /= 2; // slow it down for testing
|    | 518|+		// horizSpeed /= 2; gravity /= 2; // slow it down for testing
| 519| 519| 
| 520| 520| 		let cmpPosition = Engine.QueryInterface(this.entity, IID_Position);
| 521| 521| 		if (!cmpPosition || !cmpPosition.IsInWorld())
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 562| 562| 		// TODO: Use unit rotation to implement x/z offsets.
| 563| 563| 		let deltaLaunchPoint = new Vector3D(0, this.template[type].Projectile.LaunchPoint["@y"], 0.0);
| 564| 564| 		let launchPoint = Vector3D.add(selfPosition, deltaLaunchPoint);
| 565|    |-		
|    | 565|+
| 566| 566| 		let cmpVisual = Engine.QueryInterface(this.entity, IID_Visual);
| 567| 567| 		if (cmpVisual)
| 568| 568| 		{
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'else'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 634| 634| 			});
| 635| 635| 	}
| 636| 636| 	else
| 637|    |-	{
|    | 637|+	
| 638| 638| 		// Melee attack - hurt the target immediately
| 639| 639| 		cmpDamage.CauseDamage({
| 640| 640| 			"strengths": this.GetAttackStrengths(type),
| 644| 644| 			"type": type,
| 645| 645| 			"attackerOwner": attackerOwner
| 646| 646| 		});
| 647|    |-	}
|    | 647|+	
| 648| 648| };
| 649| 649| 
| 650| 650| /**

binaries/data/mods/public/simulation/components/Attack.js
| 505| ·»   let·cmpDamage·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_Damage);
|    | [NORMAL] ESLintBear (no-mixed-spaces-and-tabs):
|    | Mixed spaces and tabs.

binaries/data/mods/public/simulation/components/Attack.js
| 609| »   »   cmpTimer.SetTimeout(SYSTEM_ENTITY,·IID_Damage,·"MissileHit",·timeToTarget·*·1000·+·+this.template[type].Delay,·data);
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1704/display/redirect

Stan added inline comments.Thu, Jun 13, 9:06 PM
binaries/data/mods/public/simulation/components/Attack.js
116

I believe you can do <instantkill/>

With

<element><empty/></element>

Freagarach added inline comments.Thu, Jun 13, 9:43 PM
binaries/data/mods/public/simulation/components/Attack.js
116

But how to check for that then? Because then it has a 'value' of void (

({InstantKill:(void 0), MaxRange:"2"})

) so every check I try returns not what is expected :(

  • template.InstantKill
  • !!template.InstantKill
Freagarach updated this revision to Diff 8460.Thu, Jun 13, 9:55 PM
  • <InstantKill>true</InstantKill> --> <InstantKill/>
Freagarach marked an inline comment as done.Thu, Jun 13, 9:56 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 515| 515| 
| 516| 516| 		let horizSpeed = +this.template[type].Projectile.Speed;
| 517| 517| 		let gravity = +this.template[type].Projectile.Gravity;
| 518|    |-		//horizSpeed /= 2; gravity /= 2; // slow it down for testing
|    | 518|+		// horizSpeed /= 2; gravity /= 2; // slow it down for testing
| 519| 519| 
| 520| 520| 		let cmpPosition = Engine.QueryInterface(this.entity, IID_Position);
| 521| 521| 		if (!cmpPosition || !cmpPosition.IsInWorld())
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 562| 562| 		// TODO: Use unit rotation to implement x/z offsets.
| 563| 563| 		let deltaLaunchPoint = new Vector3D(0, this.template[type].Projectile.LaunchPoint["@y"], 0.0);
| 564| 564| 		let launchPoint = Vector3D.add(selfPosition, deltaLaunchPoint);
| 565|    |-		
|    | 565|+
| 566| 566| 		let cmpVisual = Engine.QueryInterface(this.entity, IID_Visual);
| 567| 567| 		if (cmpVisual)
| 568| 568| 		{
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'else'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 634| 634| 			});
| 635| 635| 	}
| 636| 636| 	else
| 637|    |-	{
|    | 637|+	
| 638| 638| 		// Melee attack - hurt the target immediately
| 639| 639| 		cmpDamage.CauseDamage({
| 640| 640| 			"strengths": this.GetAttackStrengths(type),
| 644| 644| 			"type": type,
| 645| 645| 			"attackerOwner": attackerOwner
| 646| 646| 		});
| 647|    |-	}
|    | 647|+	
| 648| 648| };
| 649| 649| 
| 650| 650| /**

binaries/data/mods/public/simulation/components/Attack.js
| 505| ·»   let·cmpDamage·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_Damage);
|    | [NORMAL] ESLintBear (no-mixed-spaces-and-tabs):
|    | Mixed spaces and tabs.

binaries/data/mods/public/simulation/components/Attack.js
| 609| »   »   cmpTimer.SetTimeout(SYSTEM_ENTITY,·IID_Damage,·"MissileHit",·timeToTarget·*·1000·+·+this.template[type].Delay,·data);
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1706/display/redirect

Stan added inline comments.Thu, Jun 13, 10:14 PM
binaries/data/mods/public/simulation/components/Attack.js
447

Looks like you figured it out 🗡
The only way I found was to check for !== undefined

447
Nescio added a subscriber: Nescio.Thu, Jun 13, 10:28 PM
Nescio added inline comments.
binaries/data/mods/public/simulation/templates/template_unit_cavalry.xml
16

Shouldn't it have a
<RestrictedClasses datatype="tokens">!Domestic</RestrictedClasses>
? Cf. template_unit_ship_fishing.xml

Stan added inline comments.Thu, Jun 13, 10:52 PM
binaries/data/mods/public/simulation/components/Attack.js
116
Freagarach added inline comments.Thu, Jun 13, 11:04 PM
binaries/data/mods/public/simulation/components/Attack.js
116

Yep, seems better; I'll try that tomorrow.

binaries/data/mods/public/simulation/templates/template_unit_cavalry.xml
16

Probably, but out of scope?

Stan added inline comments.Thu, Jun 13, 11:19 PM
binaries/data/mods/public/simulation/templates/template_unit_cavalry.xml
16

Actually I'm not sure, cause you can use the cavalry to gather from sheeps as well.

Stan awarded a token.Thu, Jun 13, 11:19 PM
Freagarach marked 3 inline comments as done.Fri, Jun 14, 8:13 PM
Freagarach added inline comments.
binaries/data/mods/public/simulation/templates/template_unit_cavalry.xml
16

Now it is defined in "Attack.js" at "GetBestAttackAgainst" that, if the target has class domestic the entity wil always use Slaughter.
So in theory one would be able to use Slaughter against ordinary units if no other attack is specified. But there is no option to use Slaughter on a unit when the target has not the domestic class. (Just tested this.) Comes from "Attack.js" "CanAttack" I guess.
So yeah, I think it should be defined here and removed from other functions. Is it in-scope to do that here? Then I can just rename the diff to something like: "Cleanup of Slaughter."

Freagarach updated this revision to Diff 8465.Fri, Jun 14, 8:16 PM
if (Object.keys(template).indexOf("InstantKill") != -1)

-->

if ("InstantKill" in template)

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 515| 515| 
| 516| 516| 		let horizSpeed = +this.template[type].Projectile.Speed;
| 517| 517| 		let gravity = +this.template[type].Projectile.Gravity;
| 518|    |-		//horizSpeed /= 2; gravity /= 2; // slow it down for testing
|    | 518|+		// horizSpeed /= 2; gravity /= 2; // slow it down for testing
| 519| 519| 
| 520| 520| 		let cmpPosition = Engine.QueryInterface(this.entity, IID_Position);
| 521| 521| 		if (!cmpPosition || !cmpPosition.IsInWorld())
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 562| 562| 		// TODO: Use unit rotation to implement x/z offsets.
| 563| 563| 		let deltaLaunchPoint = new Vector3D(0, this.template[type].Projectile.LaunchPoint["@y"], 0.0);
| 564| 564| 		let launchPoint = Vector3D.add(selfPosition, deltaLaunchPoint);
| 565|    |-		
|    | 565|+
| 566| 566| 		let cmpVisual = Engine.QueryInterface(this.entity, IID_Visual);
| 567| 567| 		if (cmpVisual)
| 568| 568| 		{
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'else'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 634| 634| 			});
| 635| 635| 	}
| 636| 636| 	else
| 637|    |-	{
|    | 637|+	
| 638| 638| 		// Melee attack - hurt the target immediately
| 639| 639| 		cmpDamage.CauseDamage({
| 640| 640| 			"strengths": this.GetAttackStrengths(type),
| 644| 644| 			"type": type,
| 645| 645| 			"attackerOwner": attackerOwner
| 646| 646| 		});
| 647|    |-	}
|    | 647|+	
| 648| 648| };
| 649| 649| 
| 650| 650| /**

binaries/data/mods/public/simulation/components/Attack.js
| 505| ·»   let·cmpDamage·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_Damage);
|    | [NORMAL] ESLintBear (no-mixed-spaces-and-tabs):
|    | Mixed spaces and tabs.

binaries/data/mods/public/simulation/components/Attack.js
| 609| »   »   cmpTimer.SetTimeout(SYSTEM_ENTITY,·IID_Damage,·"MissileHit",·timeToTarget·*·1000·+·+this.template[type].Delay,·data);
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1711/display/redirect

Stan added inline comments.Sat, Jun 15, 11:11 AM
binaries/data/mods/public/simulation/templates/template_unit_cavalry.xml
16

Up to you I guess :)

wraitii added inline comments.Sat, Jun 15, 2:56 PM
binaries/data/mods/public/simulation/templates/template_unit_cavalry.xml
16

I would say "no" given that this is more about "add instant-kill option to attacks" nowadays.

Freagarach retitled this revision from Let slaughter actually be slaughter (i.e. insta-kill). to Allow instant-kill for attacks and use it for "Slaughter"..Mon, Jun 17, 9:08 PM
Freagarach edited the summary of this revision. (Show Details)
Freagarach marked 4 inline comments as done.
wraitii requested changes to this revision.Tue, Jun 18, 2:33 PM

In light of D1938, I think instead this should be an attribute to existing damage types.

So you would have:

<Slaughter>
  <Damage>
    <Hack instantKill="true"/>
  </Damage>
</Slaughter>

And then in attack strengths you pass { "Hack": "InstantKill" }, and in Armour you handle it similarly.
This would allow enabling special resistances in the future - a unit invulnerable to some damage type shouldn't be killed even if the attack is "instant-kill".

This revision now requires changes to proceed.Tue, Jun 18, 2:33 PM

This would allow enabling special resistances in the future - a unit invulnerable to some damage type shouldn't be killed even if the attack is "instant-kill".

Sounds great (e.g. structures immune to pierce damage)!