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

Refactor targeting of target center and fix pillbox regression #13529

Merged
merged 3 commits into from Jun 24, 2017

Conversation

Projects
None yet
5 participants
@reaperrr
Contributor

reaperrr commented Jun 18, 2017

Pillboxes (and possibly other actors in external mods) themselves don't have an Attack* trait, so the previous approach would prevent them from working.

Update: Refactored #13497 due to

Would this be a good enough reason add AttackTargetCenter to WeaponInfo instead, and replace the other uses with that? Having separate flags on AttackBase vs the projectiles still doesn't sit well with me.

(which I agree with anyway, the old approach was only taken to avoid an additional projectile arg).

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

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jun 18, 2017

Member

Would this be a good enough reason add AttackTargetCenter to WeaponInfo instead, and replace the other uses with that? Having separate flags on AttackBase vs the projectiles still doesn't sit well with me.

Member

pchote commented Jun 18, 2017

Would this be a good enough reason add AttackTargetCenter to WeaponInfo instead, and replace the other uses with that? Having separate flags on AttackBase vs the projectiles still doesn't sit well with me.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 18, 2017

Contributor

Not sure. I imagine it might be tricky because the Attack/Turreted traits need that information to face the correct position, but what if one weapon has AttackTargetCenter while the other doesn't?

I suspect we might be opening a bigger can of worms than we're closing if we do that.

Contributor

reaperrr commented Jun 18, 2017

Not sure. I imagine it might be tricky because the Attack/Turreted traits need that information to face the correct position, but what if one weapon has AttackTargetCenter while the other doesn't?

I suspect we might be opening a bigger can of worms than we're closing if we do that.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jun 18, 2017

Member

That problem already exists with the ability to set separate barrel angles for each armament.
IMO it is perfectly reasonable to specify that we target the center if any associated armament has AttackTargetCenter: true.

Member

pchote commented Jun 18, 2017

That problem already exists with the ability to set separate barrel angles for each armament.
IMO it is perfectly reasonable to specify that we target the center if any associated armament has AttackTargetCenter: true.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jun 18, 2017

Contributor

I remember I wanted to ask about that CheckFire's return added in last commit but deferred..
IMO this PR should be merged and refactoring discussed outside this PR as this is small fix to already merged feature.
btw that attack member could be local attack in CheckFire, it isn't used outside this function.

Contributor

rob-v commented Jun 18, 2017

I remember I wanted to ask about that CheckFire's return added in last commit but deferred..
IMO this PR should be merged and refactoring discussed outside this PR as this is small fix to already merged feature.
btw that attack member could be local attack in CheckFire, it isn't used outside this function.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 18, 2017

Contributor

btw that attack member could be local attack in CheckFire, it isn't used outside this function.

Yes, but - as in most similar cases - the idea is to cache the attack traits to avoid look-ups, and then only check which one is currently enabled.

Contributor

reaperrr commented Jun 18, 2017

btw that attack member could be local attack in CheckFire, it isn't used outside this function.

Yes, but - as in most similar cases - the idea is to cache the attack traits to avoid look-ups, and then only check which one is currently enabled.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jun 18, 2017

Contributor

attackTraits is cached, that is fine, I mean attack variable used to retrieve enabled AttackBase. This could be local in CheckFire.

Contributor

rob-v commented Jun 18, 2017

attackTraits is cached, that is fine, I mean attack variable used to retrieve enabled AttackBase. This could be local in CheckFire.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Jun 18, 2017

Contributor

Not sure. I imagine it might be tricky because the Attack/Turreted traits need that information to face the correct position, but what if one weapon has AttackTargetCenter while the other doesn't?

We already have this issue. If any turrets has a valid facing towards the targets, all are allowed to fire. http://ppmforums.com/viewtopic.php?t=42445 have illustrated this ages ago. I doubt that would cause new issues.

Contributor

GraionDilach commented Jun 18, 2017

Not sure. I imagine it might be tricky because the Attack/Turreted traits need that information to face the correct position, but what if one weapon has AttackTargetCenter while the other doesn't?

We already have this issue. If any turrets has a valid facing towards the targets, all are allowed to fire. http://ppmforums.com/viewtopic.php?t=42445 have illustrated this ages ago. I doubt that would cause new issues.

@rob-v

rob-v approved these changes Jun 22, 2017

👍

@abcdefg30

12:04:27 (abcdefg30) pchote are you going to insist on #13529 (comment) or do we leave that to a future pr (if it happens)?
12:06:13 (pchote) I am tempted to, yes
12:06:52 (pchote) if we don't fix this properly before we ship a playtest it becomes much harder

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 22, 2017

Contributor

No problem, I was already working on it anyway.

Contributor

reaperrr commented Jun 22, 2017

No problem, I was already working on it anyway.

Refactored PR

@reaperrr reaperrr changed the title from Fix pillbox regression to Refactor targeting of target center and fix pillbox regression Jun 22, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 22, 2017

Contributor

Updated.

Contributor

reaperrr commented Jun 22, 2017

Updated.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 22, 2017

Contributor

Updated.

Note: I'm not 100% sure what to do with Turreted.
attack can be null there, so it needs a fallback, but when attack is null the turreted actor most likely won't be firing a weapon, so it might be better to face the target center in that case. Opinions?

Contributor

reaperrr commented Jun 22, 2017

Updated.

Note: I'm not 100% sure what to do with Turreted.
attack can be null there, so it needs a fallback, but when attack is null the turreted actor most likely won't be firing a weapon, so it might be better to face the target center in that case. Opinions?

@rob-v

Seems something is wrong

Introduce Weapon.TargetActorCenter and adapt projectiles
This also fixes issues with attackers that don't have their own Attack
trait.
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 22, 2017

Contributor

Updated.

Contributor

reaperrr commented Jun 22, 2017

Updated.

@rob-v

rob-v approved these changes Jun 22, 2017

👍
I liked more previous HasAnyValidWeapons with new arg than mostly duplicated HasAnyValidCenterTargetingWeapons, anyway better to review it now by other reviewers.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 22, 2017

Contributor

I liked more previous HasAnyValidWeapons with new arg

Updated, brought it back in a slightly improved fashion (no more chaining through the boolean, GetTargetPosition sets it to true directly) via a separate commit to make it easier to review.

Contributor

reaperrr commented Jun 22, 2017

I liked more previous HasAnyValidWeapons with new arg

Updated, brought it back in a slightly improved fashion (no more chaining through the boolean, GetTargetPosition sets it to true directly) via a separate commit to make it easier to review.

@pchote

pchote approved these changes Jun 24, 2017

@pchote pchote merged commit 0a1083e into OpenRA:bleed Jun 24, 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-pillbox branch Jul 23, 2017

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