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

Childactivity fixes #16246

Merged
merged 6 commits into from Mar 9, 2019

Conversation

@tovl
Copy link
Contributor

commented Feb 27, 2019

This PR makes some improvements to the Activity code relevant for the handling of childactivities. This was split from #16217 and is a prerequisite for that and #16193.

  • ResupplyActivity is changed from a CompositeActivity to a regular Activity.
  • CompositeActivity is removed.
  • Made changes to activity Cancel method and ChildActivity getter to prevent nested childactivities from prematurely being cancelled.
  • Added Actor self as a required parameter for the Queue, QueueChild and PrintActivityTree methods of Activity. Also added Actor self as a required parameter for SequenceActivities.
  • added optional pretick parameter to QueueChild to make it easier to prevent activities from skipping one tick.

Fixes #15853.

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

@pchote pchote requested a review from obrakmann Feb 27, 2019

@pchote
Copy link
Member

left a comment

Looks good overall, but I think its definitely worth exploring @obrakmann's idea of introducing an ActivityState.Canceling to help untangle the Move mess. I'm hoping this is simple, but if not we can fall back to what you've done here.

OpenRA.Game/Activities/Activity.cs Outdated Show resolved Hide resolved
OpenRA.Game/Activities/Activity.cs Outdated Show resolved Hide resolved
OpenRA.Game/Activities/Activity.cs Outdated Show resolved Hide resolved
@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2019

Also needs a rebase.

@tovl tovl force-pushed the tovl:childactivity-fixes branch 3 times, most recently from 1b477f5 to 48fb198 Mar 3, 2019

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

@tovl tovl force-pushed the tovl:childactivity-fixes branch from 48fb198 to 0e8e4ee Mar 6, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

Made some final adjustments related to the cancelling of activities:

  • Canceling now transitions to Done.
  • Childactivities can never cancel their parents.
  • When trying to cancel an activity that cannot be interrupted, the rest of the activity queue still gets removed.

The last two are especially relevant for #16142.

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

Looks good to me otherwise, 👍 once the fixups are applied and the last commit is merged into 'Introduced Canceling state' (which I think should be renamed to 'Replaced Canceled state with Canceling state' then).

Ideally I'd still prefer both @pchote and @obrakmann to approve this (at least conceptually), but if @obrakmann doesn't have time you can count mine.

@tovl tovl force-pushed the tovl:childactivity-fixes branch from 0e8e4ee to 39dff67 Mar 7, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

Applied fixups, squashed and renamed.

@tovl tovl force-pushed the tovl:childactivity-fixes branch from 39dff67 to fa121ea Mar 7, 2019

@tovl tovl force-pushed the tovl:childactivity-fixes branch from fa121ea to 7740627 Mar 7, 2019

OpenRA.Game/Activities/Activity.cs Show resolved Hide resolved
OpenRA.Game/Activities/Activity.cs Outdated Show resolved Hide resolved
OpenRA.Game/Activities/Activity.cs Outdated Show resolved Hide resolved

@tovl tovl force-pushed the tovl:childactivity-fixes branch from 7740627 to 3fa05cc Mar 9, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2019

Rebased and applied fixups.

OpenRA.Mods.Common/Activities/Move/Move.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/Move/Move.cs Outdated Show resolved Hide resolved
OpenRA.Game/Activities/Activity.cs Outdated Show resolved Hide resolved
@pchote
pchote approved these changes Mar 9, 2019
Copy link
Member

left a comment

LGTM now 👍

I count 2.5 👍's (taking @obrakmann's lack of any show-stopper comments as a :+0.5:), so will merge this now. I'm sure that future testing will find new regressions, but these are going to be best dealt with in their own followups. This PR is going to rebase-block a bunch of others, so better to get it out of the way now than to drag it out longer.

@pchote pchote merged commit 705795a into OpenRA:bleed Mar 9, 2019

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.