fix: incorrect deactivation order for inter-context dependencies (#474)

CHANGELOG.md

@@ -8,8 +8,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased]
 
 ### Added
-- [#472] Added the `DependsOnContext` attribute, for declaring dependencies between extension contexts.
-- Added `BuildContext.SetEnableUVDistributionRecalculation` to allow opting out from the automatic call to
+- [#472] [#474] Added the `DependsOnContext` attribute, for declaring dependencies between extension contexts.
+- [#473] Added `BuildContext.SetEnableUVDistributionRecalculation` to allow opting out from the automatic call to
   `Mesh.RecalculateUVDistributionMetrics` on generated meshes.
 
 ### Fixed

Editor/API/IExtensionContext.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Collections.Immutable;
 
 namespace nadena.dev.ndmf
 {
@@ -23,6 +24,8 @@ public interface IExtensionContext
 
     internal static class ExtensionContextUtil
     {
+        private static readonly Dictionary<Type, ImmutableList<Type>> RecursiveDependenciesCache = new();
+
         public static IEnumerable<Type> ContextDependencies(this Type ty, bool recurse)
         {
             if (recurse)
@@ -44,7 +47,20 @@ public static IEnumerable<Type> ContextDependencies(this Type ty)
             }
         }
 
-        private static IEnumerable<Type> RecursiveContextDependencies(Type ty)
+        private static ImmutableList<Type> RecursiveContextDependencies(Type ty)
+        {
+            if (RecursiveDependenciesCache.TryGetValue(ty, out var cached))
+            {
+                return cached;
+            }
+
+            var result = RecursiveContextDependencies0(ty).ToImmutableList();
+            RecursiveDependenciesCache[ty] = result;
+
+            return result;
+        }
+
+        private static IEnumerable<Type> RecursiveContextDependencies0(Type ty)
         {
             HashSet<Type> enqueued = new();
             Queue<Type> queue = new();

Editor/API/Solver/PluginResolver.cs

@@ -180,16 +180,19 @@ ImmutableList<ConcretePass> ToConcretePasses(BuildPhase phase, IEnumerable<Solve
 
                 var toDeactivate = new List<Type>();
                 var toActivate = new List<Type>();
-                activeExtensions.RemoveWhere(t =>
+
+                // To ensure that we deactivate extensions in the correct order, we sort them by the number of dependencies
+                // as a very crude toposort, with name as a tiebreaker (mostly for our tests)
+                foreach (var t in activeExtensions.OrderByDescending(
+                             t => (t.ContextDependencies(true).Count(), t.FullName)
+                         ).ToList())
                 {
                     if (!pass.IsExtensionCompatible(t, activeExtensions))
                     {
                         toDeactivate.Add(t);
-                        return true;
+                        activeExtensions.Remove(t);
                     }
-
-                    return false;
-                });
+                }
 
                 foreach (var t in ResolveExtensionDependencies(pass.RequiredExtensions))
                 {

UnitTests~/PluginResolverTests/ExtensionDependenciesTest.cs

@@ -1,9 +1,24 @@
-using System.Linq;
+using System.Collections.Immutable;
+using System.Linq;
 using nadena.dev.ndmf;
 using NUnit.Framework;
 
 namespace UnitTests.PluginResolverTests
 {
+    [DependsOnContext(typeof(Ctx1))]
+    public class Ctx4 : IExtensionContext
+    {
+        public void OnActivate(BuildContext context)
+        {
+
+        }
+
+        public void OnDeactivate(BuildContext context)
+        {
+
+        }
+    }
+
     [DependsOnContext(typeof(Ctx3))]
     [DependsOnContext(typeof(Ctx2))]
     public class Ctx1 : IExtensionContext
@@ -45,7 +60,7 @@ public void OnDeactivate(BuildContext context)
         }
     }
 
-    [DependsOnContext(typeof(Ctx1))]
+    [DependsOnContext(typeof(Ctx4))]
     public class Pass1 : Pass<Pass1>
     {
         protected override void Execute(BuildContext context)
@@ -58,12 +73,13 @@ public class Plugin1 : Plugin<Plugin1>
     {
         protected override void Configure()
         {
-            InPhase(BuildPhase.Generating)
-                .Run(Pass1.Instance)
-                .Then.WithCompatibleExtension(typeof(Ctx1), seq =>
+            var seq = InPhase(BuildPhase.Generating);
+            seq.Run(Pass1.Instance);
+            seq.WithCompatibleExtension(typeof(Ctx4), seq =>
                 {
                     seq.Run("test test", _ => { });
                 });
+            seq.Run("deactivate test", _ => { });
         }
     }
 
@@ -77,12 +93,16 @@ public void AssertCorrectPassDependencies()
             var phase = resolver.Passes.First(p => p.Item1 == BuildPhase.Generating).Item2;
             var pass1 = phase.First(pass => pass.InstantiatedPass is Pass1);
 
-            Assert.That(pass1.ActivatePlugins, Is.EquivalentTo(new[] { typeof(Ctx2), typeof(Ctx3), typeof(Ctx1) }));
+            Assert.AreEqual(pass1.ActivatePlugins, (new[] { typeof(Ctx2), typeof(Ctx3), typeof(Ctx1), typeof(Ctx4) }).ToImmutableList());
             Assert.That(pass1.DeactivatePlugins, Is.Empty);
 
             var pass2 = phase.First(pass => pass.InstantiatedPass.DisplayName == "test test");
             Assert.That(pass2.ActivatePlugins.IsEmpty);
             Assert.That(pass2.DeactivatePlugins.IsEmpty);
+
+            var pass3 = phase.First(pass => pass.InstantiatedPass.DisplayName == "deactivate test");
+            Assert.That(pass3.ActivatePlugins, Is.Empty);
+            Assert.AreEqual(pass3.DeactivatePlugins, (new[] { typeof(Ctx4), typeof(Ctx1), typeof(Ctx3), typeof(Ctx2) }).ToImmutableList());
         }
     }
 }