From 07d4a52e9d63f11145745e41d116962b6a57394f Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Nov 2025 09:04:13 +0000 Subject: [PATCH] Add physics module review findings for issue creation Documents bugs, missing features, and improvements found during code review of shards/modules/physics: P0 (Critical): - Memory leaks in body/constraint cleanup - ApplyForceAt logic bug (&&/|| operator) - Typo in assertion macro P1 (Missing Features): - No raycasting/shape queries - Missing shape types (cylinder, mesh, heightfield) - Missing constraint types (hinge, cone, etc) - Body sleeping disabled P2 (Improvements): - Soft body param detection broken - Hardcoded system limits - Various minor issues --- docs/physics-issues/P0-critical-bugs.md | 119 ++++++++++++++++ docs/physics-issues/P1-missing-features.md | 123 ++++++++++++++++ docs/physics-issues/P2-improvements.md | 157 +++++++++++++++++++++ 3 files changed, 399 insertions(+) create mode 100644 docs/physics-issues/P0-critical-bugs.md create mode 100644 docs/physics-issues/P1-missing-features.md create mode 100644 docs/physics-issues/P2-improvements.md diff --git a/docs/physics-issues/P0-critical-bugs.md b/docs/physics-issues/P0-critical-bugs.md new file mode 100644 index 00000000000..3b1051782dc --- /dev/null +++ b/docs/physics-issues/P0-critical-bugs.md @@ -0,0 +1,119 @@ +# Critical Physics Module Bugs + +## Issue 1: Memory Leak - Bodies Never Removed From Mirror Map + +**Severity**: Critical +**File**: `shards/modules/physics/body.cpp:159-169` + +### Description +When a `Physics.Body` shard is cleaned up, the body is disabled but never actually removed from the `PhysicsMirror` map. Over time in long-running applications, this causes unbounded memory growth. + +### Current Code +```cpp +void cleanup(SHContext *context) { + if (_instance.node && _instance.node->node) { + auto &bodyNode = _instance->node; + bodyNode->enabled = false; + bodyNode->persistence = false; + bodyNode->selfVar.reset(); + } + // Body remains in bodyMirror.map forever! +} +``` + +### Expected Behavior +The body should be removed from the mirror map when the shard is cleaned up. + +### Suggested Fix +Add a `removeNode()` call or equivalent cleanup mechanism that removes the body from `bodyMirror.map`. + +--- + +## Issue 2: Memory Leak - Constraints Never Removed From Mirror Map + +**Severity**: Critical +**File**: `shards/modules/physics/constraints.cpp:50-57` + +### Description +Same issue as bodies - constraints are disabled but never removed from the constraint mirror map. + +### Current Code +```cpp +void baseCleanup(SHContext *context) { + if (_constraint) { + _constraint->enabled = false; + _constraint->persistence = false; + _constraint.reset(); // shared_ptr released, but entry stays in map + } +} +``` + +--- + +## Issue 3: ApplyForceAt Has Incorrect Logic Operator + +**Severity**: High +**File**: `shards/modules/physics/physics.cpp:703-709` + +### Description +The `ApplyForceAt` shard uses `&&` instead of `||` when checking if force should be applied. This means a force like `(100, 0, 0)` is completely ignored because not ALL components are non-zero. + +### Current Code +```cpp +if (fpl[0] != 0.0 && fpl[1] != 0.0 && fpl[2] != 0.0) { // BUG: requires ALL non-zero + bodyInterface.AddForce(body->GetID(), fpl, at); +} +``` + +### Expected Behavior +Force should be applied if ANY component is non-zero. + +### Suggested Fix +```cpp +if (fpl[0] != 0.0 || fpl[1] != 0.0 || fpl[2] != 0.0) { + bodyInterface.AddForce(body->GetID(), fpl, at); +} +``` + +Or simply remove the check entirely since adding a zero force is harmless. + +--- + +## Issue 4: Typo `shasert` Instead of `shassert` + +**Severity**: Medium (potential compilation error) +**File**: `shards/modules/physics/core.hpp:198` + +### Description +Typo in assertion macro name will cause compilation failure if that code path is exercised. + +### Current Code +```cpp +shasert(node->data); // Should be shassert +``` + +### Fix +```cpp +shassert(node->data); +``` + +--- + +## Issue 5: Thread Buffer "Lazy GC" Can Cause Data Loss + +**Severity**: Medium-High +**File**: `shards/modules/physics/core.hpp:81-85` + +### Description +When more than 64 unique thread IDs have written to the event collector, ALL thread buffers are cleared. This could cause event loss if the clear happens mid-frame. + +### Current Code +```cpp +// TODO: Fix lazy GC +// technically okay since we schedule on the same thread pool anyways +if (threadBuffers.size() > 64) + threadBuffers.clear(); +``` + +### Suggested Fix +Implement proper cleanup of stale thread buffers rather than clearing all of them, or increase the limit and add monitoring. diff --git a/docs/physics-issues/P1-missing-features.md b/docs/physics-issues/P1-missing-features.md new file mode 100644 index 00000000000..e733c7217ae --- /dev/null +++ b/docs/physics-issues/P1-missing-features.md @@ -0,0 +1,123 @@ +# Missing Physics Features + +## Issue 1: No Raycasting or Shape Query Support + +**Severity**: High +**Component**: `shards/modules/physics/` + +### Description +The physics module has no shards for spatial queries, which are essential for most game physics use cases. + +### Missing Functionality +- `Physics.Raycast` - Cast a ray and get hit results +- `Physics.RaycastAll` - Cast a ray and get all hits +- `Physics.ShapeCast` - Sweep a shape along a path +- `Physics.CollideShape` - Check what a shape overlaps with +- `Physics.CollidePoint` - Check what bodies contain a point + +### Use Cases Blocked +- Line-of-sight checks +- Bullet/projectile hit detection +- Ground detection for characters +- Sensor/trigger areas without collision response +- Mouse picking in 3D + +### Reference +Jolt provides these via `PhysicsSystem::GetNarrowPhaseQuery()` with methods like `CastRay()`, `CastShape()`, `CollidePoint()`, etc. + +--- + +## Issue 2: Missing Common Shape Types + +**Severity**: Medium-High +**Component**: `shards/modules/physics/shapes.cpp` + +### Current Shapes +- BoxShape +- SphereShape +- CapsuleShape +- HullShape (convex hull from mesh) + +### Missing Shapes +- **CylinderShape** - Common for wheels, pillars, cans +- **MeshShape** - Concave static geometry (level collision) +- **HeightFieldShape** - Terrain collision +- **CompoundShape** - Multiple shapes per body +- **OffsetCenterOfMassShape** - Adjust center of mass +- **RotatedTranslatedShape** - Transform a shape +- **TaperedCapsuleShape** - Character controllers + +### Impact +Users cannot create terrain, complex static level geometry, or compound physics objects. + +--- + +## Issue 3: Missing Constraint Types + +**Severity**: Medium +**Component**: `shards/modules/physics/constraints.cpp` + +### Current Constraints +- FixedConstraint +- DistanceConstraint +- SliderConstraint + +### Missing Constraints +- **HingeConstraint** - Doors, wheels, rotating platforms +- **ConeConstraint** - Ragdoll shoulder/hip joints +- **PointConstraint** - Ball-socket joints +- **SwingTwistConstraint** - Advanced ragdoll joints +- **SixDOFConstraint** - Full 6 degrees of freedom control +- **PathConstraint** - Constrain to a path +- **GearConstraint** - Gear ratios between bodies +- **RackAndPinionConstraint** - Linear/rotational conversion +- **PulleyConstraint** - Pulleys and ropes + +### Impact +Cannot create proper ragdolls, vehicles with wheels, doors, or mechanical systems. + +--- + +## Issue 4: No Body Sleeping Support + +**Severity**: High (Performance) +**File**: `shards/modules/physics/core.hpp:338` + +### Description +Body sleeping is explicitly disabled with a TODO comment. This means every body in the scene is simulated every frame, even if completely stationary. + +### Current Code +```cpp +settings.mAllowSleeping = false; // TODO: Add a parameter to control this +``` + +### Impact +A scene with 1000 bodies will simulate all 1000 every frame. With sleeping enabled, stationary bodies are skipped, potentially reducing simulation cost by 90%+ in typical scenes. + +### Suggested Implementation +1. Add `AllowSleeping` parameter to `Physics.Body` (default: true) +2. Add `Physics.WakeUp` shard to manually wake bodies +3. Add `Physics.Sleep` shard to manually sleep bodies +4. Consider `Physics.SetSleepThreshold` for tuning + +--- + +## Issue 5: No Character Controller Support + +**Severity**: Medium +**Component**: `shards/modules/physics/` + +### Description +Jolt has a full character controller system (`CharacterVirtual`, `Character`) that handles: +- Ground detection +- Slope limits +- Step climbing +- Moving platform support + +This is not exposed in the Shards physics module. + +### Suggested Shards +- `Physics.CharacterController` - Create a character controller +- `Physics.CharacterMove` - Move the character with collision +- `Physics.CharacterIsGrounded` - Check if on ground +- `Physics.CharacterGetGroundNormal` - Get ground surface normal diff --git a/docs/physics-issues/P2-improvements.md b/docs/physics-issues/P2-improvements.md new file mode 100644 index 00000000000..c136fb1caf8 --- /dev/null +++ b/docs/physics-issues/P2-improvements.md @@ -0,0 +1,157 @@ +# Physics Module Improvements + +## Issue 1: Soft Body Parameter Change Detection Is Broken + +**Severity**: Medium +**Files**: `shards/modules/physics/physics.hpp:163-165`, `shards/modules/physics/soft_body.cpp:197-198` + +### Description +Soft body parameter hashing always returns 0, so parameter changes are never detected after initial creation. + +### Current Code in physics.hpp +```cpp +inline void BodyNode::updateParamHash0() { + if (shape.index() == 0) { + // ... regular body hashing + } else { + paramHash0 = 0; // Soft body: always 0! + } +} + +inline void BodyNode::updateParamHash1() { + if (shape.index() == 0) { + // ... regular body hashing + } else { + paramHash1 = 0; // Soft body: always 0! + } +} +``` + +### Impact +Changing friction, pressure, damping, etc. on soft bodies at runtime has no effect. + +### Suggested Fix +Implement proper hashing for soft body parameters similar to regular bodies. + +--- + +## Issue 2: Hardcoded Physics System Limits + +**Severity**: Low-Medium +**File**: `shards/modules/physics/core.hpp:499, 523` + +### Description +Physics system limits are hardcoded with no way to configure them. + +### Current Code +```cpp +JPH::TempAllocatorImpl tempAllocator{1024 * 1024 * 4}; // 4MB fixed + +physicsSystem.Init( + 1024 * 8, // 8192 max bodies + 0, // num body mutexes (0 = auto) + 1024, // max body pairs + 1024, // max contact constraints + ... +); +``` + +### Impact +- Large scenes may silently fail when exceeding 8K bodies +- Small scenes waste memory with 4MB temp allocator +- No way to tune for specific use cases + +### Suggested Fix +Add parameters to `Physics.Context`: +- `MaxBodies` (default: 8192) +- `MaxBodyPairs` (default: 1024) +- `MaxContactConstraints` (default: 1024) +- `TempAllocatorSize` (default: 4MB) + +--- + +## Issue 3: No Collision Event Filtering by Type + +**Severity**: Low +**File**: `shards/modules/physics/physics.cpp:168-293` + +### Description +`Physics.Collisions` returns all collision events but there's no way to filter by event type (ContactAdded vs ContactPersisted). + +### Suggested Enhancement +Add optional `EventType` parameter: +- `All` (default) - Both added and persisted +- `Enter` - Only ContactAdded (new collisions) +- `Stay` - Only ContactPersisted (ongoing collisions) + +--- + +## Issue 4: Missing Physics.IsValid or Physics.Exists Check + +**Severity**: Low +**Component**: `shards/modules/physics/` + +### Description +No way to check if a physics body reference is still valid before using it. + +### Use Case +```shards +; Store body reference +@body = (Physics.Body ...) + +; Later, check if still valid before use +@body | Physics.IsValid | If({ + @body | Physics.ApplyForce ... +}) +``` + +--- + +## Issue 5: Factory Instance Memory Leak + +**Severity**: Very Low +**File**: `shards/modules/physics/physics.cpp:745` + +### Description +The Jolt Factory singleton is allocated but never freed. + +### Current Code +```cpp +SHARDS_REGISTER_FN(physics) { + JPH::Factory::sInstance = new JPH::Factory(); // Never deleted + // ... +} +``` + +### Suggested Fix +Register cleanup in module unload, or use a static instance instead of heap allocation. + +--- + +## Issue 6: Debug Draw Help Text Is Empty + +**Severity**: Very Low +**File**: `shards/modules/physics/debug.cpp:135` + +### Current Code +```cpp +static SHOptionalString help() { return SHCCSTR(""); } // Empty! +``` + +### Suggested Fix +Add proper help text describing the debug visualization capabilities. + +--- + +## Issue 7: SoftBodyShape Help Text Is Empty + +**Severity**: Very Low +**File**: `shards/modules/physics/soft_body.cpp:397` + +### Current Code +```cpp +static SHOptionalString help() { return SHCCSTR(""); } +``` + +### Suggested Fix +Add description: "Creates a soft body collision shape from a mesh. Used with Physics.SoftBody for cloth, rubber, and deformable objects."