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.

Sign up for GitHub

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

Simplify Activity class #16354

Merged
merged 1 commit into from Mar 30, 2019
Merged

Simplify Activity class #16354

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
132 changes: 15 additions & 117 deletions OpenRA.Game/Activities/Activity.cs
View file
Open in desktop
Expand Up @@ -19,122 +19,26 @@ namespace OpenRA.Activities
public enum ActivityState { Queued, Active, Canceling, Done }

/*
* Activities are actions carried out by actors during each tick.
*
* Activities exist in a graph data structure built up amongst themselves. Each activity has a parent activity,
* optionally child activities, and usually a next activity. An actor's CurrentActivity is a pointer into that graph
* and moves through it as activities run.
*
*
* Things to be aware of when writing activities:
*
* - Use "return NextActivity" at least once somewhere in the tick method.
* - Do not use "return new SomeActivity()" as that will break the graph. Queue the new activity and use "return NextActivity" instead.
* - Do not "reuse" (with "SequenceActivities", for example) activity objects that have already finished running.
* - Do not use "return new SomeActivity()" as that will break the queue. Queue the new activity and use "return NextActivity" instead.
* - Do not "reuse" (with "SequenceActivities", for example) activity objects that have already started running.
* Queue a new instance instead.
* - Avoid calling actor.CancelActivity(). It is almost always a bug. Call activity.Cancel() instead.
* - Do not return the Parent explicitly unless you have an extremly good reason. "return NextActivity"
* will do the right thing in all circumstances.
* - You do not need to care about the ChildActivity pointer advancing through the list of children,
* the activity code already takes care of that.
* - If you want to check whether there are any follow-up activities queued, check against "NextInQueue"
* in favour of "NextActivity" to avoid checking against the Parent activity.
*/
public abstract class Activity
{
public ActivityState State { get; private set; }

/// <summary>
/// Returns the top-most activity *from the point of view of the calling activity*. Note that the root activity
/// can and likely will have next activities of its own, which would in turn be the root for their children.
/// </summary>
public Activity RootActivity
{
get
{
var p = this;
while (p.ParentActivity != null)
p = p.ParentActivity;

return p;
}
}

Activity parentActivity;
public Activity ParentActivity
{
get
{
return parentActivity;
}

protected set
{
parentActivity = value;

var next = NextInQueue;
if (next != null)
next.ParentActivity = parentActivity;
}
}

Activity childActivity;
protected Activity ChildActivity
{
get
{
return childActivity != null && childActivity.State < ActivityState.Done ? childActivity : null;
}

set
{
if (value == this || value == ParentActivity || value == NextInQueue)
childActivity = null;
else
{
childActivity = value;

if (childActivity != null)
childActivity.ParentActivity = this;
}
}
}

Activity nextActivity;

/// <summary>
/// The getter will return either the next activity or, if there is none, the parent one.
/// </summary>
public virtual Activity NextActivity
{
get
{
return nextActivity != null ? nextActivity : ParentActivity;
}

set
{
if (value == this || value == ParentActivity || (value != null && value.ParentActivity == this))
nextActivity = null;
else
{
nextActivity = value;

if (nextActivity != null)
nextActivity.ParentActivity = ParentActivity;
}
}
get { return childActivity != null && childActivity.State < ActivityState.Done ? childActivity : null; }
set { childActivity = value; }
Copy link
Member

Choose a reason for hiding this comment

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

It might be a good idea to check and throw an exception if Game.Settings.Debug.StrictActivityChecking && childActivity != null && childActivity.State < ActivityState.Done.

Something for a followup PR, perhaps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory, this could be removed in the future as well. The ChildActivity = ActivityUtils.RunActivity(...) pattern should see to it that ChildActivity gets set to null eventually, and it shouldn't ever be left in the Done state. I left it in out of caution for now.

}

/// <summary>
/// The getter will return the next activity on the same level _only_, in contrast to NextActivity.
/// Use this to check whether there are any follow-up activities queued.
/// </summary>
public Activity NextInQueue
{
get { return nextActivity; }
set { NextActivity = value; }
}
public Activity NextActivity { get; protected set; }

public bool IsInterruptible { get; protected set; }
public bool IsCanceling { get { return State == ActivityState.Canceling; } }
Expand All @@ -156,15 +60,9 @@ public Activity TickOuter(Actor self)
}

var ret = Tick(self);
if (ret == null || (ret != this && ret.ParentActivity != this))
if (ret != this)
{
// Make sure that the Parent's ChildActivity pointer is moved forwards as the child queue advances.
// The Child's ParentActivity will be set automatically during assignment.
if (ParentActivity != null && ParentActivity != ret)
ParentActivity.ChildActivity = ret;

State = ActivityState.Done;

OnLastRun(self);
}

Expand Down Expand Up @@ -204,7 +102,7 @@ internal void OnActorDisposeOuter(Actor self)
public virtual void Cancel(Actor self, bool keepQueue = false)
{
if (!keepQueue)
NextInQueue = null;
NextActivity = null;

if (!IsInterruptible)
return;
Expand All @@ -217,10 +115,10 @@ public virtual void Cancel(Actor self, bool keepQueue = false)

public virtual void Queue(Actor self, Activity activity)
{
if (NextInQueue != null)
NextInQueue.Queue(self, activity);
if (NextActivity != null)
NextActivity.Queue(self, activity);
else
NextInQueue = activity;
NextActivity = activity;
}

public virtual void QueueChild(Actor self, Activity activity, bool pretick = false)
Expand All @@ -232,17 +130,17 @@ public virtual void QueueChild(Actor self, Activity activity, bool pretick = fal
}

/// <summary>
/// Prints the activity tree, starting from the root or optionally from a given origin.
/// Prints the activity tree, starting from the top or optionally from a given origin.
///
/// Call this method from any place that's called during a tick, such as the Tick() method itself or
/// the Before(First|Last)Run() methods. The origin activity will be marked in the output.
/// </summary>
/// <param name="origin">Activity from which to start traversing, and which to mark. If null, mark the calling activity, and start traversal from the root.</param>
/// <param name="origin">Activity from which to start traversing, and which to mark. If null, mark the calling activity, and start traversal from the top.</param>
/// <param name="level">Initial level of indentation.</param>
protected void PrintActivityTree(Actor self, Activity origin = null, int level = 0)
{
if (origin == null)
RootActivity.PrintActivityTree(self, this);
self.CurrentActivity.PrintActivityTree(self, this);
else
{
Console.Write(new string(' ', level * 2));
Expand All @@ -254,8 +152,8 @@ protected void PrintActivityTree(Actor self, Activity origin = null, int level =
if (ChildActivity != null)
ChildActivity.PrintActivityTree(self, origin, level + 1);

if (NextInQueue != null)
NextInQueue.PrintActivityTree(self, origin, level);
if (NextActivity != null)
NextActivity.PrintActivityTree(self, origin, level);
}
}

Expand Down
6 changes: 3 additions & 3 deletions OpenRA.Game/Actor.cs
View file
Open in desktop
Expand Up @@ -222,13 +222,13 @@ public void QueueActivity(Activity nextActivity)
if (CurrentActivity == null)
CurrentActivity = nextActivity;
else
CurrentActivity.RootActivity.Queue(this, nextActivity);
CurrentActivity.Queue(this, nextActivity);
}

public void CancelActivity()
{
if (CurrentActivity != null)
CurrentActivity.RootActivity.Cancel(this);
CurrentActivity.Cancel(this);
}

public override int GetHashCode()
Expand Down Expand Up @@ -281,7 +281,7 @@ public void Dispose()
// If CurrentActivity isn't null, run OnActorDisposeOuter in case some cleanups are needed.
// This should be done before the FrameEndTask to avoid dependency issues.
if (CurrentActivity != null)
CurrentActivity.RootActivity.OnActorDisposeOuter(this);
CurrentActivity.OnActorDisposeOuter(this);

// Allow traits/activities to prevent a race condition when they depend on disposing the actor (e.g. Transforms)
WillDispose = true;
Expand Down
2 changes: 1 addition & 1 deletion OpenRA.Game/Traits/ActivityUtils.cs
View file
Open in desktop
Expand Up @@ -44,7 +44,7 @@ public static Activity RunActivity(Actor self, Activity act)
else
start = current;

if (act == prev || act == prev.ParentActivity)
if (act == prev)
break;
}

Expand Down
4 changes: 2 additions & 2 deletions OpenRA.Mods.Common/Activities/Air/ResupplyAircraft.cs
View file
Open in desktop
Expand Up @@ -32,13 +32,13 @@ protected override void OnFirstRun(Actor self)
{
ChildActivity = ActivityUtils.SequenceActivities(self, aircraft.GetResupplyActivities(host).ToArray());
QueueChild(self, new AllowYieldingReservation(self));
QueueChild(self, new WaitFor(() => NextInQueue != null || aircraft.ReservedActor == null));
QueueChild(self, new WaitFor(() => NextActivity != null || aircraft.ReservedActor == null));
}
else
{
ChildActivity = ActivityUtils.SequenceActivities(self, aircraft.GetResupplyActivities(host).ToArray());
QueueChild(self, new AllowYieldingReservation(self));
QueueChild(self, new TakeOff(self, (a, b, c) => NextInQueue == null && b.NextInQueue == null));
QueueChild(self, new TakeOff(self, (a, b, c) => NextActivity == null && b.NextActivity == null));
}
}

Expand Down
4 changes: 2 additions & 2 deletions OpenRA.Mods.Common/Activities/DeliverResources.cs
View file
Open in desktop
Expand Up @@ -35,8 +35,8 @@ public DeliverResources(Actor self)

public override Activity Tick(Actor self)
{
if (NextInQueue != null)
return NextInQueue;
if (NextActivity != null)
return NextActivity;

// Find the nearest best refinery if not explicitly ordered to a specific refinery:
if (harv.OwnerLinkedProc == null || !harv.OwnerLinkedProc.IsInWorld)
Expand Down
4 changes: 2 additions & 2 deletions OpenRA.Mods.Common/Activities/FindResources.cs
View file
Open in desktop
Expand Up @@ -80,8 +80,8 @@ public override Activity Tick(Actor self)
var randFrames = self.World.SharedRandom.Next(100, 175);

// Avoid creating an activity cycle
var next = NextInQueue;
NextInQueue = null;
var next = NextActivity;
NextActivity = null;
return ActivityUtils.SequenceActivities(self, next, new Wait(randFrames), this);
}
else
Expand Down