Currently the function GetPlayersToDamage is called from outside the Attacking-helper prior to calling CauseDamageOverArea, which I think is strange because we can just as well call it in Attacking when we arrive there.
I do not know whether there is any performance increase/decrease for this, but it seems more logical to me.
It was as is because CauseSplashDamage was at first only accessed from within cmpDamage (ranged splash damage) but, with the arrival of DeathDamage.js (rP19950), it was called also from outside cmpDamage. And with rP22754 many calls were brought under the Attacking-helper.
Details
Details
- Reviewers
wraitii - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP22864: Move "PlayersToDamage" from calling functions to "CauseDamageOverArea" and…
Verify that everything works as before.
Diff Detail
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
Comment Actions
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 (quote-props):
| | Unquoted property 'turnLength' found.
|----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| |++++| /zpool0/trunk/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 ','.
|----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 63| 63| "position": targetPos,
| 64| 64| "isSplash": false,
| 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 ','.
|----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 63| 63| "position": targetPos,
| 64| 64| "isSplash": false,
| 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.
|----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| |++++| /zpool0/trunk/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'.
|----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| |++++| /zpool0/trunk/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| "friendlyFire": false,
| 162| 162| "type": "Ranged",
| | [NORMAL] ESLintBear (key-spacing):
| | Extra space after key 'pierce'.
|----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| |++++| /zpool0/trunk/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| "friendlyFire": false,
| 162| 162| "type": "Ranged",
| | [NORMAL] ESLintBear (comma-spacing):
| | A space is required after ','.
|----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 172| 172| "GetAllPlayers": () => [0, 1, 2, 3, 4]
| 173| 173| });
| 174| 174|
| 175| |- let fallOff = function(x,y)
| | 175|+ let fallOff = function(x, y)
| 176| 176| {
| 177| 177| return (1 - x * x / (data.radius * data.radius)) * (1 - 25 * y * y / (data.radius * data.radius));
| 178| 178| };
| | [NORMAL] ESLintBear (key-spacing):
| | Extra space after key 'hack'.
|----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 335| 335| "origin": new Vector2D(3, 4),
| 336| 336| "radius": radius,
| 337| 337| "shape": "Circular",
| 338| |- "strengths": { "hack" : 100, "pierce" : 0, "crush": 0 },
| | 338|+ "strengths": { "hack": 100, "pierce" : 0, "crush": 0 },
| 339| 339| "friendlyFire": false,
| 340| 340| "type": "Ranged",
| 341| 341| "attackerOwner": attackerOwner
| | [NORMAL] ESLintBear (key-spacing):
| | Extra space after key 'pierce'.
|----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 335| 335| "origin": new Vector2D(3, 4),
| 336| 336| "radius": radius,
| 337| 337| "shape": "Circular",
| 338| |- "strengths": { "hack" : 100, "pierce" : 0, "crush": 0 },
| | 338|+ "strengths": { "hack" : 100, "pierce": 0, "crush": 0 },
| 339| 339| "friendlyFire": false,
| 340| 340| "type": "Ranged",
| 341| 341| "attackerOwner": attackerOwner
binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 125| » type·=·data.type·=·"Ranged";
| | [NORMAL] ESLintBear (no-multi-assign):
| | Unexpected chained assignment.
Executing section cli...Link to build: https://jenkins.wildfiregames.com/job/docker-differential/44/display/redirect
Comment Actions
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 (quote-props):
| | Unquoted property 'turnLength' found.
|----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| |++++| /zpool0/trunk/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 ','.
|----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 63| 63| "position": targetPos,
| 64| 64| "isSplash": false,
| 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 ','.
|----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 63| 63| "position": targetPos,
| 64| 64| "isSplash": false,
| 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.
|----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| |++++| /zpool0/trunk/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'.
|----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| |++++| /zpool0/trunk/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| "friendlyFire": false,
| 162| 162| "type": "Ranged",
| | [NORMAL] ESLintBear (key-spacing):
| | Extra space after key 'pierce'.
|----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| |++++| /zpool0/trunk/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| "friendlyFire": false,
| 162| 162| "type": "Ranged",
| | [NORMAL] ESLintBear (comma-spacing):
| | A space is required after ','.
|----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 172| 172| "GetAllPlayers": () => [0, 1, 2, 3, 4]
| 173| 173| });
| 174| 174|
| 175| |- let fallOff = function(x,y)
| | 175|+ let fallOff = function(x, y)
| 176| 176| {
| 177| 177| return (1 - x * x / (data.radius * data.radius)) * (1 - 25 * y * y / (data.radius * data.radius));
| 178| 178| };
| | [NORMAL] ESLintBear (key-spacing):
| | Extra space after key 'hack'.
|----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 335| 335| "origin": new Vector2D(3, 4),
| 336| 336| "radius": radius,
| 337| 337| "shape": "Circular",
| 338| |- "strengths": { "hack" : 100, "pierce" : 0, "crush": 0 },
| | 338|+ "strengths": { "hack": 100, "pierce" : 0, "crush": 0 },
| 339| 339| "friendlyFire": false,
| 340| 340| "type": "Ranged",
| 341| 341| "attackerOwner": attackerOwner
| | [NORMAL] ESLintBear (key-spacing):
| | Extra space after key 'pierce'.
|----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 335| 335| "origin": new Vector2D(3, 4),
| 336| 336| "radius": radius,
| 337| 337| "shape": "Circular",
| 338| |- "strengths": { "hack" : 100, "pierce" : 0, "crush": 0 },
| | 338|+ "strengths": { "hack" : 100, "pierce": 0, "crush": 0 },
| 339| 339| "friendlyFire": false,
| 340| 340| "type": "Ranged",
| 341| 341| "attackerOwner": attackerOwner
binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 125| » type·=·data.type·=·"Ranged";
| | [NORMAL] ESLintBear (no-multi-assign):
| | Unexpected chained assignment.
Executing section cli...Link to build: https://jenkins.wildfiregames.com/job/docker-differential/99/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/463/display/redirect
| binaries/data/mods/public/simulation/components/DeathDamage.js | ||
|---|---|---|
| 61 ↗ | (On Diff #9463) | Apparently this was wrong all the time, for it (this.template.FriendlyFire) passed a string which was not properly coped with in GetPlayersToDamage(...), so that friendlyFire was always assumed to be true. (!friendlyFire on "false" gives false since the string exists.) |
Comment Actions
I like this because it's a nice streamlining, and indeed avoids different implementations of Friendly Fire :P
One thing is that this could be reverted if we/modders ever needed to target one specific player with some attacks. TBH that feels like something we're not likely to do anytime soon, and even in a very SFy setting I think targeting only one enemy out of many is a bit odd.
Comment Actions
did not check for completeness, however committing stuff reduces rebases...
| binaries/data/mods/public/simulation/components/DeathDamage.js | ||
|---|---|---|
| 71 ↗ | (On Diff #9463) | friendlyFire doesn't appear to be an optional template value, hence we can assume it always is either "false" or "true", so I would avoid the double negation and get this.template.FriendlyFire == "true" same appears below |
| binaries/data/mods/public/simulation/helpers/Attacking.js | ||
| 190–191 ↗ | (On Diff #9463) | probably inline since they are used only once |
Comment Actions
- != "false" -> == "true".
- Inlined playersToDamage.
| binaries/data/mods/public/simulation/helpers/Attacking.js | ||
|---|---|---|
| 190–191 ↗ | (On Diff #9463) | Also inline into the for loop? |
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/515/display/redirect
| binaries/data/mods/public/simulation/helpers/Attacking.js | ||
|---|---|---|
| 190–191 ↗ | (On Diff #9463) | it isn't inside the for loop, is it? it is looped over for sure, but then it will only be calculated once, right? |
| binaries/data/mods/public/simulation/helpers/Attacking.js | ||
|---|---|---|
| 190–191 ↗ | (On Diff #9463) | I do not really know whether it would be calculated more than once, but I think I read that when in a loop the array which is iterated over changes the actual iteration changes. That would mean that it is calculated every time? |
| binaries/data/mods/public/simulation/components/tests/test_DeathDamage.js | ||
|---|---|---|
| 42 ↗ | (On Diff #9538) | Unused ? |
| binaries/data/mods/public/simulation/components/tests/test_DeathDamage.js | ||
|---|---|---|
| 53 ↗ | (On Diff #9538) | :) |
| binaries/data/mods/public/simulation/components/tests/test_DeathDamage.js | ||
|---|---|---|
| 50 ↗ | (On Diff #9538) | Any reason for this ? |
| binaries/data/mods/public/simulation/components/tests/test_DeathDamage.js | ||
|---|---|---|
| 50 ↗ | (On Diff #9538) | For what specifically? |
| binaries/data/mods/public/simulation/components/tests/test_DeathDamage.js | ||
|---|---|---|
| 50 ↗ | (On Diff #9538) | Is template gonna change during the course of the execution ? |
| binaries/data/mods/public/simulation/components/tests/test_DeathDamage.js | ||
|---|---|---|
| 50 ↗ | (On Diff #9538) | IIRC components were frozen in some commit so this could be set to false because it cannot change :) |
| binaries/data/mods/public/simulation/components/tests/test_DeathDamage.js | ||
|---|---|---|
| 50 ↗ | (On Diff #9538) | So you mean why template.FriendlyFire == "true" instead of just false? Well that is because when someone just wants to change FF in line 21, that person does not need to check the whole file for occurences of FF to change them accordingly. |
| binaries/data/mods/public/simulation/components/tests/test_DeathDamage.js | ||
|---|---|---|
| 50 ↗ | (On Diff #9538) | Yes, that is what I meant. Well if one decides to change the condition that would mean potentially changing the test coverage which would be bad wouldn't. Because then it would means some cases are not tested or tested differently, wouldn't it ? |
| binaries/data/mods/public/simulation/components/tests/test_DeathDamage.js | ||
|---|---|---|
| 50 ↗ | (On Diff #9538) | Aye, I'll change it. |
| binaries/data/mods/public/simulation/components/tests/test_DeathDamage.js | ||
|---|---|---|
| 50 ↗ | (On Diff #9538) | Thanks. Sorry for the nitpicking. But I guess test need to be as clean as the rest of the code :D |
| binaries/data/mods/public/simulation/components/tests/test_DeathDamage.js | ||
|---|---|---|
| 50 ↗ | (On Diff #9538) | No, please, nitpick all you can :) |
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/49/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/558/display/redirect