fix: use stable, precalculated BFS for tree allocation paths

data/tree_graph_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"testing"
 
+	"github.com/MarvinJWendt/testza"
 	"github.com/Vilsol/go-pob-data/poe"
 	"github.com/Vilsol/go-pob/cache"
 
@@ -23,10 +24,83 @@ func TestLoadTreeGraph(t *testing.T) {
 	TreeVersions[TreeVersion3_18].getGraph()
 }
 
-func BenchmarkGraphSearch(b *testing.B) {
+func TestCalculateAllocationPaths(t *testing.T) {
+	// Starting from the witch spell damage root, path up through both
+	// the small int nodes and the cast speed small nodes, in both paths
+	// stopping just short of Arcanist's Dominion.
+	activeNonRootNodes := []int64{33296, 1957, 739, 18866, 37569, 36542, 4397}
+	activeRootNodes := []int64{57264}   // witch spell damage root
+	inactiveRootNodes := []int64{57226} // witch ES root
+
+	activeNodes := append(activeNonRootNodes, activeRootNodes...)
+	rootNodes := append(activeRootNodes, inactiveRootNodes...)
+
+	actual := TreeVersions[TreeVersion3_18].CalculateAllocationPaths(activeNodes, rootNodes)
+
+	for _, node := range activeNonRootNodes {
+		testza.AssertEqual(t, actual[node], int64(-1), "Active non-root nodes should be mapped to -1")
+	}
+	for _, node := range inactiveRootNodes {
+		testza.AssertEqual(t, actual[node], int64(-1), "Inactive root nodes should be mapped to -1")
+	}
+	for _, node := range activeRootNodes {
+		testza.AssertEqual(t, actual[node], int64(-1), "Active root nodes should be mapped to -1")
+	}
+
+	// Small str node below Enduring Bond wheel should point to the small int node to its right
+	testza.AssertEqual(t, actual[31875], int64(4397), "Neighbor of active node should point to the neighboring active")
+
+	// ES+mana node adjacent to the inactive ES+mana root should point to that root
+	testza.AssertEqual(t, actual[59650], int64(57226), "Neighbor of inactive root node should point to the neighboring root")
+
+	// Arcanist's Dominion is adjacent to both small int node 4397 and cast speed node 18866
+	testza.AssertEqual(t, actual[11420], int64(4397), "Neighbor of multiple active nodes should point to the one with the lowest node ID")
+
+	// 27929 Deep Wisdom is distance 4 to all of the following:
+	// - small int node 4397 (via 11420 Arcanist's Dominion, 60554 Minion Damage, 32024 Minion Life)
+	// - cast speed node 18866 (also via 11420 Arcanist's Dominion, 60554 Minion Damage, 32024 Minion Life)
+	// - inactive ES+mana root 57226 (via small int nodes 21678, 32210, 8948)
+	//
+	// Even though the smallest-node-ID next hop is small int node 8948, it should recognize that because
+	// the ES+mana root is inactive, it needs to be treated as though that path requires 1 additional distance
+	// than the path to the already-active frontier would.
+	testza.AssertEqual(t, actual[27929], int64(32024), "Node equidistant from active node and inactive root should point towards active node")
+
+	// Heart of the Warrior should connect to the small node above it
+	testza.AssertEqual(t, actual[61198], int64(20551), "Faraway node with uniquely best path should connect to that path")
+
+	// Bloodletting should connect to the lower-ID'd of the two small nodes it touches
+	testza.AssertEqual(t, actual[26294], int64(17833), "Faraway node with multiple shortest-paths should connect to the one with the lowest node ID")
+
+	// Bottom-right small jewel node should point to the corresponding jewel socket
+	testza.AssertEqual(t, actual[44470], int64(12161), "Position proxies should still appear in the graph")
+
+	// Mystic Talents can only be reached via anointment
+	_, ok := actual[62596]
+	testza.AssertFalse(t, ok, "Disconnected nodes should not be included in the output")
+
+	// Forbidden Power is an ascendancy node, it isn't connected to the main graph
+	_, ok = actual[62504]
+	testza.AssertFalse(t, ok, "Disconnected nodes should not be included in the output")
+
+	testza.AssertLen(t, actual, 2075, "All connected nodes should have a path")
+}
+
+func TestCalculateAllocationPathsStability(t *testing.T) {
+	lastResult := map[int64]int64{}
+	for i := 0; i < 20; i++ {
+		result := TreeVersions[TreeVersion3_18].CalculateAllocationPaths([]int64{48828, 55373, 2151, 47062, 15144, 62103}, []int64{48828})
+		if i > 0 {
+			testza.AssertEqual(t, result, lastResult, "Results should be stable between runs")
+		}
+		lastResult = result
+	}
+}
+
+func BenchmarkCalculateAllocationPaths(b *testing.B) {
 	TreeVersions[TreeVersion3_18].getGraph()
 	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
-		TreeVersions[TreeVersion3_18].CalculateTreePath([]int64{48828, 55373, 2151, 47062, 15144, 62103}, 23881)
+		TreeVersions[TreeVersion3_18].CalculateAllocationPaths([]int64{48828, 55373, 2151, 47062, 15144, 62103}, []int64{48828})
 	}
 }

data/tree_search.go

@@ -0,0 +1,102 @@
+package data
+
+import "container/heap"
+
+type SearchState struct {
+	frontier  []int64
+	distances map[int64]int64
+	nextHops  map[int64]int64
+}
+
+// heap.Interface implementation to maintain SearchState.frontier as a
+// priority queue sorted by distance from active nodes.
+func (f SearchState) Len() int { return len(f.frontier) }
+func (f SearchState) Less(i, j int) bool {
+	iNode := f.frontier[i]
+	jNode := f.frontier[j]
+
+	if f.distances[iNode] < f.distances[jNode] {
+		return true
+	}
+
+	if f.distances[iNode] > f.distances[jNode] {
+		return false
+	}
+
+	// We want results to be consistent every time we run the algorithm,
+	// so we use the node ID as a tiebreaker
+	return iNode < jNode
+}
+func (f SearchState) Swap(i, j int) { f.frontier[i], f.frontier[j] = f.frontier[j], f.frontier[i] }
+func (f *SearchState) Push(x any)   { f.frontier = append(f.frontier, x.(int64)) }
+func (f *SearchState) Pop() any {
+	old := f.frontier
+	n := len(old)
+	x := old[n-1]
+	f.frontier = old[0 : n-1]
+	return x
+}
+
+// Calculates the next hop you should take to traverse a shortest path from any
+// arbitrary node to the nearest active node. Active nodes map to -1. Disconnected
+// tree nodes will not appear in the result.
+//
+// The algorithm will always pick consistent paths through the tree each time it is
+// run when there are multiple shortest paths (this property is important to prevent
+// the skill tree UI from flip-flopping between options as users allocate nodes).
+//
+// Requires a single BFS of the tree, + a heap push/pop pair per node.
+// Time complexity: O(V * log(V) + E)
+func (v *TreeVersionData) CalculateAllocationPaths(activeNodes []int64, rootNodes []int64) map[int64]int64 {
+	_, adjacencyMap := v.getGraph()
+
+	state := SearchState{
+		frontier:  make([]int64, len(activeNodes)+len(rootNodes)),
+		distances: make(map[int64]int64, len(adjacencyMap)),
+		nextHops:  make(map[int64]int64, len(adjacencyMap)),
+	}
+
+	for i, activeNode := range activeNodes {
+		state.frontier[i] = activeNode
+		state.distances[activeNode] = 0
+		state.nextHops[activeNode] = -1
+	}
+
+	for _, rootNode := range rootNodes {
+		_, isActive := state.distances[rootNode]
+		if !isActive {
+			// A root node is reachable, but if it isn't already
+			// active, we'd need to spend 1 distance allocating it.
+			state.distances[rootNode] = 1
+			state.nextHops[rootNode] = -1
+			state.frontier = append(state.frontier, rootNode)
+		}
+	}
+
+	// This is a BFS, but using a priority queue that sorts by both
+	// distance *and* node ID. The use of node ID ensures that we
+	// produce stable output when there are multiple shortest paths,
+	// despite the arbitrary iteration order of neighbors in the
+	// adjacency map.
+	heap.Init(&state)
+	for state.Len() > 0 {
+		currentNode := heap.Pop(&state).(int64)
+		currentDistance := state.distances[currentNode]
+
+		for adjacency := range adjacencyMap[currentNode] {
+			_, alreadyVisited := state.distances[adjacency]
+			if alreadyVisited {
+				// Visiting in heap order means that we can assume
+				// that any path we already found is no longer than
+				// this new path.
+				continue
+			}
+
+			state.distances[adjacency] = currentDistance + 1
+			state.nextHops[adjacency] = currentNode
+			heap.Push(&state, adjacency)
+		}
+	}
+
+	return state.nextHops
+}

data/tree_versions.go

@@ -141,82 +141,23 @@ func (v *TreeVersionData) getGraph() (graph.Graph[int64, int64], map[int64]map[i
 				continue
 			}
 
-			_ = g.AddEdge(targetID, *node.Skill)
 			_ = g.AddEdge(*node.Skill, targetID)
+
+			// Most edges are bidirectional, but masteries are an exception;
+			// you can't use a mastery to travel between 2 notables in a cluster
+			if targetNode.IsMastery == nil || !*targetNode.IsMastery {
+				_ = g.AddEdge(targetID, *node.Skill)
+			}
 		}
 	}
 
 	v.graph = g
 
 	// We can pre-calculate the adjacency map, as the graph won't change
+	// (at least, until we add support for thread of hope/impossible escape/etc)
 	v.adjacencyMap, _ = v.graph.AdjacencyMap()
 
 	return v.graph, v.adjacencyMap
 }
 
-func (v *TreeVersionData) CalculateTreePath(activeNodes []int64, target int64) []int64 {
-	g, adjacencyMap := v.getGraph()
-	//found := int64(-1)
-
-	mappedNodes := make(map[int64]bool, len(activeNodes))
-	for _, node := range activeNodes {
-		mappedNodes[node] = true
-	}
-
-	resultPath, _ := BFS(g, adjacencyMap, target, func(value int64) bool {
-		_, ok := mappedNodes[value]
-		return ok
-	})
-
-	return resultPath
-}
-
-// BFS is an adapted version of graph.BFS that also returns the traversal path
-func BFS[K comparable, T any](g graph.Graph[K, T], adjacencyMap map[K]map[K]graph.Edge[K], start K, visit func(K) bool) ([]K, error) {
-	if _, ok := adjacencyMap[start]; !ok {
-		return nil, fmt.Errorf("could not find start vertex with hash %v", start)
-	}
-
-	queue := make([]K, 1)
-	visited := make(map[K]K)
-
-	visited[start] = start
-	queue[0] = start
-
-	found := false
-	currentHash := start
-	for len(queue) > 0 {
-		currentHash = queue[0]
-
-		queue = queue[1:]
-
-		if stop := visit(currentHash); stop {
-			found = true
-			break
-		}
-
-		for adjacency := range adjacencyMap[currentHash] {
-			if _, ok := visited[adjacency]; !ok {
-				visited[adjacency] = currentHash
-				queue = append(queue, adjacency)
-			}
-		}
-	}
-
-	if !found {
-		return []K{}, nil
-	}
-
-	resultPath := make([]K, 0)
-	resultPath = append(resultPath, currentHash)
-
-	next := currentHash
-	for next != start {
-		next = visited[next]
-		resultPath = append(resultPath, next)
-	}
-
-	return resultPath, nil
-}
-
 var TreeVersions = make(map[TreeVersion]*TreeVersionData)

frontend/src/lib/components/skill-tree/SkillTree.svelte

@@ -12,6 +12,7 @@
   import AllGroups from '$lib/components/skill-tree/AllGroups.svelte';
   import ClassImage from '$lib/components/skill-tree/ClassImage.svelte';
   import Tooltip from '$lib/components/skill-tree/Tooltip.svelte';
+  import { calculateAllocationPath, type PrecalculatedAllocationPaths } from './paths';
 
   let currentClass: string | undefined = $state();
   $effect(() => {
@@ -37,28 +38,49 @@
   let offsetX = $state(0);
   let offsetY = $state(0);
 
-  let activeNodes: number[] | undefined = $state();
+  let activeNodes: number[] = $state([]);
+
   $effect(() => {
-    $currentBuild?.Build?.PassiveNodes?.then((newNodes) => (activeNodes = newNodes)).catch(logError);
+    $currentBuild?.Build?.PassiveNodes?.then((newNodes) => (activeNodes = newNodes ?? [])).catch(logError);
   });
 
+  function precalculateAllocationPaths(active: number[]): Promise<PrecalculatedAllocationPaths> {
+    const version = skillTreeVersion || '3_18';
+    const rootNodes = classStartNodes[skillTree.classes.findIndex((c) => c.name === currentClass)];
+    return syncWrap
+      ?.CalculateAllocationPaths(
+        version,
+        // Need to clone; can't directly marshal a state-proxy to go
+        [...active],
+        rootNodes
+      )
+      .then((paths) => paths ?? {})
+      .catch((e: Error) => {
+        logError(e);
+        return {};
+      });
+  }
+
+  // Must be recalculated when the active nodes change or when the graph topology
+  // changes (e.g. socketing a Thread of Hope).
+  let allocationPaths: Promise<PrecalculatedAllocationPaths> = $derived(precalculateAllocationPaths(activeNodes));
+
   let clickNode = (node: Node) => {
     const nodeId = node.skill ?? -1;
     if (activeNodes?.includes(nodeId)) {
       void syncWrap?.DeallocateNodes(nodeId);
       currentBuild.set($currentBuild);
     } else {
-      // TODO: Needs support for ascendancies or any other disconnect groups
-      const rootNodes = classStartNodes[skillTree.classes.findIndex((c) => c.name === currentClass)];
-      void syncWrap?.CalculateTreePath(skillTreeVersion || '3_18', [...rootNodes, ...(activeNodes ?? [])], nodeId).then((pathData) => {
-        if (!pathData) {
-          return;
-        }
-        // The first in the path is always an already allocated node
-        const isRootInPath = rootNodes.includes(pathData[0]);
-        void syncWrap?.AllocateNodes(isRootInPath ? pathData : pathData.slice(1));
-        currentBuild.set($currentBuild);
-      });
+      allocationPaths
+        .then((paths) => {
+          const path = calculateAllocationPath(paths, nodeId);
+          if (!path) {
+            return;
+          }
+          void syncWrap?.AllocateNodes(path);
+          currentBuild.set($currentBuild);
+        })
+        .catch(logError);
     }
   };
 
@@ -148,13 +170,12 @@
     }
 
     if ($hoveredNode !== undefined && currentClass) {
-      const rootNodes = classStartNodes[skillTree.classes.findIndex((c) => c.name === currentClass)];
       const target = $hoveredNode.skill!;
-      syncWrap
-        .CalculateTreePath(skillTreeVersion || '3_18', [...rootNodes, ...(activeNodes ?? [])], target)
-        .then((data) => {
-          if (data && get(hoveredNode)) {
-            hoverPath.set(data);
+      allocationPaths
+        .then((paths) => {
+          const path = calculateAllocationPath(paths, target);
+          if (path && get(hoveredNode)) {
+            hoverPath.set(path);
           }
         })
         .catch(logError);