From aad97b39c366a22fc84c2f654d9debf66389846b Mon Sep 17 00:00:00 2001 From: Maxwell Talbot Date: Wed, 8 Jan 2020 16:04:33 +0000 Subject: [PATCH 1/4] Added an alternative dependency sorter that will attempt to return a list of dependencies without cycles --- flare_dart/lib/actor_component.dart | 4 + flare_dart/lib/dependency_sorter.dart | 57 +++++ flare_dart/pubspec.yaml | 3 +- flare_dart/test/test_dependency_sorter.dart | 258 ++++++++++++++++++++ 4 files changed, 321 insertions(+), 1 deletion(-) create mode 100644 flare_dart/test/test_dependency_sorter.dart diff --git a/flare_dart/lib/actor_component.dart b/flare_dart/lib/actor_component.dart index 12ea89d..86790c7 100644 --- a/flare_dart/lib/actor_component.dart +++ b/flare_dart/lib/actor_component.dart @@ -31,6 +31,10 @@ abstract class ActorComponent { return _name; } + set name(String name) { + _name = name; + } + void resolveComponentIndices(List components) { ActorNode node = components[_parentIdx] as ActorNode; if (node != null) { diff --git a/flare_dart/lib/dependency_sorter.dart b/flare_dart/lib/dependency_sorter.dart index 98df48d..79b4d79 100644 --- a/flare_dart/lib/dependency_sorter.dart +++ b/flare_dart/lib/dependency_sorter.dart @@ -44,3 +44,60 @@ class DependencySorter { return true; } } + +class CycleDependencySorter extends DependencySorter { + HashSet _cycleNodes; + CycleDependencySorter() { + _perm = HashSet(); + _temp = HashSet(); + _cycleNodes = HashSet(); + } + + HashSet get cycleNodes => _cycleNodes; + + @override + List sort(ActorComponent root) { + _order = []; + visit(root); + return _order; + } + + @override + bool visit(ActorComponent n) { + // Follow the nodes along their dependencies. + // track visited nodes, + // When a cycle is detected, + // scrap all visited nodes tracked until the start of the cycle + // track these nodes as 'cycleNodes' + + if (_perm.contains(n)) { + // node's dependencies have already been evaluated. + } else if (_temp.contains(n)) { + if (_cycleNodes.contains(n)) { + // we're onto a cycle that has already been removed. + return false; + } + // node is being evaluated, but not complete yet, CYCLE! + ActorComponent lastComponent; + while (lastComponent != n) { + lastComponent = _order.removeLast(); + _cycleNodes.add(lastComponent); + } + return false; + } else { + _order.add(n); + _temp.add(n); + if (n.dependents != null) { + for (final ActorComponent dependant in n.dependents) { + var cycleFound = !visit(dependant); + if (cycleFound && _cycleNodes.contains(n)) { + // node is part of a cycle, we're done here. + return false; + } + } + } + _perm.add(n); + } + return true; + } +} diff --git a/flare_dart/pubspec.yaml b/flare_dart/pubspec.yaml index 172bc97..2630b64 100644 --- a/flare_dart/pubspec.yaml +++ b/flare_dart/pubspec.yaml @@ -5,4 +5,5 @@ author: "Rive Team " homepage: https://github.com/2d-inc/Flare-Flutter environment: sdk: ">=2.1.0 <3.0.0" - \ No newline at end of file +dev_dependencies: + test: ^1.9.4 \ No newline at end of file diff --git a/flare_dart/test/test_dependency_sorter.dart b/flare_dart/test/test_dependency_sorter.dart new file mode 100644 index 0000000..7ee06bf --- /dev/null +++ b/flare_dart/test/test_dependency_sorter.dart @@ -0,0 +1,258 @@ +import 'dart:typed_data'; + +import 'package:flare_dart/actor_artboard.dart'; +import 'package:flare_dart/actor_color.dart'; +import 'package:flare_dart/actor_component.dart'; +import 'package:flare_dart/actor_drop_shadow.dart'; +import 'package:flare_dart/actor_inner_shadow.dart'; +import 'package:flare_dart/actor_layer_effect_renderer.dart'; +import 'package:flare_dart/actor_node.dart'; +import 'package:flare_dart/actor.dart'; +import 'package:flare_dart/dependency_sorter.dart'; + +import 'package:test/test.dart'; + +class DummyActor extends Actor { + @override + Future loadAtlases(List rawAtlases) { + throw UnimplementedError(); + } + + @override + ColorFill makeColorFill() { + throw UnimplementedError(); + } + + @override + ColorStroke makeColorStroke() { + throw UnimplementedError(); + } + + @override + ActorDropShadow makeDropShadow() { + throw UnimplementedError(); + } + + @override + GradientFill makeGradientFill() { + throw UnimplementedError(); + } + + @override + GradientStroke makeGradientStroke() { + throw UnimplementedError(); + } + + @override + ActorInnerShadow makeInnerShadow() { + throw UnimplementedError(); + } + + @override + ActorLayerEffectRenderer makeLayerEffectRenderer() { + throw UnimplementedError(); + } + + @override + RadialGradientFill makeRadialFill() { + throw UnimplementedError(); + } + + @override + RadialGradientStroke makeRadialStroke() { + throw UnimplementedError(); + } + + @override + Future readOutOfBandAsset(String filename, context) { + throw UnimplementedError(); + } +} + +String orderString(List order) { + String output = order.fold( + '', + (previousValue, actorComponent) => + previousValue + ' ' + actorComponent.name); + return output.trim(); +} + +void main() { + group("Simple Cycle:", () { + ActorArtboard artboard; + + setUp(() async { + final actor = DummyActor(); + artboard = ActorArtboard(actor); + final nodeA = ActorNode()..name = 'A'; + final nodeB = ActorNode()..name = 'B'; + final nodeC = ActorNode()..name = 'C'; + final nodeD = ActorNode()..name = 'D'; + + /// + /// [root] <- [A] <- [B] <- [D] + /// A | A + /// | +------+ + /// [C] + artboard.addDependency(nodeA, artboard.root); + artboard.addDependency(nodeB, nodeA); + artboard.addDependency(nodeC, nodeA); + artboard.addDependency(nodeD, nodeB); + artboard.addDependency(nodeB, nodeD); + }); + + test("DependencySorter stops on cycle", () { + expect(DependencySorter().sort(artboard.root), equals(null)); + }); + + test("CycleDependencySorter produces best guess exluding cycle nodes", () { + var order = CycleDependencySorter().sort(artboard.root); + expect(order.length, equals(3)); + expect(orderString(order), equals('Unnamed A C')); + }); + }); + group("No cycle, but double dependants", () { + ActorArtboard artboard; + + setUp(() async { + final actor = DummyActor(); + artboard = ActorArtboard(actor); + final nodeA = ActorNode()..name = 'A'; + final nodeB = ActorNode()..name = 'B'; + final nodeC = ActorNode()..name = 'C'; + final nodeD = ActorNode()..name = 'D'; + + /// + /// [root] <- [A] <- [B] <- [D] + /// A A + /// | | + /// [C]-----+ + artboard.addDependency(nodeA, artboard.root); + artboard.addDependency(nodeB, nodeA); + artboard.addDependency(nodeC, nodeA); + artboard.addDependency(nodeD, nodeB); + artboard.addDependency(nodeC, nodeB); + }); + + test("DependencySorter is ok", () { + var order = DependencySorter().sort(artboard.root); + expect(order, isNotNull); + expect(orderString(order), 'Unnamed A B C D'); + }); + + test("CycleDependencySorter is ok", () { + var order = CycleDependencySorter().sort(artboard.root); + expect(order, isNotNull); + expect(order.length, equals(5)); + expect(orderString(order), equals('Unnamed A B D C')); + }); + }); + + group("Complex Cycle", () { + ActorArtboard artboard; + + setUp(() async { + final actor = DummyActor(); + artboard = ActorArtboard(actor); + final nodeA = ActorNode()..name = 'A'; + final nodeB = ActorNode()..name = 'B'; + final nodeC = ActorNode()..name = 'C'; + final nodeD = ActorNode()..name = 'D'; + + /// + /// +------+ + /// | v + /// [root] <- [A] <- [B] <- [D] + /// A | + /// | | + /// [C]<-----------+ + /// + artboard.addDependency(nodeA, artboard.root); + artboard.addDependency(nodeB, nodeA); + artboard.addDependency(nodeD, nodeB); + artboard.addDependency(nodeB, nodeD); + artboard.addDependency(nodeC, nodeA); + artboard.addDependency(nodeD, nodeC); + }); + + test("DependencySorter is ok", () { + expect(DependencySorter().sort(artboard.root), equals(null)); + }); + + test("CycleDependencySorter is ok", () { + var order = CycleDependencySorter().sort(artboard.root); + expect(order, isNotNull); + expect(order.length, equals(3)); + expect(orderString(order), equals('Unnamed A C')); + }); + }); + + group("Bad Cycle", () { + /// + /// +-------------+ + /// | | + /// | [F] | + /// | v v + /// [root] <- [A] <- [B] <- [C] <- [D] + /// A | + /// | | + /// [E]<-----------+ + /// + + test("CycleDependencySorter expected [root A E]", () { + final actor = DummyActor(); + final artboard = ActorArtboard(actor); + final nodeA = ActorNode()..name = 'A'; + final nodeB = ActorNode()..name = 'B'; + final nodeC = ActorNode()..name = 'C'; + final nodeD = ActorNode()..name = 'D'; + final nodeE = ActorNode()..name = 'E'; + final nodeF = ActorNode()..name = 'F'; + + artboard.addDependency(nodeA, artboard.root); + artboard.addDependency(nodeB, nodeA); + artboard.addDependency(nodeC, nodeB); + artboard.addDependency(nodeD, nodeC); + artboard.addDependency(nodeB, nodeD); + artboard.addDependency(nodeE, nodeA); + artboard.addDependency(nodeC, nodeE); + artboard.addDependency(nodeF, nodeC); + var dependencySorter = CycleDependencySorter(); + var order = dependencySorter.sort(artboard.root); + expect(order, isNotNull); + expect(order.length, equals(3)); + expect(orderString(order), equals('Unnamed A E')); + expect(dependencySorter.cycleNodes, + containsAll([nodeB, nodeC, nodeD, nodeF]), + skip: "CycleNodes will not include F, as it was" + " found to be dependant on a cycle before being evaluated."); + }); + + test("CycleDependencySorter reorderd expected [root A E]", () { + final actor = DummyActor(); + final artboard = ActorArtboard(actor); + final nodeA = ActorNode()..name = 'A'; + final nodeB = ActorNode()..name = 'B'; + final nodeC = ActorNode()..name = 'C'; + final nodeD = ActorNode()..name = 'D'; + final nodeE = ActorNode()..name = 'E'; + final nodeF = ActorNode()..name = 'F'; + + artboard.addDependency(nodeA, artboard.root); + artboard.addDependency(nodeE, nodeA); + artboard.addDependency(nodeC, nodeE); + artboard.addDependency(nodeF, nodeC); + artboard.addDependency(nodeB, nodeA); + artboard.addDependency(nodeC, nodeB); + artboard.addDependency(nodeD, nodeC); + artboard.addDependency(nodeB, nodeD); + var dependencySorter = CycleDependencySorter(); + var order = dependencySorter.sort(artboard.root); + expect(order, isNotNull); + expect(order.length, equals(3)); + expect(orderString(order), equals('Unnamed A E')); + expect(dependencySorter.cycleNodes, + containsAll([nodeB, nodeC, nodeD, nodeF])); + }); + }); +} From af6a7da69d0744d2321414e46781e3d5a9e5bbaf Mon Sep 17 00:00:00 2001 From: Maxwell Talbot Date: Thu, 9 Jan 2020 11:00:55 +0000 Subject: [PATCH 2/4] Replaced dependency sorter with a sorter using Tarjans algorithm to detect cycles --- flare_dart/lib/dependency_sorter.dart | 80 ++++++++++----------- flare_dart/pubspec.yaml | 2 + flare_dart/test/test_dependency_sorter.dart | 28 +++----- 3 files changed, 49 insertions(+), 61 deletions(-) diff --git a/flare_dart/lib/dependency_sorter.dart b/flare_dart/lib/dependency_sorter.dart index 79b4d79..975e119 100644 --- a/flare_dart/lib/dependency_sorter.dart +++ b/flare_dart/lib/dependency_sorter.dart @@ -1,4 +1,7 @@ import "dart:collection"; + +import "package:graphs/graphs.dart"; + import "actor_component.dart"; class DependencySorter { @@ -19,12 +22,17 @@ class DependencySorter { return _order; } - bool visit(ActorComponent n) { + bool visit(ActorComponent n, {HashSet cycleNodes}) { + cycleNodes ??= HashSet(); + if (cycleNodes.contains(n)) { + // skip any nodes on a known cycle. + return true; + } if (_perm.contains(n)) { return true; } if (_temp.contains(n)) { - print("Dependency cycle!"); + // cycle detected! return false; } @@ -33,7 +41,7 @@ class DependencySorter { List dependents = n.dependents; if (dependents != null) { for (final ActorComponent d in dependents) { - if (!visit(d)) { + if (!visit(d, cycleNodes: cycleNodes)) { return false; } } @@ -45,59 +53,43 @@ class DependencySorter { } } -class CycleDependencySorter extends DependencySorter { +class TarjansDependencySorter extends DependencySorter { HashSet _cycleNodes; - CycleDependencySorter() { + HashSet get cycleNodes => _cycleNodes; + + TarjansDependencySorter() { _perm = HashSet(); _temp = HashSet(); _cycleNodes = HashSet(); } - HashSet get cycleNodes => _cycleNodes; - @override List sort(ActorComponent root) { _order = []; - visit(root); - return _order; - } - @override - bool visit(ActorComponent n) { - // Follow the nodes along their dependencies. - // track visited nodes, - // When a cycle is detected, - // scrap all visited nodes tracked until the start of the cycle - // track these nodes as 'cycleNodes' + if (!visit(root)) { + // if we detect cycles, go find them all + _perm.clear(); + _temp.clear(); + _cycleNodes.clear(); + _order.clear(); - if (_perm.contains(n)) { - // node's dependencies have already been evaluated. - } else if (_temp.contains(n)) { - if (_cycleNodes.contains(n)) { - // we're onto a cycle that has already been removed. - return false; - } - // node is being evaluated, but not complete yet, CYCLE! - ActorComponent lastComponent; - while (lastComponent != n) { - lastComponent = _order.removeLast(); - _cycleNodes.add(lastComponent); - } - return false; - } else { - _order.add(n); - _temp.add(n); - if (n.dependents != null) { - for (final ActorComponent dependant in n.dependents) { - var cycleFound = !visit(dependant); - if (cycleFound && _cycleNodes.contains(n)) { - // node is part of a cycle, we're done here. - return false; - } + var cycles = stronglyConnectedComponents( + [root], (ActorComponent node) => node.dependents); + + cycles.forEach((cycle) { + // cycles of len 1 are not cycles. + if (cycle.length > 1) { + cycle.forEach((cycleMember) { + _cycleNodes.add(cycleMember); + }); } - } - _perm.add(n); + }); + + // revisit the tree, skipping nodes on any cycle. + visit(root, cycleNodes: _cycleNodes); } - return true; + + return _order; } } diff --git a/flare_dart/pubspec.yaml b/flare_dart/pubspec.yaml index 2630b64..6f46ee7 100644 --- a/flare_dart/pubspec.yaml +++ b/flare_dart/pubspec.yaml @@ -5,5 +5,7 @@ author: "Rive Team " homepage: https://github.com/2d-inc/Flare-Flutter environment: sdk: ">=2.1.0 <3.0.0" +dependencies: + graphs: ^0.2.0 dev_dependencies: test: ^1.9.4 \ No newline at end of file diff --git a/flare_dart/test/test_dependency_sorter.dart b/flare_dart/test/test_dependency_sorter.dart index 7ee06bf..97f5ffc 100644 --- a/flare_dart/test/test_dependency_sorter.dart +++ b/flare_dart/test/test_dependency_sorter.dart @@ -105,8 +105,8 @@ void main() { expect(DependencySorter().sort(artboard.root), equals(null)); }); - test("CycleDependencySorter produces best guess exluding cycle nodes", () { - var order = CycleDependencySorter().sort(artboard.root); + test("TarjansDependencySorter stops on cycle", () { + var order = TarjansDependencySorter().sort(artboard.root); expect(order.length, equals(3)); expect(orderString(order), equals('Unnamed A C')); }); @@ -140,11 +140,10 @@ void main() { expect(orderString(order), 'Unnamed A B C D'); }); - test("CycleDependencySorter is ok", () { - var order = CycleDependencySorter().sort(artboard.root); + test("TarjansDependencySorter stops on cycle", () { + var order = TarjansDependencySorter().sort(artboard.root); expect(order, isNotNull); - expect(order.length, equals(5)); - expect(orderString(order), equals('Unnamed A B D C')); + expect(orderString(order), 'Unnamed A B C D'); }); }); @@ -179,10 +178,9 @@ void main() { expect(DependencySorter().sort(artboard.root), equals(null)); }); - test("CycleDependencySorter is ok", () { - var order = CycleDependencySorter().sort(artboard.root); + test("TarjansDependencySorter stops on cycle", () { + var order = TarjansDependencySorter().sort(artboard.root); expect(order, isNotNull); - expect(order.length, equals(3)); expect(orderString(order), equals('Unnamed A C')); }); }); @@ -217,15 +215,12 @@ void main() { artboard.addDependency(nodeE, nodeA); artboard.addDependency(nodeC, nodeE); artboard.addDependency(nodeF, nodeC); - var dependencySorter = CycleDependencySorter(); + var dependencySorter = TarjansDependencySorter(); var order = dependencySorter.sort(artboard.root); expect(order, isNotNull); expect(order.length, equals(3)); expect(orderString(order), equals('Unnamed A E')); - expect(dependencySorter.cycleNodes, - containsAll([nodeB, nodeC, nodeD, nodeF]), - skip: "CycleNodes will not include F, as it was" - " found to be dependant on a cycle before being evaluated."); + expect(dependencySorter.cycleNodes, containsAll([nodeB, nodeC, nodeD])); }); test("CycleDependencySorter reorderd expected [root A E]", () { @@ -246,13 +241,12 @@ void main() { artboard.addDependency(nodeC, nodeB); artboard.addDependency(nodeD, nodeC); artboard.addDependency(nodeB, nodeD); - var dependencySorter = CycleDependencySorter(); + var dependencySorter = TarjansDependencySorter(); var order = dependencySorter.sort(artboard.root); expect(order, isNotNull); expect(order.length, equals(3)); expect(orderString(order), equals('Unnamed A E')); - expect(dependencySorter.cycleNodes, - containsAll([nodeB, nodeC, nodeD, nodeF])); + expect(dependencySorter.cycleNodes, containsAll([nodeB, nodeC, nodeD])); }); }); } From 8aad656acdda8f327ad96997e6b3c5cc9b0fe21c Mon Sep 17 00:00:00 2001 From: Maxwell Talbot Date: Thu, 9 Jan 2020 11:24:37 +0000 Subject: [PATCH 3/4] cleaned up tests, and clearly highlight how isolated nodes will be treated currently --- flare_dart/lib/dependency_sorter.dart | 5 + flare_dart/test/test_dependency_sorter.dart | 241 ++++++++++---------- 2 files changed, 129 insertions(+), 117 deletions(-) diff --git a/flare_dart/lib/dependency_sorter.dart b/flare_dart/lib/dependency_sorter.dart index 975e119..76f6ce7 100644 --- a/flare_dart/lib/dependency_sorter.dart +++ b/flare_dart/lib/dependency_sorter.dart @@ -53,6 +53,11 @@ class DependencySorter { } } +/// Sorts dependencies for Actors even when cycles are present +/// +/// Any nodes that form part of a cycle can be found in `cycleNodes` after `sort`. +/// NOTE: Nodes isolated by cycles will not be found in `_order` or `cycleNodes` +/// e.g. `A -> B <-> C -> D` isolates D when running a sort based on A class TarjansDependencySorter extends DependencySorter { HashSet _cycleNodes; HashSet get cycleNodes => _cycleNodes; diff --git a/flare_dart/test/test_dependency_sorter.dart b/flare_dart/test/test_dependency_sorter.dart index 97f5ffc..701f3be 100644 --- a/flare_dart/test/test_dependency_sorter.dart +++ b/flare_dart/test/test_dependency_sorter.dart @@ -80,112 +80,112 @@ String orderString(List order) { void main() { group("Simple Cycle:", () { ActorArtboard artboard; + final actor = DummyActor(); + artboard = ActorArtboard(actor); + final nodeA = ActorNode()..name = 'A'; + final nodeB = ActorNode()..name = 'B'; + final nodeC = ActorNode()..name = 'C'; + final nodeD = ActorNode()..name = 'D'; - setUp(() async { - final actor = DummyActor(); - artboard = ActorArtboard(actor); - final nodeA = ActorNode()..name = 'A'; - final nodeB = ActorNode()..name = 'B'; - final nodeC = ActorNode()..name = 'C'; - final nodeD = ActorNode()..name = 'D'; - - /// - /// [root] <- [A] <- [B] <- [D] - /// A | A - /// | +------+ - /// [C] - artboard.addDependency(nodeA, artboard.root); - artboard.addDependency(nodeB, nodeA); - artboard.addDependency(nodeC, nodeA); - artboard.addDependency(nodeD, nodeB); - artboard.addDependency(nodeB, nodeD); - }); - - test("DependencySorter stops on cycle", () { + /// + /// [root] <- [A] <- [B] <- [D] + /// A | A + /// | +------+ + /// [C] + artboard.addDependency(nodeA, artboard.root); + artboard.addDependency(nodeB, nodeA); + artboard.addDependency(nodeC, nodeA); + artboard.addDependency(nodeD, nodeB); + artboard.addDependency(nodeB, nodeD); + + test("DependencySorter cannot order", () { expect(DependencySorter().sort(artboard.root), equals(null)); }); - test("TarjansDependencySorter stops on cycle", () { + test("TarjansDependencySorter orders", () { var order = TarjansDependencySorter().sort(artboard.root); expect(order.length, equals(3)); expect(orderString(order), equals('Unnamed A C')); }); }); - group("No cycle, but double dependants", () { - ActorArtboard artboard; + group("No cycle:", () { + final actor = DummyActor(); + final artboard = ActorArtboard(actor); + final nodeA = ActorNode()..name = 'A'; + final nodeB = ActorNode()..name = 'B'; + final nodeC = ActorNode()..name = 'C'; + final nodeD = ActorNode()..name = 'D'; - setUp(() async { - final actor = DummyActor(); - artboard = ActorArtboard(actor); - final nodeA = ActorNode()..name = 'A'; - final nodeB = ActorNode()..name = 'B'; - final nodeC = ActorNode()..name = 'C'; - final nodeD = ActorNode()..name = 'D'; - - /// - /// [root] <- [A] <- [B] <- [D] - /// A A - /// | | - /// [C]-----+ - artboard.addDependency(nodeA, artboard.root); - artboard.addDependency(nodeB, nodeA); - artboard.addDependency(nodeC, nodeA); - artboard.addDependency(nodeD, nodeB); - artboard.addDependency(nodeC, nodeB); - }); - - test("DependencySorter is ok", () { + /// + /// [root] <- [A] <- [B] <- [D] + /// A A + /// | | + /// [C]-----+ + artboard.addDependency(nodeA, artboard.root); + artboard.addDependency(nodeB, nodeA); + artboard.addDependency(nodeC, nodeA); + artboard.addDependency(nodeD, nodeB); + artboard.addDependency(nodeC, nodeB); + + test("DependencySorter orders", () { var order = DependencySorter().sort(artboard.root); expect(order, isNotNull); expect(orderString(order), 'Unnamed A B C D'); }); - test("TarjansDependencySorter stops on cycle", () { + test("DependencySorter orders", () { var order = TarjansDependencySorter().sort(artboard.root); expect(order, isNotNull); expect(orderString(order), 'Unnamed A B C D'); }); }); - group("Complex Cycle", () { - ActorArtboard artboard; - - setUp(() async { - final actor = DummyActor(); - artboard = ActorArtboard(actor); - final nodeA = ActorNode()..name = 'A'; - final nodeB = ActorNode()..name = 'B'; - final nodeC = ActorNode()..name = 'C'; - final nodeD = ActorNode()..name = 'D'; - - /// - /// +------+ - /// | v - /// [root] <- [A] <- [B] <- [D] - /// A | - /// | | - /// [C]<-----------+ - /// - artboard.addDependency(nodeA, artboard.root); - artboard.addDependency(nodeB, nodeA); - artboard.addDependency(nodeD, nodeB); - artboard.addDependency(nodeB, nodeD); - artboard.addDependency(nodeC, nodeA); - artboard.addDependency(nodeD, nodeC); - }); + group("Complex Cycle A:", () { + final actor = DummyActor(); + final artboard = ActorArtboard(actor); + final nodeA = ActorNode()..name = 'A'; + final nodeB = ActorNode()..name = 'B'; + final nodeC = ActorNode()..name = 'C'; + final nodeD = ActorNode()..name = 'D'; - test("DependencySorter is ok", () { + /// + /// +------+ + /// | v + /// [root] <- [A] <- [B] <- [D] + /// A | + /// | | + /// [C]<-----------+ + /// + artboard.addDependency(nodeA, artboard.root); + artboard.addDependency(nodeB, nodeA); + artboard.addDependency(nodeD, nodeB); + artboard.addDependency(nodeB, nodeD); + artboard.addDependency(nodeC, nodeA); + artboard.addDependency(nodeD, nodeC); + + test("DependencySorter cannot order", () { expect(DependencySorter().sort(artboard.root), equals(null)); }); - test("TarjansDependencySorter stops on cycle", () { + test("TarjansDependencySorter orders", () { var order = TarjansDependencySorter().sort(artboard.root); expect(order, isNotNull); expect(orderString(order), equals('Unnamed A C')); }); }); - group("Bad Cycle", () { + group("Complex Cycle B, F is isolated:", () { + ActorArtboard artboard; + + final actor = DummyActor(); + artboard = ActorArtboard(actor); + final nodeA = ActorNode()..name = 'A'; + final nodeB = ActorNode()..name = 'B'; + final nodeC = ActorNode()..name = 'C'; + final nodeD = ActorNode()..name = 'D'; + final nodeE = ActorNode()..name = 'E'; + final nodeF = ActorNode()..name = 'F'; + /// /// +-------------+ /// | | @@ -196,56 +196,63 @@ void main() { /// | | /// [E]<-----------+ /// - - test("CycleDependencySorter expected [root A E]", () { - final actor = DummyActor(); - final artboard = ActorArtboard(actor); - final nodeA = ActorNode()..name = 'A'; - final nodeB = ActorNode()..name = 'B'; - final nodeC = ActorNode()..name = 'C'; - final nodeD = ActorNode()..name = 'D'; - final nodeE = ActorNode()..name = 'E'; - final nodeF = ActorNode()..name = 'F'; - - artboard.addDependency(nodeA, artboard.root); - artboard.addDependency(nodeB, nodeA); - artboard.addDependency(nodeC, nodeB); - artboard.addDependency(nodeD, nodeC); - artboard.addDependency(nodeB, nodeD); - artboard.addDependency(nodeE, nodeA); - artboard.addDependency(nodeC, nodeE); - artboard.addDependency(nodeF, nodeC); + artboard.addDependency(nodeA, artboard.root); + artboard.addDependency(nodeB, nodeA); + artboard.addDependency(nodeC, nodeB); + artboard.addDependency(nodeD, nodeC); + artboard.addDependency(nodeB, nodeD); + artboard.addDependency(nodeE, nodeA); + artboard.addDependency(nodeC, nodeE); + artboard.addDependency(nodeF, nodeC); + + test("TarjansDependencySorter orders", () { var dependencySorter = TarjansDependencySorter(); var order = dependencySorter.sort(artboard.root); - expect(order, isNotNull); - expect(order.length, equals(3)); expect(orderString(order), equals('Unnamed A E')); expect(dependencySorter.cycleNodes, containsAll([nodeB, nodeC, nodeD])); + expect(dependencySorter.cycleNodes, contains(nodeF), + skip: "Node F is isolated by a cycle, and does not " + " exist in 'order' or in 'cycleNodes'"); }); + }); - test("CycleDependencySorter reorderd expected [root A E]", () { - final actor = DummyActor(); - final artboard = ActorArtboard(actor); - final nodeA = ActorNode()..name = 'A'; - final nodeB = ActorNode()..name = 'B'; - final nodeC = ActorNode()..name = 'C'; - final nodeD = ActorNode()..name = 'D'; - final nodeE = ActorNode()..name = 'E'; - final nodeF = ActorNode()..name = 'F'; - - artboard.addDependency(nodeA, artboard.root); - artboard.addDependency(nodeE, nodeA); - artboard.addDependency(nodeC, nodeE); - artboard.addDependency(nodeF, nodeC); - artboard.addDependency(nodeB, nodeA); - artboard.addDependency(nodeC, nodeB); - artboard.addDependency(nodeD, nodeC); - artboard.addDependency(nodeB, nodeD); + group("Complex Cycle C, F is not isolated:", () { + ActorArtboard artboard; + + final actor = DummyActor(); + artboard = ActorArtboard(actor); + final nodeA = ActorNode()..name = 'A'; + final nodeB = ActorNode()..name = 'B'; + final nodeC = ActorNode()..name = 'C'; + final nodeD = ActorNode()..name = 'D'; + final nodeE = ActorNode()..name = 'E'; + final nodeF = ActorNode()..name = 'F'; + + /// + /// +---------------+ + /// | | + /// | [F]---+ | + /// | v | v + /// [root] <- [A] <- [B] <- [C] <-+- [D] + /// A | | + /// | | | + /// [E]<-----------+ | + /// A | + /// +------------------+ + artboard.addDependency(nodeA, artboard.root); + artboard.addDependency(nodeB, nodeA); + artboard.addDependency(nodeC, nodeB); + artboard.addDependency(nodeD, nodeC); + artboard.addDependency(nodeB, nodeD); + artboard.addDependency(nodeE, nodeA); + artboard.addDependency(nodeC, nodeE); + artboard.addDependency(nodeF, nodeC); + artboard.addDependency(nodeF, nodeE); + + test("TarjansDependencySorter orders", () { var dependencySorter = TarjansDependencySorter(); var order = dependencySorter.sort(artboard.root); - expect(order, isNotNull); - expect(order.length, equals(3)); - expect(orderString(order), equals('Unnamed A E')); + expect(orderString(order), equals('Unnamed A E F')); expect(dependencySorter.cycleNodes, containsAll([nodeB, nodeC, nodeD])); }); }); From 5451394f5a16247c4a4414b4223085196e8afe72 Mon Sep 17 00:00:00 2001 From: Luigi Rosso Date: Fri, 10 Jan 2020 14:35:09 -0800 Subject: [PATCH 4/4] Minor tweak to keep the default sorter simpler and renamed the test file so that vscode finds it (wants the file to end in _test.dart). --- flare_dart/lib/dependency_sorter.dart | 28 +++++++++++-------- ...orter.dart => dependency_sorter_test.dart} | 0 2 files changed, 17 insertions(+), 11 deletions(-) rename flare_dart/test/{test_dependency_sorter.dart => dependency_sorter_test.dart} (100%) diff --git a/flare_dart/lib/dependency_sorter.dart b/flare_dart/lib/dependency_sorter.dart index 76f6ce7..d05350e 100644 --- a/flare_dart/lib/dependency_sorter.dart +++ b/flare_dart/lib/dependency_sorter.dart @@ -22,12 +22,7 @@ class DependencySorter { return _order; } - bool visit(ActorComponent n, {HashSet cycleNodes}) { - cycleNodes ??= HashSet(); - if (cycleNodes.contains(n)) { - // skip any nodes on a known cycle. - return true; - } + bool visit(ActorComponent n) { if (_perm.contains(n)) { return true; } @@ -41,7 +36,7 @@ class DependencySorter { List dependents = n.dependents; if (dependents != null) { for (final ActorComponent d in dependents) { - if (!visit(d, cycleNodes: cycleNodes)) { + if (!visit(d)) { return false; } } @@ -55,9 +50,10 @@ class DependencySorter { /// Sorts dependencies for Actors even when cycles are present /// -/// Any nodes that form part of a cycle can be found in `cycleNodes` after `sort`. -/// NOTE: Nodes isolated by cycles will not be found in `_order` or `cycleNodes` -/// e.g. `A -> B <-> C -> D` isolates D when running a sort based on A +/// Any nodes that form part of a cycle can be found in `cycleNodes` after +/// `sort`. NOTE: Nodes isolated by cycles will not be found in `_order` or +/// `cycleNodes` e.g. `A -> B <-> C -> D` isolates D when running a sort based +/// on A class TarjansDependencySorter extends DependencySorter { HashSet _cycleNodes; HashSet get cycleNodes => _cycleNodes; @@ -68,6 +64,16 @@ class TarjansDependencySorter extends DependencySorter { _cycleNodes = HashSet(); } + @override + bool visit(ActorComponent n) { + if (cycleNodes.contains(n)) { + // skip any nodes on a known cycle. + return true; + } + + return super.visit(n); + } + @override List sort(ActorComponent root) { _order = []; @@ -92,7 +98,7 @@ class TarjansDependencySorter extends DependencySorter { }); // revisit the tree, skipping nodes on any cycle. - visit(root, cycleNodes: _cycleNodes); + visit(root); } return _order; diff --git a/flare_dart/test/test_dependency_sorter.dart b/flare_dart/test/dependency_sorter_test.dart similarity index 100% rename from flare_dart/test/test_dependency_sorter.dart rename to flare_dart/test/dependency_sorter_test.dart