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

Move ChildActivity handling into base Activity class #16481

Merged
merged 8 commits into from Jul 3, 2019

Conversation

@tovl
Copy link
Contributor

commented May 1, 2019

Depends on #16369 and #16382.

With all activities now converted to the ChildActivity paradigm, some common patterns can be deduplicated by letting Activity.TickOuter handle childactivities.

@tovl tovl force-pushed the tovl:childactivity branch 3 times, most recently from 387340f to 0b46965 May 1, 2019

@tovl tovl force-pushed the tovl:childactivity branch from 0b46965 to 54b63af May 11, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented May 12, 2019

#16382 was merged.

@tovl tovl force-pushed the tovl:childactivity branch from 54b63af to b762fef May 12, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

Rebased.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

Added a default Tick method to Activity that will immediately return NextActivity for activities that only need to run for one tick. This would make replacing instances of CallFunc with full activities slightly less verbose.

This is also useful for activities that are just containers for a bunch of childactivities without any logic of their own.

@tovl tovl force-pushed the tovl:childactivity branch 2 times, most recently from 52d8569 to 3fdb5f6 May 12, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

Rebased and removed the Done state. Once the state is set to Done, that activity will never be referenced to again because all instances of activities queueing themselves have already been removed.

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
@pchote

This comment has been minimized.

Copy link
Member

commented May 12, 2019

Once the state is set to Done, that activity will never be referenced to again because all instances of activities queueing themselves have already been removed.

Isn't this only true because we've made an effort to fix the bogus cases that were previously in the code? I'd be a lot happier if we could guarantee that we (or modders) can't accidentally break this again in the future, by keeping the state and hardcoding the StrictActivityChecking behaviour to true.

@obrakmann

This comment has been minimized.

Copy link
Contributor

commented May 12, 2019

removed the Done state. Once the state is set to Done, that activity will never be referenced to again because all instances of activities queueing themselves have already been removed.

Just for the record, I don't think this is a good idea. All current instances may have been fixed, but that doesn't mean that someone in the future (or an external modder) won't come along and introduce the mistake in another activity. Also it costs basically nothing at all.

@tovl tovl force-pushed the tovl:childactivity branch from 3fdb5f6 to df602e1 May 12, 2019

@tovl
Copy link
Contributor Author

left a comment

Yeah, that seems like a valid concern. Reverted and changed to always strictly check instead.

@pchote

This comment has been minimized.

Copy link
Member

commented May 12, 2019

Do we have any cases left where an activity returns something that isn't this or NextActivity?

If not, would it make sense to change Tick to return a bool (true = next activity, false = not finished yet) instead? This would make it much harder to violate the "Things to be aware of" points noted at the top of the class.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

I've made Tick return bool instead of Activity. Also, since we always make sure that childactivities finish before going to NextActivity (and we should), I've build this into TickOuter. This can be used to simplify some cases where we set a boolean immediately after queueing a childactivity to indicate we are done after that. Also ReturnToBase is now less convoluted to cancel.

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

@tovl tovl force-pushed the tovl:childactivity branch from 428d8a8 to 99cc579 May 15, 2019

@tovl tovl referenced this pull request May 17, 2019
@pchote

This comment has been minimized.

Copy link
Member

commented May 17, 2019

Needs a rebase after #16365.

@tovl tovl force-pushed the tovl:childactivity branch from 99cc579 to b4beb0f May 20, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

Rebased.

@pchote pchote removed the PR: Rebase me! label Jun 30, 2019

@tovl tovl force-pushed the tovl:childactivity branch 2 times, most recently from ded971b to 72ee105 Jun 30, 2019

mods/common/chrome/settings.yaml Show resolved Hide resolved

// Avoid a single tick delay if the childactivity was just queued.
if (ChildActivity != null && ChildActivity.State == ActivityState.Queued)

This comment has been minimized.

Copy link
@pchote

pchote Jun 30, 2019

Member

Style convention is to add braces around the body in this case - it may still be one logical statement, but there is potential for ambiguity about what the else belongs to.

This comment has been minimized.

Copy link
@tovl

tovl Jun 30, 2019

Author Contributor

Done.

@tovl tovl force-pushed the tovl:childactivity branch 3 times, most recently from 619fcc5 to e16170a Jun 30, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

Split out the logic changes into separate commits.

@tovl tovl force-pushed the tovl:childactivity branch from e16170a to d5a8b22 Jul 1, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

Rebased.

@reaperrr reaperrr removed the PR: Rebase me! label Jul 1, 2019

@teinarss
Copy link
Contributor

left a comment

LGTM

@teinarss teinarss merged commit 824db72 into OpenRA:bleed Jul 3, 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
8 participants
You can’t perform that action at this time.