Completely scrapped the "recently modified spells" optimization, as it was needlessly complex, bugged, and was ultimately an over-optimization.

Delfofthebla · Delfofthebla · commit fc810b207811 · 2025-11-29T17:16:00.000-07:00
Added new test to confirm that things are properly re-evaluated when spells fall off.
Minor code refactor/cleanup.
diff --git a/EQTool/Services/MarkupExtensions/EnumBindingSourceExtension.cs b/EQTool/Services/MarkupExtensions/EnumBindingSourceExtension.cs
@@ -4,6 +4,7 @@
 
 namespace EQTool.Services.MarkupExtensions
 {
+    // This extension will allow you to take an enum Type and then create a bindable list of enum values for a control.
     public class EnumBindingSourceExtension : MarkupExtension
     {
         public EnumBindingSourceExtension() { }
@@ -13,6 +14,12 @@ public EnumBindingSourceExtension(Type enumType)
             EnumType = enumType;
         }
 
+        /// <summary>
+        /// Comma-separated list of enum names to exclude
+        /// e.g. "BySpellExceptYou,SomethingElse"
+        /// </summary>
+        public string Exclude { get; set; }
+        
         private Type _EnumType;
         public Type EnumType
         {
@@ -35,12 +42,6 @@ public Type EnumType
             }
         }
 
-        /// <summary>
-        /// Comma-separated list of enum names to exclude
-        /// e.g. "BySpellExceptYou,SomethingElse"
-        /// </summary>
-        public string Exclude { get; set; }
-
         public override object ProvideValue(IServiceProvider serviceProvider)
         {
             if (_EnumType == null)
diff --git a/EQTool/Services/SpellGroupingEngine.cs b/EQTool/Services/SpellGroupingEngine.cs
@@ -9,6 +9,8 @@ namespace EQTool.Services
 {
     public class SpellGroupingEngine
     {
+        private readonly object _lock = new object();
+        
         private readonly PlayerInfo activePlayer;
         private readonly EQToolSettings settings;
 
@@ -22,39 +24,35 @@ public SpellGroupingEngine(PlayerInfo activePlayer, EQToolSettings settings)
             this.settings = settings;
         }
         
-        public void Recategorize(IEnumerable<SpellViewModel> recentSpells = null)
+        public void Recategorize()
         {
-            if (recentSpells != null && recentSpells.Any())
-                recentSpells = GetConnectedSpells(recentSpells);
-            
-            var allRelatedSpells = recentSpells ?? allSpells;
-            if (!allRelatedSpells.Any())
+            if (!allSpells.Any())
                 return;
 
             var nonConciseGroupingSpells = new List<SpellViewModel>();
             var adaptiveGroupingSpells = new List<SpellViewModel>();
             if (settings.PlayerSpellGroupingType == SpellGroupingType.Adaptive && settings.NpcSpellGroupingType == SpellGroupingType.Adaptive)
             {
-                adaptiveGroupingSpells = allRelatedSpells.Where(s => s.ColumnVisibility == Visibility.Visible).ToList();
+                adaptiveGroupingSpells = allSpells.Where(s => s.ColumnVisibility == Visibility.Visible).ToList();
             }
             else if (settings.PlayerSpellGroupingType == SpellGroupingType.Adaptive)
             {
-                nonConciseGroupingSpells.AddRange(allRelatedSpells.Where(s => !s.IsPlayerTarget));  // Handle em all, even the hidden ones
-                adaptiveGroupingSpells = allRelatedSpells.Where(s => s.ColumnVisibility == Visibility.Visible && s.IsPlayerTarget).ToList();
+                nonConciseGroupingSpells.AddRange(allSpells.Where(s => !s.IsPlayerTarget)); // Handle em all, even the hidden ones
+                adaptiveGroupingSpells = allSpells.Where(s => s.ColumnVisibility == Visibility.Visible && s.IsPlayerTarget).ToList();
             }
             else if (settings.NpcSpellGroupingType == SpellGroupingType.Adaptive)
             {
-                nonConciseGroupingSpells.AddRange(allRelatedSpells.Where(s => s.IsPlayerTarget));  // Handle em all, even the hidden ones
-                adaptiveGroupingSpells = allRelatedSpells.Where(s => s.ColumnVisibility == Visibility.Visible && !s.IsPlayerTarget).ToList();
+                nonConciseGroupingSpells.AddRange(allSpells.Where(s => s.IsPlayerTarget)); // Handle em all, even the hidden ones
+                adaptiveGroupingSpells = allSpells.Where(s => s.ColumnVisibility == Visibility.Visible && !s.IsPlayerTarget).ToList();
             }
             else
             {
-                nonConciseGroupingSpells.AddRange(allRelatedSpells);
+                nonConciseGroupingSpells.AddRange(allSpells);
             }
             
             foreach (var spell in nonConciseGroupingSpells)
                 ApplyNonAdaptiveGroupingRules(spell);
-            
+
             if (adaptiveGroupingSpells.Any())
                 PerformAdaptiveGrouping(adaptiveGroupingSpells);
         }
@@ -161,7 +159,9 @@ private SpellGroupingType GetConfiguredGroupingMode(SpellViewModel spell)
                 : settings.NpcSpellGroupingType;
 
         // -----------------------
-        // Branch-and-bound algorithm. Determine what should be grouped by target and what should be grouped by Id.
+        // Branch-and-bound algorithm.
+        // Determines what should be grouped by target, and what should be grouped by Id.
+        // It looks a little "voodoo"-ey but it's a decently optimized algo for accomplishing what we want.
         // -----------------------
         private void PerformAdaptiveGrouping(IEnumerable<SpellViewModel> visibleSpells)
         {
diff --git a/EQTool/UI/SpellWindow.xaml b/EQTool/UI/SpellWindow.xaml
@@ -220,7 +220,7 @@
                 <Grid Grid.Row="1" VerticalAlignment="Bottom">
                     <StackPanel Orientation="Horizontal"  HorizontalAlignment="Center">
                         <Button Content="R" Padding="0" FontSize="10" Width="14" Height="14" Visibility="{Binding RaidModeToggleButtonVisibility}" ToolTip="{Binding RaidModeButtonToolTip}"
-                                Opacity="{Binding DataContext.ItemOpacity, RelativeSource={RelativeSource AncestorType=local:BaseSaveStateWindow}}" Click="RaidModleToggle" />
+                                Opacity="{Binding DataContext.ItemOpacity, RelativeSource={RelativeSource AncestorType=local:BaseSaveStateWindow}}" Click="RaidModeToggle" />
                         <Button Content="O" Margin="5, 0, 0, 0" Padding="0" FontSize="10" Width="14" Height="14" Visibility="{Binding GenericButtonVisibility}" ToolTip="Clear all spells that were not cast on you"
                                 Opacity="{Binding DataContext.ItemOpacity, RelativeSource={RelativeSource AncestorType=local:BaseSaveStateWindow}}" Click="ClearSpellsNotCastOnYou" />
                         <Button Content="Y" Margin="5, 0, 0, 0" Padding="0" FontSize="10" Width="14" Height="14" Visibility="{Binding GenericButtonVisibility}" ToolTip="Clear all spells that were cast by others"
diff --git a/EQTool/UI/SpellWindow.xaml.cs b/EQTool/UI/SpellWindow.xaml.cs
@@ -18,11 +18,9 @@ public partial class SpellWindow : BaseSaveStateWindow
         private readonly SlainHandler slainHandler;
 
         public SpellWindow(
-            TimersService timersService,
             EQToolSettings settings,
             SpellWindowViewModel spellWindowViewModel,
             SettingsWindowViewModel settingsWindowViewModel,
-            LogEvents logEvents,
             EQToolSettingsLoad toolSettingsLoad,
             ActivePlayer activePlayer,
             IAppDispatcher appDispatcher,
@@ -39,7 +37,7 @@ public SpellWindow(
 
         private void RemoveSingleItem(object sender, RoutedEventArgs e)
         {
-            var vm = (sender as Button).DataContext;
+            var vm = (sender as Button)?.DataContext;
             appDispatcher.DispatchUI(() =>
             {
                 _ = spellWindowViewModel.SpellList.Remove(vm as PersistentViewModel);
@@ -48,7 +46,7 @@ private void RemoveSingleItem(object sender, RoutedEventArgs e)
 
         private void RemoveFromSpells(object sender, RoutedEventArgs e)
         {
-            var group = ((sender as Button).DataContext as dynamic)?.Name as string;
+            var group = ((sender as Button)?.DataContext as dynamic)?.Name as string;
             appDispatcher.DispatchUI(() =>
             {
                 var items = spellWindowViewModel.SpellList.Where(a => a.DisplayGroup == group).ToList();
@@ -69,7 +67,7 @@ private void ClearSpellsCastByOthers(object sender, RoutedEventArgs e)
             spellWindowViewModel.ClearSpellsCastByOthers();
         }
 
-        private void RaidModleToggle(object sender, RoutedEventArgs e)
+        private void RaidModeToggle(object sender, RoutedEventArgs e)
         {
             settingsWindowViewModel.RaidModeDetection = !settingsWindowViewModel.RaidModeDetection;
         }
diff --git a/EQTool/ViewModels/SpellWindowViewModel.cs b/EQTool/ViewModels/SpellWindowViewModel.cs
@@ -8,6 +8,7 @@
 using System.Threading.Tasks;
 using System.Windows;
 using System.Windows.Data;
+using System.Windows.Documents;
 using System.Windows.Media;
 using EQTool.Models;
 using EQTool.Services;
@@ -32,7 +33,6 @@ public class SpellWindowViewModel : BaseWindowViewModel, INotifyPropertyChanged
         private readonly PigParseApi pigParseApi;
         private readonly BoatScheduleService boatScheduleService;
         private readonly PetViewModel playerPet;
-        private HashSet<SpellViewModel> recentlyImpactedSpells = new HashSet<SpellViewModel>();
         
         internal int groupRevaluationDebounceTime { get; set; } = 1000;  // Debounce delay for re-evaluating the ByTarget / BySpell Grouping of the list. It's internal so that tests can change it for speed.
 
@@ -79,7 +79,7 @@ private void CreateTriggerList()
             if (_SpellList == null)
             {
                 _SpellList = new ObservableCollection<PersistentViewModel>();
-                SpellList.CollectionChanged += SpellList_CollectionChanged;
+                _SpellList.CollectionChanged += SpellList_CollectionChanged;
                 var view = (ListCollectionView)CollectionViewSource.GetDefaultView(_SpellList);
                 view.GroupDescriptions.Add(new PropertyGroupDescription(nameof(TimerViewModel.DisplayGroup)));
                 view.LiveGroupingProperties.Add(nameof(TimerViewModel.DisplayGroup));
@@ -800,33 +800,13 @@ public void UpdateAPITimers()
         }
         
         private CancellationTokenSource timersModifiedDebounceTs;
-        private void QueueFullSpellListReevaluation() => QueueSpellGroupingReevaluation(groupRevaluationDebounceTime, useRecentlyModified: false);
-        private void QueueSpellGroupingReevaluation(int delay, bool useRecentlyModified = true)
+        private void QueueFullSpellListReevaluation() => QueueSpellGroupingReevaluation(groupRevaluationDebounceTime);
+        private void QueueSpellGroupingReevaluation(int delay)
         {
             // We need to queue the re-evaluation with a debounce cancellation because we don't want to be doing constant iteration over the whole list while it is actively being modified.
-            // There is also an issue when the user removes whole groups at once that can cause it to remove items the user did not mean to remove due to the grouping changing in the middle of that process.
-            // This debounce is necessary to ensure we do not waste unnecessary time or cause undesirable side effects when doing bulk operations or when several timers fall off at roughly the same time.
-            
-            var evaluationItemCount = recentlyImpactedSpells.Count;
-            appDispatcher.DebounceToUI(ref timersModifiedDebounceTs, delay, () =>
-            {
-                if (useRecentlyModified)
-                {
-                    var safeCloneOfRecentlyImpacted = recentlyImpactedSpells.ToList();  // Clone it since we're about to wipe it clean but want to use it.
-                    recentlyImpactedSpells.Clear();
-                    engine.Recategorize(safeCloneOfRecentlyImpacted);
-                }
-                else
-                {
-                    recentlyImpactedSpells.Clear();
-                    engine.Recategorize(recentSpells: null);
-                }
-            },
-            shouldCancel: () => useRecentlyModified && evaluationItemCount != recentlyImpactedSpells.Count);
+            appDispatcher.DebounceToUI(ref timersModifiedDebounceTs, delay, () => engine.Recategorize());
         }
         
-        private bool IsAdaptiveMode() => settings.PlayerSpellGroupingType == SpellGroupingType.Adaptive || settings.NpcSpellGroupingType == SpellGroupingType.Adaptive;
-        
         private void SpellList_CollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
         {
             var newSpells = (e.NewItems ?? Array.Empty<SpellViewModel>()).OfType<SpellViewModel>();
@@ -835,8 +815,7 @@ private void SpellList_CollectionChanged(object sender, NotifyCollectionChangedE
             engine.AddSpells(newSpells);
             engine.RemoveSpells(removedSpells);
 
-            recentlyImpactedSpells.UnionWith(newSpells.Concat(removedSpells));
-
+            var recentlyImpactedSpells = new List<SpellViewModel>(newSpells.Concat(removedSpells));
             if (recentlyImpactedSpells.Any())
                 QueueSpellGroupingReevaluation(groupRevaluationDebounceTime);
         }
diff --git a/EQtoolsTests/SpellAdaptiveGroupingTests.cs b/EQtoolsTests/SpellAdaptiveGroupingTests.cs

Original file line number	Diff line number	Diff line change
`@@ -4,6 +4,7 @@`
`4`	`4`
`5`	`5`	`namespace EQTool.Services.MarkupExtensions`
`6`	`6`	`{`
	`7`	`+ // This extension will allow you to take an enum Type and then create a bindable list of enum values for a control.`
`7`	`8`	`public class EnumBindingSourceExtension : MarkupExtension`
`8`	`9`	`{`
`9`	`10`	`public EnumBindingSourceExtension() { }`
`@@ -13,6 +14,12 @@ public EnumBindingSourceExtension(Type enumType)`
`13`	`14`	`EnumType = enumType;`
`14`	`15`	`}`
`15`	`16`
	`17`	`+ /// <summary>`
	`18`	`+ /// Comma-separated list of enum names to exclude`
	`19`	`+ /// e.g. "BySpellExceptYou,SomethingElse"`
	`20`	`+ /// </summary>`
	`21`	`+ public string Exclude { get; set; }`
	`22`	`+`
`16`	`23`	`private Type _EnumType;`
`17`	`24`	`public Type EnumType`
`18`	`25`	`{`
`@@ -35,12 +42,6 @@ public Type EnumType`
`35`	`42`	`}`
`36`	`43`	`}`
`37`	`44`
`38`		`- /// <summary>`
`39`		`- /// Comma-separated list of enum names to exclude`
`40`		`- /// e.g. "BySpellExceptYou,SomethingElse"`
`41`		`- /// </summary>`
`42`		`- public string Exclude { get; set; }`
`43`		`-`
`44`	`45`	`public override object ProvideValue(IServiceProvider serviceProvider)`
`45`	`46`	`{`
`46`	`47`	`if (_EnumType == null)`
Original file line number	Diff line number	Diff line change
`@@ -9,6 +9,8 @@ namespace EQTool.Services`
`9`	`9`	`{`
`10`	`10`	`public class SpellGroupingEngine`
`11`	`11`	`{`
	`12`	`+ private readonly object _lock = new object();`
	`13`	`+`
`12`	`14`	`private readonly PlayerInfo activePlayer;`
`13`	`15`	`private readonly EQToolSettings settings;`
`14`	`16`
`@@ -22,39 +24,35 @@ public SpellGroupingEngine(PlayerInfo activePlayer, EQToolSettings settings)`
`22`	`24`	`this.settings = settings;`
`23`	`25`	`}`
`24`	`26`
`25`		`- public void Recategorize(IEnumerable<SpellViewModel> recentSpells = null)`
	`27`	`+ public void Recategorize()`
`26`	`28`	`{`
`27`		`- if (recentSpells != null && recentSpells.Any())`
`28`		`- recentSpells = GetConnectedSpells(recentSpells);`
`29`		`-`
`30`		`- var allRelatedSpells = recentSpells ?? allSpells;`
`31`		`- if (!allRelatedSpells.Any())`
	`29`	`+ if (!allSpells.Any())`
`32`	`30`	`return;`
`33`	`31`
`34`	`32`	`var nonConciseGroupingSpells = new List<SpellViewModel>();`
`35`	`33`	`var adaptiveGroupingSpells = new List<SpellViewModel>();`
`36`	`34`	`if (settings.PlayerSpellGroupingType == SpellGroupingType.Adaptive && settings.NpcSpellGroupingType == SpellGroupingType.Adaptive)`
`37`	`35`	`{`
`38`		`- adaptiveGroupingSpells = allRelatedSpells.Where(s => s.ColumnVisibility == Visibility.Visible).ToList();`
	`36`	`+ adaptiveGroupingSpells = allSpells.Where(s => s.ColumnVisibility == Visibility.Visible).ToList();`
`39`	`37`	`}`
`40`	`38`	`else if (settings.PlayerSpellGroupingType == SpellGroupingType.Adaptive)`
`41`	`39`	`{`
`42`		`- nonConciseGroupingSpells.AddRange(allRelatedSpells.Where(s => !s.IsPlayerTarget)); // Handle em all, even the hidden ones`
`43`		`- adaptiveGroupingSpells = allRelatedSpells.Where(s => s.ColumnVisibility == Visibility.Visible && s.IsPlayerTarget).ToList();`
	`40`	`+ nonConciseGroupingSpells.AddRange(allSpells.Where(s => !s.IsPlayerTarget)); // Handle em all, even the hidden ones`
	`41`	`+ adaptiveGroupingSpells = allSpells.Where(s => s.ColumnVisibility == Visibility.Visible && s.IsPlayerTarget).ToList();`
`44`	`42`	`}`
`45`	`43`	`else if (settings.NpcSpellGroupingType == SpellGroupingType.Adaptive)`
`46`	`44`	`{`
`47`		`- nonConciseGroupingSpells.AddRange(allRelatedSpells.Where(s => s.IsPlayerTarget)); // Handle em all, even the hidden ones`
`48`		`- adaptiveGroupingSpells = allRelatedSpells.Where(s => s.ColumnVisibility == Visibility.Visible && !s.IsPlayerTarget).ToList();`
	`45`	`+ nonConciseGroupingSpells.AddRange(allSpells.Where(s => s.IsPlayerTarget)); // Handle em all, even the hidden ones`
	`46`	`+ adaptiveGroupingSpells = allSpells.Where(s => s.ColumnVisibility == Visibility.Visible && !s.IsPlayerTarget).ToList();`
`49`	`47`	`}`
`50`	`48`	`else`
`51`	`49`	`{`
`52`		`- nonConciseGroupingSpells.AddRange(allRelatedSpells);`
	`50`	`+ nonConciseGroupingSpells.AddRange(allSpells);`
`53`	`51`	`}`
`54`	`52`
`55`	`53`	`foreach (var spell in nonConciseGroupingSpells)`
`56`	`54`	`ApplyNonAdaptiveGroupingRules(spell);`
`57`		`-`
	`55`	`+`
`58`	`56`	`if (adaptiveGroupingSpells.Any())`
`59`	`57`	`PerformAdaptiveGrouping(adaptiveGroupingSpells);`
`60`	`58`	`}`
`@@ -161,7 +159,9 @@ private SpellGroupingType GetConfiguredGroupingMode(SpellViewModel spell)`
`161`	`159`	`: settings.NpcSpellGroupingType;`
`162`	`160`
`163`	`161`	`// -----------------------`
`164`		`- // Branch-and-bound algorithm. Determine what should be grouped by target and what should be grouped by Id.`
	`162`	`+ // Branch-and-bound algorithm.`
	`163`	`+ // Determines what should be grouped by target, and what should be grouped by Id.`
	`164`	`+ // It looks a little "voodoo"-ey but it's a decently optimized algo for accomplishing what we want.`
`165`	`165`	`// -----------------------`
`166`	`166`	`private void PerformAdaptiveGrouping(IEnumerable<SpellViewModel> visibleSpells)`
`167`	`167`	`{`
Original file line number	Diff line number	Diff line change
`@@ -18,11 +18,9 @@ public partial class SpellWindow : BaseSaveStateWindow`
`18`	`18`	`private readonly SlainHandler slainHandler;`
`19`	`19`
`20`	`20`	`public SpellWindow(`
`21`		`- TimersService timersService,`
`22`	`21`	`EQToolSettings settings,`
`23`	`22`	`SpellWindowViewModel spellWindowViewModel,`
`24`	`23`	`SettingsWindowViewModel settingsWindowViewModel,`
`25`		`- LogEvents logEvents,`
`26`	`24`	`EQToolSettingsLoad toolSettingsLoad,`
`27`	`25`	`ActivePlayer activePlayer,`
`28`	`26`	`IAppDispatcher appDispatcher,`
`@@ -39,7 +37,7 @@ public SpellWindow(`
`39`	`37`
`40`	`38`	`private void RemoveSingleItem(object sender, RoutedEventArgs e)`
`41`	`39`	`{`
`42`		`- var vm = (sender as Button).DataContext;`
	`40`	`+ var vm = (sender as Button)?.DataContext;`
`43`	`41`	`appDispatcher.DispatchUI(() =>`
`44`	`42`	`{`
`45`	`43`	`_ = spellWindowViewModel.SpellList.Remove(vm as PersistentViewModel);`
`@@ -48,7 +46,7 @@ private void RemoveSingleItem(object sender, RoutedEventArgs e)`
`48`	`46`
`49`	`47`	`private void RemoveFromSpells(object sender, RoutedEventArgs e)`
`50`	`48`	`{`
`51`		`- var group = ((sender as Button).DataContext as dynamic)?.Name as string;`
	`49`	`+ var group = ((sender as Button)?.DataContext as dynamic)?.Name as string;`
`52`	`50`	`appDispatcher.DispatchUI(() =>`
`53`	`51`	`{`
`54`	`52`	`var items = spellWindowViewModel.SpellList.Where(a => a.DisplayGroup == group).ToList();`
`@@ -69,7 +67,7 @@ private void ClearSpellsCastByOthers(object sender, RoutedEventArgs e)`
`69`	`67`	`spellWindowViewModel.ClearSpellsCastByOthers();`
`70`	`68`	`}`
`71`	`69`
`72`		`- private void RaidModleToggle(object sender, RoutedEventArgs e)`
	`70`	`+ private void RaidModeToggle(object sender, RoutedEventArgs e)`
`73`	`71`	`{`
`74`	`72`	`settingsWindowViewModel.RaidModeDetection = !settingsWindowViewModel.RaidModeDetection;`
`75`	`73`	`}`