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

Clear RequestedTargets that are cancelled before the first activity tick #16566

Merged
merged 2 commits into from Jun 29, 2019

Conversation

@pchote
Copy link
Member

commented May 19, 2019

Fixes #16540 (hopefully).
Depends on #16559.

My reflexes aren't good enough to reliably reproduce the original bug, so I'll rely on @Punsho to confirm the ingame behaviour is now correct.

@pchote pchote added this to the Next Release milestone May 19, 2019

@Punsho
Copy link
Contributor

left a comment

Seems like this fixed it

@pchote pchote force-pushed the pchote:fix-stuck-requestedtarget branch from 3e221b4 to ec13f90 May 21, 2019

@pchote pchote force-pushed the pchote:fix-stuck-requestedtarget branch from ec13f90 to ebf2ca0 May 21, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

Updated, but this should be considered WIP because i'm not happy with UpdateRequestedTarget and CancelRequestedTarget as they currently look, and this hasn't been properly tested yet.

Ping @tovl as this may conflict with planned changes that I don't yet know about.

if (!keepQueue)
if (!keepQueue && NextActivity != null)
{
foreach (var a in NextActivity.ActivitiesImplementing<IActivityNotifyPreemptiveCancel>())

This comment has been minimized.

Copy link
@tovl

tovl Jun 15, 2019

Contributor

This kind of goes against the guidelines for how activities should behave that we hashed out in #16069 and other places. It is also only really relevant for AttackFollow and related activities and the bug this is meant to fix is caused specifically by AttackFollow.OnQueueAttackActivity and the resulting ambiguity over whether it is the trait or the activity that is in charge of targeting.

A proper solution would be to completely separate targeting from the activities. That would require a bigger refactor though, so doing that might better be deferred to the future.

In the mean time, if you think this absolutely needs to be fixed now, I would prefer it if this was kept contained to the activities for which it is relevant by using an override for Cancel instead of adding this to the base Activity class. the planned follow-ups to #16696 will merge these activities further so will likely negate the code duplication this might otherwise cause.

This comment has been minimized.

Copy link
@pchote

pchote Jun 16, 2019

Author Member

The issue is that Cancel is not called for queued activities, so there is nothing to override.

It should be possible now to reimplement this from the trait tick, so I will try that instead.

@pchote pchote force-pushed the pchote:fix-stuck-requestedtarget branch from ebf2ca0 to daea575 Jun 20, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

Fixed and rebased. This now implements the workaround inside AttackFollow instead of changing the core Activity plumbing.

@pchote

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

Ping @Punsho: Can you please confirm that the updated PR still works as expected?

@Punsho

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

It does

@teinarss
Copy link
Contributor

left a comment

LGTM

@reaperrr
Copy link
Contributor

left a comment

Looks ok to me

@reaperrr reaperrr merged commit ff9db0b into OpenRA:bleed Jun 29, 2019

2 checks passed

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

@pchote pchote deleted the pchote:fix-stuck-requestedtarget branch Aug 26, 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.