Initial commit
This commit is contained in:
HussienX72u
@@ -0,0 +1,143 @@
|
||||
# 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?
|
||||
Reference in New Issue
Block a user