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 explosion effects on trees #13693

Merged
merged 3 commits into from Aug 7, 2017

Conversation

Projects
None yet
5 participants
@reaperrr
Contributor

reaperrr commented Jul 22, 2017

I forgot that trees don't have the Ground TargetType (to make them invulnerable to everything but flame weapons) when refactoring the effect warhead.

Fixes #13686.

@reaperrr reaperrr added this to the Next Release milestone Jul 22, 2017

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jul 22, 2017

Member

On the topic of renaming things, we should rename the new TDTRUK to something more appropriate.

Member

pchote commented Jul 22, 2017

On the topic of renaming things, we should rename the new TDTRUK to something more appropriate.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jul 23, 2017

Contributor

On the topic of renaming things, we should rename the new TDTRUK to something more appropriate.

I'd just remove the 'TD' and maybe add the missing letter (make it TRUCK), any other suggestions?

Contributor

reaperrr commented Jul 23, 2017

On the topic of renaming things, we should rename the new TDTRUK to something more appropriate.

I'd just remove the 'TD' and maybe add the missing letter (make it TRUCK), any other suggestions?

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jul 27, 2017

Contributor

I'd just remove the 'TD' and maybe add the missing letter (make it TRUCK)

Done.

Contributor

reaperrr commented Jul 27, 2017

I'd just remove the 'TD' and maybe add the missing letter (make it TRUCK)

Done.

@ltem

This comment has been minimized.

Show comment
Hide comment
@ltem

ltem Jul 28, 2017

Contributor
  • Rename TD TowerMissle to TowerMissile ✔️
  • Rename tdtruk to truck ✔️
  • Fix RA effect waheads ✔️
  • Fix TD effect warheads to account for trees ✔️

The changes seem fine to me (I couldn't spot anything missing) and the issue is fixed but I have two questions.

  1. I couldn't reproduce the issue (missing explosions on trees) in cnc in the playtest, still there are changes for it. Why?
  2. Trees and water are not ValidTargets for Missiles in cnc , still the explosion gets enabled for it. Why? (Similiar situation for e.g. ^AACannon or ^Explosion in ra)

So 👍 from me (which is maybe devalued by my questions 😄 )

Contributor

ltem commented Jul 28, 2017

  • Rename TD TowerMissle to TowerMissile ✔️
  • Rename tdtruk to truck ✔️
  • Fix RA effect waheads ✔️
  • Fix TD effect warheads to account for trees ✔️

The changes seem fine to me (I couldn't spot anything missing) and the issue is fixed but I have two questions.

  1. I couldn't reproduce the issue (missing explosions on trees) in cnc in the playtest, still there are changes for it. Why?
  2. Trees and water are not ValidTargets for Missiles in cnc , still the explosion gets enabled for it. Why? (Similiar situation for e.g. ^AACannon or ^Explosion in ra)

So 👍 from me (which is maybe devalued by my questions 😄 )

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jul 28, 2017

Contributor

I couldn't reproduce the issue (missing explosions on trees) in cnc in the playtest, still there are changes for it. Why?

I forgot about it while making the changes, but since effect warheads in TD all have victim scans completely disabled, they won't 'find' trees and 'think' they just hit normal ground, which is a valid target.
Same goes for some AA weapons in RA.

Trees and water are not ValidTargets for Missiles in cnc , still the explosion gets enabled for it. Why?

Water tiles in TD mod contain both Water and Ground TargetTypes, and Ground is valid while Water is not listed as invalid.

Contributor

reaperrr commented Jul 28, 2017

I couldn't reproduce the issue (missing explosions on trees) in cnc in the playtest, still there are changes for it. Why?

I forgot about it while making the changes, but since effect warheads in TD all have victim scans completely disabled, they won't 'find' trees and 'think' they just hit normal ground, which is a valid target.
Same goes for some AA weapons in RA.

Trees and water are not ValidTargets for Missiles in cnc , still the explosion gets enabled for it. Why?

Water tiles in TD mod contain both Water and Ground TargetTypes, and Ground is valid while Water is not listed as invalid.

reaperrr added some commits Jul 22, 2017

Fix RA effect waheads
- take Trees into consideration (they don't have Ground TargetType)
- fixed the delayed effect warheads on Vulcan
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jul 28, 2017

Contributor

Updated.

Contributor

reaperrr commented Jul 28, 2017

Updated.

@ltem

This comment has been minimized.

Show comment
Hide comment
@ltem

ltem Jul 28, 2017

Contributor

👍

(Thanks for the explanation.)

Contributor

ltem commented Jul 28, 2017

👍

(Thanks for the explanation.)

@ltem

ltem approved these changes Jul 28, 2017

@SoScared

Appears to generates explosions effects from all ballistic/missile types.

@abcdefg30 abcdefg30 merged commit 6945ae1 into OpenRA:bleed Aug 7, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@abcdefg30
Member

abcdefg30 commented Aug 7, 2017

@reaperrr reaperrr deleted the reaperrr:fix-tree-targeting branch Feb 22, 2018

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