Page MenuHomeWildfire Games

Check for "cmpHealth" in the "TakeDamage" function in "Armour.js".
Needs ReviewPublic

Authored by Freagarach on Sun, Jun 30, 8:37 AM.

Details

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

As mentioned in D1938, TakeDamage should check for cmpHealth before trying to subtract Health.

Test Plan

Check that everything works as before.

Diff Detail

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

Event Timeline

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Armour.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Armour.js
|  56|  56| 	let cmpHealth = Engine.QueryInterface(this.entity, IID_Health);
|  57|  57| 	if (cmpHealth)
|  58|  58| 		return cmpHealth.Reduce(total);
|  59|    |-	else
|  60|    |-		return { "killed": false, "change": 0 };
|    |  59|+	return { "killed": false, "change": 0 };
|  61|  60| };
|  62|  61| 
|  63|  62| Armour.prototype.GetArmourStrengths = function()
Executing section cli...

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

Stan added a subscriber: Stan.Sun, Jun 30, 10:22 AM
Stan added inline comments.
binaries/data/mods/public/simulation/components/Armour.js
60

I'm not sure it should work because that would obviously be a bug no ?

Angen added a subscriber: Angen.Sun, Jun 30, 11:06 AM
Angen added inline comments.
binaries/data/mods/public/simulation/components/Armour.js
60

I wonder if it would have sense to have armour but no health, so this could warn or error before return.

I dont see reason why it would be bug.

Stan added inline comments.Sun, Jun 30, 11:27 AM
binaries/data/mods/public/simulation/components/Armour.js
60

Else could be nuked btw. Cause you return.

Well it might give the impression the code is working as expected (no warning) while it isn't

Freagarach updated this revision to Diff 8682.Mon, Jul 1, 9:10 PM
  • Add warning.
  • Removed unnecessary else.
Freagarach marked 3 inline comments as done.Mon, Jul 1, 9:11 PM
Vulcan added a comment.Mon, Jul 1, 9:49 PM

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

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