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

Fix and partially refactor TeslaZap #13475

Merged
merged 4 commits into from Jun 11, 2017

Conversation

Projects
None yet
3 participants
@reaperrr
Contributor

reaperrr commented Jun 7, 2017

I forgot about this when fixing projectile targeting of closest targetable position.

In addition, I used this opportunity to do some cleanup and add two properties that might be useful for modders.

reaperrr added some commits Jun 7, 2017

Minor TeslaZap cleanup
Renaming timeUntilRemove to ticksUntilRemove makes it more clear and allows to drop the comment.
Remove unused 'initialized' bool from TeslaZap
It was never set to 'true', and doing so would break Duration > 1, so it's better to just remove it.

@reaperrr reaperrr added this to the Playtest featuring updated HitShapes milestone Jun 7, 2017

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jun 11, 2017

Member

Do not draw TeslaZap if source and target are under fog

It's possible for both the source and target to be hidden, but for the zap to pass through a visible area. We still want to render it in this case!

Member

pchote commented Jun 11, 2017

Do not draw TeslaZap if source and target are under fog

It's possible for both the source and target to be hidden, but for the zap to pass through a visible area. We still want to render it in this case!

@pchote

pchote approved these changes Jun 11, 2017

Looks good aside from that one issue.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 11, 2017

Contributor

It's possible for both the source and target to be hidden, but for the zap to pass through a visible area. We still want to render it in this case!

We might have to revise our LaserZap code then, because the same should hold true there, too, and I suspect the FogObscures check might be more expensive than the actual rendering anyway.

Contributor

reaperrr commented Jun 11, 2017

It's possible for both the source and target to be hidden, but for the zap to pass through a visible area. We still want to render it in this case!

We might have to revise our LaserZap code then, because the same should hold true there, too, and I suspect the FogObscures check might be more expensive than the actual rendering anyway.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jun 11, 2017

Member

Hmm, actually it looks like TeslaZapRenderable is already doing a very similar before rendering (TeslaZapRenderable.Render).

Member

pchote commented Jun 11, 2017

Hmm, actually it looks like TeslaZapRenderable is already doing a very similar before rendering (TeslaZapRenderable.Render).

reaperrr added some commits Jun 7, 2017

Refactor TeslaZap targeting
- made it target closest targetable position, instead of CenterPosition
- made target tracking optional (enabled by default)
- made tracking independent from whether damage has already been dealt
- cache target position and update it in Tick only if tracking
Replace doneDamage bool in TeslaZap with DamageDuration
Allows it to deal continous damage via a single projectile.
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 11, 2017

Contributor

Updated, dropped the FogObscures check from this PR.

Contributor

reaperrr commented Jun 11, 2017

Updated, dropped the FogObscures check from this PR.

}
// Zap tracks target
if (info.TrackTarget && args.GuidedTarget.IsValidFor(args.SourceActor))
target = args.GuidedTarget.Positions.PositionClosestTo(args.Source);

This comment has been minimized.

@rob-v

rob-v Jun 11, 2017

Contributor

Before when args.GuidedTarget.IsValidFor(args.SourceActor) changed to False, PassiveTarget was used in Render. Now when IsValidFor changes to false, it will use a previous target, not PassiveTarget. Is it ok?

@rob-v

rob-v Jun 11, 2017

Contributor

Before when args.GuidedTarget.IsValidFor(args.SourceActor) changed to False, PassiveTarget was used in Render. Now when IsValidFor changes to false, it will use a previous target, not PassiveTarget. Is it ok?

This comment has been minimized.

@reaperrr

reaperrr Jun 11, 2017

Contributor

TrackTarget cannot change during the game.
About IsValidFor: The target might have moved since the projectile launch, so the zap would jump back to the original target position (at projectile launch) if we used args.PassiveTarget instead of last target position. This would look glitchy, especially for fast targets. Using the last target position before it became invalid is the safest approach here, in my opinion.

@reaperrr

reaperrr Jun 11, 2017

Contributor

TrackTarget cannot change during the game.
About IsValidFor: The target might have moved since the projectile launch, so the zap would jump back to the original target position (at projectile launch) if we used args.PassiveTarget instead of last target position. This would look glitchy, especially for fast targets. Using the last target position before it became invalid is the safest approach here, in my opinion.

@rob-v

rob-v approved these changes Jun 11, 2017

👍

@reaperrr reaperrr merged commit ab8bc53 into OpenRA:bleed Jun 11, 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:fix-TeslaZap branch Jul 23, 2017

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