From 29ac14af69106ddf47e36245ab0176812591424c Mon Sep 17 00:00:00 2001 From: bd_ Date: Mon, 18 Nov 2024 20:07:22 -0800 Subject: [PATCH] fix: incorrect deactivation order for inter-context dependencies (#474) --- CHANGELOG.md | 4 +-- Editor/API/IExtensionContext.cs | 18 ++++++++++- Editor/API/Solver/PluginResolver.cs | 13 +++++--- .../ExtensionDependenciesTest.cs | 32 +++++++++++++++---- 4 files changed, 53 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 26fef1b..15f154c 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/Editor/API/IExtensionContext.cs b/Editor/API/IExtensionContext.cs index 1747ce9..190c3ac 100644 --- a/Editor/API/IExtensionContext.cs +++ b/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> RecursiveDependenciesCache = new(); + public static IEnumerable ContextDependencies(this Type ty, bool recurse) { if (recurse) @@ -44,7 +47,20 @@ public static IEnumerable ContextDependencies(this Type ty) } } - private static IEnumerable RecursiveContextDependencies(Type ty) + private static ImmutableList 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 RecursiveContextDependencies0(Type ty) { HashSet enqueued = new(); Queue queue = new(); diff --git a/Editor/API/Solver/PluginResolver.cs b/Editor/API/Solver/PluginResolver.cs index 3c8a56d..5ae478e 100644 --- a/Editor/API/Solver/PluginResolver.cs +++ b/Editor/API/Solver/PluginResolver.cs @@ -180,16 +180,19 @@ ImmutableList ToConcretePasses(BuildPhase phase, IEnumerable(); var toActivate = new List(); - 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)) { diff --git a/UnitTests~/PluginResolverTests/ExtensionDependenciesTest.cs b/UnitTests~/PluginResolverTests/ExtensionDependenciesTest.cs index 14b877d..181aa7b 100644 --- a/UnitTests~/PluginResolverTests/ExtensionDependenciesTest.cs +++ b/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 { protected override void Execute(BuildContext context) @@ -58,12 +73,13 @@ public class Plugin1 : Plugin { 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()); } } } \ No newline at end of file