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

Make Attack use ChildActivities #16217

Merged
merged 1 commit into from Mar 10, 2019

Conversation

@tovl
Copy link
Contributor

commented Feb 17, 2019

Fixes #15631.
Fixes #16047.
Fixes #16191.

This PR changes the Attack and MoveAdjacent/MoveWithinRange activities to use ChildActivities instead of using self-referential SequenceActivities or using their own quasi-childactivity. In this way Move gets to properly finish when cancelled and visual glitches are avoided.

In order for this to properly work I had to make one more small change to Activity related to the nulling of childactivities on top of the one introduced in #16193. This one is relevant whenever Move is a ChildActivity of a ChildActivity. Because cancelling Move always returns true, it's parentactivity gets the state Canceled when Move is still running. This then causes the grandparent activity to null the entire tree prematurely. Changing this to null childactivities only when they have the state Done fixes this.

Also included some minor clean-up to MoveWithinRange.

@pchote

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

👍 to the idea here, but it may be a while before I can invest the time to look at the code in detail and test this.

@Inq8

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2019

This breaks aircraft for me, they become uncontrollable once they have landed to rearm.

@pchote pchote added this to the Next + 1 milestone Feb 23, 2019

@pchote pchote referenced this pull request Feb 23, 2019
@pchote

This comment has been minimized.

Copy link
Member

commented Feb 23, 2019

I can reproduce the breakage.

@tovl tovl force-pushed the tovl:attack-refactor branch from ad80868 to dee73ea Feb 24, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2019

This breaks aircraft for me, they become uncontrollable once they have landed to rearm.

The change to the Activity base class interacted with CompositeActivity in a way that broke ResupplyActivity by causing an infinite loop between parent and child activities. Because ResupplyActivity is the only activity that actually inherits from CompositeActivity I have not bothered with CompositeActivity but fixed the regression by simply making ResupplyActivity a regular Activity.

This was referenced Feb 25, 2019

@tovl tovl force-pushed the tovl:attack-refactor branch 2 times, most recently from 190e6ff to b348834 Feb 27, 2019

@tovl tovl referenced this pull request Mar 2, 2019

@tovl tovl force-pushed the tovl:attack-refactor branch from b348834 to 77407be Mar 3, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Mar 9, 2019

#16246 has been merged, so this needs a rebase now.

@tovl tovl force-pushed the tovl:attack-refactor branch from 77407be to f624bcd Mar 9, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2019

Rebased.

@pchote pchote removed the PR: Rebase me! label Mar 9, 2019

@pchote
pchote approved these changes Mar 10, 2019

@pchote pchote added the PR: Needs +2 label Mar 10, 2019

@obrakmann obrakmann merged commit 6f213dd into OpenRA:bleed Mar 10, 2019

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2019

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.