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

Jump to bottom

Move ChildActivity handling into base Activity class #16481

Merged
merged 8 commits into from Jul 3, 2019
Merged

Move ChildActivity handling into base Activity class #16481

merged 8 commits into from Jul 3, 2019

Conversation

tovl
Copy link
Contributor

@tovl tovl 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 childactivity branch 3 times, most recently from 387340f to 0b46965 Compare May 5, 2019 12:13
@reaperrr
Copy link
Contributor

#16382 was merged.

@tovl
Copy link
Contributor Author

tovl commented May 12, 2019

Rebased.

@tovl
Copy link
Contributor Author

tovl commented May 12, 2019 •
edited

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 childactivity branch 2 times, most recently from 52d8569 to 3fdb5f6 Compare May 12, 2019 17:21
@tovl
Copy link
Contributor Author

tovl 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.

pchote
pchote reviewed May 12, 2019
View reviewed changes
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
Copy link
Member

pchote 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
Copy link
Contributor

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 commented May 12, 2019
View reviewed changes
Copy link
Contributor Author

@tovl tovl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@pchote
Copy link
Member

pchote commented May 12, 2019 •
edited

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
Copy link
Contributor Author

tovl 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.

pchote
pchote reviewed May 14, 2019
View reviewed changes
OpenRA.Game/Activities/Activity.cs Outdated Show resolved Hide resolved
OpenRA.Game/Activities/Activity.cs Show resolved Hide resolved
@pchote pchote mentioned this pull request May 15, 2019
Closed
@tovl tovl mentioned this pull request May 17, 2019
Merged
@pchote
Copy link
Member

pchote commented May 17, 2019

Needs a rebase after #16365.

@tovl tovl force-pushed the childactivity branch 2 times, most recently from ded971b to 72ee105 Compare June 30, 2019 17:59
pchote
pchote reviewed Jun 30, 2019
View reviewed changes
OpenRA.Mods.Common/Activities/DeliverUnit.cs Show resolved Hide resolved
mods/common/chrome/settings.yaml Show resolved Hide resolved
OpenRA.Mods.Common/Activities/Move/MoveAdjacentTo.cs Show resolved Hide resolved
OpenRA.Mods.Common/Activities/WaitForTransport.cs Show resolved Hide resolved
OpenRA.Game/Activities/Activity.cs

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

OpenRA.Mods.Common/Traits/Buildings/TransformsIntoEntersTunnels.cs Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Buildings/TransformsIntoMobile.cs Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Buildings/TransformsIntoPassenger.cs Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Buildings/TransformsIntoRepairable.cs Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Conditions/GrantConditionOnDeploy.cs Show resolved Hide resolved
@tovl tovl force-pushed the childactivity branch 3 times, most recently from 619fcc5 to e16170a Compare June 30, 2019 19:50
@tovl
Copy link
Contributor Author

tovl commented Jun 30, 2019

Split out the logic changes into separate commits.

@pchote pchote mentioned this pull request Jun 30, 2019
Merged
pchote
pchote previously approved these changes Jul 1, 2019
View reviewed changes
@tovl
Copy link
Contributor Author

tovl commented Jul 1, 2019

Rebased.

teinarss
teinarss approved these changes Jul 3, 2019
View reviewed changes
Copy link
Contributor

@teinarss teinarss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@teinarss teinarss merged commit 824db72 into OpenRA:bleed Jul 3, 2019
@teinarss
Copy link
Contributor

teinarss commented Jul 3, 2019

https://github.com/OpenRA/OpenRA/wiki/Changelog-(bleed)/_compare/a0d93ae3c83f7f105056e47817db76ecd260f16d...83c92b3f8dac5c41e8c8b36aa92ec158cd25448b

@tovl tovl mentioned this pull request Jul 16, 2019
Merged
@pchote pchote mentioned this pull request Oct 20, 2019
Closed
@tovl tovl deleted the childactivity branch December 15, 2019 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teinarss teinarss teinarss approved these changes

@pchote pchote pchote left review comments

Assignees
No one assigned
Labels
PR: Needs +2
Projects
None yet
Milestone
Release 20191117 / 20200202
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@tovl @reaperrr @pchote @obrakmann @teinarss @abcdefg30 @ltem