Page MenuHomeWildfire Games

Do not mark unit as injured when receives 0 damage
Needs RevisionPublic

Authored by Angen on Mon, Jan 28, 9:36 PM.

Details

Reviewers
bb
elexis
wraitii
Summary

Currently if unit is damaged with 0, it is marked as injured.

Test Plan

Agree.

Diff Detail

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

Event Timeline

Angen created this revision.Mon, Jan 28, 9:36 PM
Stan awarded a token.Mon, Jan 28, 9:38 PM
Stan added reviewers: bb, elexis.
Vulcan added a subscriber: Vulcan.Mon, Jan 28, 9:38 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/Health.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Health.js
|  64|  64| 	this.UpdateActor();
|  65|  65| };
|  66|  66| 
|  67|    |-//// Interface functions ////
|    |  67|+// // Interface functions ////
|  68|  68| 
|  69|  69| /**
|  70|  70|  * Returns the current hitpoint value.
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Health.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Health.js
|  96|  96| 
|  97|  97| 	var cmpRangeManager = Engine.QueryInterface(SYSTEM_ENTITY, IID_RangeManager);
|  98|  98| 	if (cmpRangeManager)
|  99|    |-	{
|    |  99|+	
| 100| 100| 		if (this.hitpoints < this.GetMaxHitpoints())
| 101| 101| 			cmpRangeManager.SetEntityFlag(this.entity, "injured", true);
| 102| 102| 		else
| 103| 103| 			cmpRangeManager.SetEntityFlag(this.entity, "injured", false);
| 104|    |-	}
|    | 104|+	
| 105| 105| 
| 106| 106| 	this.RegisterHealthChanged(old);
| 107| 107| };
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '||' should be placed at the end of the line.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Health.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Health.js
| 113| 113| 
| 114| 114| Health.prototype.IsUnhealable = function()
| 115| 115| {
| 116|    |-	return (this.template.Unhealable == "true"
| 117|    |-		|| this.GetHitpoints() <= 0
|    | 116|+	return (this.template.Unhealable == "true" ||
|    | 117|+		this.GetHitpoints() <= 0
| 118| 118| 		|| this.GetHitpoints() >= this.GetMaxHitpoints());
| 119| 119| };
| 120| 120| 
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '||' should be placed at the end of the line.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Health.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Health.js
| 114| 114| Health.prototype.IsUnhealable = function()
| 115| 115| {
| 116| 116| 	return (this.template.Unhealable == "true"
| 117|    |-		|| this.GetHitpoints() <= 0
| 118|    |-		|| this.GetHitpoints() >= this.GetMaxHitpoints());
|    | 117|+		|| this.GetHitpoints() <= 0 ||
|    | 118|+		this.GetHitpoints() >= this.GetMaxHitpoints());
| 119| 119| };
| 120| 120| 
| 121| 121| Health.prototype.GetIdleRegenRate = function()
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required after '{'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Health.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Health.js
| 258| 258| 		cmpFogging.Activate();
| 259| 259| 
| 260| 260| 	if (this.hitpoints == this.GetMaxHitpoints())
| 261|    |-		return {"old": this.hitpoints, "new":this.hitpoints};
|    | 261|+		return { "old": this.hitpoints, "new":this.hitpoints};
| 262| 262| 
| 263| 263| 	// If we're already dead, don't allow resurrection
| 264| 264| 	if (this.hitpoints == 0)
|    | [NORMAL] ESLintBear (key-spacing):
|    | Missing space before value for key 'new'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Health.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Health.js
| 258| 258| 		cmpFogging.Activate();
| 259| 259| 
| 260| 260| 	if (this.hitpoints == this.GetMaxHitpoints())
| 261|    |-		return {"old": this.hitpoints, "new":this.hitpoints};
|    | 261|+		return {"old": this.hitpoints, "new": this.hitpoints};
| 262| 262| 
| 263| 263| 	// If we're already dead, don't allow resurrection
| 264| 264| 	if (this.hitpoints == 0)
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Health.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Health.js
| 258| 258| 		cmpFogging.Activate();
| 259| 259| 
| 260| 260| 	if (this.hitpoints == this.GetMaxHitpoints())
| 261|    |-		return {"old": this.hitpoints, "new":this.hitpoints};
|    | 261|+		return {"old": this.hitpoints, "new":this.hitpoints };
| 262| 262| 
| 263| 263| 	// If we're already dead, don't allow resurrection
| 264| 264| 	if (this.hitpoints == 0)
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Health.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Health.js
| 276| 276| 
| 277| 277| 	this.RegisterHealthChanged(old);
| 278| 278| 
| 279|    |-	return { "old": old, "new": this.hitpoints};
|    | 279|+	return { "old": old, "new": this.hitpoints };
| 280| 280| };
| 281| 281| 
| 282| 282| //// Private functions ////
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Health.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Health.js
| 279| 279| 	return { "old": old, "new": this.hitpoints};
| 280| 280| };
| 281| 281| 
| 282|    |-//// Private functions ////
|    | 282|+// // Private functions ////
| 283| 283| 
| 284| 284| Health.prototype.CreateCorpse = function(leaveResources)
| 285| 285| {

binaries/data/mods/public/simulation/components/Health.js
| 117| »   »   ||·this.GetHitpoints()·<=·0
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/Health.js
| 118| »   »   ||·this.GetHitpoints()·>=·this.GetMaxHitpoints());
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/Health.js
| 171| »   var·cmpTimer·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_Timer);
|    | [NORMAL] JSHintBear:
|    | 'cmpTimer' is already defined.

binaries/data/mods/public/simulation/components/Health.js
| 230| »   »   »   }
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'case'.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1010/

elexis added a comment.EditedTue, Jan 29, 1:21 PM

Was there a reason for the == case?

Agree.

Like subjectively?

Imarok added a subscriber: Imarok.Tue, Jan 29, 3:14 PM
In D1769#71333, @elexis wrote:

Was there a reason for the == case?

Was there since leper implemented Healing in rP11536

Stan added a subscriber: Stan.

I guess it's there not to keep marking the unit as injured every time you get through this. Dunno what's the performance impact, but maybe it's negligible. One could also check for the RangeManager entity flag each time, but that's more lines.

Angen added a comment.EditedTue, Jan 29, 8:14 PM

If unit is marked as injured, healers get it as unit to heal. So if unit with maxed health would be marked as as injured, healers would waist their time for unit that does not need it. I think, can someone think that this is not good change and disagree ?

Also there is actually check in UnitAI if unit is can be healed where is hp == maxHp taken into account, but unit still stays marked as injured.

Stan added a comment.Tue, Jan 29, 8:37 PM
In D1769#71392, @Angen wrote:

If unit is marked as injured, healers get it as unit to heal. So if unit with maxed health would be marked as as injured, healers would waist their time for unit that does not need it. I think, can someone think that this is not good change and disagree ?

So "injured" is only used by healers ? It doesn't set a variant in cmpVisual ? (Just checked, no)

Also there is actually check in UnitAI if unit is can be healed where is hp == maxHp taken into account, but unit still stays marked as injured.

Does the duplication make sense, or is it just UnitAI being weird ? In this case maybe it would be nice to include it in this or another patch.

binaries/data/mods/public/simulation/components/Health.js
100

This is another example it's fixed in D1770, but maybe the checks should be unified.

270

I guess the weirdness above is fixed here. Unit gets an increase, then cancel.

Angen added a comment.Tue, Jan 29, 9:06 PM

In unitAi healer filters possible targets with injured mark using Health.prototype.IsUnhealable function (and another checks) so unit with maxed health is not healed and stays injured and checked in next search if still in range.

wraitii requested changes to this revision.Wed, Jan 30, 8:36 AM
wraitii added a subscriber: wraitii.

Let's just refactor this function entirely instead. See inline comments.

binaries/data/mods/public/simulation/components/Health.js
247

Handle death in a separate function and return early, add a check for "amount > 0" at the very top - no need to do anything on 0 damage - then move the "injured" change below the death-check.

This revision now requires changes to proceed.Wed, Jan 30, 8:36 AM

If we do a big refactoring in the same patch, then the >= 0 > 0 change will be hidden therein. So perhaps better to do that separately?

Commit message can specify that imo. And anybody looking will find this diff and notice what happened.

Stan added a comment.Thu, Jan 31, 3:49 PM

Needs a rebase after D1770

Angen added a comment.Sun, Feb 3, 3:30 PM

@wraitii extend this diff or make another one?