Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DamageTypes to Kill() and make some traits use it. #14777

Merged
merged 1 commit into from Mar 8, 2018

Conversation

Projects
None yet
4 participants
@MustaphaTR
Copy link
Member

MustaphaTR commented Feb 3, 2018

I gave it a default value of null, so it doesn't have to be defined and act like it is now without that.

This changes /kill command and Chronoshiftable to use Kill, instead of InflictDamage.

There are 2 testcases, one on Demo Truck which makes it only explode by AttackSuicides, KillsSelf(IC) or Chronoshiftable, but not direct damage. Kinda like Generals GLA Terrorist. Other one makes D2K Sardaukar only explode when crushed.

If you think another usage of Kill() may benefit from having DamageTypes, i can add that too.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:damage-types branch 2 times, most recently from 0da415e to 886f22f Feb 3, 2018

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Feb 3, 2018

If you think another usage of Kill() may benefit from having DamageTypes, i can add that too.

AttackLeap, please.

@@ -20,6 +20,9 @@ namespace OpenRA.Mods.Common.Traits
[Desc("Does a suicide attack where it moves next to the target when used in combination with `Explodes`.")]
class AttackSuicidesInfo : ITraitInfo, Requires<IMoveInfo>
{
[Desc("Types of damage that this trait causes to self while suiciding. Leave empty for no damage.")]

This comment has been minimized.

@GraionDilach

GraionDilach Feb 3, 2018

Contributor

"Leave empty for no damage types." maybe?

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:damage-types branch from 886f22f to b8df44b Feb 4, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

MustaphaTR commented Feb 4, 2018

Updated.

@@ -321,12 +321,12 @@ public void InflictDamage(Actor attacker, Damage damage)
health.InflictDamage(this, attacker, damage, false);
}

public void Kill(Actor attacker)
public void Kill(Actor attacker, HashSet<string> damageTypes = null)

This comment has been minimized.

@GraionDilach

GraionDilach Feb 10, 2018

Contributor

I'd set this to an empty hashset tbh.

This comment has been minimized.

@MustaphaTR

MustaphaTR Feb 10, 2018

Author Member

I tryed that but it didn't work, i don't remember the exact error and not at home right now, so can't redo it to see.

This comment has been minimized.

@abcdefg30

abcdefg30 Feb 10, 2018

Member

Default values must be "compile time constants". It wouldn't surprise me if an empty hashset wasn't one.

This comment has been minimized.

@MustaphaTR

MustaphaTR Feb 11, 2018

Author Member

Yes, that was the error.

This comment has been minimized.

@GraionDilach

GraionDilach Feb 23, 2018

Contributor

Nevermind me then.

@@ -163,9 +164,12 @@ public void InflictDamage(Actor self, Actor attacker, Damage damage, bool ignore
}
}

public void Kill(Actor self, Actor attacker)
public void Kill(Actor self, Actor attacker, HashSet<string> damageTypes = null)

This comment has been minimized.

@GraionDilach

GraionDilach Feb 10, 2018

Contributor

And then remove the optionality from here.

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

This probably needs a rebase now with the viscfusing PR, but then 👍.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:damage-types branch from b8df44b to 9f482ea Feb 24, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

MustaphaTR commented Feb 24, 2018

Couldn't find anything in Visceroid PR that should be effecting this, but Conditional Attack Suicides were requiring this to be rebased, so rebased it.

Edit: AppVeyor looks broken, i think you can ignore it.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Mar 8, 2018

Note to other reviewers: this is a step towards #12802.

@reaperrr reaperrr force-pushed the MustaphaTR:damage-types branch from 9f482ea to c8ea4d9 Mar 8, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Mar 8, 2018

Looks good to me. I've removed the testcases and fixed a typo I found and will merge this once Travis/AV have run through.

@reaperrr reaperrr merged commit 5e7e3bb into OpenRA:bleed Mar 8, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MustaphaTR MustaphaTR deleted the MustaphaTR:damage-types branch Mar 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.