Skip to content

Commit

Permalink
fix: Use concurrent queue and dictionary to avoid race conditions.
Browse files Browse the repository at this point in the history
With Unity2019.1.0a13 the example scene had failures with WorksQueue.Dequeue()
initially.  Presumably this is because the WorksQueue.Count test and Dequeue()
happen at different times.  To fix this, I switched WorksQueue to a
ConcurrentQueue and use TryDequeue() which dequeues atomically if there is an
item available and returns true; otherwise, it returns false.

After that was fixed, a new error popped up when the trees ran out. This
appeared to be caused by a race condition between the test for HasKey and Get.
To fix this, I switched the dictionaries in ReGoapState to ConcurrentDictionary
classes. And I removed Get() in favor of TryGetValue() which avoids that race
condition.

The example scene works.  All unit tests pass.

I hope these bugs mean that Unity is using more threads than it was before.
  • Loading branch information
shanecelis committed May 24, 2019
1 parent 506600c commit c21d7c2
Show file tree
Hide file tree
Showing 5 changed files with 45 additions and 39 deletions.
25 changes: 14 additions & 11 deletions ReGoap/Core/ReGoapState.cs
Original file line number Diff line number Diff line change
@@ -1,21 +1,23 @@
using System;
using System.Collections.Generic;
using System.Collections.Concurrent;

namespace ReGoap.Core
{
public class ReGoapState<T, W>
{
// can change to object
private Dictionary<T, W> values;
private readonly Dictionary<T, W> bufferA;
private readonly Dictionary<T, W> bufferB;
private ConcurrentDictionary<T, W> values;
private readonly ConcurrentDictionary<T, W> bufferA;
private readonly ConcurrentDictionary<T, W> bufferB;

public static int DefaultSize = 20;

private ReGoapState()
{
bufferA = new Dictionary<T, W>(DefaultSize);
bufferB = new Dictionary<T, W>(DefaultSize);
int concurrencyLevel = 5; // No idea.
bufferA = new ConcurrentDictionary<T, W>(concurrencyLevel, DefaultSize);
bufferB = new ConcurrentDictionary<T, W>(concurrencyLevel, DefaultSize);
values = bufferA;
}

Expand Down Expand Up @@ -258,18 +260,19 @@ public void Set(T key, W value)

public void Remove(T key)
{
lock (values)
{
values.Remove(key);
}
values.TryRemove(key, out _);
}

public Dictionary<T, W> GetValues()
public ConcurrentDictionary<T, W> GetValues()
{
lock (values)
return values;
}

public bool TryGetValue(T key, out W value) {
return values.TryGetValue(key, out value);
}

public bool HasKey(T key)
{
lock (values)
Expand All @@ -282,4 +285,4 @@ public void Clear()
values.Clear();
}
}
}
}
4 changes: 2 additions & 2 deletions ReGoap/Unity/FSMExample/Actions/CraftRecipeAction.cs
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ public override bool CheckProceduralCondition(GoapActionStackData<string, object

public override ReGoapState<string, object> GetPreconditions(GoapActionStackData<string, object> stackData)
{
if (stackData.settings.HasKey("workstationPosition"))
preconditions.Set("isAtPosition", stackData.settings.Get("workstationPosition"));
if (stackData.settings.TryGetValue("workstationPosition", out var workstationPosition))
preconditions.Set("isAtPosition", workstationPosition);
return preconditions;
}

Expand Down
18 changes: 12 additions & 6 deletions ReGoap/Unity/FSMExample/Actions/GatherResourceAction.cs
Original file line number Diff line number Diff line change
Expand Up @@ -47,19 +47,22 @@ protected virtual string GetNeededResourceFromGoal(ReGoapState<string, object> g
public override ReGoapState<string, object> GetPreconditions(GoapActionStackData<string, object> stackData)
{
preconditions.Clear();
if (stackData.settings.HasKey("resource"))
if (stackData.settings.HasKey("resource")
&& stackData.settings.TryGetValue("resourcePosition", out var resourcePosition))
{
preconditions.Set("isAtPosition", stackData.settings.Get("resourcePosition"));
preconditions.Set("isAtPosition", resourcePosition);
}
return preconditions;
}

public override ReGoapState<string, object> GetEffects(GoapActionStackData<string, object> stackData)
{
effects.Clear();
if (stackData.settings.HasKey("resource"))
if (stackData.settings.TryGetValue("resource", out var obj))
{
effects.Set("hasResource" + ((IResource)stackData.settings.Get("resource")).GetName(), true);
var resource = (IResource) obj;
if (resource != null)
effects.Set("hasResource" + resource.GetName(), true);
}
return effects;
}
Expand All @@ -85,7 +88,10 @@ public override List<ReGoapState<string, object>> GetSettings(GoapActionStackDat
}
else
{
var score = stackData.currentState.HasKey("isAtPosition") ? (wantedResource.position - (Vector3)stackData.currentState.Get("isAtPosition")).magnitude : 0.0f;
var score = stackData.currentState.TryGetValue("isAtPosition",
out object isAtPosition)
? (wantedResource.position - (Vector3)isAtPosition).magnitude
: 0.0f;
score += ReservedCostMultiplier * wantedResource.resource.GetReserveCount();
score += ResourcesCostMultiplier * (MaxResourcesCount - wantedResource.resource.GetCapacity());
if (score < bestScore)
Expand Down Expand Up @@ -175,4 +181,4 @@ protected void Update()
}
}
}
}
}
20 changes: 11 additions & 9 deletions ReGoap/Unity/FSMExample/Actions/GenericGoToAction.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ public override void Run(IReGoapAction<string, object> previous, IReGoapAction<s
{
base.Run(previous, next, settings, goalState, done, fail);

if (settings.HasKey("objectivePosition"))
smsGoto.GoTo((Vector3) settings.Get("objectivePosition"), OnDoneMovement, OnFailureMovement);
if (settings.TryGetValue("objectivePosition", out var v))
smsGoto.GoTo((Vector3) v, OnDoneMovement, OnFailureMovement);
else
failCallback(this);
}
Expand All @@ -43,9 +43,9 @@ public override bool CheckProceduralCondition(GoapActionStackData<string, object

public override ReGoapState<string, object> GetEffects(GoapActionStackData<string, object> stackData)
{
if (stackData.settings.HasKey("objectivePosition"))
if (stackData.settings.TryGetValue("objectivePosition", out var objectivePosition))
{
effects.Set("isAtPosition", stackData.settings.Get("objectivePosition"));
effects.Set("isAtPosition", objectivePosition);
if (stackData.settings.HasKey("reconcilePosition"))
effects.Set("reconcilePosition", true);
}
Expand All @@ -58,9 +58,9 @@ public override ReGoapState<string, object> GetEffects(GoapActionStackData<strin

public override List<ReGoapState<string, object>> GetSettings(GoapActionStackData<string, object> stackData)
{
if (stackData.goalState.HasKey("isAtPosition"))
if (stackData.goalState.TryGetValue("isAtPosition", out var isAtPosition))
{
settings.Set("objectivePosition", stackData.goalState.Get("isAtPosition"));
settings.Set("objectivePosition", isAtPosition);
return base.GetSettings(stackData);
}
else if (stackData.goalState.HasKey("reconcilePosition") && stackData.goalState.Count == 1)
Expand All @@ -76,9 +76,11 @@ public override List<ReGoapState<string, object>> GetSettings(GoapActionStackDat
public override float GetCost(GoapActionStackData<string, object> stackData)
{
var distance = 0.0f;
if (stackData.settings.HasKey("objectivePosition") && stackData.currentState.HasKey("isAtPosition"))
if (stackData.settings.TryGetValue("objectivePosition", out object objectivePosition)
&& stackData.currentState.TryGetValue("isAtPosition", out object isAtPosition))
{
distance = ((Vector3)stackData.settings.Get("objectivePosition") - (Vector3)stackData.currentState.Get("isAtPosition")).magnitude;
if (objectivePosition is Vector3 p && isAtPosition is Vector3 r)
distance = (p - r).magnitude;
}
return base.GetCost(stackData) + Cost + distance;
}
Expand All @@ -93,4 +95,4 @@ protected virtual void OnDoneMovement()
doneCallback(this);
}
}
}
}
17 changes: 6 additions & 11 deletions ReGoap/Unity/ReGoapPlannerManager.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.Generic;
using System.Collections.Concurrent;
using System.Threading;
using ReGoap.Core;
using ReGoap.Planner;
Expand All @@ -11,7 +12,7 @@ namespace ReGoap.Unity
public class ReGoapPlannerThread<T, W>
{
private readonly ReGoapPlanner<T, W> planner;
public static Queue<ReGoapPlanWork<T, W>> WorksQueue;
public static ConcurrentQueue<ReGoapPlanWork<T, W>> WorksQueue;
private bool isRunning = true;
private readonly Action<ReGoapPlannerThread<T, W>, ReGoapPlanWork<T, W>, IReGoapGoal<T, W>> onDonePlan;

Expand All @@ -37,16 +38,10 @@ public void MainLoop()

public void CheckWorkers()
{
if (WorksQueue.Count > 0)
{
ReGoapPlanWork<T, W> checkWork;
lock (WorksQueue)
{
checkWork = WorksQueue.Dequeue();
}
if (WorksQueue.TryDequeue(out ReGoapPlanWork<T, W> checkWork)) {
var work = checkWork;
planner.Plan(work.Agent, work.BlacklistGoal, work.Actions,
(newGoal) => onDonePlan(this, work, newGoal));
(newGoal) => onDonePlan(this, work, newGoal));
}
}
}
Expand Down Expand Up @@ -90,7 +85,7 @@ protected virtual void Awake()
Instance = this;

doneWorks = new List<ReGoapPlanWork<T, W>>();
ReGoapPlannerThread<T, W>.WorksQueue = new Queue<ReGoapPlanWork<T, W>>();
ReGoapPlannerThread<T, W>.WorksQueue = new ConcurrentQueue<ReGoapPlanWork<T, W>>();
planners = new ReGoapPlannerThread<T, W>[ThreadsCount];
threads = new Thread[ThreadsCount];

Expand Down Expand Up @@ -199,4 +194,4 @@ public ReGoapPlanWork(IReGoapAgent<T, W> agent, IReGoapGoal<T, W> blacklistGoal,
Callback = callback;
}
}
}
}

0 comments on commit c21d7c2

Please sign in to comment.