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 ArmorTypes to HitShape and adapt damage warheads #14991

Merged
merged 2 commits into from Oct 26, 2018

Conversation

Projects
None yet
4 participants
@reaperrr
Copy link
Contributor

reaperrr commented Mar 25, 2018

Supersedes #13675.

This allows to assign Armor traits to specific HitShapes, allowing to make certain parts of actors more vulnerable or durable than others.
Can also be used to simulate directional armor in a good-enough way that this closes #9268 and therefore closes #7718.
Also closes #13755.

Testcase can be found here: https://github.com/reaperrr/OpenRA/tree/hitshape-armor-v2-test

The main problems of #13675 that brought it down,

The core of the problem is that this introduces a second channel for applying damage (DoImpact with and without a hitshape) right in the middle of a chain of inheritance and overrides. This makes it very difficult to know which will be used, and means that the same weapon/warheads/vs fired by different means will end up doing different amounts of damage to the same actor.
This feature really needs to be integrated from the bottom up, and not just bolted on the side. Require all damaging warheads to specify the hitshape, and fix the higher level code to be able to choose the appropriate one based on whatever rules make sense in each case.

should be adressed by this updated version.

All damage warheads now absolutely require an enabled hitshape to deal damage to actors, and now that we have the Polygon shape type, there's no real excuse for an actor to have overlapping shapes anymore, so just taking the shape closest to impact for damage calculations should be a sufficient solution.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Apr 30, 2018

A lint rule to detect overlapping hitshapes would be nice.

@AoAGeneral AoAGeneral referenced this pull request May 26, 2018

Merged

TD Balance 20180520 #15150

@reaperrr reaperrr force-pushed the reaperrr:hitshape-armor-v2 branch from b3d0281 to edd31fc Sep 27, 2018

Show resolved Hide resolved OpenRA.Mods.Common/Warheads/DamageWarhead.cs
DoImpact(victim, firedBy, damageModifiers);
{
// Cannot be damaged without a Health trait
if (!victim.Info.HasTraitInfo<HealthInfo>())

This comment has been minimized.

@pchote

pchote Sep 27, 2018

Member

Can we move this inside IsValidAgainst? Same comment for the other warheads.

if (!IsValidAgainst(victim, firedBy))
continue;

var impactToVictimDistance = new WDist((victim.CenterPosition - pos).Length);

This comment has been minimized.

@pchote

pchote Sep 27, 2018

Member

You take the .Length, wrap it in a WDist, only to call .Length on that again below!
IMO use (victim.CenterPosition - pos).Length directly in the lerp.

@reaperrr reaperrr force-pushed the reaperrr:hitshape-armor-v2 branch 2 times, most recently from e0c8c29 to ad5dd9e Sep 28, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Sep 28, 2018

Updated.

@reaperrr reaperrr force-pushed the reaperrr:hitshape-armor-v2 branch from ad5dd9e to b04c3c3 Sep 28, 2018

// FindActors*On*Circle is needed to include actors who are close enough to Spread[0] that their HitShape might intersect with it.
// However, we only want to exclude actors where CenterPosition is inside Range[1], so keeping *InCircle is correct there.
// TODO: Either properly account for hitshape intersection with Spread[1] instead, or drop ring support entirely.
// None of our shipping mods currently use this at time of writing.

This comment has been minimized.

@reaperrr

reaperrr Sep 28, 2018

Contributor

Note: If neither the TODO nor then 2nd option are good enough for this PR, I'll need help coming up with a way to check the hitshape intersection, I can't get my head around how to check the reverse of what we normally do.

This comment has been minimized.

@pchote

pchote Oct 1, 2018

Member

Lets remove the ring code. I don't understand why we had this in the first place.

@reaperrr reaperrr force-pushed the reaperrr:hitshape-armor-v2 branch from b04c3c3 to 9528c1c Oct 1, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Oct 1, 2018

Rebased and updated.

Removed ring code, changed ArmorTypes to BitSet.

@reaperrr reaperrr force-pushed the reaperrr:hitshape-armor-v2 branch from 9528c1c to 1cdbeab Oct 14, 2018

@abcdefg30
Copy link
Member

abcdefg30 left a comment

Looks good to me. 👍


var closestSpreadPos = WPos.Lerp(pos, victim.CenterPosition, Spread.Length, (victim.CenterPosition - pos).Length);
var closestActiveShape = victim.TraitsImplementing<HitShape>().Where(Exts.IsTraitEnabled)
.MinByOrDefault(t => t.Info.Type.DistanceFromEdge(closestSpreadPos, victim));

This comment has been minimized.

@abcdefg30

abcdefg30 Oct 15, 2018

Member

Calling DistanceFromEdge twice is probably not too expensive, but we could avoid it by using Select(t => t.Info.Type.DistanceFromEdge(closestSpreadPos, victim) before MinByOrDefault (and adapting the following if check).

if (!IsValidAgainst(victim, firedBy))
continue;

var closestSpreadPos = WPos.Lerp(pos, victim.CenterPosition, Spread.Length, (victim.CenterPosition - pos).Length);

This comment has been minimized.

@pchote

pchote Oct 25, 2018

Member

If Spread.Length is bigger than (victim.CenterPosition - pos).Length this will lerp past the center of the actor and out the other side!

I'll push a fix to this alongside a few other cleanups.

continue;

// Fully damage actors whose HitShape intersects with warhead Spread, otherwise don't damage at all
var distance = closestActiveShape.Info.Type.DistanceFromEdge(pos, victim);

This comment has been minimized.

@pchote

pchote Oct 25, 2018

Member

This doesn't account for the spread at all!

I'll push a fix to this alongside a few other cleanups.

reaperrr added some commits Mar 25, 2018

Fix FindActorsOnCircle summary
The summary was not entirely correct:
Since FAOC simply adds the largest existing OuterRadius to the specified circle range, it's still possible (very likely, in fact) for this helper to return actors whose HitShape is entirely outside the specified range (*without* the added largest OuterRadius).
Make HitShape mandatory for damaging actors and refactor warheads.
* Adds support for linking Armor traits to HitShapes.
* Adds spread support to TargetDamageWarhead
* Removes ring-damage support from HealthPercentageDamage
* Removes IsValidAgainst check from DoImpact(Actor victim...) overload
  and instead lets warheads perform the check beforehand
  (to avoid HitShape look-ups on invalid targets).
* Reduces duplication and improves readability of Warhead implementations

@pchote pchote force-pushed the reaperrr:hitshape-armor-v2 branch from 1cdbeab to e90abe9 Oct 25, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 25, 2018

I have updated this on @reaperrr's behalf:

  • Removed duplicated DistanceFromEdge calls (@abcdefg30's point above)
  • Fixed spread calculations on TargetDamageWarhead and HealthPercentageDamageWarhead
  • Made HealthPercentageDamageWarhead a subclass of TargetDamageWarhead to further reduce duplication
  • Squashed commits to avoid broken compilation (which is a real pain if hit during a bisect)
  • Fixed a couple of minor code nits (mainly removing redundant variable definitions)

I've tested that regular weapons, MAD Tanks, and MAD Tanks with warheads replaced by TargetDamageWarhead appear to work as expected. There are some big changes in this push, so I would like if @abcdefg30 or @reaperrr could sign-off on this before merging.

@pchote

pchote approved these changes Oct 25, 2018

@pchote pchote added this to the Next + 1 milestone Oct 25, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 25, 2018

I think this now also closes #13755?

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Oct 25, 2018

I think this now also closes #13755?

Yep, updated original post. I'll give this a last look tomorrow.

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Oct 26, 2018

Changes look good to me and didn't spot any regressions, so lgtm.

@reaperrr reaperrr merged commit f18ce8c into OpenRA:bleed Oct 26, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

reaperrr commented Oct 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment