Rework everything again

lib/mod.dart

@@ -31,9 +31,6 @@ class Mod {
   final bool isBaseGame; // Is this the base RimWorld game
   final bool isExpansion; // Is this a RimWorld expansion
 
-  int loadBeforeNotPlaced = 0;
-  int loadAfterPlaced = 0;
-
   Mod({
     required this.name,
     required this.id,

lib/mod_list.dart

@@ -6,9 +6,12 @@ import 'package:rimworld_modman/mod.dart';
 import 'package:xml/xml.dart';
 
 class LoadOrder {
-  final List<Mod> order = [];
+  List<Mod> order = [];
   final List<String> errors = [];
-  List<String> get loadOrder => order.map((e) => e.id).toList();
+
+  List<String> get loadOrder {
+    return order.map((mod) => mod.id).toList();
+  }
 
   LoadOrder();
 
@@ -251,150 +254,119 @@ class ModList {
 
   LoadOrder generateLoadOrder([LoadOrder? loadOrder]) {
     loadOrder ??= LoadOrder();
-    final modMap = {for (final m in activeMods.values) m.id: m};
+    final logger = Logger.instance;
-    final errors = _validateIncompatibilities(modMap);
-    if (errors.isNotEmpty) {
-      loadOrder.errors.addAll(errors);
-    }
-
-    // Hard dependency graph
-    final inDegree = <String, int>{};
-    final adjacency = <String, List<String>>{};
-
-    // Soft constraint reverse mappings
-    final reverseLoadBefore = <String, List<Mod>>{};
-    final reverseLoadAfter = <String, List<Mod>>{};
-
-    // Initialize data structures
     for (final mod in activeMods.values) {
-      mod.loadBeforeNotPlaced = mod.loadBefore.length;
+      for (final incomp in mod.incompatibilities) {
-      mod.loadAfterPlaced = 0;
+        if (activeMods.containsKey(incomp)) {
-
+          loadOrder.errors.add(
-      reverseLoadBefore[mod.id] = [];
+            'Incompatibility detected: ${mod.id} is incompatible with $incomp',
-      reverseLoadAfter[mod.id] = [];
+          );
-      inDegree[mod.id] = 0;
-      adjacency[mod.id] = [];
-    }
-
-    // Build dependency graph and reverse soft constraints
-    for (final mod in activeMods.values) {
-      for (final depId in mod.dependencies) {
-        adjacency[depId]!.add(mod.id);
-        inDegree[mod.id] = (inDegree[mod.id] ?? 0) + 1;
-      }
-
-      for (final targetId in mod.loadBefore) {
-        final target = modMap[targetId];
-        if (target != null) {
-          reverseLoadBefore[targetId]!.add(mod);
-        }
-      }
-
-      for (final targetId in mod.loadAfter) {
-        final target = modMap[targetId];
-        if (target != null) {
-          reverseLoadAfter[targetId]!.add(mod);
         }
       }
     }
 
-    final heap = PriorityQueue<Mod>((a, b) {
+    loadOrder.order.addAll(activeMods.values.toList());
-      // 1. Base game first
+
-      if (a.isBaseGame != b.isBaseGame) {
+    loadOrder.order.sort((a, b) {
-        return a.isBaseGame ? -1 : 1;
+      if (a.isBaseGame && !b.isBaseGame) return -1;
-      }
+      if (!a.isBaseGame && b.isBaseGame) return 1;
-      // 2. Expansions next
+      if (a.isExpansion && !b.isExpansion) return -1;
-      if (a.isExpansion != b.isExpansion) {
+      if (!a.isExpansion && b.isExpansion) return 1;
-        return a.isExpansion ? -1 : 1;
-      }
-      // 3. Soft constraints: Prioritize mods that need to be placed earlier
-      final aUnmetBefore = a.loadBeforeNotPlaced;
-      final bUnmetBefore = b.loadBeforeNotPlaced;
-      if (aUnmetBefore != bUnmetBefore) {
-        return bUnmetBefore.compareTo(aUnmetBefore); // Higher unmetBefore first
-      }
-      // If tied, deprioritize mods with more unmet `loadAfter`
-      final aUnmetAfter = a.loadAfter.length - a.loadAfterPlaced;
-      final bUnmetAfter = b.loadAfter.length - b.loadAfterPlaced;
-      if (aUnmetAfter != bUnmetAfter) {
-        return aUnmetAfter.compareTo(bUnmetAfter); // Lower unmetAfter first
-      }
-      // 4. Smaller size last
       return b.size.compareTo(a.size);
     });
 
-    // Initialize heap with available mods
+    Map<String, List<Mod>> relations = {};
-    for (final mod in activeMods.values) {
+    for (int i = loadOrder.order.length - 1; i >= 0; i--) {
-      if (inDegree[mod.id] == 0) {
+      final mod = loadOrder!.order[i];
-        heap.add(mod);
+      logger.info('Processing mod: ${mod.id}');
-      }
+      loadOrder = shuffleMod(mod, loadOrder, relations);
     }
 
-    while (heap.isNotEmpty) {
+    return loadOrder!;
-      final current = heap.removeFirst();
-      loadOrder.order.add(current);
-
-      // Update dependents' in-degree
-      for (final neighborId in adjacency[current.id]!) {
-        inDegree[neighborId] = inDegree[neighborId]! - 1;
-        if (inDegree[neighborId] == 0) {
-          heap.add(modMap[neighborId]!);
-        }
   }
 
-      // Update soft constraints
+  // The point of relations and the recursive call is to handle the case where
-      _updateReverseConstraints(
+  // A mod depends on a mod that depends on another mod
-        current,
+  // So we move our first mod A to after B
-        reverseLoadBefore,
+  // But then we move B after C and A is no longer guranteed to be after B
-        loadOrder,
+  // So we update it too just in case
-        heap,
+  // To make sure we have A B C
-        (mod) => mod.loadBeforeNotPlaced--,
+  // Now it opens us to a stack overflow...
-      );
+  LoadOrder shuffleMod(
-      _updateReverseConstraints(
+    Mod mod,
-        current,
+    LoadOrder loadOrder,
-        reverseLoadAfter,
+    Map<String, List<Mod>> relations, [
-        loadOrder,
+    Map<String, bool>? seen,
-        heap,
+  ]) {
-        (mod) => mod.loadAfterPlaced++,
+    final logger = Logger.instance;
-      );
+    logger.info('Starting shuffleMod for mod: ${mod.id}');
-    }
-
-    if (loadOrder.order.length != activeMods.length) {
-      loadOrder.errors.add("Cyclic dependencies detected");
-    }
 
+    // Prevent infinite loops
+    seen ??= <String, bool>{};
+    if (seen[mod.id] == true) {
+      logger.info('Mod ${mod.id} has already been seen, skipping.');
       return loadOrder;
     }
+    seen[mod.id] = true;
+    logger.info('Marking mod ${mod.id} as seen.');
 
-  List<String> _validateIncompatibilities(Map<String, Mod> modMap) {
+    for (final dependency in mod.dependencies) {
-    final errors = <String>[];
+      logger.info('Checking dependency: $dependency for mod ${mod.id}');
-    for (final mod in modMap.values) {
+      final depMod = mods[dependency];
-      for (final incompatibleId in mod.incompatibilities) {
+      if (depMod == null) {
-        if (modMap.values.any((m) => m.id == incompatibleId)) {
+        loadOrder.errors.add(
-          errors.add("Incompatible mods: ${mod.id} and $incompatibleId");
+          'Missing dependency: ${mod.id} requires mod with ID $dependency',
-        }
+        );
-      }
+        logger.warning(
-    }
+          'Missing dependency: ${mod.id} requires mod with ID $dependency',
-    return errors;
+        );
+        continue;
       }
 
-  void _updateReverseConstraints(
+      if (loadOrder.order.indexOf(mod) < loadOrder.order.indexOf(depMod)) {
-    Mod current,
+        logger.info('Reordering: ${mod.id} should come after ${depMod.id}');
-    Map<String, List<Mod>> reverseMap,
+        loadOrder.order.removeAt(loadOrder.order.indexOf(mod));
-    LoadOrder loadOrder,
+        loadOrder.order.insert(loadOrder.order.indexOf(depMod) + 1, mod);
-    PriorityQueue<Mod> heap,
+        relations[mod.id] = [...relations[mod.id] ?? [], depMod];
-    void Function(Mod) update,
-  ) {
-    reverseMap[current.id]?.forEach((affectedMod) {
-      if (!loadOrder.order.contains(affectedMod)) {
-        update(affectedMod);
-        // If mod is already in heap, re-add to update position
-        if (heap.contains(affectedMod)) {
-          heap.remove(affectedMod);
-          heap.add(affectedMod);
       }
     }
-    });
+
+    for (final loadAfter in mod.loadAfter) {
+      logger.info('Checking loadAfter: $loadAfter for mod ${mod.id}');
+      final loadAfterMod = mods[loadAfter];
+      if (loadAfterMod != null &&
+          loadOrder.order.indexOf(mod) <
+              loadOrder.order.indexOf(loadAfterMod)) {
+        logger.info(
+          'Reordering: ${mod.id} should come after ${loadAfterMod.id}',
+        );
+        loadOrder.order.removeAt(loadOrder.order.indexOf(mod));
+        loadOrder.order.insert(loadOrder.order.indexOf(loadAfterMod) + 1, mod);
+        relations[mod.id] = [...relations[mod.id] ?? [], loadAfterMod];
+      }
+    }
+
+    for (final loadBefore in mod.loadBefore) {
+      logger.info('Checking loadBefore: $loadBefore for mod ${mod.id}');
+      final loadBeforeMod = mods[loadBefore];
+      if (loadBeforeMod != null &&
+          loadOrder.order.indexOf(mod) >
+              loadOrder.order.indexOf(loadBeforeMod)) {
+        logger.info(
+          'Reordering: ${mod.id} should come before ${loadBeforeMod.id}',
+        );
+        loadOrder.order.removeAt(loadOrder.order.indexOf(mod));
+        loadOrder.order.insert(loadOrder.order.indexOf(loadBeforeMod), mod);
+        relations[mod.id] = [...relations[mod.id] ?? [], loadBeforeMod];
+      }
+    }
+
+    for (final relatedMod in relations[mod.id] ?? []) {
+      logger.info('Recursively shuffling related mod: ${relatedMod.id}');
+      loadOrder = shuffleMod(relatedMod, loadOrder, relations, seen);
+    }
+    logger.info('Completed shuffleMod for mod: ${mod.id}');
+    return loadOrder;
   }
 
   LoadOrder loadDependencies(

test/mod_list_regressive_test.dart

@@ -200,10 +200,10 @@ void main() {
     final expected = [
       'brrainz.harmony',
       'ludeon.rimworld',
-      'ludeon.rimworld.anomaly',
-      'ludeon.rimworld.biotech',
-      'ludeon.rimworld.ideology',
       'ludeon.rimworld.royalty',
+      'ludeon.rimworld.ideology',
+      'ludeon.rimworld.biotech',
+      'ludeon.rimworld.anomaly',
       'dubwise.rimatomics',
       'jecrell.doorsexpanded',
       'dubwise.rimefeller',

test/mod_list_test.dart

@@ -1,4 +1,3 @@
-import 'package:rimworld_modman/logger.dart';
 import 'package:rimworld_modman/mod.dart';
 import 'package:rimworld_modman/mod_list.dart';
 import 'package:test/test.dart';
@@ -246,7 +245,7 @@ void main() {
       final result = list.loadRequired();
 
       // We say the mods are incompatible but load them anyway, who are we to decide what isn't loaded?
-      final expected = ['harmony', 'prepatcher', 'incompatible'];
+      final expected = ['incompatible', 'harmony', 'prepatcher'];
       expect(result.errors, isNotEmpty);
       expect(result.errors.any((e) => e.contains('incompatible')), isTrue);
       expect(result.errors.any((e) => e.contains('harmony')), isTrue);
@@ -303,7 +302,7 @@ void main() {
 
       // We try to not disable mods...... But cyclic dependencies are just hell
       // Can not handle it
-      final expected = [];
+      final expected = ['modB', 'modA', 'modC'];
       expect(result.errors, isNotEmpty);
       expect(result.errors.any((e) => e.contains('modA')), isTrue);
       expect(result.errors.any((e) => e.contains('modB')), isTrue);
@@ -446,26 +445,9 @@ void main() {
 
       final result = list.loadRequired();
 
+      final expected = ['modD', 'modC', 'modB', 'modA'];
       expect(result.errors, isEmpty);
-      // All mods in the chain should be enabled
+      expect(result.loadOrder, equals(expected));
-      expect(result.loadOrder.contains('modA'), isTrue);
-      expect(result.loadOrder.contains('modB'), isTrue);
-      expect(result.loadOrder.contains('modC'), isTrue);
-      expect(result.loadOrder.contains('modD'), isTrue);
-
-      // The order should be D -> C -> B -> A
-      expect(
-        result.loadOrder.indexOf('modD'),
-        lessThan(result.loadOrder.indexOf('modC')),
-      );
-      expect(
-        result.loadOrder.indexOf('modC'),
-        lessThan(result.loadOrder.indexOf('modB')),
-      );
-      expect(
-        result.loadOrder.indexOf('modB'),
-        lessThan(result.loadOrder.indexOf('modA')),
-      );
     });
   });
 
@@ -504,11 +486,12 @@ void main() {
       };
       list.enableAll();
 
-      // Should return errors with both missing dependencies mentioned
       final result = list.generateLoadOrder();
+      final expected = ['modA', 'modB'];
       expect(result.errors, isNotEmpty);
       expect(result.errors.any((e) => e.contains('missing1')), isTrue);
       expect(result.errors.any((e) => e.contains('missing2')), isTrue);
+      expect(result.loadOrder, equals(expected));
     });
   });
 
@@ -527,8 +510,9 @@ void main() {
       // Should not throw exception for soft constraints
       // But might generate a warning that we could check for
       final order = list.generateLoadOrder();
+      final expected = ['modA'];
       expect(order.errors, isEmpty);
-      expect(order.loadOrder.contains('modA'), isTrue);
+      expect(order.loadOrder, equals(expected));
     });
 
     test('Should handle missing loadAfter relationships', () {
@@ -544,8 +528,9 @@ void main() {
 
       // Should not throw exception for soft constraints
       final order = list.generateLoadOrder();
+      final expected = ['modA'];
       expect(order.errors, isEmpty);
-      expect(order.loadOrder.contains('modA'), isTrue);
+      expect(order.loadOrder, equals(expected));
     });
   });
 
@@ -563,10 +548,12 @@ void main() {
       list.enableAll();
 
       final result = list.generateLoadOrder();
+      final expected = ['modA', 'modB'];
       expect(result.errors, isNotEmpty);
-      expect(result.errors.any((e) => e.contains('Incompatible')), isTrue);
+      expect(result.errors.any((e) => e.contains('incompatible')), isTrue);
       expect(result.errors.any((e) => e.contains('modA')), isTrue);
       expect(result.errors.any((e) => e.contains('modB')), isTrue);
+      expect(result.loadOrder, equals(expected));
     });
 
     test('Should handle a combination of errors simultaneously', () {
@@ -583,9 +570,11 @@ void main() {
       list.enableAll();
 
       final result = list.generateLoadOrder();
+      final expected = ['modA', 'modB'];
       expect(result.errors, isNotEmpty);
       expect(result.errors.any((e) => e.contains('missingDep')), isTrue);
-      expect(result.errors.any((e) => e.contains('Incompatible')), isTrue);
+      expect(result.errors.any((e) => e.contains('incompatible')), isTrue);
+      expect(result.loadOrder, equals(expected));
     });
   });
 
@@ -664,11 +653,10 @@ void main() {
       list.enableAll();
 
       final result = list.generateLoadOrder();
+      final expected = ['modA', 'modB'];
 
       expect(result.errors, isEmpty);
-      expect(result.loadOrder.length, equals(2));
+      expect(result.loadOrder, equals(expected));
-      expect(result.loadOrder.contains('modA'), isTrue);
-      expect(result.loadOrder.contains('modB'), isTrue);
     });
 
     test('Should return errors for missing dependencies', () {
@@ -683,10 +671,11 @@ void main() {
       list.enableAll();
 
       final result = list.generateLoadOrder();
+      final expected = ['modA'];
 
       expect(result.errors, isNotEmpty);
       expect(result.errors.any((e) => e.contains('nonExistentMod')), isTrue);
-      // The load order might be empty or contain valid mods only
+      expect(result.loadOrder, equals(expected));
     });
 
     test('Should return both valid load order and errors', () {
@@ -702,12 +691,11 @@ void main() {
       list.enableAll();
 
       final result = list.generateLoadOrder();
+      final expected = ['modA', 'modB'];
 
       expect(result.errors, isNotEmpty);
       expect(result.errors.any((e) => e.contains('nonExistentMod')), isTrue);
-      // modA should still be in the load order even though modB has errors
+      expect(result.loadOrder, equals(expected));
-      expect(result.loadOrder.contains('modA'), isTrue);
-      // modB might be excluded from load order due to its error
     });
   });
 
@@ -731,7 +719,7 @@ void main() {
         list.enableAll();
 
         final result = list.generateLoadOrder();
-
+        final expected = ['modA', 'modB'];
         // Verify all missing dependencies are reported
         expect(result.errors, isNotEmpty);
         expect(result.errors.any((e) => e.contains('missingDep1')), isTrue);
@@ -739,12 +727,11 @@ void main() {
         expect(result.errors.any((e) => e.contains('missingDep3')), isTrue);
 
         // Verify errors include the mod that requires the missing dependency
-        expect(result.errors.any((e) => e.contains('Mod A')), isTrue);
+        expect(result.errors.any((e) => e.contains('modA')), isTrue);
-        expect(result.errors.any((e) => e.contains('Mod B')), isTrue);
+        expect(result.errors.any((e) => e.contains('modB')), isTrue);
 
         // But mods should still be loaded anyway (the "It's fucked but anyway" philosophy)
-        expect(result.loadOrder.contains('modA'), isTrue);
+        expect(result.loadOrder, equals(expected));
-        expect(result.loadOrder.contains('modB'), isTrue);
       },
     );
   });
@@ -762,9 +749,10 @@ void main() {
       list.enableAll();
 
       final result = list.generateLoadOrder();
+      final expected = ['modA'];
 
       // Should still generate a valid load order despite missing soft constraints
-      expect(result.loadOrder.contains('modA'), isTrue);
+      expect(result.loadOrder, equals(expected));
 
       // System should track or report the missing loadBefore targets
       // This may be implementation-specific - modify if needed based on how your system handles this
@@ -788,9 +776,10 @@ void main() {
       list.enableAll();
 
       final result = list.generateLoadOrder();
+      final expected = ['existingMod', 'modA'];
 
-      // Should still generate a valid load order despite missing soft constraints
+
-      expect(result.loadOrder.contains('modA'), isTrue);
+      // Should still generatdeequals(mopected)
       expect(result.loadOrder.contains('existingMod'), isTrue);
 
       // The existing loadAfter relationship should be honored
@@ -820,7 +809,7 @@ void main() {
         };
         list.enableAll();
 
-        final result = list.generateLoadOrder();
+        final expected = ['modA'];        final result = list.generateLoadOrder();
 
         // Should report the missing dependency
         expect(result.errors, isNotEmpty);
@@ -833,7 +822,7 @@ void main() {
           isFalse,
         );
 
-        // Mod should still be loaded despite missing dependencies
+        // Mod should still be , equals(expected)pendencies
         expect(result.loadOrder.contains('modA'), isTrue);
       },
     );
@@ -863,6 +852,7 @@ void main() {
         list.enableAll();
 
         final result = list.generateLoadOrder();
+        final expected = ['modB', 'modC', 'modA'];
 
         // Should report all missing dependencies
         expect(result.errors, isNotEmpty);
@@ -870,24 +860,11 @@ void main() {
         expect(result.errors.any((e) => e.contains('missingDep2')), isTrue);
 
         // Should indicate which mods are affected by these missing dependencies
-        expect(result.errors.any((e) => e.contains('Mod B')), isTrue);
+        expect(result.errors.any((e) => e.contains('modB')), isTrue);
-        expect(result.errors.any((e) => e.contains('Mod C')), isTrue);
+        expect(result.errors.any((e) => e.contains('modC')), isTrue);
 
         // But all mods should still be loaded in the best possible order
-        expect(result.loadOrder.contains('modA'), isTrue);
+        expect(result.loadOrder, equals(expected));
-        expect(result.loadOrder.contains('modB'), isTrue);
-        expect(result.loadOrder.contains('modC'), isTrue);
-
-        // And existing constraints should be respected
-        // modA depends on modB and modC, so it should load after them
-        expect(
-          result.loadOrder.indexOf('modB'),
-          lessThan(result.loadOrder.indexOf('modA')),
-        );
-        expect(
-          result.loadOrder.indexOf('modC'),
-          lessThan(result.loadOrder.indexOf('modA')),
-        );
       },
     );