Skip to content

Fix aircraft stalling for invalid targets#21455

Merged
PunkPun merged 1 commit intoOpenRA:bleedfrom
tjk-ws:flyattack
Jun 29, 2024
Merged

Fix aircraft stalling for invalid targets#21455
PunkPun merged 1 commit intoOpenRA:bleedfrom
tjk-ws:flyattack

Conversation

@tjk-ws
Copy link
Copy Markdown
Contributor

@tjk-ws tjk-ws commented Jun 17, 2024

#21451 introduced a regression that helicopters in TD lost their targets when out of ammo. The checks for armaments being paused (as is the usual ammo case) versus invalid are separated, so the activity is completed only in the latter case.

Copy link
Copy Markdown
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation is still bogus. Sorry, #21451 should be reverted and instead the correct behaviour would be to cancel if all armaments are disabled instead of paused

It seems the scenario for IsTraitDisabled is not handled at all in FlyAttack

@tjk-ws
Copy link
Copy Markdown
Contributor Author

tjk-ws commented Jun 17, 2024

This implementation is still bogus. Sorry, #21451 should be reverted and instead the correct behaviour would be to cancel if all armaments are disabled instead of paused

It seems the scenario for IsTraitDisabled is not handled at all in FlyAttack

Added, though the valid target check is still needed because mods may change the targets' targetability during battles.

@PunkPun
Copy link
Copy Markdown
Member

PunkPun commented Jun 17, 2024

shouldn't a rearmable unit cancel its activity when the target is invalid as well? And when armaments are disabled?

@tjk-ws
Copy link
Copy Markdown
Contributor Author

tjk-ws commented Jun 17, 2024

shouldn't a rearmable unit cancel its activity when the target is invalid as well? And when armaments are disabled?

My understanding is that it would have passed the first check on paused armaments. Then it would be caught by the invalid target/armament check like any non-rearmable unit

@PunkPun
Copy link
Copy Markdown
Member

PunkPun commented Jun 17, 2024

What about a case where a unit has its armaments paused but the target is also invalid? Or if the target is valid only for the armamants that are paused and not all armamanets are paused?

@tjk-ws
Copy link
Copy Markdown
Contributor Author

tjk-ws commented Jun 17, 2024

Updated EDIT: seems to still stall for a case of a Yak with additional Colt45 with no ammo requirement force-firing a phase transport with default targetable trait disabled when cloaking

@tjk-ws
Copy link
Copy Markdown
Contributor Author

tjk-ws commented Jun 18, 2024

This second cause of stalling I've found seems to be unrelated - if all of an actor's targetable traits are disabled, target.IsValidFor() returns false and the check for a true useLastVisibleTarget runs instead. Nothing stops from the aircraft from moving to the target but it will not actually perform an attack run. However a targetable trait which does not need to correspond to any of the aircraft's weapons but which will always be enabled seems to prevent this.

When this happens target.Type is always Actor and lastVisibleMaximumRange is always 0 (because attackAircraft.GetMaximumRangeVersusTarget actually uses the weapons, which can't attack the target), if those are good starting points for a check.

@tjk-ws tjk-ws marked this pull request as draft June 18, 2024 04:28
@tjk-ws tjk-ws marked this pull request as ready for review June 21, 2024 12:19
@tjk-ws
Copy link
Copy Markdown
Contributor Author

tjk-ws commented Jun 21, 2024

For the sake of consistency and limiting the scope of the commit, I've taken the approach in base AttackFollow where the activity is always completed in the zero range case, the behavior of cancelling on targets becoming untargetable is consistent with e.g. ground turreted units.

@tjk-ws tjk-ws changed the title Fix TD helicopters losing target when out of ammo Fix aircraft stalling for invalid targets Jun 21, 2024
@PunkPun PunkPun merged commit a16542e into OpenRA:bleed Jun 29, 2024
@PunkPun
Copy link
Copy Markdown
Member

PunkPun commented Jun 29, 2024

changelog

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants