#3610 closed enhancement (fixed)
[PATCH] Projectiles do not cause any damage if the shooter dies
Reported by: | vikas MK | Owned by: | Leander Hemelhof |
---|---|---|---|
Priority: | Nice to Have | Milestone: | Alpha 21 |
Component: | UI & Simulation | Keywords: | patch |
Cc: | leper | Patch: |
Description (last modified by )
Currently, projectile Damage depends on the shooter being alive.
This should not be the case and to achieve that some of the related logic should be moved from simulation/components/Attack.js
to cmpDamage
(in the same directory, which would be a system component created from simulation/helpers/Damage.js
)
cmpDamage
should keep information about who fired (player/entity) and when the damage should be applied. Most of this information is already stored in the current Damage
helper object.
cmpDamage
then uses a delay (see cmpTimer
and the current usage of it in cmpAttack
) to cause this damage.
Note that this ticket enables doing #1912 and #1911 (and possibly #1910) without any huge changes.
Attachments (15)
Change History (57)
follow-up: 3 comment:2 by , 8 years ago
I think we need a CmpProjectileManager that will handle all projectiles, either as specific entities or as something special. This would also provide a convenient interface to check if we are obstructed by walls or other buildings for say, arrows, or ultimately provide realistic physics for cannonballs...
comment:3 by , 8 years ago
Replying to wraitii:
I think we need a CmpProjectileManager that will handle all projectiles, either as specific entities or as something special. This would also provide a convenient interface to check if we are obstructed by walls or other buildings for say, arrows, or ultimately provide realistic physics for cannonballs...
The projectile manager handles the visual only and is only used for rendering. Also this new component is usable for more things than just projectiles.
Having buildings block projectiles seems like asking for shitty performance, also I see no reason why that should be in a damage related component instead of eg cmpAttack. Physics for cannonballs should be a purely graphical thing and not related to the simulation at all.
comment:5 by , 8 years ago
Description: | modified (diff) |
---|---|
Keywords: | simple added; projectile system component removed |
Summary: | Moving missilehit from the shooter component to a system component → Projectiles do not cause any damage if the shooter dies |
Note that there are some details (starting at 03:37) in this irc log: http://irclogs.wildfiregames.com/2015-11-15-QuakeNet-%230ad-dev.log
by , 8 years ago
Attachment: | DamageAfterDeathOfAtacker.patch added |
---|
Changes the damage helper to a system component with the projectile hit handling logic
comment:6 by , 8 years ago
Keywords: | patch review added |
---|---|
Milestone: | Backlog → Alpha 21 |
Owner: | set to |
Status: | new → assigned |
Having taken some inspiration from the irc link linked above, this patch changes the damage helper to a system component wich also contains most of the projectile hit logic. This separates damage from the attacker, alowing projectiles to deal damage even after the attacker dies.
Completely moving the helper to a system component looked like the cleanest solution to me, and allows for future expansion, if needed.
comment:7 by , 8 years ago
We waited for this for a long time ! I will look at the code more deeply. I put it in the review queue. (Here are some quick useless formal remarks: Attack.js
- L579: i guess the commentary is not needed
- L66: perhaps you can use let
Damage.js
- L6-8: could you skip the Init function ?
- L27 : commentary perhaps not needed, or use JavaDoc as you do in the other part of the code
- L28 : the name of the function should start with Caps)
by , 8 years ago
Attachment: | DamageAfterDeathOfAtacker_v2.patch added |
---|
Second version taking fatherbushido's regards into account
by , 8 years ago
Attachment: | DamageAfterDeathOfAtacker_v2.2.patch added |
---|
Second version taking fatherbushido's regards into account
comment:8 by , 8 years ago
Summary: | Projectiles do not cause any damage if the shooter dies → [PATCH] Projectiles do not cause any damage if the shooter dies |
---|
Thanks for the remarks !
I didn't change the TestCollision function at first because I just copied it over from the Attack component. This new patch changes the function name and changes the comment above to a JavaDoc. It also deals with the other remarks.
Edit: Please ignore the _v2.2.patch. I seem to be unable to delete it.
comment:9 by , 8 years ago
I tested it with a low projectile speed ballista and it has the expected behavior. Here are some quick remarks again
- (for future reviewer) the patch apply to data/, not to binary/
- AttackDetection.js L67-70 could perhaps be merged with a
||
or something like that - I understand why you need that entityOwners map but i wonder if we can in a way or another skip that ?
Moreover, there is a more serious issue pointed out by Sanderd17 (see 2016-07-04 irc logs). The entityOwners map should be in the Init else it will not be serialized (for multiplayer and saved games).
Then while you are at it, we can consider you are rewriting Damage, so you can remove depreciated for each loops.
by , 8 years ago
Attachment: | DamageAfterDeathOfAtacker_v3.patch added |
---|
Cleanup and fixing of the serialization issue
comment:10 by , 8 years ago
This patch should have all the requested cleanups. I also hope that I have created this patch from the right directory this time.
I did some minor tweaks with the entityOwner map. The owner of the attacker is now sent as data in the timer and only added to the map if the attacker is dead when the projectile hits. The for each loops are regular for loops now. Please let me know if there is a more preferred way of changing those.
comment:11 by , 8 years ago
We (allow to) use for (let of ... instead of for (let i = 0; i < ents.length; ++i) in the code.
I will do a full review in 2 weeks (if nobody does it before).
comment:12 by , 8 years ago
Testing the patch, i got some errors like ERROR: Script message handler OnGlobalAttacked failed ERROR: Error in timer on entity 1, IID 57, function MissileHit: ReferenceError: cmpAttackOwnership is not defined
But the logic is ok. And it works as expected in game for the damage part.
I suggest to get rid of this entityOwner map. I suggest you yo use the attackerOwner from PerformAttack and add it in data in all MT_Attacked message and in data parsed in CauseDamage, CauseSplashDamaged, and as arg in AttackAlert function. Tell me if i miss something.
(Then we will have to look if there is issue with Guard and AI (see Guard.js, UnitAI.js, AIProxy.js). But it should be ok.)
by , 8 years ago
Attachment: | DamageAfterDeathOfAtacker_v4.patch added |
---|
Removal of the entityOwner map and some more cleanup
comment:13 by , 8 years ago
I tested that error and concluded that it only happens in games created before adding the entityOwner map to the init function, so that shouldn't happen anymore.
This patch should pass the attacker owner as an argument to AttackAlert in AttackDetection.js using the passed data. I did a few tests and everything seems to work at the moment.
All the for-loops in the Damage component should now also be in the form of "for (let foo of bar)"
comment:14 by , 8 years ago
Thanks for being so fast and working on that. That should be ok now.
(In binaries/data/mods/public/simulation/components/Attack.js, we can perhaps do
let attackOwner = Engine.QueryInterface(this.entity, IID_Ownership).GetOwner()
just before L484 as it is uses twice in the data stuff. But don't upload a new patch only for that.)
I will now check what happens with Guard and AI (if there could be some owner issue). Then we need to look at writting tests and finally get another team member overview.
comment:15 by , 8 years ago
Keywords: | rfc added; review removed |
---|
@LeanderH, I do really appreciate your work, but per the new SubmittingPatches guidelines, tests for altered and added simulation code should also be provided.
We were having more and more code stability issues, and by adding tests, we hope to be able to improve stability of code. Thanks a lot for wanting to work on this.
See f.e. #4121 on how to do this.
by , 8 years ago
Attachment: | DamageAfterDeathOfAtacker_v5.patch added |
---|
Adds a test for the new damage component
comment:16 by , 8 years ago
This patch should implement a damage test, testing the changed functions. I rewrote it a couple of times, so please let me know if there are any traces left from previous versions (unneeded variables, ...).
Currently the test runs fine on my system.
Of course, all feedback is welcome.
comment:17 by , 8 years ago
Thank you for working on this, some comments I made on IRC, only style related, a team programmer will probably have functionnality comments
<Stan> Not much to say on the above patch <Stan> typo l23 on damage.js <Stan> some vars could be let l203-204-205-206 <Stan> line 50 of Damage.js, maybe d could be replaced by something more meaningful <Stan> (distance maybe ?) <Stan> for functionnality and cleanliness I guess you have more knowledge elexis
comment:18 by , 8 years ago
Thank you very much for the patch. I will (pre)review that quickly. (We need to well document components/interfaces and use the adhoc jsdoc tags in the new Damage component wich was missing in the old data helper, but don't upload a new patch for that before the review).
comment:19 by , 8 years ago
- Nice line-ending awareness ;)
Some style things:
- Damage.js:
- L11 missing spaces
- L20 unneeded parenthesis
- L23 use actual JSdoc syntax
- L33 newline
- L44 unneeded parenthesis
- L116 unneeded comment
- Use let everywhere
- L145, L150 if( -> if (
- L147 148 can be merged
- L164 lines should end with a semicolon or operator
- L196 and following unneeded comments
- L214 various missing spaces
- L236 use
.map()
or construct that using the number of players, perhaps usingnew Array().fill()
test_Damage.js
- L27 missing spaces
I'm happy to leave the logic checks up to fatherbushido :p
comment:20 by , 8 years ago
Few more things:
test_Damage.js
:
- Omit the unneeded return using
() => ({ "foo": bar })
Damage.js
:
- L18 Would move the first argument also to a newline
Attack.js
:
- L555 too much indentation
- L569 should there be a
+
?
comment:21 by , 8 years ago
(Pre)Review of the patch
From the logic point of view :
Attack.js
- L484 Perhaps you have to add a check. So an early return or throw an error like
let cmpOwnership = Engine.QueryInterface(this.entity, IID_Ownership); if (cmpOwnership) error("!PerfromAttack called with an invalid ownership") else let attackerOwner = cmpOwnership.GetOwner();
- L569 data.range = this.template.Ranged.Splash.Range; -> data.radius = this.template.Ranged.Splash.Range;
AttackDetection.js
- L61 same Ownership check as in attack ? (You can also use cmpPlayerTarget = QueryOwnerInterface(target) to merge some lines)
- perhaps use let playerID = cmpPlayer.GetPlayerID() as it is use three times sin that function.
- L65 can be remove
From the formal point of view :
I know that most of the formal issues were not introduced by you, but while at moving that part of the code, we have to fix them.
- see all elexis remarks
- in objects use space after { and before }
- when a function have a JSDoc comment, fix it in using the right JSDoc tags, like (from JSDoc doc):
/** * Assign the project to an employee. * @param {Object} employee - The employee who is responsible for the project. * @param {string} employee.name - The name of the employee. * @param {string} employee.department - The employee's department. */
- Btw notice that the comment of Damage.TestCollision "point = <Vector2D>" is wrong as point is a Vector3D (or an object { "x": number, "y": number, "z": number })
- Look at the message documentation in binaries/data/mods/public/simulation/components/interfaces/Damage.js and binaries/data/mods/public/simulation/components/interfaces/Attack.js
Review of the test
Here you can perhaps only focus on the Damage part and not the whole process. Imo you can only test the Damage functions: mock almost all and just parse data through CauseSplashDamage and CauseDamage and check that the sent message is the good.
by , 8 years ago
Attachment: | DamageAfterDeathOfAtacker_v6.patch added |
---|
Added correct JSDoc to the damage component and some changes based on feedback.
comment:22 by , 8 years ago
First of all, thanks for the feedback Stan, elexis and fatherbushido. This patch should (hopefully) implement the requested changes.
Another change I made is to the TargetKilled function (L271 of Damage.js). I added the attackerOwner as parameter to allow the retreval of the StatisticsTracker component from the attacker's player entity, even when the attacker is dead.
comment:23 by , 8 years ago
let cmpOwnership = Engine.QueryInterface(this.entity, IID_Ownership); let attackerOwner; if (!cmpOwnership) error("PerformAttack called with an invalid ownership component"); else attackerOwner = cmpOwnership.GetOwner();
use early return instead of displaying an error there is no need to continue
let cmpOwnership = Engine.QueryInterface(this.entity, IID_Ownership); if (!cmpOwnership) return; let attackerOwner; = cmpOwnership.GetOwner();
For the rest I can't really tell for now, so I'd leave that to fatherbushido, and elexis
comment:24 by , 8 years ago
Thank you for the update of the patch, that sounds very good, i will do (i hope tommorow) a (last) review of it ; if it's ok we will put it in the review queue.
EDIT:
i modified directly some stuff in the last patch:
- when you change d in distance, one was forgotten
- I rewrite the first part of AttackDetection using QueryOwnerInterface
- in PerformAttack, i don't recompute the owner for capture
- I update the messages doc in interface
- take care in the version i upload there is not the test
- i will now check the docs, check the tests and do a final test.
Say me if you see something wrong.
by , 8 years ago
Attachment: | DamageAfterDeathOfAtacker_v6-1.patch added |
---|
comment:25 by , 8 years ago
That patch looks fine to me. I don't see anything wrong with it. I do have noticed that my last patch broke the test. The only thing missing is a mock for the function IsInWorld in the target's position component. That should be fixed in my next patch. I also decided to merge two if-statements in AttackDetection.js (L64 and L67 in v6-1) to make the code look cleaner.
comment:26 by , 8 years ago
Thx for your patience and your work!
- You forgot to add Damage component and Damage interface in the last patch
- Test seems ok (I would not have do it like that but it's only a test)
- In game tests seems ok
When the 2 files will be added, change the tag rfc into review.
comment:27 by , 8 years ago
Keywords: | review added; rfc removed |
---|
This should have added the two missing files.
comment:28 by , 8 years ago
Keywords: | rfc added; review removed |
---|
Note that some of the issues are in the code moved, and should not be part of this patch, but of a followup patch that. I also didn't explicitly check for issues introduced in the moved code (didn't diff the removed against the added code).
Attack.prototype.PerformAttack
- should not abort if there is no ownership (and if it is -1 there is some bug somewhere, so we should expose it by just making something fail loudly), this is a behaviour change that should not be part of this patch (not that I see any value in having that in another).
AttackDetection.prototype.AttackAlert
- shouldn't return if
cmpPlayer
is not set, but error out (as in just omit the check). attackerOwner
should only be used as a fallback in case the attacker does not exist anymore (so in case!cmpAttackerOwnership
(or.GetOwner() == -1
), as in the current patch theattacker
parameter is unused).- Check for owner and not for equivalence of components (shouldn't matter, but the previous code expressed intent)
- shouldn't return if
Damage.prototype.TestCollision
- Checks for
!targetPosition
twice, remove the latter check. - Since we have
if (foo) return bar;
the else (and the additional indentation of that block) isn't needed.
- Checks for
Damage.prototype.MissileHit
playersToDamage
should most likely be inlined or at least the if setting it should do some inliningQueryPlayerIDInterface(data.attackerOwner).GetEnemies()
.- Should we really return if
!targetPosition
instead of only failing in the one case where that is actually needed? - The
cmpProjectileManager.RemoveProjectile(data.projectileId);
call in the loop is wrong, and that is done for both the if and the else branch, so move that there. This also means that the early return likely doesn't remove the projectile. - Also the loop seems wrong as it will damage all nearby units, unless I am missing something.
Damage.prototype.CauseSplashDamage
missing spaces after some colons in the data passed toCauseDamage
.Damage.prototype.CauseDamage
only usescmpHealth
in one if, but returns if it isn't present (though IIRC theDamageReceiver
code will likely fail somewhere, but that should be fixed there IMO; still dealing damage to something without health sounds like an issue, so that should be exposed)Damage.prototype.EntitiesNearPoint
- should probably use a new function in
cmpPlayerManager
to get all the player ids instead of usingPlayerManager.prototype.GetAllPlayerEntities
which should be removed. (Though this is definitely out of scope for this ticket) - The naming of variables should be fixed, but L260-262 should just be inlined.
- should probably use a new function in
Damage.prototype.TargetKilled
should use the owner ofkillerEntity
(can we get a better name for that?) before falling back onattackerOwner
.- A test utilizing
cmpAttack
to test whether the damage is dealt when the attacker is killed after firing would be great (as that could be used to test that the current code is broken, and that this patch fixes it). (Basically a regression test)
Nice work so far, keep it up.
comment:29 by , 8 years ago
Thanks for the review. This patch should hopefully contain the requested tweaks.
As for the loop in Damage.prototype.MissileHit
, The enemies of the attacker are passed to the ExecuteQueryAroundPos function, so it looks like it only targets the enemies in range.
by , 8 years ago
Attachment: | DamageAfterDeathOfAtacker_v9.patch added |
---|
Contains changes based on review.
comment:30 by , 8 years ago
Attack.prototype.PerformAttack
- The
owner != -1
check in the capture case ofAttack.prototype.PerformAttack
should be kept (for that case only). - L586 missing space after
:
.
- The
AttackDetection.prototype.AttackAlert
?:
has lower operator precedence than&&
so remove the parenthesis.- Use
playerId
instead ofEngine.QueryInterface(this.entity, IID_Ownership).GetOwner()
in the if. (The point of this ticket is not to add behaviour changes, apart from the one that is done by the move to a system component) - Also move the
cmpAttackerOwnership
init/decl to where it was previously.
Damage.prototype.Init
put the{
on a new line, and the};
on another line (same as egcmpWonder
).Damage.prototype.TestCollision
- remove the braces for the
if (foo) return bar;
, add a blank line after the return. - I'd remove the empty line after the
angle
decl, and merge thedistance
lines.
- remove the braces for the
Damage.prototype.MissileHit
- Given that the
projectileId
is not used in any of the called functions, the removal of the projectile could happen right at the start, thus removing the duplication of those two removals. - Yes, the loop only damages entities in range, but it does target all of them, not only one (which means actually not hitting the wanted unit, but all at the position; regardless of splash damage!). (This could be handled in another ticket)
- As the removal of the projectile now happens at the start, we could just return in the
if (testCollision)
on L112, then we don't need the else or the additional level of indentation.
- Given that the
Damage.prototype.CauseDamage
- move
cmpHealth
closer to where it is used. - add an empty line before
Engine.PostMessage
- move
Damage.prototype.TargetKilled
- Missing
let
- Typo in the variable name
- Could use
let attackerOwner = Engine.QueryInterface(attacker, IID_Ownership).GetOwner() || attackerOwner;
'iff' that doesn't error in case the first thing fails.
- Missing
- It would be great if the regression test would actually be a test of
cmpAttack
instead ofcmpDamage
, but that doesn't work since we'd have to change the test if we'd add that before the rest (to verfiy that it is indeed broken before the patch and fixed afterwards), so ignore this bullet point.
by , 8 years ago
Attachment: | DamageAfterDeathOfAtacker_v10.patch added |
---|
Fixed some mistakes and changes based on feedback.
comment:31 by , 8 years ago
Thanks again. I tested the proposed change of the definition of attackerOwner in Damage.prototype.TargetKilled
, but it does cause an error if the component is null. I just left it as it was, since that seems to be the cleanest I can make it.
comment:32 by , 8 years ago
AttackDetection.prototype.AttackAlert
- Why did you remove
cmpTargetOwnership
instead of using that (that one check was nice as it was before the patch)?
- Why did you remove
Damage.prototype.MissileHit
- Adding a comment before L109 that this is checking for direct hits would be nice. eg
// Direct hits
- 'if(' -> 'if (' as per wiki:Coding_Conventions
- Adding a comment before L109 that this is checking for direct hits would be nice. eg
Damage.prototype.EntitiesNearPoint
- The comment on L249 is invalid, as the next line doesn't use a dummy entity, and hasn't done so for quite some time, nuke that.
Damage.prototype.TargetKilled
- Does not check for owner != -1 before using it, should fix that by using the same check as you already have in another place.
Looks good apart from that, I'll look into getting it committed this weekend.
comment:33 by , 8 years ago
This patch should take care of those things. I reintroduced the original check in AttackDetection.prototype.AttackAlert
, but I didn't reintroduce the variable because it's only used once.
comment:34 by , 8 years ago
If i might suggest some formal things:
Damage.js
-L34 int -> number
-L36 int -> number
-L70 { 'hack': <float>, 'pierce': <float>, 'crush': <float> } -> { "hack": number, "pierce": number, "crush": number }
-L146 { 'hack': <float>, 'pierce': <float>, 'crush': <float> } -> { "hack": number, "pierce": number, "crush": number }
-L150 [data.playersToDamage] -> data.playersToDamage
-L201 { 'hack': <float>, 'pierce': <float>, 'crush': <float> } -> { "hack": number, "pierce": number, "crush": number }
-L204 float -> number
-L234 [players] -> players
by , 8 years ago
Attachment: | DamageAfterDeathOfAtacker_v12.patch added |
---|
Some minor changes to the JSDocs.
comment:35 by , 8 years ago
Thanks for the feedback. This patch should implement those changes. So I don't need to specify which parameters are optional using square brackets?
by , 8 years ago
Attachment: | DamageAfterDeathOfAtacker_v13.patch added |
---|
Re-added the optional parameter indicators.
comment:37 by , 8 years ago
No problem. The coding conventions don't really specify anything about optional parameters, so I wanted to be sure. This patch should add them back.
comment:40 by , 8 years ago
Keywords: | simple rfc removed |
---|
Thanks for the patch.
I split out the regression test (and added a comment about what we are testing for) into a commit of its own. Also sorted the scripts that are loaded.
In the second patch I fixed the removal of projectiles (after checking the code again, sorry for not having done that before) as misses should leave the projectile, which will be removed after some delay by the ProjectileManager. In addition to that I fixed the case where the target isn't hit, but there are other possible targets nearby the location where the projectile hit, which previously caused all possible targets to be damaged (which would add splash damage without damage scaling to projectiles not having that property).
Can I look forward to you working on one of the related tickets listed in the description?
Have a look at #1910. Also #1911 #1909 might help you design it.