#1910 closed task (fixed)
[PATCH] Unit death damage
Reported by: | Pureon | Owned by: | Mate-86 |
---|---|---|---|
Priority: | Nice to Have | Milestone: | Alpha 23 |
Component: | UI & Simulation | Keywords: | patch |
Cc: | Josh | Patch: |
Description (last modified by )
When a unit is destroyed the destruction causes damage to nearby units.
This will be configured in the unit template.
Attachments (13)
Change History (62)
comment:1 by , 11 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
by , 11 years ago
Attachment: | playerMgr.diff added |
---|
comment:2 by , 11 years ago
Cc: | added |
---|---|
Milestone: | Backlog → Alpha 14 |
Priority: | If Time Permits → Nice to Have |
For this ticket I've been trying to set up a new "Damage" component with more flexible functions for both splash and normal damage. I'm hoping to get this into alpha 14 and I'm bumping the priority also because as things are now, splash damage is hardcoded to only work with projectiles.
comment:3 by , 11 years ago
Not a full review or anything, but from a quick glance I'm a bit confused as to why you choose to implement this as a system component? Why not make it a regular component that can be used by templates that require the splash damage effect? Gameplay components should ideally never be implemented as system components, since they need to be hardcoded into the engine, reducing moddability.
I would also suggest giving your component a more descriptive name, like DeathBlast
or something, or people will confuse it with other components concerned with damage.
comment:4 by , 11 years ago
Well my idea with this was to entirely abstract damage so that it was independent of various attack types. This would facilitate modders who would be able to discard the functions/logic in attack.js but keep these functions in Damage.js.
For example, lets say a modder is building a game where natural disasters can happen. They would be able to construct new functions that call, for example, SplashDamage if an earthquake was triggered. This would be entirely independent of templates and (most) other gameplay components.
This also helps with future damage-related functions which would be able to be consolidated into one component instead of being scattered among many components as they are now.
Another more concrete problem with moving the template data from Attack.js is that Attack.js still needs much of that template data so if it was moved it would effectively be a copy. That could become very problematic to maintain and synchronize.
by , 11 years ago
The new component with all essential functions working (At the moment this is more relevant to #1496, but I'm posting it here for consistency)
by , 11 years ago
Attachment: | Damagev2.diff added |
---|
Diff of all changes to hopefully fix #1496 including test functions.
comment:5 by , 11 years ago
Milestone: | Alpha 14 → Backlog |
---|
I'm not going to finish this for awhile, the attached files are primarily for #1496.
comment:6 by , 10 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
As I've moved on to mostly UI/network programming in 0AD, this ticket would serve better as a starter task for someone new to the simulation. The changes should be mostly contained in Attack.js and Damage.js. My patches can all be ignored as irrelevant for any future work on this ticket (much more refined versions were committed to fix #1496 a few months ago).
comment:7 by , 10 years ago
I just had a look at this issue. I think it is obvious that the splash damage should depend on the unit type. Is there any suggestion which unit properties should define the strength and the radius of the splash damage? I don't think the damage caused upon death of the unit should be the same as the damage caused by a regular attack. In my opinion this would mean to define a fourth type of attack beside hack, pierce and crush.
comment:8 by , 10 years ago
Yes, you need a new attack type, but besides "ranged", "melee", "charge", "slaughter" and maybe others. Not anew attack strength type.
comment:9 by , 8 years ago
Owner: | set to |
---|
comment:10 by , 8 years ago
I wanted to provide a similar solution like the splash damage of ranged attack (eg. onager) but the existing solution for that does not work for when I created a test map. I've checked the code and found these:
1) simulation/components/Damage.js:90 says let playersToDamage = !data.friendlyFire ? QueryPlayerIDInterface(data.attackerOwner).GetEnemies() : null;
but data.friendlyFire
is always true because the string "false" is assigned to it from the template and non-empty strings are interpreted as boolean "true"
2) The same line says if the friendlyFire == true
then playersToDamage = null
but this is wrong because Damage.js:241-242 says if (!origin || !radius || !players) return [];
. Which means the players should not be null.
Could you please confirm if I understood the problem correctly? Shall I fix this issue in my patch #1910?
comment:11 by , 8 years ago
Cc: | added; removed |
---|---|
Keywords: | simple review added; Simple removed |
Milestone: | Backlog → Alpha 21 |
Nice find! I should have checked the patch at #3610 a bit better. The second issue was an oversight in r18645 which indeed breaks splash damage for non-friendly-fire cases.
CCing fatherbushido since we should fix that before the release, else I'd have suggested that you fix this, but in a separate patch. (fatherbushido: Keep this ticket open, I just don't want to open a new ticket just for this fix.)
The playersToDamage
fix is a bit long, so in case someone wants to make that shorter, please do. Also some tests that help catch this issue would be nice (as in friendly-fire splash damage).
EDIT: Not tested, but should probably work.
by , 8 years ago
Attachment: | 1910_splash_damage_broken.patch added |
---|
comment:12 by , 8 years ago
Cc: | added |
---|
I do still actually monitor all this. Re-adding myself as CC'ed.
follow-up: 18 comment:13 by , 8 years ago
Thanks Mate-86!
leper: I should be blamed too.
(It was hard to noticed as in the current game only the quinquereme has the friendly fire. It was removed some years ago for the catapult.)
I tested the patch wich works as expected. I will look at writing a test for that. We can reduce the L94-L98 with something like
playersToDamage = [...Array(Engine.QueryInterface(SYSTEM_ENTITY, IID_PlayerManager).GetNumPlayers()).keys()];
but I don't know if it's the good way to do.
I wonder too if we should not swap the if and the else (though it makes sense to use the generic case before).
by , 8 years ago
Attachment: | 1910-death-damage-1.patch added |
---|
comment:14 by , 8 years ago
Thx for the feedback and for the fix! Reusing this splash damage fix I've created a patch for the original problem (dying unit causing damage). Some questions:
- Is "Death" the proper name for this attack type?
- Are there any example units intended to have death attack? Currently the slinger is used for an example.
- Does directed (linear) death damage make sense or only circular?
- Shall I update the tooltips? (tech tree + session)
Unit tests coming soon ...
comment:15 by , 8 years ago
mate-86: if you could first make a test for the leper's fix, it would be nice!
comment:16 by , 8 years ago
Manual test in the game was successful for splash damage. I can create the unit tests as well.
comment:18 by , 8 years ago
Replying to fatherbushido:
We can reduce the L94-L98 with something like
playersToDamage = [...Array(Engine.QueryInterface(SYSTEM_ENTITY, IID_PlayerManager).GetNumPlayers()).keys()];but I don't know if it's the good way to do.
Benchmark both versions, likely decide that the performance impact is minimal, likely use the shorter version.
Also using a function like in Mate-86's last patch is a nice idea, though that could be made shorter by just returning instead of keeping the full if-else; also passing the data object to it directly might be worth considering.
comment:19 by , 8 years ago
I think I will commit it this evening, then in the Mate-86's patch, he could use that new function (as it will be use 2 times).
comment:20 by , 8 years ago
I'm struggling with the unit test and I won't be available in the weekend. I can finish that next Monday-Tuesday the earliest. Feel free to take it over if it's urgent for the release.
comment:22 by , 8 years ago
Keywords: | rfc added; review removed |
---|
comment:26 by , 8 years ago
First one looks good, that bug seems to have been there for ages, nice find.
The second one is the result of me trying to fix something else, then not doing that and leaving that in. But for actual splash damage the break should be removed too.
I didn't test anything, so better ask someone else. Also someone should push this ticket to A22 at some point and possibly open a new ticket for more bugs that don't really have anything to do with this ticket.
comment:27 by , 8 years ago
Cc: | removed |
---|---|
Keywords: | patch added; simple removed |
comment:28 by , 8 years ago
Milestone: | Alpha 21 → Alpha 22 |
---|
by , 8 years ago
Attachment: | 1910-death-damage-2.patch added |
---|
comment:29 by , 8 years ago
I've uploaded a patch for the death damage with unit test. There are still some open questions:
- Is "Death" the proper name for this attack type?
- Are there any example units intended to have death attack? Currently the slinger is used for an example.
- Does directed (linear) death damage make sense or only circular?
- Shall I update the tooltips? (tech tree + session)
comment:30 by , 8 years ago
- No clue.
- As an example, I would have choose a structure like a fortress or a cc.
- You just have to use the spash damage so both could be done in the same move ? (but circular makes more sense)
- I don't know what we want to be displayed, but it's indeed better to have that too.
I have not yet look at the patch in details, perhaps someone have a clearer view of that than me, but:
- I don't know if that must be a part of attack (is it an attack ?)
- if it's in attack, perhaps you can use a splash schema to avoid duplication
- Take care to not call that too late in Health.js
Thanks for working on that part of the Bermuda Triangle #1910 #1911 #1912 (leper © expression).
by , 8 years ago
Attachment: | 1910-death-damage-3.patch added |
---|
comment:31 by , 8 years ago
- I've added example for fortress and cc but values need to be revised
- It's a bit complicated because the linear damage uses the vector pointing from the attacker to the defender. In MissileHit it's given but don't how to calculate for the DeathDamage because the defender is not specified or ambiguous then.
- Added in the patch. It looks like this: http://imgur.com/a/6k2U9
- is it an attack ? => Same for me but then please suggest another component then.
- splashSchema created to avoid duplication
- too late in Health.js => I don't see the problem here. Please clarify!
comment:32 by , 8 years ago
all that seems nice. i forgot, as leper said in a previous comment, you can save an identation level in getplayerstodamzge by doing return at l76 after the if and removing else
by , 8 years ago
Attachment: | 1910-death-damage-4.patch added |
---|
comment:33 by , 8 years ago
- I fixed getplayerstodamage to save an indentation level.
- for 7, I tried to move
CauseDeathDamage
upper in the function but it resulted run-time error. Looks like it must come afterthis.hitpoints = 0;
otherwise it would call theDestroyEntity
function again. (CauseDeathDamage
kills the originator entity itself again.)CauseSplashDamage
does not get any info whether it shoue attacker or not.er or not.
comment:34 by , 7 years ago
Summary: | Unit death damage → [PATCH] Unit death damage |
---|
comment:36 by , 7 years ago
Partial review:
Templates L147 not sure if that comment is very useful, probably just leave it there because of consistence in the file.
Attack L194 destroyed L632 test for "cmpPosition" too. L646, I don't think this attack should have a direction now, but that is a design decision, which is not up to me.
Let the splash attack also use the new function GetPlayersToDamage instead of checking that in damage.js
The UnitAI will treat this new attack type as a new way of attacking, so it will perform this attack when the unit is not even close to dying. To prevent this "abuse" the CanAttack and GetBestAttackAgainst function in attack.js should exclude this type of attack.
Damage L77 var --> let
Test_attack L146, L205, L208 use { "foo": bar } not {foo: bar} L192 var --> let
by , 7 years ago
Attachment: | 1910-death-damage-5.patch added |
---|
follow-up: 38 comment:37 by , 7 years ago
@bb: Thx for checking! Most of the issues fixed in the new patch. Others:
- "Let the splash attack also use the new function
GetPlayersToDamage
instead of checking that in damage.js" => Sorry I don't understand this one. Could you please tell me the lines in the code which need to be changed? - In Attack.js both
CanAttack
andGetBestAttackAgainst
useGetAttackTypes
which returns a hard-coded list not including death damage (L207).
comment:38 by , 7 years ago
- "Let the splash attack also use the new function
GetPlayersToDamage
instead of checking that in damage.js" => Sorry I don't understand this one. Could you please tell me the lines in the code which need to be changed?
Actually I was mistaking here too, anyway L112 of damage should be merged with L121(?) (current L110), then the code is correct imho. (So nuke the playersToDamage
var).
besides that some more: Test_attack L85 "false" --> false "L146, L205, L208 use { "foo": bar } not {foo: bar}", look at whitespace too...
I don't know if balance is alright since nearby citizen skrims just get killed when destroying a cc, but at least the range needs an increase since the attack range is inside foorprint thus there is no attack.
comment:39 by , 7 years ago
Ok, another patch should fix those. I tweaked the range a bit: http://imgur.com/a/KmjpJ
by , 7 years ago
Attachment: | 1910-death-damage-6.patch added |
---|
comment:40 by , 7 years ago
I think, the GetPlayersToDamage
is wrong enough. Because, @param {boolean} friendlyFire - a flag indicating if allied entities are also damaged
and you return GetEnemies()
if !friendlyFire
, but there are neutral players except allies.
So you should return GetEnemies() + GetNeutrals()
, but GetNeutrals()
isn't defined, so you could use GetEnemies().concat(GetPlayersByDiplomacy("IsNeutral"))
(or define this function and use GetEnemies().concat(GetNeutrals())
).
Also, I think, instead of the loop you could use the same thing:
return GetPlayersByDiplomacy("IsNeutral").concat(GetAllies(), GetEnemies());
I'm not sure that we should have this radius of damage, buildings don't have barrels of gunpowder. Probably, make the radius smaller and damage too?
The rest of the patch looks good.
by , 7 years ago
Attachment: | 1910-death-damage-7.patch added |
---|
comment:41 by , 7 years ago
@vladislavbelov thx for checking the patch! I've adressed your comments in 1910-death-damage-7.patch.
comment:42 by , 7 years ago
Don't focus on the example. I don't even know if we will use it in the 'main' game, but for mods sure it's nice. I am not able to review the patch atm but the part in Health.js needs to be carefully checked.
Meanwhile, you can perhaps works on the other ticket ?
comment:43 by , 7 years ago
You could write this:
if (!friendlyFire) return nonAllies; return nonAllies.concat(player.GetAllies());
Shorter:
return friendlyFire ? nonAllies.concat(player.GetAllies()) : nonAllies;
by , 7 years ago
Attachment: | 1910-death-damage-8.patch added |
---|
comment:44 by , 7 years ago
- I've adressed the latest comment from vladislavbelov in patch-8
- I've put some questions in 1911 (seems to depend on 1909 which has some additional questions to clarify)
- for 1912 I can look at the patch submitted by Leander
comment:45 by , 7 years ago
- We're using
{
on the next line instead of sameif (type == "Death") {
. - Do we need this comment here:
"friendlyFire": template.Attack.Death.FriendlyFire != "false" // true if undefined
, isn't obvious?
comment:46 by , 7 years ago
1.fixed
- the
// true if undefined
is already used in trunk, shall I remove that too? http://trac.wildfiregames.com/browser/ps/trunk/binaries/data/mods/public/globalscripts/Templates.js#L138
comment:47 by , 7 years ago
Description: | modified (diff) |
---|
Sorry Mate-86 for the delay, are you still interested on that one? If so you can create an account on code.wildfiregames.com and upload the patch here. I read some stuff.
From a gameplay point of view that things could be used in main mod for the iberian ship or the fireraiser.
Concerning the current patch, I guess we could get rid of the Attack.prototype.CauseDeathDamage function. Moreover, even if it makes sense to have the stats in the Attack component as it's nothing else than a splash damage, I would be more for putting them in a small component like "Explosion" or "ExplodeOnDeath" or something like that which call directly CauseSplashDamage. Both (having that in Attack or not) have pros and cons. That can then listen to death message (bad) or directly call from the Health component like you did (better). EDIT: keeping like in the patch is perhaps better :p
comment:51 by , 7 years ago
Keywords: | rfc removed |
---|---|
Milestone: | Work In Progress → Alpha 23 |
Thanks for that work started one year ago.
Diff of simulation/components/PlayerManager.js