-
Notifications
You must be signed in to change notification settings - Fork 17
Review physics module code quality #1223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
sinkingsugar
wants to merge
1
commit into
devel
Choose a base branch
from
claude/review-physics-module-01ADaYVyU3UprHhWyGogZufk
base: devel
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name in the code example is inconsistent. Line 14 checks
if (_instance.node && _instance.node->node)but line 15 declaresauto &bodyNode = _instance->node;. This should beauto &bodyNode = _instance.node;to match the condition check.