JuegoSim/Assets/_Project/Data/Markdowns/socket_snap_review.md

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)

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

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

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

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

[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:

  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

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?