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

Fix lastVisibleTarget not being set in FlyAttack when target is invisible. #17135

Merged
merged 1 commit into from Oct 13, 2019

Conversation

@tovl
Copy link
Contributor

tovl commented Sep 21, 2019

Fixes #17050

This issue happens when an attack order is given the moment the unit disappears under fog. At that point the actor is invisible to the player, but target is not yet updated to be a frozen actor and so lastvisible* is never set. This results in a Fly activity being queued with a max range of zero and the target only being updated after that Fly activity is finished.

There seems to be no clear reason to exclude targets that are invisible to the player here, because lastVisibleTarget is immediately converted to a terrain type anyway. I may be missing something though. Ping @pchote

@reaperrr reaperrr added this to the Next Release milestone Sep 21, 2019
@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 4, 2019

The idea is to not accidentally leak actor positions under the fog if the activity is called at an unexpected time (i.e. any time aside from the same tick when it becomes hidden).

@tovl

This comment has been minimized.

Copy link
Contributor Author

tovl commented Oct 4, 2019

There should not be any information about actor positions leaking from this, because lastVisibleTarget is set to the underlying terrain, not to the actual target.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 4, 2019

It's set to the underlying terrain at the position of the target, which may be an arbitrary position under the fog/shroud!

@tovl tovl force-pushed the tovl:attackinvisible branch from 4180b4e to 1c843e4 Oct 11, 2019
@tovl

This comment has been minimized.

Copy link
Contributor Author

tovl commented Oct 11, 2019

Implemented pchote's solution instead.

@reaperrr reaperrr merged commit 33d089a into OpenRA:bleed Oct 13, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.