Support custom physX url #2672
Support custom physX url #2672
Conversation
WalkthroughA new end-to-end test for PhysX physics with a custom library URL was added, including custom scripts for entity movement and collision response. The test configuration was updated to include this case. The PhysX physics engine implementation was enhanced to accept custom URLs for loading its runtime scripts, with a new interface for URL options and updated constructor logic. Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant Engine
participant PhysXPhysics
participant ScriptLoader
TestRunner->>Engine: Initialize with PhysXPhysics(runtimeMode, urlOptions)
Engine->>PhysXPhysics: Construct (runtimeMode, urlOptions)
PhysXPhysics->>ScriptLoader: Load PhysX script from custom URL (if provided)
ScriptLoader-->>PhysXPhysics: PhysX runtime loaded
PhysXPhysics-->>Engine: Initialization complete
Engine->>TestRunner: Scene ready for testing
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ 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.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/physics-physx/src/PhysXPhysics.ts (1)
54-69: Consider adding URL validation for security.Loading scripts from custom URLs could introduce security risks. Consider adding validation to ensure URLs are from trusted sources.
constructor(runtimeMode: PhysXRuntimeMode = PhysXRuntimeMode.Auto, urlOptions?: IPhysXUrlOptions) { this._runTimeMode = runtimeMode; + // Validate URLs to ensure they're from trusted domains + const validateUrl = (url: string | undefined, defaultUrl: string): string => { + if (!url) return defaultUrl; + // Add validation logic here, e.g., check for allowed domains + return url; + }; + + const defaultPhysXUrl = "https://mdn.alipayobjects.com/rms/afts/file/A*nL1PSrCPoZ0AAAAAAAAAAAAAARQnAQ/physx.release.js"; + const defaultPhysXDowngradeUrl = "https://mdn.alipayobjects.com/rms/afts/file/A*V4pqRqM65UMAAAAAAAAAAAAAARQnAQ/physx.release.downgrade.js"; + + this._physXUrl = validateUrl(urlOptions?.physXUrl, defaultPhysXUrl); + this._physXDowngradeUrl = validateUrl(urlOptions?.physXDowngradeUrl, defaultPhysXDowngradeUrl); - this._physXUrl = - urlOptions?.physXUrl ?? - "https://mdn.alipayobjects.com/rms/afts/file/A*nL1PSrCPoZ0AAAAAAAAAAAAAARQnAQ/physx.release.js"; - this._physXDowngradeUrl = - urlOptions?.physXDowngradeUrl ?? - "https://mdn.alipayobjects.com/rms/afts/file/A*V4pqRqM65UMAAAAAAAAAAAAAARQnAQ/physx.release.downgrade.js"; }e2e/case/physx-customUrl.ts (3)
22-23: Fix formatting issues.There are some formatting issues flagged by static analysis.
- Entity, + Entity } from "@galacean/engine"; -WebGLEngine.create({ canvas: "canvas", physics: new PhysXPhysics(PhysXRuntimeMode.Auto, { - physXUrl: "../physx.release.js" -}) }).then((engine) => { +WebGLEngine.create({ + canvas: "canvas", + physics: new PhysXPhysics(PhysXRuntimeMode.Auto, { + physXUrl: "../physx.release.js" + }) +}).then((engine) => {Also applies to: 59-61
🧰 Tools
🪛 ESLint
[error] 22-22: Delete
,(prettier/prettier)
112-127: Use constants for magic numbers in MoveScript.The script has hard-coded values that would be more maintainable as named constants.
class MoveScript extends Script { pos: number = -5; vel: number = 0.05; velSign: number = -1; + private readonly MAX_POS = 5; + private readonly MIN_POS = -15; onPhysicsUpdate() { - if (this.pos >= 5) { + if (this.pos >= this.MAX_POS) { this.velSign = -1; } - if (this.pos <= -15) { + if (this.pos <= this.MIN_POS) { this.velSign = 1; } this.pos += this.vel * this.velSign; this.entity.transform.worldPosition.set(this.pos, 0, 0); } }
88-94: Use initialization helper for better readability.The material setup could be made more concise with a helper function.
- const sphereMtl = new PBRMaterial(engine); + // Helper function to create and configure a material + function createPBRMaterial(engine, color) { + const material = new PBRMaterial(engine); + material.baseColor.set( + typeof color === 'object' ? color.r : color, + typeof color === 'object' ? color.g : color, + typeof color === 'object' ? color.b : color, + 1.0 + ); + material.metallic = 0.0; + material.roughness = 0.5; + return material; + } + + const sphereMtl = createPBRMaterial(engine, Math.random()); const sphereRenderer = sphereEntity.addComponent(MeshRenderer); - sphereMtl.baseColor.set(Math.random(), Math.random(), Math.random(), 1.0); - sphereMtl.metallic = 0.0; - sphereMtl.roughness = 0.5; sphereRenderer.mesh = PrimitiveMesh.createSphere(engine, radius); sphereRenderer.setMaterial(sphereMtl);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (2)
e2e/.dev/physx.release.wasmis excluded by!**/*.wasme2e/fixtures/originImage/Physics_physx-customUrl.jpgis excluded by!**/*.jpg
📒 Files selected for processing (3)
e2e/case/physx-customUrl.ts(1 hunks)e2e/config.ts(1 hunks)packages/physics-physx/src/PhysXPhysics.ts(3 hunks)
🧰 Additional context used
🪛 ESLint
e2e/case/physx-customUrl.ts
[error] 22-22: Delete ,
(prettier/prettier)
[error] 59-59: Replace ·canvas:·"canvas", with ⏎··canvas:·"canvas",⏎·
(prettier/prettier)
[error] 60-60: Replace ·· with ····
(prettier/prettier)
[error] 61-61: Replace })· with ··})⏎
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: codecov
- GitHub Check: build (22.x, windows-latest)
- GitHub Check: e2e (22.x)
🔇 Additional comments (6)
packages/physics-physx/src/PhysXPhysics.ts (4)
54-55: Nicely implemented custom URL support!The private members and constructor parameter handling with nullish coalescing is a clean approach to enable custom URLs while maintaining defaults.
Also applies to: 62-69
57-61: Good documentation for the new parameter.The JSDoc comments clearly explain the purpose of the new
urlOptions< 8000 /code> parameter.
110-113: Implementation correctly uses the custom URLs.The script loading logic now properly references the class variables instead of hardcoded strings.
306-311: Well-defined interface for URL options.The
IPhysXUrlOptionsinterface clearly specifies the expected properties and includes helpful JSDoc comments.e2e/config.ts (1)
228-232: Test configuration properly added.The new test case is correctly registered in the E2E test configuration following the existing pattern.
e2e/case/physx-customUrl.ts (1)
59-61:❓ Verification inconclusive
Test demonstrates the custom URL feature well.
The test properly initializes PhysX with a custom URL, showcasing the new functionality.
🏁 Script executed:
#!/bin/bash # Check if the custom URL path is correct in the context of the test environment fd "physx.release.js" --type fileLength of output: 78
Verify the custom PhysX URL path
The test demonstrates the custom URL feature well, but the specified path (
../physx.release.js) must actually resolve topackages/physics-physx/libs/physx.release.jsin the e2e test server. Please confirm that your test environment serves that file at the given relative URL. If not, adjust thephysXUrl(for example to../../packages/physics-physx/libs/physx.release.js) or update your static‐file configuration so the file is reachable.🧰 Tools
🪛 ESLint
[error] 59-59: Replace
·canvas:·"canvas",with⏎··canvas:·"canvas",⏎·(prettier/prettier)
[error] 60-60: Replace
··with····(prettier/prettier)
[error] 61-61: Replace
})·with··})⏎(prettier/prettier)
| class CollisionScript extends Script { | ||
| onTriggerExit(other: ColliderShape) { | ||
| (<BlinnPhongMaterial>sphereRenderer.getMaterial()).baseColor.set(1, 1, 1, 1.0); | ||
| } | ||
|
|
||
| onTriggerEnter(other: ColliderShape) { | ||
| (sphereRenderer.getMaterial() as BlinnPhongMaterial).baseColor = ( | ||
| other.collider.entity.getComponent(MeshRenderer)?.getMaterial() as PBRMaterial | ||
| ).baseColor; | ||
| } | ||
|
|
||
| onTriggerStay(other: ColliderShape) {} | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent material types and empty method.
There's a mismatch in material types and an empty method that could be improved.
class CollisionScript extends Script {
onTriggerExit(other: ColliderShape) {
- (<BlinnPhongMaterial>sphereRenderer.getMaterial()).baseColor.set(1, 1, 1, 1.0);
+ const material = sphereRenderer.getMaterial() as PBRMaterial;
+ material.baseColor.set(1, 1, 1, 1.0);
}
onTriggerEnter(other: ColliderShape) {
- (sphereRenderer.getMaterial() as BlinnPhongMaterial).baseColor = (
- other.collider.entity.getComponent(MeshRenderer)?.getMaterial() as PBRMaterial
- ).baseColor;
+ const sphereMaterial = sphereRenderer.getMaterial() as PBRMaterial;
+ const otherRenderer = other.collider.entity.getComponent(MeshRenderer);
+ if (otherRenderer) {
+ const otherMaterial = otherRenderer.getMaterial() as PBRMaterial;
+ sphereMaterial.baseColor.copyFrom(otherMaterial.baseColor);
+ }
}
- onTriggerStay(other: ColliderShape) {}
+ // No need for empty method, can be removed if not required
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class CollisionScript extends Script { | |
| onTriggerExit(other: ColliderShape) { | |
| (<BlinnPhongMaterial>sphereRenderer.getMaterial()).baseColor.set(1, 1, 1, 1.0); | |
| } | |
| onTriggerEnter(other: ColliderShape) { | |
| (sphereRenderer.getMaterial() as BlinnPhongMaterial).baseColor = ( | |
| other.collider.entity.getComponent(MeshRenderer)?.getMaterial() as PBRMaterial | |
| ).baseColor; | |
| } | |
| onTriggerStay(other: ColliderShape) {} | |
| } | |
| class CollisionScript extends Script { | |
| onTriggerExit(other: ColliderShape) { | |
| - (<BlinnPhongMaterial>sphereRenderer.getMaterial()).baseColor.set(1, 1, 1, 1.0); | |
| + const material = sphereRenderer.getMaterial() as PBRMaterial; | |
| + material.baseColor.set(1, 1, 1, 1.0); | |
| } | |
| onTriggerEnter(other: ColliderShape) { | |
| - (sphereRenderer.getMaterial() as BlinnPhongMaterial).baseColor = ( | |
| - other.collider.entity.getComponent(MeshRenderer)?.getMaterial() as PBRMaterial | |
| - ).baseColor; | |
| + const sphereMaterial = sphereRenderer.getMaterial() as PBRMaterial; | |
| + const otherRenderer = other.collider.entity.getComponent(MeshRenderer); | |
| + if (otherRenderer) { | |
| + const otherMaterial = otherRenderer.getMaterial() as PBRMaterial; | |
| + sphereMaterial.baseColor.copyFrom(otherMaterial.baseColor); | |
| + } | |
| } | |
| - onTriggerStay(other: ColliderShape) {} | |
| + // No need for empty method, can be removed if not required | |
| } |
🤖 Prompt for AI Agents
In e2e/case/physx-customUrl.ts around lines 130 to 142, the onTriggerEnter
method assigns baseColor directly which may cause type inconsistencies between
BlinnPhongMaterial and PBRMaterial. Refactor to consistently use the same
material type or properly convert colors to avoid type mismatch. Also, remove or
implement the empty onTriggerStay method to avoid unused code.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.5 #2672 +/- ##
===========================================
- Coverage 68.97% 68.65% -0.33%
===========================================
Files 975 976 +1
Lines 102785 102914 +129
Branches 8906 8911 +5
===========================================
- Hits 70895 70653 -242
- Misses 31621 31989 +368
- Partials 269 272 +3
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:
|
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
New Features
Chores