fix some performance issues

add AI notes

BVH_Review_Notes.md

+# BVH / AABB Broad-Phase – Review Notes
+
+Notes on the Bounding Volume Hierarchy (BVH) infrastructure and its integration into the
+three geometry checks, based on a read of the source code and of
+`Bounding_Volume_Hierarchy_Splitstrategien-3.pdf`.
+
+The notes are observations and *suggestions* — the document itself is explicit that the
+heuristic/policy work is still exploratory ("Entwicklungsstand"), so several points below
+are confirmations that the code matches that intent, and several are concrete improvement
+candidates.
+
+## Scope reviewed
+
+Core data structure (`CityDoctorModel/.../datastructure/aabb`):
+- `BoundingVolumeHierarchyTree`, `Node`, `BvhBuildItem`, `AABB`
+- `BinarySplitters` / `BinarySplitResult`, `OctonarySplitters` / `OctonarySplitResult`
+
+Check integration (`CityDoctorValidation/.../checks`):
+- `geometry/RingSelfIntCheck`, `geometry/NestedRingsCheck`
+- `aabb/SolidSelfIntCheckAABB`, `util/SelfIntersectionUtil`
+- `util/BvhUsagePolicy`
+
+The three "check methods" referred to in the task are Solid Self Intersection,
+Ring Self Intersection and Nested Rings.
+
+---
+
+## 1. Correctness / behavioural concerns
+
+These are the items worth checking first because they can change results or silently
+degrade the tree quality.
+
+### 1.1 `AABB.findLongestAxis()` breaks ties toward a potentially degenerate axis
+`AABB.findLongestAxis()` (`AABB.java:280`) returns `2` (the Z axis) whenever no axis is
+*strictly* the largest:
+
+```java
+if (x > y && x > z) return 0;
+if (y > x && y > z) return 1;
+return 2;            // tie -> Z
+```
+
+For typical CityGML footprints with `extentX == extentY` and `extentZ == 0`, the longest
+axis is reported as Z with extent `0`. The binary splitters then split on Z, where every
+element centre equals the split value, so:
+- `BINARY_SPATIAL_MEDIAN` puts everything on one side → invalid → falls back to
+  `objectMean` → also all-equal → falls back to `objectMedian` (`BinarySplitters.java:75`,
+  `48`).
+- The final median split is a count split along a meaningless axis, producing a tree that
+  is balanced by count but spatially poor → worse query pruning.
+
+This is not a result error (the exact phase still decides), but it is a silent performance
+trap on exactly the flat, axis-symmetric data that is common here. **Suggestion:** break
+ties toward the axis with the largest *non-zero* extent (and never select an axis whose
+extent is `≤ degenerateTolerance`).
+
+### 1.2 `BinarySplitters.objectMedian` returns `subList` views, the other splitters return copies
+`objectMedian` (`BinarySplitters.java:20`) returns `items.subList(0, mid)` /
+`items.subList(mid, size)` — live views over the same backing list — whereas `objectMean`
+and `spatialMedian` build fresh `ArrayList`s. During recursion the child lists are sorted
+again in place (`items.sort(...)`), which mutates the backing array of the parent's
+view. It happens to be safe today because the left/right regions are disjoint and never
+structurally modified, but:
+- it is fragile (a future change that structurally edits a partition would throw
+  `ConcurrentModificationException` from the parent view), and
+- nested `subList`-of-`subList` chains add an offset-indirection per level, so
+  `get(i)`/sort cost grows with depth.
+
+**Suggestion:** make the splitters consistent — copy the partitions into new lists (or, if
+the views are kept deliberately for allocation reasons, document the invariant loudly and
+add a test that exercises deep nesting).
+
+### 1.3 `Node.isLeaf()` couples "leaf" to "element != null"
+`Node.isLeaf()` (`Node.java:17`) is `children.isEmpty() && element != null`. This works
+only because no element stored in the tree is ever `null`. The index-based solid tree
+stores autoboxed `Integer`s and rings/edges/vertices/polygons are non-null, so it is fine
+today — but it is an implicit precondition. A `null` element would make a real leaf
+invisible to every query (`collectMatchingElements`, `BoundingVolumeHierarchyTree.java:159`).
+**Suggestion:** model leaf-ness explicitly (e.g. a boolean/`type` field or subclasses)
+rather than inferring it from `element != null`.
+
+### 1.4 The contained-query broad phase is sound — keep the comment
+`getAllElementsContainedIn` (`BoundingVolumeHierarchyTree.java:140`) visits internal nodes
+by *overlap* but only adds leaves that are fully *contained*. This is the correct choice
+(an internal node AABB can extend past the container while still holding contained leaves)
+and is necessary for Nested Rings to avoid false negatives. Worth keeping the existing
+explanatory comment and a regression test, since it is an easy thing to "optimise" into a
+bug later.
+
+---
+
+## 2. Performance observations
+
+### 2.1 `RingSelfIntCheck` uses `edges.indexOf(e2)` inside the candidate loop
+In `checkRingBvh` (`RingSelfIntCheck.java:262`) each BVH candidate is mapped back to its
+index with `edges.indexOf(e2)`, which is an O(n) linear scan of the edge list. Inside the
+per-edge candidate loop this reintroduces O(n²) behaviour and partially defeats the point
+of the broad phase. **Suggestion:** precompute an `IdentityHashMap<Edge,Integer>` once (or
+store the index as the BVH element, like the solid check does with `Integer` indices) and
+look it up in O(1).
+
+### 2.2 `SolidSelfIntCheckAABB` always builds a tree and ignores `BvhUsagePolicy`
+`SolidSelfIntCheckAABB.check` (`SolidSelfIntCheckAABB.java:91`) builds a BVH whenever
+`polys.size() > 1`, with a hard-coded `binaryBuilder()` (AUTO → `BINARY_SPATIAL_MEDIAN`).
+It does **not** call `BvhUsagePolicy.shouldUseTree(...)` nor `chooseSplitStrategy(...)`,
+unlike `RingSelfIntCheck` and `NestedRingsCheck`. Consequences:
+- small solids pay full build cost for no benefit (the policy's whole purpose), and
+- the solid check can never use an octonary strategy or the size/flatness-based choice.
+
+**Suggestion:** route the solid check through the same policy gate for consistency, or
+document why the solid check intentionally always uses a tree.
+
+### 2.3 Inconsistent default `maxDepth` across entry points
+`computeDefaultMaxDepth` (`BoundingVolumeHierarchyTree.java:104`) gives a size-aware depth
+(`ceil(2·log2 n)`, clamped to `[4,32]`) and is used by `newWithStrategy`/`newOctonary`.
+But `binaryBuilder()`/`octonaryBuilder()` default to the flat `DEFAULT_MAX_DEPTH = 32`
+(`:441`, `:450`), and `SolidSelfIntCheckAABB` uses `binaryBuilder()`. So different checks
+get different depth policies for the same input size. **Suggestion:** apply the size-aware
+default in the builders as well (or have all production entry points go through one
+factory).
+
+### 2.4 Per-leaf `ArrayList` allocation
+Every `Node` allocates an `ArrayList` for children (`Node.java:8`), including leaves. With
+the binary default `maxLeafSize = 1` this is one empty list per element. For large solids
+that is a measurable allocation/GC overhead. **Suggestion:** lazily create the children
+list, or share an immutable empty list for leaves.
+
+### 2.5 Query allocates a fresh result list and two lambdas per call
+`getAllIntersectingElements` / `getAllElementsContainedIn` allocate a new `ArrayList` plus
+two `Predicate` lambdas on every call (`:124`, `:140`). In RSI this is invoked once per
+edge and once per vertex-edge test. Minor, but on hot paths a reusable visitor or a
+plain recursive method without per-call lambdas would avoid it.
+
+### 2.6 Octonary `objectMedian` sorts the full list three times
+`OctonarySplitters.objectMedian` (`OctonarySplitters.java:42`) sorts the whole list by X,
+then Y, then Z to extract the three per-axis medians. That is 3× O(n log n) per node.
+A single pass with `nth_element`-style selection (or computing all three medians from one
+copy) would cut build time on large nodes. Acceptable for now given the exploratory stage,
+but worth noting for when a strategy is "frozen".
+
+---
+
+## 3. Policy / heuristic notes (`BvhUsagePolicy`)
+
+### 3.1 Code and the document's example RSI rule diverge (expected, but flag it)
+The PDF §21.5 sketches a candidate RSI rule (`thin > 0.75` or `aspect ≳ 80` →
+`OCTONARY_SPATIAL_MEDIAN`). The production `chooseSplitStrategy`
+(`BvhUsagePolicy.java:269`) currently returns `BINARY_SPATIAL_MEDIAN` unconditionally for
+RSI. This matches the document's stated plan (candidate rules are *not* yet promoted into
+`BvhUsagePolicy`), so it is correct as-is — but the divergence is worth a one-line comment
+in the code pointing at `candidate-rules-v1`, so a reader doesn't assume the example rule
+is already live.
+
+### 3.2 Magic thresholds are undocumented in code
+`shouldUseTree` mixes several constants: `elementCount < 20`, `averageRelativeBoxExtent >
+0.65 && elementCount < 128` (`:253`–`:261`), plus the per-check thresholds 16 / 8 / 32
+(`:15`–`:17`). The PDF explains these are provisional, but the constants in the source have
+no comment tying them back to the report or to `candidate-rules-v1`. **Suggestion:** add a
+short javadoc referencing the policy version so they can be audited/frozen together (the
+report's §21.6 "Versionierung und Einfrieren" goal).
+
+### 3.3 `BvhInputSummary.ofElements` materialises an intermediate list
+`ofElements` (`:116`) does `elements.stream().map(aabbFunction).toList()` and then
+`AABB.enclosing` iterates again. For the "cheap metrics" goal this is fine, but note it is
+2 passes + a temporary list; a single fold would keep it strictly linear with no allocation
+if this is ever called on large inputs in production.
+
+### 3.4 `shouldUseTree(checkType, int)` path has no geometric signal
+The count-only overload builds a `countOnly` summary with `averageRelativeBoxExtent = 0`,
+so the `> 0.65` guard can never trigger. `RingSelfIntCheck` AUTO uses exactly this overload
+(`RingSelfIntCheck.java:160`), i.e. RSI decides "use tree?" purely on edge count and never
+on box geometry, while Nested Rings uses the full summary. That asymmetry may be
+intentional (edges are cheap and uniform) but is worth a deliberate decision rather than an
+accident of which overload was called.
+
+---
+
+## 4. Code-cleanliness / maintainability
+
+- **Duplicated solid-intersection variants.** `SelfIntersectionUtil` carries
+  `calculateSolidSelfIntersection0`, `calculateSolidSelfIntersection` (no tree),
+  `calculateSolidSelfIntersection(...tree)`, and `calculateSolidSelfIntersectionWithTree`
+  (two overloads). Several are explicitly "for comparison". Once the heuristic study is
+  done, prune the dead variants or move the comparison-only ones into test support to keep
+  the production class focused. Multiple `// TODO maybe later use another tree here`
+  markers (`:170`, `:237`) should be resolved or ticketed.
+- **Unused / parallel AABB API.** `AABB` exposes both `overlaps` (inclusive) and
+  `intersects` (exclusive), plus `doAnyBoxesOverlap` and `ofPolygons`, which the BVH path
+  does not use. Confirm callers exist; otherwise trim, or at least document why two
+  overlap predicates with different boundary semantics coexist (easy to pick the wrong one).
+- **`NestedRingsCheck.isUseAabbFilter()`** returns `variant != OLD` (`:118`), so it reports
+  `true` for AUTO and every BVH variant. The legacy boolean accessor is now lossy; callers
+  relying on it get a coarse answer. Consider deprecating the boolean API in favour of
+  `getVariant()`.
+- **Author/wording.** New files are authored by Baris Numanoglu; the report's author name
+  in the PDF appears as "Bari Numanolu" (PDF text-extraction may have dropped characters) —
+  worth confirming the name is spelled consistently in the final document.
+
+---
+
+## 5. Things that look correct / good (worth keeping)
+
+- Generic `BoundingVolumeHierarchyTree<E>` with an injected `Function<E,AABB>` keeps the
+  structure cleanly decoupled from model classes — exactly as the report describes.
+- Precomputing centres in `BvhBuildItem` (`:13`) avoids recomputing them during splitting.
+- The split fallback chains (spatial → mean → median) guarantee progress and termination
+  together with the count/`maxDepth`/degenerate stop criteria, so the build can't loop on
+  pathological inputs.
+- Broad-phase / exact-phase separation is respected everywhere: every check still calls the
+  original exact predicate (`doPolygonsIntersect`, `areAllPointsInside`, segment distance),
+  so the BVH cannot change *which* errors are reported — only the candidate set. This is the
+  single most important invariant and the correctness tests (`checks/aabb/correctness`)
+  target it directly.
+- The staged methodology in the test landscape (correctness → synthetic measurement →
+  cheap metrics → variant timing → bucketing → real-data cross-check) matches the report and
+  is a sound way to justify an AUTO policy before freezing it.
+
+---
+
+## 6. Suggested priorities
+
+1. Fix `findLongestAxis` tie-breaking (§1.1) — cheap, removes a silent perf cliff on flat
+   data.
+2. Replace `edges.indexOf` in RSI with an index map (§2.1) — restores expected complexity.
+3. Decide on the solid check's policy gating + strategy (§2.2) so all three checks behave
+   consistently.
+4. Make the binary splitters consistent about copy-vs-view (§1.2).
+5. Document/anchor the policy constants to a version id ahead of the "freeze the policy"
+   step in the report (§3.2, §3.1).

CityDoctorParent/CityDoctorModel/src/main/java/de/hft/stuttgart/citydoctor2/datastructure/aabb/BoundingVolumeHierarchyTree.java

@@ -38,6 +38,8 @@ public class BoundingVolumeHierarchyTree<E> {
 		return BoundingVolumeHierarchyTree.<E>binaryBuilder()
 				.elements(elements)
 				.aabbFunction(aabbFunction)
+				.maxDepth(computeDefaultMaxDepth(
+						elements != null ? elements.size() : 0))
 				.build();
 	}
 
@@ -47,6 +49,8 @@ public class BoundingVolumeHierarchyTree<E> {
 		return BoundingVolumeHierarchyTree.<E>octonaryBuilder()
 				.elements(elements)
 				.aabbFunction(aabbFunction)
+				.maxDepth(computeDefaultMaxDepth(
+						elements != null ? elements.size() : 0))
 				.build();
 	}
 
@@ -124,10 +128,11 @@ public class BoundingVolumeHierarchyTree<E> {
 	public List<E> getAllIntersectingElements(AABB query) {
 		Objects.requireNonNull(query, "query");
 		List<E> result = new ArrayList<>();
+		Predicate<AABB> shouldVisitNode = box -> box.overlaps(query);
 		collectMatchingElements(
 				root,
-				box -> box.overlaps(query),
-				box -> box.overlaps(query),
+				shouldVisitNode,
+				shouldVisitNode,
 				result);
 		return result;
 	}

CityDoctorParent/CityDoctorModel/src/main/java/de/hft/stuttgart/citydoctor2/datastructure/aabb/Node.java

@@ -5,20 +5,22 @@ import java.util.List;
 
 public class Node<E> {
 	
-    private final List<Node<E>> children = new ArrayList<>();
+    private List<Node<E>> children;
     private final AABB aabb;
     private final E element;
-
+    
     public Node(E element, AABB aabb) {
     	this.aabb = aabb;
         this.element = element;
     }
 
     public boolean isLeaf() {
+    	if (children == null) {
+    		return true;
+    	}
         return children.isEmpty() && element != null;
     }
     
-    
     public AABB getAabb() {
 		return aabb;
 	}
@@ -28,6 +30,9 @@ public class Node<E> {
 	}
     
     public List<Node<E>> getChildren() {
+    	if (children == null) {
+    		children = new ArrayList<>();
+    	}
     	return children;
 	}
 }

CityDoctorParent/CityDoctorValidation/src/main/java/de/hft/stuttgart/citydoctor2/checks/geometry/RingSelfIntCheck.java

@@ -20,6 +20,7 @@ package de.hft.stuttgart.citydoctor2.checks.geometry;
 
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.IdentityHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -240,6 +241,11 @@ public class RingSelfIntCheck extends Check {
 		BoundingVolumeHierarchyTree<Edge> edgeTree = BoundingVolumeHierarchyTree.newWithStrategy(
 				edges, e -> AABB.of(e.getFrom(), e.getTo(), epsilon), splitStrategy);
 
+		IdentityHashMap<Edge, Integer> map = new IdentityHashMap<Edge, Integer>();
+		for (int i = 0; i < edges.size(); i++) {
+			map.put(edges.get(i), i);
+		}
+	
 		for (int i = 0; i < edges.size(); i++) {
 			Edge e1 = edges.get(i);
 			AABB q = AABB.of(e1.getFrom(), e1.getTo(), epsilon);
@@ -259,7 +265,7 @@ public class RingSelfIntCheck extends Check {
 					continue;
 				}
 
-				int j = edges.indexOf(e2);
+				int j = map.get(e2);
 				if (j <= i) {
 					continue;
 				}

CityDoctorParent/CityDoctorValidation/src/test/java/de/hft/stuttgart/citydoctor2/checks/aabb/heuristics/BvhStrategyHeuristicExplorationTest.java

@@ -13,6 +13,7 @@ import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 
+import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Tag;
 import org.junit.jupiter.api.Test;
 
@@ -48,6 +49,7 @@ public class BvhStrategyHeuristicExplorationTest {
 	private static final Path OUTPUT_DIRECTORY = Path.of("target", "bvh-exploration");
 
 	@Test
+	@Disabled
 	public void exploreSsiNestedAndRsiStrategyCandidates() throws IOException {
 		// Keep all three checks in one table so metric trends can be compared side by side.
 		List<BvhHeuristicTimingSupport.Observation> observations = new ArrayList<>();

CityDoctorParent/CityDoctorValidation/src/test/java/de/hft/stuttgart/citydoctor2/checks/aabb/heuristics/RealCityGmlBvhHeuristicExplorationTest.java

@@ -27,6 +27,7 @@ import java.util.stream.Collectors;
 import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.core.config.Configurator;
+import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Tag;
 import org.junit.jupiter.api.Test;
 
@@ -68,6 +69,7 @@ public class RealCityGmlBvhHeuristicExplorationTest {
 	private static final PrintStream SILENT_OUT = new PrintStream(OutputStream.nullOutputStream());
 
 	@Test
+	@Disabled
 	public void printRealCityGmlMetricsAndBvhTiming() throws Exception {
 		// Each subdirectory is treated as one labelled dataset group.
 		assumeTrue(Files.isDirectory(REAL_CITYGML_ROOT),