Fix the memory leak issue of the physics #2648
Conversation
WalkthroughThis update centralizes the management of the native physics engine by moving the static Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Engine
participant PhysicsScene
participant NativePhysics
App->>Engine: Initialize()
Engine->>NativePhysics: Assign to Engine._nativePhysics
Engine->>PhysicsScene: Create PhysicsScene with Engine._nativePhysics
PhysicsScene->>NativePhysics: Create native physics scene
App->>Scene: Destroy()
Scene->>PhysicsScene: _destroy()
PhysicsScene->>NativePhysics: destroy()
PhysicsScene->>Engine: Cleanup references
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a memory leak issue in the physics engine by updating native physics resource management and ensuring proper cleanup of simulation callbacks and scenes. Key changes include:
- Replacing direct accesses to PhysicsScene._nativePhysics with Engine._nativePhysics for improved resource management.
- Adding explicit deletion of simulation callback and native scene resources in PhysXPhysicsScene.
- Implementing destroy methods for LitePhysics and updating API definitions in design interfaces.
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/physics-physx/src/PhysXPhysicsScene.ts | Added deletion and proper management of simulation callbacks and native scene resources. |
| packages/physics-physx/src/PhysXPhysics.ts | Updated native script URLs and changed native physics reference. |
| packages/physics-lite/src/LitePhysicsScene.ts, LitePhysics.ts | Introduced empty destroy() methods to satisfy updated interface contracts. |
| packages/design/src/physics/IPhysicsScene.ts, IPhysics.ts | Added destroy() methods to the physics interfaces. |
| packages/core/src/physics/* | Updated native physics calls to use Engine._nativePhysics. |
| packages/core/src/Scene.ts, Engine.ts | Adjusted cleanup routines to correctly reset native physics references. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2648 +/- ##
==========================================
+ Coverage 68.90% 68.92% +0.02%
==========================================
Files 961 961
Lines 100604 100691 +87
Branches 8715 8730 +15
==========================================
+ Hits 69322 69403 +81
- Misses 31022 31028 +6
Partials 260 260
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/physics-lite/src/LitePhysics.ts (1)
152-155: Consider adding comment explaining empty implementationWhile the empty implementation of destroy() is likely appropriate for this lite physics implementation, it would be helpful to add a comment explaining that this is intentional (e.g., "No resources to clean up in lite implementation").
/** * {@inheritDoc IPhysics.destroy } */ - destroy(): void {} + destroy(): void { + // No resources to clean up in lite implementation + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
packages/core/src/Engine.ts(3 hunks)packages/core/src/Scene.ts(1 hunks)packages/core/src/physics/CharacterController.ts(2 hunks)packages/core/src/physics/DynamicCollider.ts(2 hunks)packages/core/src/physics/PhysicsMaterial.ts(2 hunks)packages/core/src/physics/PhysicsScene.ts(2 hunks)packages/core/src/physics/StaticCollider.ts(2 hunks)packages/core/src/physics/joint/FixedJoint.ts(2 hunks)packages/core/src/physics/joint/HingeJoint.ts(2 hunks)packages/core/src/physics/joint/SpringJoint.ts(2 hunks)packages/core/src/physics/shape/BoxColliderShape.ts(2 hunks)packages/core/src/physics/shape/CapsuleColliderShape.ts(2 hunks)packages/core/src/physics/shape/PlaneColliderShape.ts(2 hunks)packages/core/src/physics/shape/SphereColliderShape.ts(2 hunks)packages/design/src/physics/IPhysics.ts(1 hunks)packages/design/src/physics/IPhysicsScene.ts(1 hunks)packages/physics-lite/src/LitePhysics.ts(1 hunks)packages/physics-lite/src/LitePhysicsScene.ts(1 hunks)packages/physics-physx/src/PhysXPhysics.ts(1 hunks)packages/physics-physx/src/PhysXPhysicsScene.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
packages/core/src/physics/StaticCollider.ts (1)
packages/core/src/Engine.ts (1)
Engine(52-692)
packages/core/src/physics/shape/PlaneColliderShape.ts (1)
packages/core/src/Engine.ts (1)
Engine(52-692)
packages/core/src/physics/shape/CapsuleColliderShape.ts (1)
packages/core/src/Engine.ts (1)
Engine(52-692)
packages/core/src/physics/joint/HingeJoint.ts (1)
packages/core/src/Engine.ts (1)
Engine(52-692)
packages/core/src/physics/joint/SpringJoint.ts (1)
packages/core/src/Engine.ts (1)
Engine(52-692)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: codecov
- GitHub Check: e2e (22.x)
🔇 Additional comments (30)
packages/core/src/physics/StaticCollider.ts (1)
1-1: LGTM - Physics reference centralization looks goodThe change shifts the reference from
PhysicsScene._nativePhysicstoEngine._nativePhysics, which aligns with the broader refactoring effort to centralize physics management. This is a good architectural change that will help address the memory leak issue by improving lifecycle management.Also applies to: 16-16
packages/design/src/physics/IPhysics.ts (1)
137-141: Essential addition for proper resource cleanupAdding the
destroy()method to theIPhysicsinterface is a critical part of fixing the memory leak. This ensures that all implementations provide a way to properly clean up physics resources when they're no longer needed.packages/core/src/physics/joint/SpringJoint.ts (1)
2-2: LGTM - Consistent with physics reference centralizationThe change to use
Engine._nativePhysicsinstead ofPhysicsScene._nativePhysicsis consistent with the other changes and helps centralize physics resource management in the Engine class. This is part of the architectural improvement to address the memory leak.Also applies to: 89-89
packages/core/src/physics/shape/CapsuleColliderShape.ts (1)
3-3: LGTM - Physics reference centralization properly implementedThe change from
PhysicsScene._nativePhysicstoEngine._nativePhysicsfollows the same pattern as the other files. This consistent implementation across all physics-related classes ensures that the memory leak fix is comprehensive.Also applies to: 58-58
packages/design/src/physics/IPhysicsScene.ts (1)
60-64: Appropriate interface extension for resource management.Adding the
destroy()method to theIPhysicsSceneinterface establishes a standardized way for physics scenes to clean up their resources, which directly addresses the memory leak issue mentioned in the PR.packages/core/src/physics/PhysicsMaterial.ts (2)
2-2: LGTM: Import change aligns with refactoring.Replacing the PhysicsScene import with Engine import is consistent with the centralization of physics resources management.
20-20: LGTM: Centralized physics reference.Using
Engine._nativePhysicsinstead ofPhysicsScene._nativePhysicsproperly centralizes the physics interface reference, which helps address the memory leak issue by ensuring consistent resource management.packages/core/src/physics/joint/HingeJoint.ts (2)
3-3: LGTM: Import change aligns with refactoring.Replacing the PhysicsScene import with Engine import is consistent with the centralization of physics resources management.
163-163: LGTM: Centralized physics reference.Using
Engine._nativePhysicsinstead ofPhysicsScene._nativePhysicsproperly centralizes the physics interface reference, which helps address the memory leak issue by ensuring consistent resource management.packages/core/src/physics/shape/PlaneColliderShape.ts (2)
2-2: LGTM: Import change aligns with refactoring.Replacing the PhysicsScene import with Engine import is consistent with the centralization of physics resources management.
11-11: LGTM: Centralized physics reference.Using
Engine._nativePhysicsinstead ofPhysicsScene._nativePhysicsproperly centralizes the physics interface reference, which helps address the memory leak issue by ensuring consistent resource management.packages/core/src/physics/DynamicCollider.ts (1)
3-3: Import and usage of Engine._nativePhysics looks goodThe change from PhysicsScene to Engine for accessing the native physics interface is consistent with the PR objective of addressing memory leaks by centralizing physics resource management.
Also applies to: 309-312
packages/core/src/physics/shape/SphereColliderShape.ts (1)
3-3: Properly updated to use Engine._nativePhysicsThe change correctly aligns this component with the new centralized physics resource management approach.
Also applies to: 27-31
packages/core/src/physics/shape/BoxColliderShape.ts (1)
4-4: Import and usage of Engine._nativePhysics looks goodThe change correctly aligns this component with the centralized physics resource management pattern.
Also applies to: 29-33
packages/core/src/physics/joint/FixedJoint.ts (2)
1-1: Updated import to use Engine instead of PhysicsSceneThis change is part of the memory leak fix by centralizing the native physics interface in the Engine class instead of PhysicsScene.
12-12: Changed physics interface reference sourceThe code now references
Engine._nativePhysicsinstead ofPhysicsScene._nativePhysics, which centralizes the native physics interface management at the engine level.packages/core/src/physics/CharacterController.ts (2)
3-3: Updated import to use Engine instead of PhysicsSceneThis change is part of the memory leak fix by centralizing the native physics interface in the Engine class instead of PhysicsScene.
83-83: Changed physics interface reference sourceThe code now references
Engine._nativePhysicsinstead ofPhysicsScene._nativePhysics, which centralizes the native physics interface management at the engine level.packages/physics-physx/src/PhysXPhysics.ts (2)
93-93: Updated URL for PhysX JavaScript runtimeThe source URL for loading the PhysX JavaScript runtime has been updated to a new location at alipayobjects.com.
95-95: Updated URL for PhysX WebAssembly runtimeThe source URL for loading the PhysX WebAssembly runtime has been updated to a new location at alipayobjects.com.
packages/physics-lite/src/LitePhysicsScene.ts (1)
150-153: Good implementation of the destroy method.The added empty
destroy()method appropriately implements theIPhysicsManager.destroyinterface. Since this is a lightweight physics implementation without any resource allocations that need cleanup, an empty implementation is appropriate.packages/core/src/Engine.ts (3)
61-62: Good refactoring - centralizing physics access.Moving the native physics reference to a static property on the Engine class is a good architectural decision. This centralization creates a single point of access to the physics system.
626-627: Proper initialization of the physics reference.Properly assigns the physics instance to the newly added static property. This change complements the architectural refactoring.
509-510: Good cleanup - preventing memory leaks.Explicitly setting
_nativePhysicsManagerto null during engine destruction helps prevent memory leaks by allowing the garbage collector to reclaim resources.packages/core/src/physics/PhysicsScene.ts (2)
219-219: Good update to use centralized physics reference.Updated to use
Engine._nativePhysicsinstead of the previous static reference. This aligns with the architectural change of centralizing physics access in the Engine class.
445-453: Excellent addition of resource cleanup.Added a proper destroy method that:
- Checks if the native physics scene exists before cleanup
- Calls the native destroy method to release resources
- Nullifies the reference to allow garbage collection
This is an essential improvement for preventing memory leaks when scenes are destroyed.
packages/physics-physx/src/PhysXPhysicsScene.ts (4)
24-24: Good improvement - storing the callback instance.Added a private member to store the PhysX simulation callback instance, which allows proper cleanup later. This is an important change for memory management.
94-101: Good enhancement - proper resource management.The PhysX simulation callback is now stored in a class member for later cleanup, and the scene description object is explicitly deleted after use. These changes help prevent memory leaks.
199-210: Great improvement in callback management.Properly manages the lifecycle of the PhysX query filter callback by:
- Creating it explicitly outside the raycast call
- Passing it to the raycast function
- Explicitly deleting it after use
This pattern prevents memory leaks by ensuring proper cleanup of native resources.
222-230: Excellent addition of comprehensive resource cleanup.Added a complete destroy method that:
- Releases the PhysX scene through proper API calls
- Deletes the native scene object
- Deletes the simulation callback instance
- Nullifies the reference to prevent further access
This is a crucial improvement for preventing memory leaks when PhysX resources are no longer needed.
|
|
||
| this.physics._destroy(); |
There was a problem hiding this comment.
Added explicit physics destruction to fix memory leak
This is the critical change that fixes the memory leak issue. The scene now explicitly calls physics._destroy() during scene destruction, which properly cleans up physics resources that were previously leaking.
packages/core/src/Engine.ts
Outdated
| @@ -505,7 +507,7 @@ export class Engine extends EventDispatcher { | |||
| this._hardwareRenderer.destroy(); | |||
|
|
|||
| this.removeAllEventListeners(); | |||
|
|
|||
| this._nativePhysicsManager = null; | |||
| if (this._nativePhysicsScene) { | ||
| this._nativePhysicsScene.destroy(); | ||
| this._nativePhysicsScene = null; | ||
| } |
There was a problem hiding this comment.
Only this._nativePhysicsScene.destroy(); is OK!
| @@ -147,6 +147,11 @@ export class LitePhysicsScene implements IPhysicsScene { | |||
| throw "Physics-lite don't support removeCharacterController. Use Physics-PhysX instead!"; | |||
| } | |||
|
|
|||
| /** | |||
| * {@inheritDoc IPhysicsManager.destroy } | |||
| this._pxScene.release(); | ||
| this._pxScene.delete(); | ||
| this._physXSimulationCallbackInstance.delete(); | ||
| this._physXSimulationCallbackInstance = null; |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
link: #2647
Summary by CodeRabbit
New Features
Bug Fixes
Documentation