Use derivative tiebreaker

Author: iamthenoname

node-graph/gcore/src/subpath/structs.rs

@@ -228,6 +228,24 @@ impl BezierHandles {
 			_ => self,
 		}
 	}
+
+	/// Returns the tangent vector at the start of the curve (t=0)
+	pub fn derivative_at_start(&self, start: DVec2, end: DVec2) -> DVec2 {
+		match self {
+			BezierHandles::Linear => end - start,
+			BezierHandles::Quadratic { handle } => 2.0 * (handle - start),
+			BezierHandles::Cubic { handle_start, .. } => 3.0 * (handle_start - start),
+		}
+	}
+
+	/// Returns the tangent vector at the end of the curve (t=1)
+	pub fn derivative_at_end(&self, start: DVec2, end: DVec2) -> DVec2 {
+		match self {
+			BezierHandles::Linear => end - start,
+			BezierHandles::Quadratic { handle } => 2.0 * (end - handle),
+			BezierHandles::Cubic { handle_end, .. } => 3.0 * (end - handle_end),
+		}
+	}
 }
 
 /// Representation of a bezier curve with 2D points.

node-graph/gcore/src/vector/IMPROVEMENT_PLAN.md

@@ -0,0 +1,346 @@
+# Vector Mesh Fill Rendering - Implementation Plan
+
+## Overview
+This document outlines the improvements needed for the vector mesh fill rendering system based on code review feedback for PR #3388. All changes will be made to `node-graph/gcore/src/vector/vector_attributes.rs`.
+
+---
+
+## Issues and Solutions
+
+### 1. CRITICAL: Multiple Edges Between Same Vertices
+
+**Problem:**
+When multiple edges connect the same two vertices (e.g., two curves with different curvatures), they produce the same angle during neighbor sorting. This causes incorrect edge ordering when finding minimal faces.
+
+**Solution:**
+Add a derivative-based tiebreaker to distinguish edges with the same angle.
+
+**Implementation Steps:**
+
+1. **Add `derivative_at_start()` method to `BezierHandles` in `node-graph/gcore/src/subpath/structs.rs`:**
+   ```rust
+   /// Returns the tangent vector at the start of the curve (t=0)
+   pub fn derivative_at_start(&self, start: DVec2, end: DVec2) -> DVec2 {
+       match self {
+           BezierHandles::Linear => end - start,
+           BezierHandles::Quadratic { handle } => 2.0 * (handle - start),
+           BezierHandles::Cubic { handle_start, .. } => 3.0 * (handle_start - start),
+       }
+   }
+   ```
+
+2. **Add `derivative_at_end()` method to `BezierHandles` in `node-graph/gcore/src/subpath/structs.rs`:**
+   ```rust
+   /// Returns the tangent vector at the end of the curve (t=1)
+   pub fn derivative_at_end(&self, start: DVec2, end: DVec2) -> DVec2 {
+       match self {
+           BezierHandles::Linear => end - start,
+           BezierHandles::Quadratic { handle } => 2.0 * (end - handle),
+           BezierHandles::Cubic { handle_end, .. } => 3.0 * (end - handle_end),
+       }
+   }
+   ```
+
+3. **Update sorting in `find_closed_regions()` (around line 1052-1059):**
+   - After comparing angles, if they're equal (within epsilon), compare derivative angles
+   - For edges leaving a point, use `derivative_at_start()`
+   - Account for reversed edges by using the appropriate derivative direction
+
+**Code Location:** Lines 1052-1059 in `vector_attributes.rs`
+
+**Priority:** CRITICAL (may cause rendering bugs with multiple edges)
+
+---
+
+### 2. PERFORMANCE: Unnecessary Edge Calculation
+
+**Problem:**
+In `find_minimal_face_from_edge()`, the code recalculates angles and searches through all neighbors to find the next edge (lines 1115-1123), even though neighbors are already sorted by angle in `find_closed_regions()`.
+
+**Current Approach:**
+```rust
+let next = neighbors.iter()
+    .filter(|(seg, _next, _rev)| *seg != prev_segment)
+    .min_by(|a, b| {
+        let dir_a = ...;
+        let dir_b = ...;
+        let angle_a = angle_between(prev_direction, dir_a);
+        let angle_b = angle_between(prev_direction, dir_b);
+        angle_a.partial_cmp(&angle_b).unwrap_or(std::cmp::Ordering::Equal)
+    })?;
+```
+
+**Better Approach:**
+```rust
+// Find the index of the edge we came from
+let prev_index = neighbors.iter()
+    .position(|(seg, _next, _rev)| *seg == prev_segment)?;
+
+// The next edge in the sorted list is the minimal face edge
+// Wrap around using modulo to handle circular list
+let next = neighbors[(prev_index + 1) % neighbors.len()];
+```
+
+**Code Location:** Lines 1115-1123 in `vector_attributes.rs`
+
+**Benefits:**
+- O(n) → O(1) lookup per vertex
+- Eliminates angle calculations in the hot loop
+- Simpler, more readable code
+
+**Priority:** NICE TO HAVE (performance optimization)
+
+---
+
+### 3. FEATURE: Outer Face Culling
+
+**Problem:**
+The current implementation includes the outer boundary face (e.g., the outer square of a grid mesh), which should often be excluded.
+
+**Solution:**
+Use the shoelace formula to compute signed area and filter out counterclockwise (outer) faces.
+
+**Implementation Steps:**
+
+1. **Add `compute_signed_area()` helper function:**
+   ```rust
+   fn compute_signed_area(&self, face: &FoundSubpath) -> f64 {
+       let mut area = 0.0;
+       for edge in &face.edges {
+           let start_pos = self.point_domain.positions()[edge.start];
+           let end_pos = self.point_domain.positions()[edge.end];
+           area += (end_pos.x - start_pos.x) * (end_pos.y + start_pos.y);
+       }
+       area * 0.5
+   }
+   ```
+
+2. **Filter faces in `find_closed_regions()` before returning:**
+   ```rust
+   regions.retain(|face| {
+       // Always include 2-edge faces (floating point issues with area)
+       if face.edges.len() == 2 {
+           return true;
+       }
+       
+       let area = self.compute_signed_area(face);
+       // Keep clockwise faces (negative area) and exclude counterclockwise (positive area)
+       area < 0.0
+   });
+   ```
+
+**Code Location:** End of `find_closed_regions()` (around line 1078)
+
+**Additional Considerations:**
+- Floating point precision: use a small epsilon if needed
+- Consider making this behavior optional via a parameter
+- Document the winding order convention
+
+**Priority:** NICE TO HAVE (feature enhancement)
+
+---
+
+### 4. CODE QUALITY: Loop Style
+
+**Problem:**
+The loop uses an awkward pattern with manual iteration counting and break condition.
+
+**Current Code (lines 1097-1104):**
+```rust
+let mut iteration = 0;
+let max_iterations = adjacency.len() * 2;
+
+loop {
+    iteration += 1;
+    
+    if iteration > max_iterations {
+        return None;
+    }
+    // ...
+}
+```
+
+**Better Code:**
+```rust
+let max_iterations = adjacency.len() * 2;
+
+for _iteration in 1..=max_iterations {
+    // ... loop body ...
+}
+
+// If we exit the loop without returning, the path didn't close
+None
+```
+
+**Code Location:** Lines 1097-1104 in `vector_attributes.rs`
+
+**Benefits:**
+- More idiomatic Rust
+- Clearer intent
+- Automatic bounds checking
+
+**Priority:** NITPICK (code style)
+
+---
+
+### 5. CODE QUALITY: Redundant Check
+
+**Problem:**
+The infinite loop prevention check at line 1145 is redundant with the max_iterations check.
+
+**Current Code (line 1145):**
+```rust
+// Prevent infinite loops
+if path.len() > adjacency.len() {
+    return None;
+}
+```
+
+**Action:**
+Remove this check entirely. The `for` loop (after implementing #4) or the `max_iterations` check already prevents infinite loops.
+
+**Code Location:** Line 1145 in `vector_attributes.rs`
+
+**Priority:** NITPICK (code cleanup)
+
+---
+
+### 6. CODE QUALITY: Cycle Detection Condition
+
+**Problem:**
+The cycle detection at line 1138 includes `e.id != next_segment` which may be unnecessary.
+
+**Current Code (line 1138):**
+```rust
+if path.iter().any(|e| e.end == next_point && e.id != next_segment) {
+    return None;
+}
+```
+
+**Investigation Needed:**
+- Is it possible for `e.end == next_point` and `e.id == next_segment`?
+- If the first condition is sufficient, simplify to:
+```rust
+if path.iter().any(|e| e.end == next_point) {
+    return None;
+}
+```
+
+**Code Location:** Line 1138 in `vector_attributes.rs`
+
+**Priority:** NITPICK (code clarity)
+
+---
+
+### 7. CODE QUALITY: Code Organization
+
+**Problem:**
+The `if next_point == target` check happens before pushing to path, then duplicates the path-push logic inside.
+
+**Current Code (lines 1125-1134):**
+```rust
+if next_point == target {
+    // Completed the cycle
+    path.push(HalfEdge::new(next_segment, current, next_point, next_reversed));
+    
+    // Mark all half-edges as used
+    for edge in &path {
+        used_half_edges.insert((edge.id, edge.reverse));
+    }
+    
+    return Some(FoundSubpath::new(path));
+}
+```
+
+**Better Organization:**
+Move this check to after line 1144 (after the cycle detection) and reuse the path-push logic:
+
+```rust
+// Check if we've created a cycle (might not be back to start)
+if path.iter().any(|e| e.end == next_point && e.id != next_segment) {
+    return None;
+}
+
+path.push(HalfEdge::new(next_segment, current, next_point, next_reversed));
+
+// Check if we've completed the face
+if next_point == target {
+    // Mark all half-edges as used
+    for edge in &path {
+        used_half_edges.insert((edge.id, edge.reverse));
+    }
+    return Some(FoundSubpath::new(path));
+}
+
+prev_segment = next_segment;
+current = next_point;
+```
+
+**Code Location:** Lines 1125-1147 in `vector_attributes.rs`
+
+**Benefits:**
+- Eliminates code duplication
+- More logical flow
+- Easier to maintain
+
+**Priority:** NITPICK (code organization)
+
+---
+
+## Implementation Order
+
+### Phase 1: Critical Fixes
+1. **Multiple Edges Handling** (#1)
+   - Add derivative methods to BezierHandles
+   - Update sorting logic with tiebreaker
+
+### Phase 2: Performance & Features
+2. **Performance Optimization** (#2)
+   - Replace min_by with index-based lookup
+3. **Outer Face Culling** (#3)
+   - Add signed area calculation
+   - Filter counterclockwise faces
+
+### Phase 3: Code Quality
+4. **Loop Style** (#4)
+   - Convert to for loop
+5. **Remove Redundant Check** (#5)
+   - Delete redundant infinite loop check
+6. **Cycle Detection** (#6)
+   - Investigate and simplify if possible
+7. **Code Organization** (#7)
+   - Reorganize target-check logic
+
+---
+
+## Testing Considerations
+
+After implementing these changes, test with:
+
+1. **Multiple edges test:** Create a mesh with multiple curves between the same vertices
+2. **Grid mesh test:** Verify outer face is properly culled
+3. **Complex mesh test:** Test with various mesh topologies
+4. **Performance test:** Measure improvement from optimization #2
+5. **Edge cases:**
+   - 2-edge faces
+   - Self-intersecting paths
+   - Degenerate curves
+
+---
+
+## Notes
+
+- All changes maintain backward compatibility
+- No API changes required (internal implementation only)
+- Changes are localized to vector_attributes.rs and subpath/structs.rs
+- Derivative methods in BezierHandles may be useful for other features in the future
+
+---
+
+## Reviewer Approval Status
+
+**Status:** ✅ Approved with suggestions
+
+**Critical Items:** Issue #1 (if reviewer's understanding is correct)
+**Nice to Have:** Issues #2, #3
+**Nitpicks:** Issues #4-7