# Socket Snapping System — Full Code Review

## Pipeline Overview

```
Every frame (GhostManagerV2.Update)
  │
  ├─► SnapSystem.UpdateSockets(surfacePoint, ghostRoot)
  │       Physics.OverlapSphere → find SocketPoints on nearby blocks
  │       → highlight closest one
  │
  ├─► SocketIterator.GetCurrentActiveSocket()
  │       → which socket on the GHOST is active (scroll to cycle)
  │
  ├─► GhostManagerV2 feeds both into BlockEditContext (SetFrameState)
  │
  └─► IGhostMovementStrategy.UpdateMovement(ctx)
          FreeMoveStrategy / AxisMoveStrategy reads ctx.TargetSocket + ctx.MySocket
          → snaps ghost position + rotation

On block placement (SocketManagerChecker)
  └─► Physics.OverlapSphere per socket → match & mark occupied
```

---

## 🔴 Bugs

### 1. `SocketPoint.Update()` — occupancy derived from a null check (not the flag)
```csharp
private void Update()
{
    isOccupied = occupyingObject != null;  // ← overwrites SetOccupied/Release every frame
}
```
- `SetOccupied(obj)` correctly sets `isOccupied = true`
- But `Update()` runs **next frame** and re-derives it from the reference
- If `occupyingObject` gets destroyed externally without calling `Release()`, `isOccupied` snaps to `false` silently — ghost can snap to a "taken" socket
- **Worse:** `Release()` sets `isOccupied = false` AND nulls the ref. Correct. But `Update()` then runs again and re-confirms it. This is redundant and fragile.
- **Fix:** Remove `Update()` entirely. `SetOccupied`/`Release` already maintain state correctly.

### 2. `SnapSystem.UpdateSockets` — highlights ALL found sockets as `Compatible` without checking compatibility
```csharp
foreach (SocketPoint socket in root.GetComponentsInChildren<SocketPoint>())
{
    if (socket.IsOccupied()) continue;
    nearbySockets.Add(socket);
    socket.Highlight(HighlightState.Compatible);  // ← no compatibility check!
}
```
The ghost's active socket (`mySocket`) is checked for compatibility in `GhostManagerV2` AFTER this runs. But by then the socket is already highlighted green even if it's actually incompatible.

### 3. `SnapSystem` finds closest socket to `surfacePoint`, not to `mySocket`
```csharp
closestSocket = FindClosest(detectionPoint, nearbySockets);
```
`detectionPoint` is the **ray hit on the world surface**, not the ghost's active socket position. For blocks resting on a table, this may select a socket far from the ghost's actual attachment point. Should be closest to `mySocket.transform.position`.

### 4. `SocketManagerChecker.getClosestSocketBy` — skips sockets with the same normal direction
```csharp
if (socketPoint.GetNormal() == otherSocketPoint.GetNormal())
    continue;
```
This uses `==` on `Vector3` (epsilon comparison). Two sockets on perfectly parallel faces will silently be skipped even if they are meant to connect. This should compare **dot product < threshold** or be removed entirely.

### 5. `SocketContainer` — two separate lists (`sockets` + `iterativeSockets`) can desync
```csharp
[SerializeField] private List<SocketPoint> sockets = new List<SocketPoint>();
[SerializeField] private List<SocketPoint> iterativeSockets = new List<SocketPoint>();
```
- `RemoveSocket` removes from `sockets` but **not** from `iterativeSockets`
- `GetSocketCount()` returns `sockets.Count` but iteration uses `iterativeSockets.Count`
- An index-out-of-bounds crash is possible if they differ

### 6. `SnapSystem` — `GetComponentsInChildren` called every frame inside `UpdateSockets`
This runs on every nearby block, every frame. It's a GC allocation + tree traversal. With many blocks, this becomes expensive very quickly.

---

## 🟡 Design Issues

### 7. Compatibility check split across two places
- `SnapSystem` ignores compatibility when building `nearbySockets`
- `GhostManagerV2.Update` re-checks compatibility:
  ```csharp
  bool compatible = targetSocket != null && (mySocket == null || targetSocket.CanAccept(mySocket));
  ```
- `FreeMoveStrategy.TrySocketSnap` checks it **again**
- `AxisMoveStrategy.TrySocketSnap` checks it **again**

The same check runs 3-4× per frame. A single, central filter in `SnapSystem` would be cleaner and faster.

### 8. `CanAccept` has a private overload that the interface `ISocket` exposes publicly
```csharp
private bool CanAccept(ISocket otherSocket) { ... }  // private
public bool CanAccept(ISocket otherSocket, bool isJoint = false) { ... }  // public via ISocket
```
The private method exists only because the public one calls it when `isJoint == false`. This is confusing. Both should just be one method.

### 9. `SocketContainer.using System.Net.Sockets` — wrong namespace imported
Line 4: `using System.Net.Sockets;` — this is the networking library, not your socket system. It's unused and a potential naming collision source.

### 10. `SnapSystem` uses `col.transform.root` — breaks for nested prefabs
If a block is a child of another GameObject at runtime (e.g., parented to a platform), `root` returns the platform, not the block. All sockets on the platform get searched. Should use the block's own transform, not root.

---

## ✅ What's Working Well

- **`SocketIterator`** — clean, event-driven, no polling. Good.
- **`SnapSystem` deduplication** with `visitedRoots HashSet` — prevents double-counting sockets from multi-collider blocks.
- **`GhostMovementContext` struct** — passed by `ref`, avoids allocation.
- **Type + radius validation** in `SocketPoint.CanAccept` — correct Meccano-style pin/hole matching.
- **Highlight state machine** (`HighlightState` enum) — clean visual feedback separation.

---

## 🚀 Recommended Architecture

The core improvement is: **let `SnapSystem` own the full compatibility pipeline**, so strategies just ask "what's the best snap?" without repeating the check.

```
SnapSystem.UpdateSockets(surfacePoint, ghostRoot, mySocket?)
  │
  ├─ Physics.OverlapSphere (cached collider array, no GC)
  ├─ For each block root (deduplicated)
  │     For each SocketPoint
  │       if occupied → skip
  │       if mySocket != null && !socket.CanAccept(mySocket) → highlight Incompatible, skip
  │       else → add to nearbySockets, highlight Compatible
  │
  ├─ closestSocket = FindClosest(mySocket?.position ?? surfacePoint, nearbySockets)
  └─ closestSocket → highlight Active
```

**Key changes:**
1. **Pass `mySocket` into `SnapSystem.UpdateSockets`** → compatibility filtering happens once, centrally
2. **Closest = nearest to `mySocket` position**, not ray hit point
3. **Cache `GetComponentsInChildren` results** using a static `SocketPoint[]` buffer (or scan-on-block-add via events) to avoid per-frame allocations
4. **Remove `SocketPoint.Update()`** — occupancy maintained purely by `SetOccupied`/`Release`
5. **Single list in `SocketContainer`** — or make iterative list a view/subset of the main list, not a separate serialized field

Would you like me to implement these fixes?