diff --git a/OpenRA.Game/GameRules/ActorInfo.cs b/OpenRA.Game/GameRules/ActorInfo.cs index cce7eb280e..6b18022f05 100644 --- a/OpenRA.Game/GameRules/ActorInfo.cs +++ b/OpenRA.Game/GameRules/ActorInfo.cs @@ -100,9 +100,15 @@ namespace OpenRA var unresolved = source.Except(resolved); var testResolve = new Func((a, b) => a == b || a.IsAssignableFrom(b)); - var more = unresolved.Where(u => u.Dependencies.All(d => resolved.Exists(r => testResolve(d, r.Type)))); - // Re-evaluate the vars above until sorted + // This query detects which unresolved traits can be immediately resolved as all their direct dependencies are met. + var more = unresolved.Where(u => + u.Dependencies.All(d => // To be resolvable, all dependencies must be satisfied according to the following conditions: + resolved.Exists(r => testResolve(d, r.Type)) && // There must exist a resolved trait that meets the dependency. + !unresolved.Any(u1 => testResolve(d, u1.Type)))); // All matching traits that meet this dependency must be resolved first. + + // Continue resolving traits as long as possible. + // Each time we resolve some traits, this means dependencies for other traits may then be possible to satisfy in the next pass. while (more.Any()) resolved.AddRange(more); @@ -122,14 +128,14 @@ namespace OpenRA exceptionString += u.Type + ": { " + string.Join(", ", deps) + " }\r\n"; } - throw new Exception(exceptionString); + throw new YamlException(exceptionString); } constructOrderCache = resolved.Select(r => r.Trait).ToList(); return constructOrderCache; } - static IEnumerable PrerequisitesOf(ITraitInfo info) + public static IEnumerable PrerequisitesOf(ITraitInfo info) { return info .GetType() diff --git a/OpenRA.Mods.Common/Traits/Cloak.cs b/OpenRA.Mods.Common/Traits/Cloak.cs index a7ca2ab830..9f2bac1920 100644 --- a/OpenRA.Mods.Common/Traits/Cloak.cs +++ b/OpenRA.Mods.Common/Traits/Cloak.cs @@ -80,8 +80,11 @@ namespace OpenRA.Mods.Common.Traits void INotifyCreated.Created(Actor self) { upgradeManager = self.TraitOrDefault(); + + // The upgrade manager exists, but may not have finished being created yet. + // We'll defer the upgrades until the end of the tick, at which point it will be ready. if (Cloaked) - GrantUpgrades(self); + self.World.AddFrameEndTask(_ => GrantUpgrades(self)); } public bool Cloaked { get { return !IsTraitDisabled && remainingTime <= 0; } } diff --git a/OpenRA.Mods.Common/Traits/Upgrades/UpgradeManager.cs b/OpenRA.Mods.Common/Traits/Upgrades/UpgradeManager.cs index 84cfa545d6..81d2e0e4ad 100644 --- a/OpenRA.Mods.Common/Traits/Upgrades/UpgradeManager.cs +++ b/OpenRA.Mods.Common/Traits/Upgrades/UpgradeManager.cs @@ -17,12 +17,18 @@ using OpenRA.Traits; namespace OpenRA.Mods.Common.Traits { [Desc("Attach this to a unit to enable dynamic upgrades by warheads, experience, crates, support powers, etc.")] - public class UpgradeManagerInfo : ITraitInfo, Requires + public class UpgradeManagerInfo : TraitInfo, IRulesetLoaded { - public object Create(ActorInitializer init) { return new UpgradeManager(init); } + public void RulesetLoaded(Ruleset rules, ActorInfo info) + { + if (!info.Name.StartsWith("^") && !info.TraitInfos().Any()) + throw new YamlException( + "There are no upgrades to be managed for actor '{0}'. You are either missing some upgradeable traits, or this UpgradeManager trait is not required.".F( + info.Name)); + } } - public class UpgradeManager : ITick + public class UpgradeManager : INotifyCreated, ITick { class TimedUpgrade { @@ -67,20 +73,21 @@ namespace OpenRA.Mods.Common.Traits } readonly List timedUpgrades = new List(); - readonly Lazy> upgrades; readonly Dictionary levels = new Dictionary(); + Dictionary upgrades; - public UpgradeManager(ActorInitializer init) + void INotifyCreated.Created(Actor self) { - upgrades = Exts.Lazy(() => - { - var ret = new Dictionary(); - foreach (var up in init.Self.TraitsImplementing()) - foreach (var t in up.UpgradeTypes) - ret.GetOrAdd(t).Traits.Add(up); + upgrades = new Dictionary(); + foreach (var up in self.TraitsImplementing()) + foreach (var t in up.UpgradeTypes) + upgrades.GetOrAdd(t).Traits.Add(up); + } - return ret; - }); + void CheckCanManageUpgrades() + { + if (upgrades == null) + throw new InvalidOperationException("Upgrades cannot be managed until the actor has been fully created."); } /// Upgrade level increments are limited to dupesAllowed per source, i.e., if a single @@ -139,8 +146,10 @@ namespace OpenRA.Mods.Common.Traits public void GrantUpgrade(Actor self, string upgrade, object source) { + CheckCanManageUpgrades(); + UpgradeState s; - if (!upgrades.Value.TryGetValue(upgrade, out s)) + if (!upgrades.TryGetValue(upgrade, out s)) return; // Track the upgrade source so that the upgrade can be removed without conflicts @@ -151,8 +160,10 @@ namespace OpenRA.Mods.Common.Traits public void RevokeUpgrade(Actor self, string upgrade, object source) { + CheckCanManageUpgrades(); + UpgradeState s; - if (!upgrades.Value.TryGetValue(upgrade, out s)) + if (!upgrades.TryGetValue(upgrade, out s)) return; if (!s.Sources.Remove(source)) @@ -164,14 +175,17 @@ namespace OpenRA.Mods.Common.Traits /// Returns true if the actor uses the given upgrade. Does not check the actual level of the upgrade. public bool AcknowledgesUpgrade(Actor self, string upgrade) { - return upgrades.Value.ContainsKey(upgrade); + CheckCanManageUpgrades(); + return upgrades.ContainsKey(upgrade); } /// Returns true only if the actor can accept another level of the upgrade. public bool AcceptsUpgrade(Actor self, string upgrade) { + CheckCanManageUpgrades(); + UpgradeState s; - if (!upgrades.Value.TryGetValue(upgrade, out s)) + if (!upgrades.TryGetValue(upgrade, out s)) return false; return s.Traits.Any(up => up.AcceptsUpgradeLevel(self, upgrade, GetOverallLevel(up) + 1)); @@ -179,8 +193,10 @@ namespace OpenRA.Mods.Common.Traits public void RegisterWatcher(string upgrade, Action action) { + CheckCanManageUpgrades(); + UpgradeState s; - if (!upgrades.Value.TryGetValue(upgrade, out s)) + if (!upgrades.TryGetValue(upgrade, out s)) return; s.Watchers.Add(action); @@ -192,6 +208,8 @@ namespace OpenRA.Mods.Common.Traits /// GrantTimedUpgrade). public void Tick(Actor self) { + CheckCanManageUpgrades(); + foreach (var u in timedUpgrades) { u.Tick(); @@ -201,7 +219,7 @@ namespace OpenRA.Mods.Common.Traits u.Sources.RemoveWhere(source => source.Remaining <= 0); - foreach (var a in upgrades.Value[u.Upgrade].Watchers) + foreach (var a in upgrades[u.Upgrade].Watchers) a(u.Duration, u.Remaining); } diff --git a/OpenRA.Test/OpenRA.Game/ActorInfoTest.cs b/OpenRA.Test/OpenRA.Game/ActorInfoTest.cs index bbb0804e34..b3219869e3 100644 --- a/OpenRA.Test/OpenRA.Game/ActorInfoTest.cs +++ b/OpenRA.Test/OpenRA.Game/ActorInfoTest.cs @@ -10,7 +10,6 @@ #endregion using System; -using System.Collections.Generic; using System.Linq; using System.Text.RegularExpressions; using NUnit.Framework; @@ -28,89 +27,49 @@ namespace OpenRA.Test class MockEInfo : MockTraitInfo, Requires { } class MockFInfo : MockTraitInfo, Requires { } - class MockA2Info : MockTraitInfo { } - class MockB2Info : MockTraitInfo { } - class MockC2Info : MockTraitInfo { } - - class MockStringInfo : MockTraitInfo { public string AString = null; } - [TestFixture] public class ActorInfoTest { - [SetUp] - public void SetUp() + [TestCase(TestName = "Trait ordering sorts in dependency order correctly")] + public void TraitOrderingSortsCorrectly() { + var unorderedTraits = new ITraitInfo[] { new MockBInfo(), new MockCInfo(), new MockAInfo(), new MockBInfo() }; + var actorInfo = new ActorInfo("test", unorderedTraits); + var orderedTraits = actorInfo.TraitsInConstructOrder().ToArray(); + + CollectionAssert.AreEquivalent(unorderedTraits, orderedTraits); + + for (var i = 0; i < orderedTraits.Length; i++) + { + var traitTypesThatMustOccurBeforeThisTrait = ActorInfo.PrerequisitesOf(orderedTraits[i]); + var traitTypesThatOccurAfterThisTrait = orderedTraits.Skip(i + 1).Select(ti => ti.GetType()); + var traitTypesThatShouldOccurEarlier = traitTypesThatOccurAfterThisTrait.Intersect(traitTypesThatMustOccurBeforeThisTrait); + CollectionAssert.IsEmpty(traitTypesThatShouldOccurEarlier, "Dependency order has not been satisfied."); + } } - [TestCase(TestName = "Sort traits in order of dependency")] - public void TraitsInConstructOrderA() - { - var actorInfo = new ActorInfo("test", new MockCInfo(), new MockBInfo(), new MockAInfo()); - - var i = new List(actorInfo.TraitsInConstructOrder()); - - Assert.That(i[0], Is.InstanceOf()); - Assert.That(i[1].GetType().Name, Is.EqualTo("MockBInfo")); - Assert.That(i[2].GetType().Name, Is.EqualTo("MockCInfo")); - } - - [TestCase(TestName = "Exception reports missing dependencies")] - public void TraitsInConstructOrderB() + [TestCase(TestName = "Trait ordering exception reports missing dependencies")] + public void TraitOrderingReportsMissingDependencies() { var actorInfo = new ActorInfo("test", new MockBInfo(), new MockCInfo()); + var ex = Assert.Throws(() => actorInfo.TraitsInConstructOrder()); - try - { - actorInfo.TraitsInConstructOrder(); - throw new Exception("Exception not thrown!"); - } - catch (Exception e) - { - // Is.StringContaining is deprecated in NUnit 3, but we need to support NUnit 2 so we ignore the warning. - #pragma warning disable CS0618 - Assert.That(e.Message, Is.StringContaining("MockA")); - Assert.That(e.Message, Is.StringContaining("MockB")); - Assert.That(e.Message, Is.StringContaining("MockC")); - Assert.That(e.Message, Is.StringContaining("MockInherit"), "Should recognize base classes"); - Assert.That(e.Message, Is.StringContaining("IMock"), "Should recognize interfaces"); - #pragma warning restore CS0618 - } + StringAssert.Contains(typeof(MockAInfo).Name, ex.Message, "Exception message did not report a missing dependency."); + StringAssert.Contains(typeof(MockBInfo).Name, ex.Message, "Exception message did not report a missing dependency."); + StringAssert.Contains(typeof(MockCInfo).Name, ex.Message, "Exception message did not report a missing dependency."); + StringAssert.Contains(typeof(MockInheritInfo).Name, ex.Message, "Exception message did not report a missing dependency (from a base class)."); + StringAssert.Contains(typeof(IMock).Name, ex.Message, "Exception message did not report a missing dependency (from an interface)."); } - [TestCase(TestName = "Exception reports cyclic dependencies")] - public void TraitsInConstructOrderC() + [TestCase(TestName = "Trait ordering exception reports cyclic dependencies")] + public void TraitOrderingReportsCyclicDependencies() { var actorInfo = new ActorInfo("test", new MockDInfo(), new MockEInfo(), new MockFInfo()); + var ex = Assert.Throws(() => actorInfo.TraitsInConstructOrder()); - try - { - actorInfo.TraitsInConstructOrder(); - throw new Exception("Exception not thrown!"); - } - catch (Exception e) - { - var count = ( - new Regex("MockD").Matches(e.Message).Count + - new Regex("MockE").Matches(e.Message).Count + - new Regex("MockF").Matches(e.Message).Count) / 3.0; - - Assert.That(count, Is.EqualTo(Math.Floor(count)), "Should be symmetrical"); - } - } - - // This needs to match the logic used in RulesetCache.LoadYamlRules - ActorInfo CreateActorInfoFromYaml(string name, string mapYaml, params string[] yamls) - { - var nodes = mapYaml == null ? new List() : MiniYaml.FromString(mapYaml); - var sources = yamls.ToList(); - if (mapYaml != null) - sources.Add(mapYaml); - - var yaml = MiniYaml.Merge(sources.Select(s => MiniYaml.FromString(s))); - var allUnits = yaml.ToDictionary(node => node.Key, node => node.Value); - var unit = allUnits[name]; - var creator = new ObjectCreator(typeof(ActorInfoTest).Assembly); - return new ActorInfo(creator, name, unit); + StringAssert.Contains(typeof(MockDInfo).Name, ex.Message, "Exception message should report all cyclic dependencies."); + StringAssert.Contains(typeof(MockEInfo).Name, ex.Message, "Exception message should report all cyclic dependencies."); + StringAssert.Contains(typeof(MockFInfo).Name, ex.Message, "Exception message should report all cyclic dependencies."); } } }