Skip to content
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

Targeting regression #16913

Closed
matjaeck opened this issue Aug 9, 2019 · 8 comments

Comments

@matjaeck
Copy link
Contributor

commented Aug 9, 2019

In release-20190314, when you give an attack order while out of range of the target and then a move order so the unit passes the target, it will attack as long it is in attack range. This does no longer work on 7cfc650, see the gifs below.

Release-20190314:

stickyrelease

Bleed

bleedstickybug

@matjaeck

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

Regression was introduced by 30de4df (90ddf24, the commit before, is not affected).

@reaperrr reaperrr added this to the Next Release milestone Aug 9, 2019

@reaperrr reaperrr added the Regression label Aug 9, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

Possibly related to https://forum.openra.net/viewtopic.php?p=310611#p310611 which provides a replay for debugging.

@pchote

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

Confirmed that this is a regression from 30de4df, and specifically because of 30de4df#diff-d888e8822ba56bbbab4330304a9b80f1L278-R286 for tanks.

Digging deeper, the "correct" behavior here relied on a bug that was fixed by switching to child activities:

Previously, AttackFollow would insert the MoveWithinRange into the activity queue before it, and issuing the move order to go past the target would cancel that activity. The original AttackFollow would simply be nulled out, not having a chance to tick. The requestedTarget stays set to the original target.

Now, AttackFollow is the one cancelled by the move, which gives it the chance to demote the target from requestedTarget to opportunityTarget. The opportunity target is then immediately cleared for reasons that I haven't yet debugged and have likely changed when the opportunity logic was rewritten.

@pchote

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

The opportunity target is cancelled because it is not in range when it is demoted, and it isn't reacquired later because tanks won't automatically acquire structures in the Defend stance.

We could add a workaround of not resetting the opportunity target if there is nothing else to switch to, but I don't see the point there - it will lose it as soon as it sees anything else along the route.

Its not clear to me what the expected behaviour here should even be when we try to consider all the edge cases that apply (different ways of cancelling the activity, whether or not the tank sees an auto-targetable enemy while en-route, whether we lose visibility for the target while en-route, etc).

IMO the only reasonable option, unless someone can give a detailed spec for the point above, is to call this an intentional (or at least acceptable) change and leave it be.

@pchote

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

@tovl do you have any thoughts on this?

@tovl

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

I tend to agree with @pchote. The target being cancelled because it is out or range sounds perfectly logical and consistent to me. I'm afraid that trying to emulate the buggy behaviour can not be done in a manner that is both consistent and transparent. My vote would be for wontfix.

@matjaeck

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

The target being cancelled because it is out or range sounds perfectly logical and consistent to me.

Agreed. Consistency, clean code and predictable behavior is more important than restoring this feature.

@abcdefg30

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

Closing as won't fix/not a bug then.

@abcdefg30 abcdefg30 closed this Aug 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.