Page MenuHomeWildfire Games

Support friendly fire for projectile attacks.
ClosedPublic

Authored by Freagarach on Jun 15 2019, 8:32 AM.

Details

Reviewers
wraitii
Angen
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP23519: Support friendly fire for projectile attacks.
Summary

This allows friendly fire for ranged attacks.

The now changed function was added in rP11886, where it implicitly assumed friendly fire was false. In rP14231 that was made a bit more explicit.

Test Plan

Set a unit's FF boolean to true and see that as it misses a target it can damage your own units. (Easier testable with D1971.)

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Angen added a subscriber: Angen.Jun 15 2019, 10:55 AM
Angen added inline comments.
binaries/data/mods/public/globalscripts/Templates.js
204 ↗(On Diff #8469)

this is not true anymore, so you or have to give friendlyfire = true to every ranged attack with splash damage in template currently without friendlyFire element or keep this logic, what I do not suggest because current logic if not in template = true is at least strange

205 ↗(On Diff #8469)

this information is redundant and I highly suggest to remove it as it is the same as information at line 199, or if that would cause problems for dealing damage please reuse ret.attack[type].friendlyFire here

Stan added a subscriber: Stan.Jun 15 2019, 11:20 AM
Stan added inline comments.
binaries/data/mods/public/simulation/components/Attack.js
165 ↗(On Diff #8469)

you should use &quote; instead of "'" in the help sentence :)

599 ↗(On Diff #8469)

this will return true, and "false", I guess it should return boolean always ;)

wraitii requested changes to this revision.Jun 15 2019, 11:42 AM
wraitii added a subscriber: wraitii.

Design-wise:

  • Should FriendlyFire be a boolean of <FriendlyFire/> ? Making it a boolean seems OK to me. I'm not sure what is most consistent with the rest of the codebase
  • Should it be added to the attack or to the projectile?
  • Is there any case to make for an attack that would have friendly fire when hitting directly, but no friendly fire from splash damage? I think some magic/SF attacks might want something like that...

On (2), I don't think this should be added to the Ranged attack root. Rather, it should be added to the projectile - because friendly fire for non-ranged attacks makes no sense, and "ranged" attacks only real difference is the presence of a projectile. See rP20676 and rP22184. If you add it to the projectile, the splash damage definition can keep its FriendlyFire attribute, since Melee attacks dealing splash damage with or without friendly fire makes sense. In fact, the splash from ranged attack should probably be moved under projectile but that's out of scope.

binaries/data/mods/public/globalscripts/Templates.js
199 ↗(On Diff #8469)

If you make it a non-optional element, then you can do template.Attack[type].FriendlyFire == "false", which is nicer.

204 ↗(On Diff #8469)

We enable Friendly Fire by default - I think we should make it explicit indeed.

This revision now requires changes to proceed.Jun 15 2019, 11:42 AM
Stan added a comment.Jun 15 2019, 12:00 PM

On (2), I don't think this should be added to the Ranged attack root. Rather, it should be added to the projectile - because friendly fire for non-ranged attacks makes no sense, and "ranged" attacks only real difference is the presence of a projectile. See rP20676 and rP22184. If you add it to the projectile, the splash damage definition can keep its FriendlyFire attribute, since Melee attacks dealing splash damage with or without friendly fire makes sense. In fact, the splash from ranged attack should probably be moved under projectile but that's out of scope.

What about a unit swinging a spear, or is that splash damage ?

In D1973#82335, @Stan wrote:

On (2), I don't think this should be added to the Ranged attack root. Rather, it should be added to the projectile - because friendly fire for non-ranged attacks makes no sense, and "ranged" attacks only real difference is the presence of a projectile. See rP20676 and rP22184. If you add it to the projectile, the splash damage definition can keep its FriendlyFire attribute, since Melee attacks dealing splash damage with or without friendly fire makes sense. In fact, the splash from ranged attack should probably be moved under projectile but that's out of scope.

What about a unit swinging a spear, or is that splash damage ?

That's melee splash damage - which we support perfectly, and which should be able to do friendly fire or not (think of a Sci-Fi melee sword that only kills enemies or something).

Freagarach updated this revision to Diff 8562.Jun 19 2019, 7:23 PM
Freagarach marked 6 inline comments as done.
Freagarach edited the summary of this revision. (Show Details)
  • Created FF under "Projectile".
    • Updated templates accordingly.
  • Refactored splash data.
    • Updated tests accordingly.

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '||' should be placed at the end of the line.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Templates.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Templates.js
|  87|  87| 		// If the elements are still strings, split them by space or by '+'
|  88|  88| 		if (typeof sublist == "string")
|  89|  89| 			sublist = sublist.split(/[+\s]+/);
|  90|    |-		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1)
|  91|    |-		                    || (c[0] != "!" && classes.indexOf(c) != -1)))
|    |  90|+		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1) ||
|    |  91|+		                    (c[0] != "!" && classes.indexOf(c) != -1)))
|  92|  92| 			return true;
|  93|  93| 	}
|  94|  94| 

binaries/data/mods/public/globalscripts/Templates.js
|  91| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.
|    | [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
| 526| 526| 
| 527| 527| 		let horizSpeed = +this.template[type].Projectile.Speed;
| 528| 528| 		let gravity = +this.template[type].Projectile.Gravity;
| 529|    |-		//horizSpeed /= 2; gravity /= 2; // slow it down for testing
|    | 529|+		// horizSpeed /= 2; gravity /= 2; // slow it down for testing
| 530| 530| 
| 531| 531| 		let cmpPosition = Engine.QueryInterface(this.entity, IID_Position);
| 532| 532| 		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
| 573| 573| 		// TODO: Use unit rotation to implement x/z offsets.
| 574| 574| 		let deltaLaunchPoint = new Vector3D(0, this.template[type].Projectile.LaunchPoint["@y"], 0.0);
| 575| 575| 		let launchPoint = Vector3D.add(selfPosition, deltaLaunchPoint);
| 576|    |-		
|    | 576|+
| 577| 577| 		let cmpVisual = Engine.QueryInterface(this.entity, IID_Visual);
| 578| 578| 		if (cmpVisual)
| 579| 579| 		{
|    | [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
| 610| 610| 			"statusEffects": this.template[type].StatusEffects
| 611| 611| 		};
| 612| 612| 		if (this.template[type].Splash)
| 613|    |-		{
|    | 613|+		
| 614| 614| 			data.splash = {
| 615| 615| 				"friendlyFire": this.template[type].Splash.FriendlyFire != "false",
| 616| 616| 				"radius": +this.template[type].Splash.Range,
| 618| 618| 				"strengths": this.GetAttackStrengths(type + ".Splash"),
| 619| 619| 				"bonus": this.GetBonusTemplate(type + ".Splash")
| 620| 620| 			};
| 621|    |-		}
|    | 621|+		
| 622| 622| 		cmpTimer.SetTimeout(SYSTEM_ENTITY, IID_Damage, "MissileHit", timeToTarget * 1000 + +this.template[type].Delay, data);
| 623| 623| 	}
| 624| 624| 	else if (type == "Capture")
|    | [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
| 647| 647| 			});
| 648| 648| 	}
| 649| 649| 	else
| 650|    |-	{
|    | 650|+	
| 651| 651| 		// Melee attack - hurt the target immediately
| 652| 652| 		cmpDamage.CauseDamage({
| 653| 653| 			"strengths": this.GetAttackStrengths(type),
| 657| 657| 			"type": type,
| 658| 658| 			"attackerOwner": attackerOwner
| 659| 659| 		});
| 660|    |-	}
|    | 660|+	
| 661| 661| };
| 662| 662| 
| 663| 663| /**

binaries/data/mods/public/simulation/components/Attack.js
| 516| ·»   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
| 622| »   »   cmpTimer.SetTimeout(SYSTEM_ENTITY,·IID_Damage,·"MissileHit",·timeToTarget·*·1000·+·+this.template[type].Delay,·data);
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.
|    | [NORMAL] ESLintBear (quote-props):
|    | Unquoted property 'turnLength' found.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|  25|  25| 
|  26|  26| 	let cmpDamage = ConstructComponent(SYSTEM_ENTITY, "Damage");
|  27|  27| 	let cmpTimer = ConstructComponent(SYSTEM_ENTITY, "Timer");
|  28|    |-	cmpTimer.OnUpdate({ turnLength: 1 });
|    |  28|+	cmpTimer.OnUpdate({ "turnLength": 1 });
|  29|  29| 	let attacker = 11;
|  30|  30| 	let atkPlayerEntity = 1;
|  31|  31| 	let attackerOwner = 6;
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|  63|  63| 		"attackerOwner": attackerOwner,
|  64|  64| 		"position": targetPos,
|  65|  65| 		"projectileId": 9,
|  66|    |-		"direction": new Vector3D(1,0,0)
|    |  66|+		"direction": new Vector3D(1, 0,0)
|  67|  67| 	};
|  68|  68| 
|  69|  69| 	AddMock(atkPlayerEntity, IID_Player, {
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|  63|  63| 		"attackerOwner": attackerOwner,
|  64|  64| 		"position": targetPos,
|  65|  65| 		"projectileId": 9,
|  66|    |-		"direction": new Vector3D(1,0,0)
|    |  66|+		"direction": new Vector3D(1,0, 0)
|  67|  67| 	};
|  68|  68| 
|  69|  69| 	AddMock(atkPlayerEntity, IID_Player, {
|    | [NORMAL] ESLintBear (quote-props):
|    | Unquoted property 'turnLength' found.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 114| 114| 
| 115| 115| 	function TestDamage()
| 116| 116| 	{
| 117|    |-		cmpTimer.OnUpdate({ turnLength: 1 });
|    | 117|+		cmpTimer.OnUpdate({ "turnLength": 1 });
| 118| 118| 		TS_ASSERT(damageTaken);
| 119| 119| 		damageTaken = false;
| 120| 120| 	}
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'hack'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 156| 156| 		"origin": origin,
| 157| 157| 		"radius": 10,
| 158| 158| 		"shape": "Linear",
| 159|    |-		"strengths": { "hack" : 100, "pierce" : 0, "crush": 0 },
|    | 159|+		"strengths": { "hack": 100, "pierce" : 0, "crush": 0 },
| 160| 160| 		"direction": new Vector3D(1, 747, 0),
| 161| 161| 		"playersToDamage": [2],
| 162| 162| 		"type": "Ranged",
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'pierce'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 156| 156| 		"origin": origin,
| 157| 157| 		"radius": 10,
| 158| 158| 		"shape": "Linear",
| 159|    |-		"strengths": { "hack" : 100, "pierce" : 0, "crush": 0 },
|    | 159|+		"strengths": { "hack" : 100, "pierce": 0, "crush": 0 },
| 160| 160| 		"direction": new Vector3D(1, 747, 0),
| 161| 161| 		"playersToDamage": [2],
| 162| 162| 		"type": "Ranged",
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 163| 163| 		"attackerOwner": attackerOwner
| 164| 164| 	};
| 165| 165| 
| 166|    |-	let fallOff = function(x,y)
|    | 166|+	let fallOff = function(x, y)
| 167| 167| 	{
| 168| 168| 		return (1 - x * x / (data.radius * data.radius)) * (1 - 25 * y * y / (data.radius * data.radius));
| 169| 169| 	};
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'hack'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 315| 315| 		"origin": new Vector2D(3, 4),
| 316| 316| 		"radius": radius,
| 317| 317| 		"shape": "Circular",
| 318|    |-		"strengths": { "hack" : 100, "pierce" : 0, "crush": 0 },
|    | 318|+		"strengths": { "hack": 100, "pierce" : 0, "crush": 0 },
| 319| 319| 		"playersToDamage": [2],
| 320| 320| 		"type": "Ranged",
| 321| 321| 		"attackerOwner": 1
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'pierce'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 315| 315| 		"origin": new Vector2D(3, 4),
| 316| 316| 		"radius": radius,
| 317| 317| 		"shape": "Circular",
| 318|    |-		"strengths": { "hack" : 100, "pierce" : 0, "crush": 0 },
|    | 318|+		"strengths": { "hack" : 100, "pierce": 0, "crush": 0 },
| 319| 319| 		"playersToDamage": [2],
| 320| 320| 		"type": "Ranged",
| 321| 321| 		"attackerOwner": 1

binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 125| »   type·=·data.type·=·"Ranged";
|    | [NORMAL] ESLintBear (no-multi-assign):
|    | Unexpected chained assignment.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
|  23|  23| 	let cmpTimer = Engine.QueryInterface(SYSTEM_ENTITY, IID_Timer);
|  24|  24| 	let turnLength = cmpTimer.GetLatestTurnLength();
|  25|  25| 	return new Vector3D(
|  26|    |-			(curPos.x * (turnLength - lateness) + prevPos.x * lateness) / turnLength,
|    |  26|+		(curPos.x * (turnLength - lateness) + prevPos.x * lateness) / turnLength,
|  27|  27| 			0,
|  28|  28| 			(curPos.z * (turnLength - lateness) + prevPos.z * lateness) / turnLength);
|  29|  29| };
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
|  24|  24| 	let turnLength = cmpTimer.GetLatestTurnLength();
|  25|  25| 	return new Vector3D(
|  26|  26| 			(curPos.x * (turnLength - lateness) + prevPos.x * lateness) / turnLength,
|  27|    |-			0,
|    |  27|+		0,
|  28|  28| 			(curPos.z * (turnLength - lateness) + prevPos.z * lateness) / turnLength);
|  29|  29| };
|  30|  30| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
|  25|  25| 	return new Vector3D(
|  26|  26| 			(curPos.x * (turnLength - lateness) + prevPos.x * lateness) / turnLength,
|  27|  27| 			0,
|  28|    |-			(curPos.z * (turnLength - lateness) + prevPos.z * lateness) / turnLength);
|    |  28|+		(curPos.z * (turnLength - lateness) + prevPos.z * lateness) / turnLength);
|  29|  29| };
|  30|  30| 
|  31|  31| /**
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
| 113| 113| 
| 114| 114| 	// Do this first in case the direct hit kills the target
| 115| 115| 	if ("splash" in data)
| 116|    |-	{
|    | 116|+	
| 117| 117| 		this.CauseSplashDamage({
| 118| 118| 			"attacker": data.attacker,
| 119| 119| 			"origin": Vector2D.from3D(data.position),
| 126| 126| 			"type": data.type,
| 127| 127| 			"attackerOwner": data.attackerOwner
| 128| 128| 		});
| 129|    |-	}
|    | 129|+	
| 130| 130| 
| 131| 131| 	let cmpProjectileManager = Engine.QueryInterface(SYSTEM_ENTITY, IID_ProjectileManager);
| 132| 132| 
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'else'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
| 218| 218| 				damageMultiplier = 0;
| 219| 219| 		}
| 220| 220| 		else // In case someone calls this function with an invalid shape.
| 221|    |-		{
|    | 221|+		
| 222| 222| 			warn("The " + data.shape + " splash damage shape is not implemented!");
| 223|    |-		}
|    | 223|+		
| 224| 224| 
| 225| 225| 		if (data.splashBonus)
| 226| 226| 			damageMultiplier *= GetDamageBonus(data.attacker, ent, data.type, data.splashBonus);
Executing section cli...

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

Looks good - didn't test yet, should do so with the attack ground patch ;) .
I think you should add some tests for this.

binaries/data/mods/public/simulation/components/Damage.js
115 ↗(On Diff #8562)

I believe our convention is to check for data.splash

154 ↗(On Diff #8562)

Too long a line.

Stan added inline comments.Jun 29 2019, 11:35 AM
binaries/data/mods/public/simulation/components/Attack.js
191 ↗(On Diff #8562)

Maybe it could be optionnal ?

binaries/data/mods/public/simulation/components/Damage.js
115 ↗(On Diff #8562)

Depends whether we need to check for the key or the value, doesn't it ?

In this case we are definitely checking for the value. Maybe we shouldn't do splash if radius is 0 ?

binaries/data/mods/public/simulation/components/tests/test_Damage.js
354 ↗(On Diff #8562)

Isn't it supposed to be a "false" string ?

454 ↗(On Diff #8562)

Maybe

data.splash = {
    "friendlyFire": "false",
    "radius": 10,
    "shape": "Circular"
};
Freagarach marked 4 inline comments as done.Jul 1 2019, 8:32 PM

I think you should add some tests for this.

In the test, ExecuteQueryAroundPos is mocked and that is where, in "Damage.js", the FF is checked (EntitiesNearPoint). That might be the reason why FF is not tested for splash damage either?

binaries/data/mods/public/simulation/components/Attack.js
191 ↗(On Diff #8562)

Could be, but:

If you make it a non-optional element, then you can do template.Attack[type].FriendlyFire == "false", which is nicer.

(See above in "Templates.js".)

binaries/data/mods/public/simulation/components/Damage.js
115 ↗(On Diff #8562)

I think it would be strange to have data.splash but without a radius. Do you have a use-case?

wraitii added inline comments.Jul 1 2019, 8:53 PM
binaries/data/mods/public/simulation/components/Attack.js
191 ↗(On Diff #8562)

None of the other items are optional, so in terms of coherency it's probably better if it's not, but I agree that repeating these everywhere is kind of annoying...

binaries/data/mods/public/simulation/components/Damage.js
115 ↗(On Diff #8562)

I guess with techs / auras it might sorta happen? And then possibly we want to disable splash?

Freagarach updated this revision to Diff 8678.Jul 1 2019, 8:53 PM
Freagarach marked an inline comment as done.
  • Updated "Templates.js" because of FF under Projectile.
  • Check for data.splash in "Damage.js".
  • Split "Look for nearby units"-line.
  • Refactored new data.splash.
Freagarach added inline comments.Jul 1 2019, 8:56 PM
binaries/data/mods/public/simulation/components/Attack.js
191 ↗(On Diff #8562)

One could say the same of e.g. Gravity ;)

binaries/data/mods/public/simulation/components/Damage.js
115 ↗(On Diff #8562)

Yeah, was thinking about that as well. radius > 0?

Vulcan added a comment.Jul 1 2019, 9:03 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '||' should be placed at the end of the line.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Templates.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Templates.js
|  87|  87| 		// If the elements are still strings, split them by space or by '+'
|  88|  88| 		if (typeof sublist == "string")
|  89|  89| 			sublist = sublist.split(/[+\s]+/);
|  90|    |-		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1)
|  91|    |-		                    || (c[0] != "!" && classes.indexOf(c) != -1)))
|    |  90|+		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1) ||
|    |  91|+		                    (c[0] != "!" && classes.indexOf(c) != -1)))
|  92|  92| 			return true;
|  93|  93| 	}
|  94|  94| 

binaries/data/mods/public/globalscripts/Templates.js
|  91| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.
|    | [NORMAL] ESLintBear (quote-props):
|    | Unquoted property 'turnLength' found.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|  25|  25| 
|  26|  26| 	let cmpDamage = ConstructComponent(SYSTEM_ENTITY, "Damage");
|  27|  27| 	let cmpTimer = ConstructComponent(SYSTEM_ENTITY, "Timer");
|  28|    |-	cmpTimer.OnUpdate({ turnLength: 1 });
|    |  28|+	cmpTimer.OnUpdate({ "turnLength": 1 });
|  29|  29| 	let attacker = 11;
|  30|  30| 	let atkPlayerEntity = 1;
|  31|  31| 	let attackerOwner = 6;
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|  63|  63| 		"attackerOwner": attackerOwner,
|  64|  64| 		"position": targetPos,
|  65|  65| 		"projectileId": 9,
|  66|    |-		"direction": new Vector3D(1,0,0)
|    |  66|+		"direction": new Vector3D(1, 0,0)
|  67|  67| 	};
|  68|  68| 
|  69|  69| 	AddMock(atkPlayerEntity, IID_Player, {
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|  63|  63| 		"attackerOwner": attackerOwner,
|  64|  64| 		"position": targetPos,
|  65|  65| 		"projectileId": 9,
|  66|    |-		"direction": new Vector3D(1,0,0)
|    |  66|+		"direction": new Vector3D(1,0, 0)
|  67|  67| 	};
|  68|  68| 
|  69|  69| 	AddMock(atkPlayerEntity, IID_Player, {
|    | [NORMAL] ESLintBear (quote-props):
|    | Unquoted property 'turnLength' found.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 114| 114| 
| 115| 115| 	function TestDamage()
| 116| 116| 	{
| 117|    |-		cmpTimer.OnUpdate({ turnLength: 1 });
|    | 117|+		cmpTimer.OnUpdate({ "turnLength": 1 });
| 118| 118| 		TS_ASSERT(damageTaken);
| 119| 119| 		damageTaken = false;
| 120| 120| 	}
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'hack'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 156| 156| 		"origin": origin,
| 157| 157| 		"radius": 10,
| 158| 158| 		"shape": "Linear",
| 159|    |-		"strengths": { "hack" : 100, "pierce" : 0, "crush": 0 },
|    | 159|+		"strengths": { "hack": 100, "pierce" : 0, "crush": 0 },
| 160| 160| 		"direction": new Vector3D(1, 747, 0),
| 161| 161| 		"playersToDamage": [2],
| 162| 162| 		"type": "Ranged",
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'pierce'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 156| 156| 		"origin": origin,
| 157| 157| 		"radius": 10,
| 158| 158| 		"shape": "Linear",
| 159|    |-		"strengths": { "hack" : 100, "pierce" : 0, "crush": 0 },
|    | 159|+		"strengths": { "hack" : 100, "pierce": 0, "crush": 0 },
| 160| 160| 		"direction": new Vector3D(1, 747, 0),
| 161| 161| 		"playersToDamage": [2],
| 162| 162| 		"type": "Ranged",
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 163| 163| 		"attackerOwner": attackerOwner
| 164| 164| 	};
| 165| 165| 
| 166|    |-	let fallOff = function(x,y)
|    | 166|+	let fallOff = function(x, y)
| 167| 167| 	{
| 168| 168| 		return (1 - x * x / (data.radius * data.radius)) * (1 - 25 * y * y / (data.radius * data.radius));
| 169| 169| 	};
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'hack'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 315| 315| 		"origin": new Vector2D(3, 4),
| 316| 316| 		"radius": radius,
| 317| 317| 		"shape": "Circular",
| 318|    |-		"strengths": { "hack" : 100, "pierce" : 0, "crush": 0 },
|    | 318|+		"strengths": { "hack": 100, "pierce" : 0, "crush": 0 },
| 319| 319| 		"playersToDamage": [2],
| 320| 320| 		"type": "Ranged",
| 321| 321| 		"attackerOwner": 1
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'pierce'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 315| 315| 		"origin": new Vector2D(3, 4),
| 316| 316| 		"radius": radius,
| 317| 317| 		"shape": "Circular",
| 318|    |-		"strengths": { "hack" : 100, "pierce" : 0, "crush": 0 },
|    | 318|+		"strengths": { "hack" : 100, "pierce": 0, "crush": 0 },
| 319| 319| 		"playersToDamage": [2],
| 320| 320| 		"type": "Ranged",
| 321| 321| 		"attackerOwner": 1

binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 125| »   type·=·data.type·=·"Ranged";
|    | [NORMAL] ESLintBear (no-multi-assign):
|    | Unexpected chained assignment.
|    | [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
| 527| 527| 
| 528| 528| 		let horizSpeed = +this.template[type].Projectile.Speed;
| 529| 529| 		let gravity = +this.template[type].Projectile.Gravity;
| 530|    |-		//horizSpeed /= 2; gravity /= 2; // slow it down for testing
|    | 530|+		// horizSpeed /= 2; gravity /= 2; // slow it down for testing
| 531| 531| 
| 532| 532| 		let cmpPosition = Engine.QueryInterface(this.entity, IID_Position);
| 533| 533| 		if (!cmpPosition || !cmpPosition.IsInWorld())
|    | [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
| 647| 647| 			});
| 648| 648| 	}
| 649| 649| 	else
| 650|    |-	{
|    | 650|+	
| 651| 651| 		// Melee attack - hurt the target immediately
| 652| 652| 		cmpDamage.CauseDamage({
| 653| 653| 			"strengths": this.GetAttackStrengths(type),
| 657| 657| 			"type": type,
| 658| 658| 			"attackerOwner": attackerOwner
| 659| 659| 		});
| 660|    |-	}
|    | 660|+	
| 661| 661| };
| 662| 662| 
| 663| 663| /**

binaries/data/mods/public/simulation/components/Attack.js
| 517| ·»   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
| 622| »   »   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/simulation/components/Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
|  23|  23| 	let cmpTimer = Engine.QueryInterface(SYSTEM_ENTITY, IID_Timer);
|  24|  24| 	let turnLength = cmpTimer.GetLatestTurnLength();
|  25|  25| 	return new Vector3D(
|  26|    |-			(curPos.x * (turnLength - lateness) + prevPos.x * lateness) / turnLength,
|    |  26|+		(curPos.x * (turnLength - lateness) + prevPos.x * lateness) / turnLength,
|  27|  27| 			0,
|  28|  28| 			(curPos.z * (turnLength - lateness) + prevPos.z * lateness) / turnLength);
|  29|  29| };
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
|  24|  24| 	let turnLength = cmpTimer.GetLatestTurnLength();
|  25|  25| 	return new Vector3D(
|  26|  26| 			(curPos.x * (turnLength - lateness) + prevPos.x * lateness) / turnLength,
|  27|    |-			0,
|    |  27|+		0,
|  28|  28| 			(curPos.z * (turnLength - lateness) + prevPos.z * lateness) / turnLength);
|  29|  29| };
|  30|  30| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
|  25|  25| 	return new Vector3D(
|  26|  26| 			(curPos.x * (turnLength - lateness) + prevPos.x * lateness) / turnLength,
|  27|  27| 			0,
|  28|    |-			(curPos.z * (turnLength - lateness) + prevPos.z * lateness) / turnLength);
|    |  28|+		(curPos.z * (turnLength - lateness) + prevPos.z * lateness) / turnLength);
|  29|  29| };
|  30|  30| 
|  31|  31| /**
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'else'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
| 217| 217| 				damageMultiplier = 0;
| 218| 218| 		}
| 219| 219| 		else // In case someone calls this function with an invalid shape.
| 220|    |-		{
|    | 220|+		
| 221| 221| 			warn("The " + data.shape + " splash damage shape is not implemented!");
| 222|    |-		}
|    | 222|+		
| 223| 223| 
| 224| 224| 		if (data.splashBonus)
| 225| 225| 			damageMultiplier *= GetDamageBonus(data.attacker, ent, data.type, data.splashBonus);
Executing section cli...

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

Freagarach updated this revision to Diff 8697.Jul 3 2019, 10:04 AM
  • Made Vulcan/Jenkins a tad happier.
  • Check for this.template[type].Splash.Range > 0 in Attack.js.
Freagarach marked 3 inline comments as done.Jul 3 2019, 10:05 AM
Freagarach added inline comments.
binaries/data/mods/public/simulation/components/Attack.js
191 ↗(On Diff #8562)

Okay, a thought here. The argument to have it non-optional is coherency. I like things being optional, to keep templates as clean as possible. Would it be an idea to make more things optional? E.g.:

  • Delay
  • Projectile gravity

So just make everything that *can* be optional optional.
However, I also acknowledge that making things optional blurs the code in the components, which would vote for non-optional.

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
|  23|  23| 	let cmpTimer = Engine.QueryInterface(SYSTEM_ENTITY, IID_Timer);
|  24|  24| 	let turnLength = cmpTimer.GetLatestTurnLength();
|  25|  25| 	return new Vector3D(
|  26|    |-			(curPos.x * (turnLength - lateness) + prevPos.x * lateness) / turnLength,
|    |  26|+		(curPos.x * (turnLength - lateness) + prevPos.x * lateness) / turnLength,
|  27|  27| 			0,
|  28|  28| 			(curPos.z * (turnLength - lateness) + prevPos.z * lateness) / turnLength);
|  29|  29| };
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
|  24|  24| 	let turnLength = cmpTimer.GetLatestTurnLength();
|  25|  25| 	return new Vector3D(
|  26|  26| 			(curPos.x * (turnLength - lateness) + prevPos.x * lateness) / turnLength,
|  27|    |-			0,
|    |  27|+		0,
|  28|  28| 			(curPos.z * (turnLength - lateness) + prevPos.z * lateness) / turnLength);
|  29|  29| };
|  30|  30| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
|  25|  25| 	return new Vector3D(
|  26|  26| 			(curPos.x * (turnLength - lateness) + prevPos.x * lateness) / turnLength,
|  27|  27| 			0,
|  28|    |-			(curPos.z * (turnLength - lateness) + prevPos.z * lateness) / turnLength);
|    |  28|+		(curPos.z * (turnLength - lateness) + prevPos.z * lateness) / turnLength);
|  29|  29| };
|  30|  30| 
|  31|  31| /**
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'else'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Damage.js
| 217| 217| 				damageMultiplier = 0;
| 218| 218| 		}
| 219| 219| 		else // In case someone calls this function with an invalid shape.
| 220|    |-		{
|    | 220|+		
| 221| 221| 			warn("The " + data.shape + " splash damage shape is not implemented!");
| 222|    |-		}
|    | 222|+		
| 223| 223| 
| 224| 224| 		if (data.splashBonus)
| 225| 225| 			damageMultiplier *= GetDamageBonus(data.attacker, ent, data.type, data.splashBonus);

binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 125| »   type·=·data.type·=·"Ranged";
|    | [NORMAL] ESLintBear (no-multi-assign):
|    | Unexpected chained assignment.
|    | [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
| 527| 527| 
| 528| 528| 		let horizSpeed = +this.template[type].Projectile.Speed;
| 529| 529| 		let gravity = +this.template[type].Projectile.Gravity;
| 530|    |-		//horizSpeed /= 2; gravity /= 2; // slow it down for testing
|    | 530|+		// horizSpeed /= 2; gravity /= 2; // slow it down for testing
| 531| 531| 
| 532| 532| 		let cmpPosition = Engine.QueryInterface(this.entity, IID_Position);
| 533| 533| 		if (!cmpPosition || !cmpPosition.IsInWorld())
|    | [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
| 647| 647| 			});
| 648| 648| 	}
| 649| 649| 	else
| 650|    |-	{
|    | 650|+	
| 651| 651| 		// Melee attack - hurt the target immediately
| 652| 652| 		cmpDamage.CauseDamage({
| 653| 653| 			"strengths": this.GetAttackStrengths(type),
| 657| 657| 			"type": type,
| 658| 658| 			"attackerOwner": attackerOwner
| 659| 659| 		});
| 660|    |-	}
|    | 660|+	
| 661| 661| };
| 662| 662| 
| 663| 663| /**

binaries/data/mods/public/simulation/components/Attack.js
| 517| ·»   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
| 622| »   »   cmpTimer.SetTimeout(SYSTEM_ENTITY,·IID_Damage,·"MissileHit",·timeToTarget·*·1000·+·+this.template[type].Delay,·data);
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '||' should be placed at the end of the line.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Templates.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Templates.js
|  87|  87| 		// If the elements are still strings, split them by space or by '+'
|  88|  88| 		if (typeof sublist == "string")
|  89|  89| 			sublist = sublist.split(/[+\s]+/);
|  90|    |-		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1)
|  91|    |-		                    || (c[0] != "!" && classes.indexOf(c) != -1)))
|    |  90|+		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1) ||
|    |  91|+		                    (c[0] != "!" && classes.indexOf(c) != -1)))
|  92|  92| 			return true;
|  93|  93| 	}
|  94|  94| 

binaries/data/mods/public/globalscripts/Templates.js
|  91| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.
Executing section cli...

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

Angen added inline comments.Jul 3 2019, 11:04 AM
binaries/data/mods/public/simulation/components/Attack.js
191 ↗(On Diff #8562)

I would say that gravity should be always stated and delay can be optional.

Or friendlyfire is not optional or make it false by default so if not in template it is false. It goes against common sence. Element is called FriendlyFire so it should be false by default. It is not called DisableFriendylFire.

Personally I think if there is good template inheritance for units, a lot of things does not have to be repeated. Problem can be with structures.

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Damage.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Damage.js
|  23|  23| 	let cmpTimer = Engine.QueryInterface(SYSTEM_ENTITY, IID_Timer);
|  24|  24| 	let turnLength = cmpTimer.GetLatestTurnLength();
|  25|  25| 	return new Vector3D(
|  26|    |-			(curPos.x * (turnLength - lateness) + prevPos.x * lateness) / turnLength,
|    |  26|+		(curPos.x * (turnLength - lateness) + prevPos.x * lateness) / turnLength,
|  27|  27| 			0,
|  28|  28| 			(curPos.z * (turnLength - lateness) + prevPos.z * lateness) / turnLength);
|  29|  29| };
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Damage.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Damage.js
|  24|  24| 	let turnLength = cmpTimer.GetLatestTurnLength();
|  25|  25| 	return new Vector3D(
|  26|  26| 			(curPos.x * (turnLength - lateness) + prevPos.x * lateness) / turnLength,
|  27|    |-			0,
|    |  27|+		0,
|  28|  28| 			(curPos.z * (turnLength - lateness) + prevPos.z * lateness) / turnLength);
|  29|  29| };
|  30|  30| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Damage.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Damage.js
|  25|  25| 	return new Vector3D(
|  26|  26| 			(curPos.x * (turnLength - lateness) + prevPos.x * lateness) / turnLength,
|  27|  27| 			0,
|  28|    |-			(curPos.z * (turnLength - lateness) + prevPos.z * lateness) / turnLength);
|    |  28|+		(curPos.z * (turnLength - lateness) + prevPos.z * lateness) / turnLength);
|  29|  29| };
|  30|  30| 
|  31|  31| /**
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'else'.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Damage.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Damage.js
| 216| 216| 				damageMultiplier = 0;
| 217| 217| 		}
| 218| 218| 		else // In case someone calls this function with an invalid shape.
| 219|    |-		{
|    | 219|+		
| 220| 220| 			warn("The " + data.shape + " splash damage shape is not implemented!");
| 221|    |-		}
|    | 221|+		
| 222| 222| 
| 223| 223| 		if (data.splashBonus)
| 224| 224| 			damageMultiplier *= GetDamageBonus(data.attacker, ent, data.type, data.splashBonus);

binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 125| »   type·=·data.type·=·"Ranged";
|    | [NORMAL] ESLintBear (no-multi-assign):
|    | Unexpected chained assignment.
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '||' should be placed at the end of the line.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|  87|  87| 		// If the elements are still strings, split them by space or by '+'
|  88|  88| 		if (typeof sublist == "string")
|  89|  89| 			sublist = sublist.split(/[+\s]+/);
|  90|    |-		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1)
|  91|    |-		                    || (c[0] != "!" && classes.indexOf(c) != -1)))
|    |  90|+		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1) ||
|    |  91|+		                    (c[0] != "!" && classes.indexOf(c) != -1)))
|  92|  92| 			return true;
|  93|  93| 	}
|  94|  94| 

binaries/data/mods/public/globalscripts/Templates.js
|  91| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Attack.js
| 535| 535| 
| 536| 536| 		let horizSpeed = +this.template[type].Projectile.Speed;
| 537| 537| 		let gravity = +this.template[type].Projectile.Gravity;
| 538|    |-		//horizSpeed /= 2; gravity /= 2; // slow it down for testing
|    | 538|+		// horizSpeed /= 2; gravity /= 2; // slow it down for testing
| 539| 539| 
| 540| 540| 		let cmpPosition = Engine.QueryInterface(this.entity, IID_Position);
| 541| 541| 		if (!cmpPosition || !cmpPosition.IsInWorld())
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'else'.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Attack.js
| 655| 655| 			});
| 656| 656| 	}
| 657| 657| 	else
| 658|    |-	{
|    | 658|+	
| 659| 659| 		// Melee attack - hurt the target immediately
| 660| 660| 		cmpDamage.CauseDamage({
| 661| 661| 			"strengths": this.GetAttackStrengths(type),
| 665| 665| 			"type": type,
| 666| 666| 			"attackerOwner": attackerOwner
| 667| 667| 		});
| 668|    |-	}
|    | 668|+	
| 669| 669| };
| 670| 670| 
| 671| 671| /**

binaries/data/mods/public/simulation/components/Attack.js
| 525| ·»   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
| 630| »   »   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/docker-differential/97/display/redirect

Freagarach updated this revision to Diff 9480.Aug 24 2019, 2:18 PM
Freagarach retitled this revision from Enable support for friendly fire for normal ranged attacks. to Support friendly fire for projectile attacks..
Freagarach edited the summary of this revision. (Show Details)

Rebased.

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '||' should be placed at the end of the line.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|  87|  87| 		// If the elements are still strings, split them by space or by '+'
|  88|  88| 		if (typeof sublist == "string")
|  89|  89| 			sublist = sublist.split(/[+\s]+/);
|  90|    |-		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1)
|  91|    |-		                    || (c[0] != "!" && classes.indexOf(c) != -1)))
|    |  90|+		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1) ||
|    |  91|+		                    (c[0] != "!" && classes.indexOf(c) != -1)))
|  92|  92| 			return true;
|  93|  93| 	}
|  94|  94| 

binaries/data/mods/public/globalscripts/Templates.js
|  91| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/473/display/redirect

Freagarach updated this revision to Diff 9706.Sep 11 2019, 8:56 AM
Freagarach marked 2 inline comments as done.

!= false => == true.

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/127/display/redirect

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '||' should be placed at the end of the line.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|  87|  87| 		// If the elements are still strings, split them by space or by '+'
|  88|  88| 		if (typeof sublist == "string")
|  89|  89| 			sublist = sublist.split(/[+\s]+/);
|  90|    |-		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1)
|  91|    |-		                    || (c[0] != "!" && classes.indexOf(c) != -1)))
|    |  90|+		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1) ||
|    |  91|+		                    (c[0] != "!" && classes.indexOf(c) != -1)))
|  92|  92| 			return true;
|  93|  93| 	}
|  94|  94| 

binaries/data/mods/public/globalscripts/Templates.js
|  91| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/636/display/redirect

Stan added inline comments.Sep 11 2019, 9:38 AM
binaries/data/mods/public/simulation/components/Attack.js
545 ↗(On Diff #9706)

could change that one as well.

binaries/data/mods/public/simulation/components/tests/test_Attack.js
82 ↗(On Diff #9706)

Shouldn't that be text, as it would be in the template ?

binaries/data/mods/public/simulation/components/tests/test_Damage.js
43 ↗(On Diff #9706)

Shouldn't that be text, as it would be in the template ?

Freagarach updated this revision to Diff 10000.Sep 29 2019, 7:53 AM
Freagarach marked 3 inline comments as done.
  • Another (close) != "false" -> == "true".
  • Text in test.

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/347/display/redirect

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '||' should be placed at the end of the line.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|  87|  87| 		// If the elements are still strings, split them by space or by '+'
|  88|  88| 		if (typeof sublist == "string")
|  89|  89| 			sublist = sublist.split(/[+\s]+/);
|  90|    |-		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1)
|  91|    |-		                    || (c[0] != "!" && classes.indexOf(c) != -1)))
|    |  90|+		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1) ||
|    |  91|+		                    (c[0] != "!" && classes.indexOf(c) != -1)))
|  92|  92| 			return true;
|  93|  93| 	}
|  94|  94| 

binaries/data/mods/public/globalscripts/Templates.js
|  91| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/863/display/redirect

Angen added a reviewer: Angen.Feb 20 2020, 8:25 PM

quick look, looks good.
critical look comes later,

you should add tests for friendlyfire = true.
testing just one side of coin is bad practice

Freagarach updated this revision to Diff 11405.Feb 20 2020, 9:52 PM
  • Rebased.
  • Add test for FF == true for splash.

A test for FF == true for normal ranged attacks could be added.

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '||' should be placed at the end of the line.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|  87|  87| 		// If the elements are still strings, split them by space or by '+'
|  88|  88| 		if (typeof sublist == "string")
|  89|  89| 			sublist = sublist.split(/[+\s]+/);
|  90|    |-		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1)
|  91|    |-		                    || (c[0] != "!" && classes.indexOf(c) != -1)))
|    |  90|+		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1) ||
|    |  91|+		                    (c[0] != "!" && classes.indexOf(c) != -1)))
|  92|  92| 			return true;
|  93|  93| 	}
|  94|  94| 

binaries/data/mods/public/globalscripts/Templates.js
|  91| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/Attack.js
| 422| »   return·{
|    | [NORMAL] ESLintBear (consistent-return):
|    | Function expected no return value.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1797/display/redirect

Angen requested changes to this revision.Feb 24 2020, 8:46 PM
Angen added inline comments.
binaries/data/mods/public/simulation/components/DelayedDamage.js
72 ↗(On Diff #11405)

could you please to break this to multiple lines ?

This revision now requires changes to proceed.Feb 24 2020, 8:46 PM
Freagarach updated this revision to Diff 11429.Feb 25 2020, 8:26 PM
Freagarach marked an inline comment as done.

More readable line.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1816/display/redirect

Angen accepted this revision.Feb 25 2020, 9:16 PM

It is correct to allow templates decide if friendly fire is enabled instead hardcoding it.
Changes to code are complete and correct.
All required templates have been updated.
Test are passing.

Thanks @Angen (and @wraitii) for the review and the commit :)

This revision was not accepted when it landed; it landed in state Needs Review.Mar 7 2020, 11:58 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Mar 7 2020, 11:58 AM