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 HitShape trait #13314

Merged
merged 5 commits into from Jun 3, 2017

Conversation

Projects
None yet
5 participants
@reaperrr
Contributor

reaperrr commented May 17, 2017

...move Health.Shape there, add ITargetablePositions, and remove ITargetablePositions from Building.

This is the easier, uncontroversial part of #13196 and should not cause any regressions on bleed.

Closes #7717.
Closes #10349.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 18, 2017

Contributor

I've come up with an idea for asymmetric footprint hit-shapes, based on a comment by @obrakmann on #10383.

Basically, what I've done on a local branch is moving IHitShape from Health to TargetableLocations.

This brings a number of advantages:

  • you can have multiple shapes per actor, without the need to refactor the shapes themselves
  • you can enable/disable them at will via conditions
  • you can have actors that block projectiles without the requirement for a health trait

Example, using the RA refinery:
ra-ref

BodyOrientation:
	CameraPitch: 255
TargetableLocations:
	UseFootprint: false
	TargetablePositionOffsets: 0,0,0, 0,-1024,0, 0,1024,0
	Shape: Rectangle
		TopLeft: -1536, -512
		BottomRight: 1536, 512
TargetableLocations@TOP:
	TargetablePositionOffsets: 1024,0,0
	Shape: Rectangle
		TopLeft: -512, -1536
		BottomRight: 512, -512
TargetableLocations@BOTTOMLEFT:
	TargetablePositionOffsets: -1024,-1024,0
	Shape: Rectangle
		TopLeft: -1536, 512
		BottomRight: -512, 1536
Contributor

reaperrr commented May 18, 2017

I've come up with an idea for asymmetric footprint hit-shapes, based on a comment by @obrakmann on #10383.

Basically, what I've done on a local branch is moving IHitShape from Health to TargetableLocations.

This brings a number of advantages:

  • you can have multiple shapes per actor, without the need to refactor the shapes themselves
  • you can enable/disable them at will via conditions
  • you can have actors that block projectiles without the requirement for a health trait

Example, using the RA refinery:
ra-ref

BodyOrientation:
	CameraPitch: 255
TargetableLocations:
	UseFootprint: false
	TargetablePositionOffsets: 0,0,0, 0,-1024,0, 0,1024,0
	Shape: Rectangle
		TopLeft: -1536, -512
		BottomRight: 1536, 512
TargetableLocations@TOP:
	TargetablePositionOffsets: 1024,0,0
	Shape: Rectangle
		TopLeft: -512, -1536
		BottomRight: 512, -512
TargetableLocations@BOTTOMLEFT:
	TargetablePositionOffsets: -1024,-1024,0
	Shape: Rectangle
		TopLeft: -1536, 512
		BottomRight: -512, 1536
@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 18, 2017

Member

What about actors that can take aoe damage but is not itself targetable?

Member

pchote commented May 18, 2017

What about actors that can take aoe damage but is not itself targetable?

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 18, 2017

Contributor

I guess you're right, although I can't come up with any examples (unless bridges fall into that category already?).
I still think we should untie shapes from Health though, as the above advantages would still apply.

Maybe we should move it to a dedicated HitShape trait, afterall? That way, we and/or 3rd-party modders could make other stuff like Shield traits etc. interact with shapes, while avoiding clashes with Health.

Anyway, I think we can or even should leave this PR dedicated to its original purpose then and deal with any shape trait refactoring in its own PR, but in my opinion multiple rectangle shapes are still a better solution for asymmetric structures in the legacy mods than resorting to skewing shapes (that's still a desired feature on its own, I'm just not convinced we should use it for the legacy mods).

Contributor

reaperrr commented May 18, 2017

I guess you're right, although I can't come up with any examples (unless bridges fall into that category already?).
I still think we should untie shapes from Health though, as the above advantages would still apply.

Maybe we should move it to a dedicated HitShape trait, afterall? That way, we and/or 3rd-party modders could make other stuff like Shield traits etc. interact with shapes, while avoiding clashes with Health.

Anyway, I think we can or even should leave this PR dedicated to its original purpose then and deal with any shape trait refactoring in its own PR, but in my opinion multiple rectangle shapes are still a better solution for asymmetric structures in the legacy mods than resorting to skewing shapes (that's still a desired feature on its own, I'm just not convinced we should use it for the legacy mods).

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach May 21, 2017

Contributor

What about actors that can take aoe damage but is not itself targetable?

IMO those should use a custom strictly-warheads targettype to be damaged. I plan to have such a system where actors can gain such untargetability from a weapon jammer actions (following examples I've seen in Rise of the Reds, like the Shtora of the Golem or the Anvil Bot).

Contributor

GraionDilach commented May 21, 2017

What about actors that can take aoe damage but is not itself targetable?

IMO those should use a custom strictly-warheads targettype to be damaged. I plan to have such a system where actors can gain such untargetability from a weapon jammer actions (following examples I've seen in Rise of the Reds, like the Shtora of the Golem or the Anvil Bot).

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 21, 2017

Contributor

What about actors that can take aoe damage but is not itself targetable?

Actually, now that I've thought about it some more, I think that wouldn't be a problem.

All that TargetableLocations does by default, without customization, is provide a single targetable position at the actors' CenterPosition. That's what the Target code falls back to on all actors, anyway. TargetableLocations does not really make the actor targetable afterall, that's what Targetable is for.

Basically, moving HitShapes to TargetableLocations and then adding that trait to the actors in your example should not have any impact, let alone cause any regressions, because the CenterPosition of any actor is considered a 'targetable position' on bleed already.

However, the actual problems I see are that
a) having the shape reside on TargetableLocations might be a little unintuitive to some modders, and
b) making shapes disableable via conditions will likely be tricky due to the way they are loaded (out of scope for now, at least), so having them on a conditional trait when they're not disableable would be misleading.

Making the shape a separate trait is probably the 'safer' and more future-proof approach.

Contributor

reaperrr commented May 21, 2017

What about actors that can take aoe damage but is not itself targetable?

Actually, now that I've thought about it some more, I think that wouldn't be a problem.

All that TargetableLocations does by default, without customization, is provide a single targetable position at the actors' CenterPosition. That's what the Target code falls back to on all actors, anyway. TargetableLocations does not really make the actor targetable afterall, that's what Targetable is for.

Basically, moving HitShapes to TargetableLocations and then adding that trait to the actors in your example should not have any impact, let alone cause any regressions, because the CenterPosition of any actor is considered a 'targetable position' on bleed already.

However, the actual problems I see are that
a) having the shape reside on TargetableLocations might be a little unintuitive to some modders, and
b) making shapes disableable via conditions will likely be tricky due to the way they are loaded (out of scope for now, at least), so having them on a conditional trait when they're not disableable would be misleading.

Making the shape a separate trait is probably the 'safer' and more future-proof approach.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 23, 2017

Member

having the shape reside on TargetableLocations might be a little unintuitive to some modders

This is just a naming issue, which goes away if we e.g. rename TargetableLocations to HitShape or HitBox Health.Shape to HitShape.Type.

making shapes disableable via conditions will likely be tricky due to the way they are loaded

I don't see the problem here - we load all of them exactly once, and then pick between them at runtime?

Member

pchote commented May 23, 2017

having the shape reside on TargetableLocations might be a little unintuitive to some modders

This is just a naming issue, which goes away if we e.g. rename TargetableLocations to HitShape or HitBox Health.Shape to HitShape.Type.

making shapes disableable via conditions will likely be tricky due to the way they are loaded

I don't see the problem here - we load all of them exactly once, and then pick between them at runtime?

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 23, 2017

Contributor

Yeah, the former occured to me yesterday as well, and the latter was actually just a brain snafu + mistake on my local branch (I forgot that Exts.IsTraitEnabled doesn't work on TraitInfos, so my shapes always stayed enabled).

I just can't log into gh from work so I didn't have the chance to update my comment before yours.

About the names, I think HitShape is better than HitBox (circles and capsules aren't boxes, but certainly shapes), I'm fine with Shape -> Type.

Contributor

reaperrr commented May 23, 2017

Yeah, the former occured to me yesterday as well, and the latter was actually just a brain snafu + mistake on my local branch (I forgot that Exts.IsTraitEnabled doesn't work on TraitInfos, so my shapes always stayed enabled).

I just can't log into gh from work so I didn't have the chance to update my comment before yours.

About the names, I think HitShape is better than HitBox (circles and capsules aren't boxes, but certainly shapes), I'm fine with Shape -> Type.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 24, 2017

Contributor

I'll update this after #13340 has been merged.

Contributor

reaperrr commented May 24, 2017

I'll update this after #13340 has been merged.

@reaperrr reaperrr referenced this pull request May 27, 2017

Closed

Footprint HitShape #10349

@reaperrr reaperrr changed the title from Add TargetableLocations trait to Add HitShape trait May 27, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 27, 2017

Contributor

Updated, though it still depends on #13340.

Contributor

reaperrr commented May 27, 2017

Updated, though it still depends on #13340.

@reaperrr reaperrr added this to the Next release milestone May 27, 2017

@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8

atlimit8 May 29, 2017

Member

#13340 has been merged.

Member

atlimit8 commented May 29, 2017

#13340 has been merged.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 29, 2017

Contributor

Rebased.

Contributor

reaperrr commented May 29, 2017

Rebased.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 29, 2017

Member

I hit a crash when starting a game on "Dual Cold Front" map in RA:

Exception of type `System.InvalidOperationException`: Actor does not have trait of type `OpenRA.Mods.Common.Traits.BodyOrientation`

Not sure which actor is causing it.

Edit: see my review comment about Requires<BodyOrientationInfo>. The linter will tell you which actor needs fixing after that.

Member

pchote commented May 29, 2017

I hit a crash when starting a game on "Dual Cold Front" map in RA:

Exception of type `System.InvalidOperationException`: Actor does not have trait of type `OpenRA.Mods.Common.Traits.BodyOrientation`

Not sure which actor is causing it.

Edit: see my review comment about Requires<BodyOrientationInfo>. The linter will tell you which actor needs fixing after that.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 29, 2017

Member
  • TS crash sites are supposed to be indestructible map deco as per original (they had "Immune=yes")

I remember failing missions where you had to protect the crash sites from being destroyed...? In any case, it will be a long time before we have to worry about these missions so I see no reason not to remove the health here for now.

Member

pchote commented May 29, 2017

  • TS crash sites are supposed to be indestructible map deco as per original (they had "Immune=yes")

I remember failing missions where you had to protect the crash sites from being destroyed...? In any case, it will be a long time before we have to worry about these missions so I see no reason not to remove the health here for now.

@pchote

Looks mostly good. Here's a few code issues that I noticed.

Show outdated Hide outdated OpenRA.Mods.Common/Projectiles/Bullet.cs
@@ -75,6 +74,10 @@ void IRenderAboveWorld.RenderAboveWorld(Actor self, WorldRenderer wr)
TargetLineRenderable.DrawTargetMarker(wr, hc, hb);
}
var activeShapes = shapes.Where(Exts.IsTraitEnabled);

This comment has been minimized.

@pchote

pchote May 29, 2017

Member

You might as well move this inline, it's short and you're only using it once.

@pchote

pchote May 29, 2017

Member

You might as well move this inline, it's short and you're only using it once.

This comment has been minimized.

@reaperrr

reaperrr May 31, 2017

Contributor

I actually use if for var positions in line 80, too.

@reaperrr

reaperrr May 31, 2017

Contributor

I actually use if for var positions in line 80, too.

Show outdated Hide outdated OpenRA.Mods.Common/Traits/HitShape.cs
Show outdated Hide outdated OpenRA.Mods.Common/Traits/HitShape.cs
Show outdated Hide outdated OpenRA.Mods.Common/Warheads/CreateEffectWarhead.cs
Show outdated Hide outdated OpenRA.Mods.Common/WorldExtensions.cs
Show outdated Hide outdated OpenRA.Mods.Common/UtilityCommands/UpgradeRules.cs
Show outdated Hide outdated OpenRA.Mods.Common/Traits/HitShape.cs
Show outdated Hide outdated OpenRA.Mods.Common/UtilityCommands/UpgradeRules.cs
Show outdated Hide outdated OpenRA.Mods.Common/Traits/HitShape.cs
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 29, 2017

Contributor

I remember failing missions where you had to protect the crash sites from being destroyed...? In any case, it will be a long time before we have to worry about these missions so I see no reason not to remove the health here for now.

Those CACRSHxx are just small, 1-cell crashed helis etc., you're mixing them up with the Scrin Crash site.

Those small map deco crash sites never had any mission relevance and were always indestructible, unselectable, untargetable etc. in the original.

Not sure which actor is causing it.

Probably the badger (crate-dropper), because it doesn't happen on every run. It's the bridges actually, the reason it didn't crash every time for me was just a local one (played around with the exception).

Contributor

reaperrr commented May 29, 2017

I remember failing missions where you had to protect the crash sites from being destroyed...? In any case, it will be a long time before we have to worry about these missions so I see no reason not to remove the health here for now.

Those CACRSHxx are just small, 1-cell crashed helis etc., you're mixing them up with the Scrin Crash site.

Those small map deco crash sites never had any mission relevance and were always indestructible, unselectable, untargetable etc. in the original.

Not sure which actor is causing it.

Probably the badger (crate-dropper), because it doesn't happen on every run. It's the bridges actually, the reason it didn't crash every time for me was just a local one (played around with the exception).

reaperrr added some commits May 29, 2017

Fix inconsistent rules in preparation of HitShape refactor
- TD C17 and A10 are not targetable and therefore technically invulnerable, so we should remove Health and Armor
- D2k ^carryall.colorpicker and TS ^mmch.colorpicker shouldn't have Health
- TS crash sites are supposed to be indestructible map deco as per original (they had "Immune=yes")
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 31, 2017

Contributor

you're mixing them up with the Scrin Crash site.

I just remembered, you're most likely actually talking about missions where you had to defend GDI Kodiak (GAKODK) or Nod Montauk (NAMNTK) during ion storms. Those don't have anything to do with CACRSHxx either, though, the latter were really only used as map decoration.

Contributor

reaperrr commented May 31, 2017

you're mixing them up with the Scrin Crash site.

I just remembered, you're most likely actually talking about missions where you had to defend GDI Kodiak (GAKODK) or Nod Montauk (NAMNTK) during ion storms. Those don't have anything to do with CACRSHxx either, though, the latter were really only used as map decoration.

reaperrr added some commits May 21, 2017

Lint check and yaml enforcement for HitShape
Now that Health no longer provides a HitShape, actors with Health need at
least one HitShape trait.
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 31, 2017

Contributor

Updated, adressed everything that was pointed out.

Contributor

reaperrr commented May 31, 2017

Updated, adressed everything that was pointed out.

Updated code.

Add TargetableOffsets to HitShape
And remove ITargetablePositions from Building.
Also, added UseFootprintOffsets to replicate the old Building behavior for easier setup of TargetablePositions for buildings.
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 1, 2017

Contributor

Updated.

Contributor

reaperrr commented Jun 1, 2017

Updated.

@reaperrr reaperrr modified the milestones: Playtest featuring updated HitShapes, Next release Jun 1, 2017

@pchote

pchote approved these changes Jun 3, 2017

Code looks good now, and I didn't notice any issues during quick playtests in TS and TD. 👍

@pchote pchote added the PR: Needs +2 label Jun 3, 2017

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jun 3, 2017

Member

This is one of our highest priority open PRs, so can another @OpenRA/engine-hackers please review ASAP?

Member

pchote commented Jun 3, 2017

This is one of our highest priority open PRs, so can another @OpenRA/engine-hackers please review ASAP?

@cjshmyr

cjshmyr approved these changes Jun 3, 2017

Looks good here and in game (TS was untested because of the armament NRE).

@reaperrr reaperrr merged commit 82758a8 into OpenRA:bleed Jun 3, 2017

2 checks passed

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

@reaperrr reaperrr deleted the reaperrr:TargetableLocations branch Jul 23, 2017

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