Page MenuHomeWildfire Games

Allow instant-kill for attacks. (And use it for "Slaughter"-attack.)
Needs ReviewPublic

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

Details

Reviewers
wraitii
Angen
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

This patch allows an "Infinity"-value for damage types, that immediately kills an entity if a damage type has this value.

Test Plan
  1. Give an entity (1) the value Infinity in any damage-type.
    1. Check that the damage-type value in the GUI is the infinite-sign.
  2. Attack an entity (2) with (1).
    1. See that it is instant-kill.
  3. Upgrade attack of (1) and armour of (2).
  4. Perform same checks as above.

Diff Detail

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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Freagarach added a reviewer: Restricted Owners Package.Jun 11 2019, 6:52 PM
wraitii requested changes to this revision.Jun 12 2019, 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.Jun 12 2019, 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.Jun 13 2019, 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.Jun 13 2019, 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.Jun 13 2019, 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.Jun 13 2019, 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.Jun 13 2019, 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.Jun 13 2019, 9:55 PM
  • <InstantKill>true</InstantKill> --> <InstantKill/>
Freagarach marked an inline comment as done.Jun 13 2019, 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.Jun 13 2019, 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.Jun 13 2019, 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.Jun 13 2019, 10:52 PM
binaries/data/mods/public/simulation/components/Attack.js
116
Freagarach added inline comments.Jun 13 2019, 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.Jun 13 2019, 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.Jun 13 2019, 11:19 PM
Freagarach marked 3 inline comments as done.Jun 14 2019, 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.Jun 14 2019, 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.Jun 15 2019, 11:11 AM
binaries/data/mods/public/simulation/templates/template_unit_cavalry.xml
16

Up to you I guess :)

wraitii added inline comments.Jun 15 2019, 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"..Jun 17 2019, 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.Jun 18 2019, 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.Jun 18 2019, 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)!

Freagarach updated this revision to Diff 8710.Wed, Jul 3, 8:05 PM
Freagarach retitled this revision from Allow instant-kill for attacks and use it for "Slaughter". to Allow instant-kill for attacks and immunity for armour. Use instant-kill for "Slaughter"-attack..
Freagarach edited the summary of this revision. (Show Details)
Freagarach edited the test plan for this revision. (Show Details)
Freagarach added a subscriber: Angen.
  • The value of a damage type can be infinite now (in armour as well as in attack).

@Angen, what to teach AI about this? Just a large number for the damage-strength?

ToDo:

  • Unit tests.
Owners added a subscriber: Restricted Owners Package.Wed, Jul 3, 8:05 PM

ToDo:

  • Unit tests.

DamageReceiver is mocked. Anyone an idea how to test this? Apparently I can't figure the logic of the tests :(

Vulcan added a comment.Wed, Jul 3, 8:55 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
| 525| 525| 
| 526| 526| 		let horizSpeed = +this.template[type].Projectile.Speed;
| 527| 527| 		let gravity = +this.template[type].Projectile.Gravity;
| 528|    |-		//horizSpeed /= 2; gravity /= 2; // slow it down for testing
|    | 528|+		// horizSpeed /= 2; gravity /= 2; // slow it down for testing
| 529| 529| 
| 530| 530| 		let cmpPosition = Engine.QueryInterface(this.entity, IID_Position);
| 531| 531| 		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
| 572| 572| 		// TODO: Use unit rotation to implement x/z offsets.
| 573| 573| 		let deltaLaunchPoint = new Vector3D(0, this.template[type].Projectile.LaunchPoint["@y"], 0.0);
| 574| 574| 		let launchPoint = Vector3D.add(selfPosition, deltaLaunchPoint);
| 575|    |-		
|    | 575|+
| 576| 576| 		let cmpVisual = Engine.QueryInterface(this.entity, IID_Visual);
| 577| 577| 		if (cmpVisual)
| 578| 578| 		{
|    | [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
| 644| 644| 			});
| 645| 645| 	}
| 646| 646| 	else
| 647|    |-	{
|    | 647|+	
| 648| 648| 		// Melee attack - hurt the target immediately
| 649| 649| 		cmpDamage.CauseDamage({
| 650| 650| 			"strengths": this.GetAttackStrengths(type),
| 654| 654| 			"type": type,
| 655| 655| 			"attackerOwner": attackerOwner
| 656| 656| 		});
| 657|    |-	}
|    | 657|+	
| 658| 658| };
| 659| 659| 
| 660| 660| /**

binaries/data/mods/public/simulation/components/Attack.js
| 515| ·»   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
| 619| »   »   cmpTimer.SetTimeout(SYSTEM_ENTITY,·IID_Damage,·"MissileHit",·timeToTarget·*·1000·+·+this.template[type].Delay,·data);
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 392| 392| function getRepairTimeTooltip(entState)
| 393| 393| {
| 394| 394| 	return sprintf(translate("%(label)s %(details)s"), {
| 395|    |-			"label": headerFont(translate("Number of repairers:")),
|    | 395|+		"label": headerFont(translate("Number of repairers:")),
| 396| 396| 			"details": entState.repairable.numBuilders
| 397| 397| 		}) + "\n" + (entState.repairable.numBuilders ?
| 398| 398| 		sprintf(translatePlural(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 393| 393| {
| 394| 394| 	return sprintf(translate("%(label)s %(details)s"), {
| 395| 395| 			"label": headerFont(translate("Number of repairers:")),
| 396|    |-			"details": entState.repairable.numBuilders
|    | 396|+		"details": entState.repairable.numBuilders
| 397| 397| 		}) + "\n" + (entState.repairable.numBuilders ?
| 398| 398| 		sprintf(translatePlural(
| 399| 399| 			"Add another worker to speed up the repairs by %(second)s second.",
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 394| 394| 	return sprintf(translate("%(label)s %(details)s"), {
| 395| 395| 			"label": headerFont(translate("Number of repairers:")),
| 396| 396| 			"details": entState.repairable.numBuilders
| 397|    |-		}) + "\n" + (entState.repairable.numBuilders ?
|    | 397|+	}) + "\n" + (entState.repairable.numBuilders ?
| 398| 398| 		sprintf(translatePlural(
| 399| 399| 			"Add another worker to speed up the repairs by %(second)s second.",
| 400| 400| 			"Add another worker to speed up the repairs by %(second)s seconds.",
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 414| 414| function getBuildTimeTooltip(entState)
| 415| 415| {
| 416| 416| 	return sprintf(translate("%(label)s %(details)s"), {
| 417|    |-			"label": headerFont(translate("Number of builders:")),
|    | 417|+		"label": headerFont(translate("Number of builders:")),
| 418| 418| 			"details": entState.foundation.numBuilders
| 419| 419| 		}) + "\n" + (entState.foundation.numBuilders ?
| 420| 420| 		sprintf(translatePlural(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 415| 415| {
| 416| 416| 	return sprintf(translate("%(label)s %(details)s"), {
| 417| 417| 			"label": headerFont(translate("Number of builders:")),
| 418|    |-			"details": entState.foundation.numBuilders
|    | 418|+		"details": entState.foundation.numBuilders
| 419| 419| 		}) + "\n" + (entState.foundation.numBuilders ?
| 420| 420| 		sprintf(translatePlural(
| 421| 421| 			"Add another worker to speed up the construction by %(second)s second.",
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 416| 416| 	return sprintf(translate("%(label)s %(details)s"), {
| 417| 417| 			"label": headerFont(translate("Number of builders:")),
| 418| 418| 			"details": entState.foundation.numBuilders
| 419|    |-		}) + "\n" + (entState.foundation.numBuilders ?
|    | 419|+	}) + "\n" + (entState.foundation.numBuilders ?
| 420| 420| 		sprintf(translatePlural(
| 421| 421| 			"Add another worker to speed up the construction by %(second)s second.",
| 422| 422| 			"Add another worker to speed up the construction by %(second)s seconds.",
Executing section cli...

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

Angen requested changes to this revision.Thu, Jul 4, 1:24 PM

Generally

  1. GetBestAttackAgainst - has to check attack damages vs armour damages
  2. CanAttack - the same as 1

Reason : unit having only pierce damage can be now send to fight unit with infinit pierce armour

The same logic applies for AI, see AttackPlan.js function update there is sorting of tartgets, needs to be changed to filter out entities one cannot harm at all.

This revision now requires changes to proceed.Thu, Jul 4, 1:24 PM
In D1965#84927, @Angen wrote:

Generally

  1. GetBestAttackAgainst - has to check attack damages vs armour damages
  2. CanAttack - the same as 1

Reason : unit having only pierce damage can be now send to fight unit with infinit pierce armour
The same logic applies for AI, see AttackPlan.js function update there is sorting of tartgets, needs to be changed to filter out entities one cannot harm at all.

To start with: I agree. Pure for casual gamers sake.
Devil's advocate: not harming != not be able to attack, right?

Angen added a comment.Thu, Jul 4, 6:13 PM

not able to harm target === cannot attack target

In D1965#84931, @Angen wrote:

not able to harm target === cannot attack target

While I can hit someone in full armour with my bare hands, (s)he will not receive much harm ;)

Angen added a comment.Thu, Jul 4, 6:51 PM
In D1965#84931, @Angen wrote:

not able to harm target === cannot attack target

While I can hit someone in full armour with my bare hands, (s)he will not receive much harm ;)

In reality yes, but if you allow that in game, units will pick targets they are unable to deal any damage and players will be frustrated and ai broken.

In D1965#84934, @Angen wrote:

In reality yes, but if you allow that in game, units will pick targets they are unable to deal any damage and players will be frustrated and ai broken.

Yeah, that's why I said that I agree with you that it should be in game.

Nescio added a comment.Thu, Jul 4, 7:19 PM

Being able to make e.g. city walls or siege towers completely immune to pierce damage seems far more elegant than merely assigning an arbitrarily high armour value as is currently the case. However, units should never be able to attack entities they can't harm.

Nescio added inline comments.Thu, Jul 4, 8:27 PM
binaries/data/mods/public/simulation/templates/template_unit_cavalry.xml
15–16

By the way, the farm field has <Amount>Infinity</Amount>, not "infinite"; shouldn't the same form be used here for consistency?

In D1965#84927, @Angen wrote:

Generally

  1. GetBestAttackAgainst - has to check attack damages vs armour damages
  2. CanAttack - the same as 1

Reason : unit having only pierce damage can be now send to fight unit with infinit pierce armour
The same logic applies for AI, see AttackPlan.js function update there is sorting of tartgets, needs to be changed to filter out entities one cannot harm at all.

So that I understand: for this diff, you are saying that we need to change GestBestAttackAgainst to not consider attacks that deal only damage that the target is invulnerable against?

If so, @Freagarach , it might be a good idea to move 'infinite armour' to a separate patch, as indeed it has potential for more unitAI breakage than infinite damage.

FeXoR added a subscriber: FeXoR.Thu, Jul 4, 9:50 PM

Huh? If you want some animals to be killed by one hit give them 1 health.

What's going on here? Oo

In D1965#84976, @FeXoR wrote:

Huh? If you want some animals to be killed by one hit give them 1 health.
What's going on here? Oo

This lets modders, combined with D1938, make a unit that can insta-kill anything, for example, as a cheat unit.

FeXoR added a comment.EditedThu, Jul 4, 10:18 PM

Ok, then please use this in the ticket description rather than confusing me poor soul :p

Also not both of this can be true (from current description):
A immediately kills an entity if an attack has this value;
B makes the entity immune to this damage type if the armour has this value.

If Unit X with A attacks unit Y with B (same damage type):
Either A or B (never both).

Freagarach updated this revision to Diff 8731.EditedFri, Jul 5, 7:11 PM
Freagarach marked an inline comment as done.
Freagarach retitled this revision from Allow instant-kill for attacks and immunity for armour. Use instant-kill for "Slaughter"-attack. to Allow instant-kill for attacks. (And use it for "Slaughter"-attack.).
Freagarach edited the summary of this revision. (Show Details)
Freagarach edited the test plan for this revision. (Show Details)
Freagarach updated the Trac tickets for this revision.

Split immunity (armour).

for example, as a cheat unit.

Another example: a special unit to kill heroes (viz. spy in Stratego).

Vulcan added a comment.Fri, Jul 5, 7:14 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
| 525| 525| 
| 526| 526| 		let horizSpeed = +this.template[type].Projectile.Speed;
| 527| 527| 		let gravity = +this.template[type].Projectile.Gravity;
| 528|    |-		//horizSpeed /= 2; gravity /= 2; // slow it down for testing
|    | 528|+		// horizSpeed /= 2; gravity /= 2; // slow it down for testing
| 529| 529| 
| 530| 530| 		let cmpPosition = Engine.QueryInterface(this.entity, IID_Position);
| 531| 531| 		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
| 572| 572| 		// TODO: Use unit rotation to implement x/z offsets.
| 573| 573| 		let deltaLaunchPoint = new Vector3D(0, this.template[type].Projectile.LaunchPoint["@y"], 0.0);
| 574| 574| 		let launchPoint = Vector3D.add(selfPosition, deltaLaunchPoint);
| 575|    |-		
|    | 575|+
| 576| 576| 		let cmpVisual = Engine.QueryInterface(this.entity, IID_Visual);
| 577| 577| 		if (cmpVisual)
| 578| 578| 		{
|    | [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
| 644| 644| 			});
| 645| 645| 	}
| 646| 646| 	else
| 647|    |-	{
|    | 647|+	
| 648| 648| 		// Melee attack - hurt the target immediately
| 649| 649| 		cmpDamage.CauseDamage({
| 650| 650| 			"strengths": this.GetAttackStrengths(type),
| 654| 654| 			"type": type,
| 655| 655| 			"attackerOwner": attackerOwner
| 656| 656| 		});
| 657|    |-	}
|    | 657|+	
| 658| 658| };
| 659| 659| 
| 660| 660| /**

binaries/data/mods/public/simulation/components/Attack.js
| 515| ·»   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
| 619| »   »   cmpTimer.SetTimeout(SYSTEM_ENTITY,·IID_Damage,·"MissileHit",·timeToTarget·*·1000·+·+this.template[type].Delay,·data);
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 392| 392| function getRepairTimeTooltip(entState)
| 393| 393| {
| 394| 394| 	return sprintf(translate("%(label)s %(details)s"), {
| 395|    |-			"label": headerFont(translate("Number of repairers:")),
|    | 395|+		"label": headerFont(translate("Number of repairers:")),
| 396| 396| 			"details": entState.repairable.numBuilders
| 397| 397| 		}) + "\n" + (entState.repairable.numBuilders ?
| 398| 398| 		sprintf(translatePlural(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 393| 393| {
| 394| 394| 	return sprintf(translate("%(label)s %(details)s"), {
| 395| 395| 			"label": headerFont(translate("Number of repairers:")),
| 396|    |-			"details": entState.repairable.numBuilders
|    | 396|+		"details": entState.repairable.numBuilders
| 397| 397| 		}) + "\n" + (entState.repairable.numBuilders ?
| 398| 398| 		sprintf(translatePlural(
| 399| 399| 			"Add another worker to speed up the repairs by %(second)s second.",
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 394| 394| 	return sprintf(translate("%(label)s %(details)s"), {
| 395| 395| 			"label": headerFont(translate("Number of repairers:")),
| 396| 396| 			"details": entState.repairable.numBuilders
| 397|    |-		}) + "\n" + (entState.repairable.numBuilders ?
|    | 397|+	}) + "\n" + (entState.repairable.numBuilders ?
| 398| 398| 		sprintf(translatePlural(
| 399| 399| 			"Add another worker to speed up the repairs by %(second)s second.",
| 400| 400| 			"Add another worker to speed up the repairs by %(second)s seconds.",
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 414| 414| function getBuildTimeTooltip(entState)
| 415| 415| {
| 416| 416| 	return sprintf(translate("%(label)s %(details)s"), {
| 417|    |-			"label": headerFont(translate("Number of builders:")),
|    | 417|+		"label": headerFont(translate("Number of builders:")),
| 418| 418| 			"details": entState.foundation.numBuilders
| 419| 419| 		}) + "\n" + (entState.foundation.numBuilders ?
| 420| 420| 		sprintf(translatePlural(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 415| 415| {
| 416| 416| 	return sprintf(translate("%(label)s %(details)s"), {
| 417| 417| 			"label": headerFont(translate("Number of builders:")),
| 418|    |-			"details": entState.foundation.numBuilders
|    | 418|+		"details": entState.foundation.numBuilders
| 419| 419| 		}) + "\n" + (entState.foundation.numBuilders ?
| 420| 420| 		sprintf(translatePlural(
| 421| 421| 			"Add another worker to speed up the construction by %(second)s second.",
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 416| 416| 	return sprintf(translate("%(label)s %(details)s"), {
| 417| 417| 			"label": headerFont(translate("Number of builders:")),
| 418| 418| 			"details": entState.foundation.numBuilders
| 419|    |-		}) + "\n" + (entState.foundation.numBuilders ?
|    | 419|+	}) + "\n" + (entState.foundation.numBuilders ?
| 420| 420| 		sprintf(translatePlural(
| 421| 421| 			"Add another worker to speed up the construction by %(second)s second.",
| 422| 422| 			"Add another worker to speed up the construction by %(second)s seconds.",
Executing section cli...

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

Angen resigned from this revision.Fri, Jul 5, 7:17 PM

requested changes are not more valid because split

FeXoR added a comment.Sun, Jul 7, 12:21 PM

The fundamental system of damage in 0 A.D. has been suspect to discussions every now and then and I think it's current approach is the result of those.
So it's really something that I consider "working well".
IMO things should use it and not extend it for some edge cases by adding code that has to be maintained for little gain.

So before even considering committing this there should be a serious reason for it like a widely agreed upon wanted feature of 0 A.D. needs it or a widely played mod requests it.

Don't make 0 A.D. become a conceptual mess, plz!

Bunch more of thinking on this: in practice, the only difference between giving a stupidly high number of damage and "infinity" damage is that the GUI will show something odd. Further, what is "stupidly high"? 10000? 100000? For slaughter giving a value of 25 is enough, but that shows the problem: different templates have different values.

It seems a little inelegant to rely on magic numbers for this, and it would help the GUI to have some concept of "this attack kills instantly".
But I also have to agree that it seems rather un-necessary, and it does add some special-casing :\

Maybe someone else feels strongly about it?