Fix additive blend mode bug when canvas is transparency #2659
Conversation
WalkthroughThe alpha blending configuration for the Additive blend mode in the Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant Engine
participant Scene
participant Material
participant RenderTarget
TestRunner->>Engine: Initialize WebGL engine (non-alpha canvas)
Engine->>Scene: Create main scene with semi-transparent background
Scene->>Material: Create additive transparent material
Scene->>Material: Create default unlit material
Scene->>RenderTarget: Create offscreen render texture (512x512)
Scene->>RenderTarget: Render scene to offscreen texture
TestRunner->>Scene: Create second scene for transparency simulation
Scene->>Material: Create custom unlit material with premultiplied alpha blending
Scene->>RenderTarget: Use offscreen texture as base texture
Scene->>RenderTarget: Render final scene simulating transparent canvas
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ 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: 7
🔭 Outside diff range comments (2)
packages/physics-physx/src/PhysXPhysicsScene.ts (1)
64-72:⚠️ Potential issueOptional callbacks must be null-checked
_onContactEnter,_onContactExit, and_onContactStayare declared optional (?:) but are invoked directly inside the simulation callback.
If a user suppliesundefined, the engine will throw at the first contact event.- onContactBegin: (collision) => { - this._onContactEnter(collision); - }, + onContactBegin: (collision) => { + this._onContactEnter?.(collision); + }, - onContactEnd: (collision) => { - this._onContactExit(collision); - }, + onContactEnd: (collision) => { + this._onContactExit?.(collision); + }, - onContactPersist: (collision) => { - this._onContactStay(collision); - }, + onContactPersist: (collision) => { + this._onContactStay?.(collision); + },packages/loader/src/ktx2/KTX2Loader.ts (1)
86-93: 🛠️ Refactor suggestionValidate the compatibility of
restoredTextureWhen
restoredTextureis provided, the code assumes:
- face count (1 vs 6),
- dimensions,
- and GPU format
still match the current transcode result.
If the same URL is repointed or the capability check now yields a different target format, the old instance becomes invalid and subsequentsetPixelBuffercalls may assert or upload corrupt data.Add a guard such as:
if ( restoredTexture && (faceCount === 6) !== restoredTexture.isCube || restoredTexture.width !== width || restoredTexture.height !== height ) { restoredTexture.destroy(); restoredTexture = undefined; }
🧹 Nitpick comments (7)
tests/src/core/DeviceLost.test.ts (2)
41-42: Translate the Chinese comment for better documentation.- // 注意：以下部分文件可能会版本迭代而解析报错，若出现此情况，只需更新对应版本的编辑器资产文件即可 + // Note: Some of the following files may have parsing errors due to version iterations. If this occurs, simply update the corresponding version of the editor asset files.
63-77: Improved device loss simulation with event handling.The test now uses a more realistic approach by:
- Listening for
devicelostanddevicerestoredevents- Using internal engine methods to simulate the device loss/restore cycle
- Resolving the promise after restoration is complete
This better simulates real-world device loss scenarios.
Use double quotes for string literals to maintain consistency with the codebase style:
- console.log('new Promise'); + console.log("new Promise");- console.log('context lost'); + console.log("context lost");- console.log('context restored'); + console.log("context restored");🧰 Tools
🪛 ESLint
[error] 64-64: Replace
'new·Promise'with"new·Promise"(prettier/prettier)
[error] 66-66: Replace
'context·lost'with"context·lost"(prettier/prettier)
[error] 68-68: Replace
'context·restored'with"context·restored"(prettier/prettier)
[error] 77-77: Insert
;(prettier/prettier)
packages/loader/src/KTXLoader.ts (3)
17-20: Avoid leaking loader-specific fields intofetchoptions
requestConfigis created by blindly spreadingLoadItem.
LoadItemusually carries keys (e.g.assetType,params) that are not part ofRequestInitand will be forwarded to the internal_request/fetchcall. Most browsers ignore unknown keys, but the type-cast hides potential mis-use and linters/TS won’t help.Consider filtering only fetch–related properties before the spread, e.g.
-const requestConfig = <RequestConfig>{ - ...item, - type: "arraybuffer" -}; +const { url, params, ...requestInit } = item; // strip loader-only fields +const requestConfig: RequestConfig = { + ...requestInit, + type: "arraybuffer" +};
24-24: Reliance on private._request()APICalling the underscore-prefixed
_requestbreaks the public/private barrier and risks future breakage if the resource manager internals change.
If no public alternative exists, consider exposing a vetted helper onResourceManagerrather than inlining private usage here and in every loader.
45-74: Content restoration only rewrites pixel data
restoreContent()repopulates the mipmaps but does not:
- verify the texture is still the correct size/format,
- restore sampler state (
wrapMode*,filterMode,anisoLevel), or- regenerate mip-maps if
mipmap === trueand the driver discarded them.If GPU loss occurs these states may be reset. Re-applying them (or at least validating dimensions/format) makes the restorer more robust.
You could, for example:
resource.width !== mipmaps[0].width && resource.resize(...); resource.height !== mipmaps[0].height && ... resource.generateMipmaps?.();packages/loader/src/ktx2/KTX2Loader.ts (2)
227-235: Isolate fetch config from loader meta dataSame concern as in
KTXLoader: passing the entireLoadItemtoRequestInitinjects non-standard keys (params,assetType…) into the fetch options.-const requestConfig = <RequestConfig>{ - ...item, - type: "arraybuffer" -}; +const { params, url, ...requestInit } = item; +const requestConfig: RequestConfig = { + ...requestInit, + type: "arraybuffer" +};
260-275: Unknown keyparamsforwarded to_request
requestConfigis later reused insideKTX2ContentRestorer.restoreContent().
Because it still containsparams, the same non-standard property is sent to_requestagain.Either strip
paramsbefore cachingrequestConfigor store it separately to avoid polluting each subsequent network request.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
packages/core/src/Engine.ts(2 hunks)packages/core/src/Polyfill.ts(2 hunks)packages/core/src/Scene.ts(1 hunks)packages/core/src/Utils.ts(1 hunks)packages/core/src/material/BaseMaterial.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/loader/src/EnvLoader.ts(2 hunks)packages/loader/src/HDRLoader.ts(2 hunks)packages/loader/src/KTXLoader.ts(4 hunks)packages/loader/src/MeshLoader.ts(1 hunks)packages/loader/src/ktx2/KTX2Loader.ts(4 hunks)packages/loader/src/resource-deserialize/index.ts(1 hunks)packages/loader/src/resource-deserialize/resources/mesh/MeshDecoder.ts(2 hunks)packages/loader/src/resource-deserialize/resources/scene/EditorTextureLoader.ts(1 hunks)packages/loader/src/resource-deserialize/resources/texture2D/TextureDecoder.ts(2 hunks)packages/loader/src/resource-deserialize/utils/Decorator.ts(1 hunks)packages/physics-lite/src/LitePhysics.ts(1 hunks)packages/physics-lite/src/LitePhysicsScene.ts(5 hunks)packages/physics-physx/src/PhysXCharacterController.ts(1 hunks)packages/physics-physx/src/PhysXPhysics.ts(4 hunks)packages/physics-physx/src/PhysXPhysicsScene.ts(10 hunks)packages/physics-physx/src/joint/PhysXJoint.ts(0 hunks)packages/physics-physx/src/shape/PhysXColliderShape.ts(2 hunks)tests/src/core/DeviceLost.test.ts(2 hunks)tests/src/core/Polyfill.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/physics-physx/src/joint/PhysXJoint.ts
🧰 Additional context used
🧬 Code Graph Analysis (14)
packages/core/src/physics/joint/SpringJoint.ts (1)
packages/core/src/Engine.ts (1)
Engine(52-696)
packages/core/src/physics/shape/PlaneColliderShape.ts (1)
packages/core/src/Engine.ts (1)
Engine(52-696)
packages/core/src/physics/joint/HingeJoint.ts (1)
packages/core/src/Engine.ts (1)
Engine(52-696)
packages/core/src/physics/CharacterController.ts (2)
packages/design/src/physics/ICharacterController.ts (1)
ICharacterController(7-51)packages/core/src/Engine.ts (1)
Engine(52-696)
packages/core/src/physics/DynamicCollider.ts (1)
packages/core/src/Engine.ts (1)
Engine(52-696)
packages/core/src/Engine.ts (1)
packages/design/src/physics/IPhysics.ts (1)
IPhysics(16-142)
packages/loader/src/resource-deserialize/utils/Decorator.ts (2)
packages/loader/src/resource-deserialize/utils/BufferReader.ts (1)
BufferReader(2-144)packages/core/src/asset/AssetPromise.ts (1)
AssetPromise(4-234)
packages/core/src/physics/shape/CapsuleColliderShape.ts (1)
packages/core/src/Engine.ts (1)
Engine(52-696)
packages/loader/src/EnvLoader.ts (2)
packages/core/src/Engine.ts (1)
resourceManager(170-172)packages/core/src/asset/AssetPromise.ts (1)
resolve(25-35)
tests/src/core/DeviceLost.test.ts (4)
packages/core/src/Engine.ts (1)
resourceManager(170-172)packages/core/src/index.ts (1)
AssetType(29-29)packages/core/src/texture/TextureCube.ts (1)
TextureCube(12-199)packages/core/src/texture/Texture2D.ts (1)
Texture2D(12-185)
packages/loader/src/HDRLoader.ts (1)
packages/core/src/Engine.ts (1)
resourceManager(170-172)
packages/loader/src/resource-deserialize/resources/texture2D/TextureDecoder.ts (3)
packages/loader/src/resource-deserialize/utils/BufferReader.ts (1)
BufferReader(2-144)packages/core/src/texture/Texture2D.ts (1)
Texture2D(12-185)packages/core/src/asset/AssetPromise.ts (1)
AssetPromise(4-234)
packages/loader/src/resource-deserialize/resources/mesh/MeshDecoder.ts (3)
packages/loader/src/resource-deserialize/utils/BufferReader.ts (1)
BufferReader(2-144)packages/core/src/mesh/ModelMesh.ts (1)
ModelMesh(20-1333)packages/core/src/asset/AssetPromise.ts (2)
AssetPromise(4-234)resolve(25-35)
packages/loader/src/ktx2/KTX2Loader.ts (7)
packages/core/src/texture/Texture2D.ts (1)
Texture2D(12-185)packages/core/src/texture/TextureCube.ts (1)
TextureCube(12-199)packages/core/src/asset/request.ts (1)
RequestConfig(25-30)packages/core/src/Engine.ts (1)
resourceManager(170-172)packages/loader/src/index.ts (1)
KTX2Loader(33-33)packages/core/src/asset/AssetPromise.ts (2)
resolve(25-35)AssetPromise(4-234)packages/core/src/index.ts (1)
ContentRestorer(25-25)
🪛 ESLint
tests/src/core/DeviceLost.test.ts
[error] 1-1: Replace ·AmbientLight,·AssetType,·BlinnPhongMaterial,·Camera,·DirectLight,·MeshRenderer,·PrimitiveMesh,·Texture2D,·TextureCube· with ⏎··AmbientLight,⏎··AssetType,⏎··BlinnPhongMaterial,⏎··Camera,⏎··DirectLight,⏎··MeshRenderer,⏎··PrimitiveMesh,⏎··Texture2D,⏎··TextureCube⏎
(prettier/prettier)
[error] 45-45: Insert ;
(prettier/prettier)
[error] 49-49: Insert ;
(prettier/prettier)
[error] 50-51: Delete ⏎······
(prettier/prettier)
[error] 52-52: Replace ········ with ······
(prettier/prettier)
[error] 53-53: Delete ··
(prettier/prettier)
[error] 54-55: Replace ··}⏎····) with });
(prettier/prettier)
[error] 59-60: Replace ⏎ with ;
(prettier/prettier)
[error] 64-64: Replace 'new·Promise' with "new·Promise"
(prettier/prettier)
[error] 66-66: Replace 'context·lost' with "context·lost"
(prettier/prettier)
[error] 68-68: Replace 'context·restored' with "context·restored"
(prettier/prettier)
[error] 77-77: Insert ;
(prettier/prettier)
[error] 82-83: Delete ⏎
(prettier/prettier)
tests/src/core/Polyfill.test.ts
[error] 41-41: Delete ······
(prettier/prettier)
[error] 44-44: Delete ······
(prettier/prettier)
[error] 45-45: Delete ·
(prettier/prettier)
[error] 46-46: Delete ······
(prettier/prettier)
[error] 53-53: Delete ····
(prettier/prettier)
[error] 55-55: Delete ····
(prettier/prettier)
[error] 57-57: Delete ······
(prettier/prettier)
[error] 59-59: Delete ····
(prettier/prettier)
[error] 61-61: Delete ····
(prettier/prettier)
[error] 62-62: Insert ·
(prettier/prettier)
[error] 64-64: Replace resolve with (resolve)
(prettier/prettier)
[error] 68-68: Delete ····
(prettier/prettier)
🪛 Biome (1.9.4)
tests/src/core/Polyfill.test.ts
[error] 40-40: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 45-45: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 47-47: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 63-63: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
🔇 Additional comments (57)
packages/core/src/material/BaseMaterial.ts (1)
187-188: Fix for additive blend mode with transparent canvasThis change correctly adjusts the alpha blending factors for the additive blend mode. By changing the source alpha blend factor to
Zeroand destination alpha blend factor toOne, the alpha channel of the destination is preserved, which fixes the transparency issues with additive blending.This is the standard approach for additive blending when working with transparency, ensuring that while colors are added together, the alpha channel remains unaffected.
packages/core/src/Polyfill.ts (3)
9-9: Implementation of AudioContext polyfill looks goodGood addition of the AudioContext polyfill registration in the main registerPolyfill method.
14-14: Nice capitalization fixFixed capitalization in the log message for consistency.
38-71: Well-implemented AudioContext polyfill for iOS 12 compatibilityThis polyfill implementation properly handles legacy iOS devices that only support webkitAudioContext. The implementation correctly:
- Checks for the absence of AudioContext but presence of webkitAudioContext
- Properly logs the polyfill application
- Wraps the original decodeAudioData method to support both Promise-based returns while maintaining callback compatibility
packages/loader/src/HDRLoader.ts (3)
403-407: Improved code organization by caching URL and using resource manager.The change extracts the URL into a local variable for better readability and consistently uses it throughout the method. This improves maintainability and reduces potential errors from accessing
item.urlmultiple times.
409-409: Consistently using cached URL variable.Using the cached
urlvariable instead of accessingitem.urldirectly provides consistency with the earlier change and reduces the risk of errors ifitemwere to change.
431-438: Improved resource restoration with better variable caching.This change enhances code clarity and maintainability by:
- Caching the resource and engine in local variables
- Using the engine's resource manager for requests instead of a standalone function
- Consistently using the cached variables throughout the method
This approach provides better readability and follows the pattern used in other loaders.
packages/core/src/physics/StaticCollider.ts (1)
1-1: Centralized physics interface by moving from PhysicsScene to Engine.This change aligns with the broader refactoring where the static
_nativePhysicsproperty was moved fromPhysicsScenetoEngine. This architectural improvement centralizes the physics interface management in the Engine class, which is a more logical location since the Engine is responsible for initializing and managing all subsystems.Also applies to: 16-16
packages/core/src/physics/shape/CapsuleColliderShape.ts (1)
3-3: Centralized physics interface by moving from PhysicsScene to Engine.This change is part of the same architectural improvement as in other physics files, centralizing the native physics interface in the Engine class. This makes the code more maintainable and logically structures the responsibilities, with Engine managing all core subsystems including physics.
Also applies to: 58-58
packages/core/src/physics/PhysicsMaterial.ts (1)
2-2: Centralized physics engine access implementation.This change moves the native physics interface access from
PhysicsScenetoEngine, providing a more centralized approach to managing the physics engine.Also applies to: 20-20
packages/core/src/physics/joint/HingeJoint.ts (1)
3-3: Updated to use centralized physics engine access.This change aligns with the broader refactoring to centralize physics engine access through the
Engineclass, making the physics subsystem more maintainable.Also applies to: 163-163
packages/core/src/physics/DynamicCollider.ts (1)
3-3: Updated physics engine reference for consistency.This change follows the pattern of accessing the physics engine through the centralized
Engine._nativePhysicsstatic property, which provides a more unified approach to physics engine management.Also applies to: 309-309
packages/core/src/physics/shape/PlaneColliderShape.ts (1)
2-2: Updated to use centralized physics engine reference.This change consistently applies the physics engine refactoring pattern by accessing the native physics interface through
Engine._nativePhysicsinstead ofPhysicsScene._nativePhysics.Also applies to: 11-11
packages/physics-lite/src/LitePhysics.ts (1)
152-155: Added resource cleanup method.The new
destroy()method implements theIPhysicsinterface's resource cleanup contract. While the implementation is empty for the lite physics version, this addition ensures interface compliance and supports proper resource management in the engine lifecycle.packages/core/src/physics/shape/SphereColliderShape.ts (2)
3-3: LGTM: Import change aligns with physics interface centralizationThis change correctly updates the import to use
Engineinstead ofPhysicsSceneas part of the refactoring that centralizes the native physics interface.
27-31: LGTM: Native physics reference updated correctlyThe constructor now properly references
Engine._nativePhysicsinstead ofPhysicsScene._nativePhysicsfor creating the native sphere collider shape. This follows the pattern of centralizing physics interface access through the Engine class.packages/design/src/physics/IPhysics.ts (1)
138-141: LGTM: Good addition of resource cleanup methodAdding the
destroy()method to the physics interface ensures proper resource management, particularly for native physics resources. This aligns with good practices for explicit resource cleanup and complements the changes in the PhysicsScene implementation.packages/physics-physx/src/PhysXCharacterController.ts (1)
139-139: Important native memory management fixAdding
desc.delete()after controller creation ensures the native PhysX descriptor is properly cleaned up, preventing memory leaks in the native PhysX binding. This is an important fix for resource management when working with native code.packages/design/src/physics/IPhysicsScene.ts (1)
61-64: LGTM: Consistent resource cleanup additionAdding the
destroy()method to the IPhysicsScene interface maintains consistency with the IPhysics interface update and formalizes the contract for proper scene cleanup. This is important for managing the lifecycle of physics resources.packages/core/src/physics/joint/FixedJoint.ts (1)
1-1: Architectural improvement: Centralized physics interface managementThe refactoring to use
Engine._nativePhysicsinstead ofPhysicsScene._nativePhysicsis a good architectural change that centralizes the physics interface management in the Engine class. This makes the code more maintainable and easier to reason about.Also applies to: 12-12
packages/core/src/physics/CharacterController.ts (1)
3-3: Consistent refactoring for centralized physics managementThe change to reference
Engine._nativePhysicsinstead ofPhysicsScene._nativePhysicsis consistent with the architectural change across the codebase. This improves maintainability by centralizing the physics interface under the Engine class.Also applies to: 83-83
packages/core/src/physics/joint/SpringJoint.ts (1)
2-2: Centralized physics interface - architecture improvementThis change properly aligns with the broader refactoring effort to centralize the native physics interface in the
Engineclass. The implementation is correct and consistent with other physics-related files.Also applies to: 89-89
packages/core/src/physics/shape/BoxColliderShape.ts (2)
4-4: Consistent physics interface refactoringThe modification to use
Engine._nativePhysicsinstead ofPhysicsScene._nativePhysicsis part of a well-executed refactoring that centralizes physics interface management. This is consistent with changes in other files and improves the overall architecture.Also applies to: 29-33
1-49: PR objective vs. code changes discrepancyThe PR objective mentions fixing an "additive blend mode bug when canvas has transparency," but the changes I'm reviewing are related to physics interface refactoring. There might be a mismatch between the PR description and the actual code changes, or the blend mode fix might be in other files not included in this review.
I recommend verifying whether:
- The PR description accurately reflects the changes being made
- There are additional files not included in this review that address the blend mode issue
- The physics refactoring somehow relates to the blend mode fix (though this seems unlikely)
packages/core/src/physics/PhysicsScene.ts (2)
219-219: Good architecture change: Using Engine._nativePhysics instead of PhysicsScene._nativePhysicsThis change centralizes the physics engine management in the Engine class, which is a better architectural approach for managing global physics resources.
445-450: Well implemented resource cleanup methodAdding the
_destroy()method ensures proper cleanup of physics resources when scenes are destroyed. The use of the optional chaining operator (?.) correctly handles potential null/undefined values.packages/core/src/Scene.ts (1)
517-518: Proper physics cleanup in scene destructionGood addition to ensure physics resources are properly cleaned up when a scene is destroyed, preventing memory leaks and resource issues.
packages/loader/src/resource-deserialize/utils/Decorator.ts (1)
7-7: Enhanced API flexibility with rest parametersThe addition of rest parameters (
...arg: any[]) to the decode function signature allows for more flexible usage, particularly useful for supporting resource restoration.packages/core/src/Engine.ts (2)
61-62: Good architecture: Centralized physics engine managementAdding a static property to centralize the physics interface management in the Engine class improves the architecture by providing a single point of access for physics functionality.
626-631: Helpful warning for physics engine reconfigurationThe added warning message provides good feedback when multiple physics engines are configured, improving debuggability and developer experience.
packages/physics-physx/src/shape/PhysXColliderShape.ts (3)
149-149: Improved memory management by explicitly deleting the geometry.This change properly releases native resources, preventing memory leaks in the PhysX integration.
157-159: Good improvement to prevent memory leaks.Creating a temporary PxShapeFlags object and explicitly deleting it after use is an important memory management pattern for native objects.
172-174: Consistent application of PhysX resource management pattern.This follows the same pattern implemented in other methods - creating a temporary PhysX object, using it, and explicitly deleting it afterward to prevent memory leaks.
packages/loader/src/resource-deserialize/index.ts (4)
8-10: Simple reordering of export statements.No functional change, just reordering of exports.
18-18: Added variadic parameter support to decode function.This change enables forwarding additional arguments to decoder implementations, supporting new restoration features.
21-21: Updated decoder call to forward additional arguments.This forwards the variadic parameters to the specific decoder implementation, supporting features like content restoration.
28-30: Reordered export statements without functional change.Simple reordering of exports at the end of the file.
packages/loader/src/resource-deserialize/resources/texture2D/TextureDecoder.ts (2)
7-7: Added support for content restoration via optional parameter.The decode method now accepts an optional
restoredTextureparameter to support reusing existing texture instances during decoding.
23-23: Implemented texture instance reuse for content restoration.The code now reuses the provided texture instance if available, otherwise creates a new one, supporting efficient content restoration workflows.
packages/loader/src/resource-deserialize/resources/mesh/MeshDecoder.ts (3)
1-1: Updated imports to include required types.Added imports for required types to support the updated decode method signature.
12-12: Added support for content restoration via optional parameter.Similar to the TextureDecoder update, this change allows reusing existing mesh instances during decoding.
14-14: Implemented mesh instance reuse for content restoration.The code now reuses the provided mesh instance if available, otherwise creates a new one, supporting efficient content restoration workflows.
packages/physics-physx/src/PhysXPhysics.ts (6)
50-52: Resource tracking for proper cleanup.Added new member variables to track PhysX native objects, which is essential for properly releasing memory in the destroy method.
129-131: Improved resource cleanup with explicit deletion.Added explicit calls to
delete()on PhysX native objects, which prevents memory leaks by properly releasing native resources when the physics system is destroyed.
259-260: Enhanced initialization with separated tolerances scale.The
tolerancesScaleis now instantiated separately and stored as a member variable, allowing it to be properly cleaned up later.
266-268: Storing native PhysX objects for lifecycle management.These assignments ensure that the PhysX native objects are properly tracked for later cleanup, completing the resource management pattern introduced with the new member variables.
96-96:✅ Verification successful
URL update for PhysX JavaScript module.
The URL for loading the PhysX JavaScript implementation has been updated to a newer version.
🏁 Script executed:
#!/bin/bash # Verify the PhysX JavaScript URL is accessible curl -I "https://mdn.alipayobjects.com/rms/afts/file/A*V4pqRqM65UMAAAAAAAAAAAAAARQnAQ/physx.release.downgrade.js"Length of output: 1843
PhysX JavaScript URL Verified
The updated PhysX loader URL is reachable and returns HTTP 200.
• File:
packages/physics-physx/src/PhysXPhysics.ts
– Line 96:
ts script.src = `https://mdn.alipayobjects.com/rms/afts/file/A*V4pqRqM65UMAAAAAAAAAAAAAARQnAQ/physx.release.downgrade.js`;
• Verification:curl -Ireturned HTTP/2 200 OKNo further changes required.
98-98:✅ Verification successful
URL update for PhysX WebAssembly module.
The URL for loading the PhysX WebAssembly implementation has been updated to a newer version.
🏁 Script executed:
#!/bin/bash # Verify the PhysX WebAssembly URL is accessible curl -I "https://mdn.alipayobjects.com/rms/afts/file/A*jlLNQqp6mBsAAAAAAAAAAAAAARQnAQ/physx.release.js"Length of output: 1833
Confirmed updated PhysX WebAssembly URL.
The URL at packages/physics-physx/src/PhysXPhysics.ts (line 98) returns HTTP 200 and serves the PhysX release script as expected. No further action needed.packages/loader/src/EnvLoader.ts (2)
49-49: Using a local variable to store and reuse the URL.Extracting
item.urlto a local variable improves code maintainability by ensuring consistency across multiple usages and making the code more readable.Also applies to: 52-52, 55-55
86-93: Improved resource restoration with engine resource manager.This change properly implements content restoration by:
- Using a local variable for clarity
- Accessing the engine through the resource
- Using the engine's resource manager for requests instead of a standalone function
- Explicitly passing the resource to methods
This approach ensures proper resource restoration after device loss events.
tests/src/core/DeviceLost.test.ts (3)
1-4: Expanded imports to support enhanced device loss testing.The imports now include additional components needed for the more comprehensive device loss test, including
expectfromvitestfor assertions.🧰 Tools
🪛 ESLint
[error] 1-1: Replace
·AmbientLight,·AssetType,·BlinnPhongMaterial,·Camera,·DirectLight,·MeshRenderer,·PrimitiveMesh,·Texture2D,·TextureCube·with⏎··AmbientLight,⏎··AssetType,⏎··BlinnPhongMaterial,⏎··Camera,⏎··DirectLight,⏎··MeshRenderer,⏎··PrimitiveMesh,⏎··Texture2D,⏎··TextureCube⏎(prettier/prettier)
39-60: Added comprehensive resource loading for realistic device loss testing.Loading multiple resource types (AmbientLight, TextureCube, KTX2, EditorTexture2D) provides a more realistic test scenario for device loss and restoration, ensuring that different resource types can be properly restored.
🧰 Tools
🪛 ESLint
[error] 45-45: Insert
;(prettier/prettier)
[error] 49-49: Insert
;(prettier/prettier)
[error] 50-51: Delete
⏎······(prettier/prettier)
[error] 52-52: Replace
········with······(prettier/prettier)
[error] 53-53: Delete
··(prettier/prettier)
[error] 54-55: Replace
··}⏎····)with});(prettier/prettier)
[error] 59-60: Replace
⏎with;(prettier/prettier)
79-82: Added assertions to verify successful content restoration.These expectations verify that the loaded textures and ambient light resources have been properly restored after the device loss event, ensuring that the content restoration mechanism is working correctly.
packages/physics-lite/src/LitePhysicsScene.ts (2)
53-144: Improved documentation accuracy.Updated the method documentation comments to correctly reference
IPhysicsSceneinstead ofIPhysicsManager, which improves code accuracy and maintainability.
150-153: Added standard destroy method for interface compliance.Added an empty
destroy()method to comply with theIPhysicsSceneinterface. While empty, this maintains consistency across physics implementations and follows the interface contract.packages/physics-physx/src/PhysXPhysicsScene.ts (1)
226-232: Potential double-free of query-filter flags
_pxFilterData.flagsis a bit-flag enum, not a heap-allocated PhysX object. Deleting it separately risks undefined behaviour or a double-free after_pxFilterData.delete().Verify with the PhysX wasm binding, but the usual pattern is just:
- this._pxFilterData.flags.delete(); - this._pxFilterData.delete(); + this._pxFilterData.delete();packages/loader/src/ktx2/KTX2Loader.ts (1)
101-112: Ensure size & format are re-synced on texture reuse
Texture2D/TextureCubehold width/height/format immutable after construction.
Re-using an instance when the new buffer differs in any of those aspects silently produces undefined behaviour. Consider recreating the texture or asserting equality before reuse.-if (faceCount !== 6) { - texture = restoredTexture || new Texture2D(engine, width, height, engineFormat, mipmap); +if (faceCount !== 6) { + if ( + !restoredTexture || + restoredTexture.width !== width || + restoredTexture.height !== height || + restoredTexture.format !== engineFormat + ) { + texture = new Texture2D(engine, width, height, engineFormat, mipmap); + } else { + texture = restoredTexture; + }
tests/src/core/Polyfill.test.ts
Outdated
| it("AudioContext polyfill", async () => { | ||
| delete window.AudioContext; | ||
|
|
||
| (window as any).webkitAudioContext = class MockWebkitAudioContext { | ||
| state = "suspended"; | ||
|
|
||
| constructor() { } | ||
|
|
||
| decodeAudioData(arrayBuffer: ArrayBuffer, successCallback: Function) { | ||
| setTimeout(() => { | ||
| successCallback({ duration: 10 } as AudioBuffer); | ||
| }, 10); | ||
| } | ||
| }; | ||
|
|
||
| window.AudioContext = (window as any).webkitAudioContext; | ||
|
|
||
| expect(window.AudioContext).to.equal((window as any).webkitAudioContext); | ||
|
|
||
| const context = new window.AudioContext(); | ||
|
|
||
| const originalDecodeAudioData = context.decodeAudioData; | ||
|
|
||
| AudioContext.prototype.decodeAudioData = function(arrayBuffer: ArrayBuffer): Promise<AudioBuffer> { | ||
| const self = this; | ||
| return new Promise(resolve => { | ||
| originalDecodeAudioData.apply(self, [arrayBuffer, resolve]); | ||
| }); | ||
| }; | ||
|
|
||
| const arrayBuffer = new ArrayBuffer(10); | ||
| const result = await context.decodeAudioData(arrayBuffer); | ||
| expect(result).to.have.property("duration", 10); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Improve test to directly validate the Polyfill implementation
The current test reimplements similar functionality to what's in the Polyfill._registerAudioContext() method rather than directly testing it. Consider refactoring to directly test the polyfill implementation.
Here's a suggested improvement that directly tests the polyfill implementation:
- it("AudioContext polyfill", async () => {
- delete window.AudioContext;
-
- (window as any).webkitAudioContext = class MockWebkitAudioContext {
- state = "suspended";
-
- constructor() { }
-
- decodeAudioData(arrayBuffer: ArrayBuffer, successCallback: Function) {
- setTimeout(() => {
- successCallback({ duration: 10 } as AudioBuffer);
- }, 10);
- }
- };
-
- window.AudioContext = (window as any).webkitAudioContext;
-
- expect(window.AudioContext).to.equal((window as any).webkitAudioContext);
-
- const context = new window.AudioContext();
-
- const originalDecodeAudioData = context.decodeAudioData;
-
- AudioContext.prototype.decodeAudioData = function(arrayBuffer: ArrayBuffer): Promise<AudioBuffer> {
- const self = this;
- return new Promise(resolve => {
- originalDecodeAudioData.apply(self, [arrayBuffer, resolve]);
- });
- };
-
- const arrayBuffer = new ArrayBuffer(10);
- const result = await context.decodeAudioData(arrayBuffer);
- expect(result).to.have.property("duration", 10);
+ it("AudioContext polyfill", async () => {
+ // Store original AudioContext
+ const OriginalAudioContext = window.AudioContext;
+
+ // Setup test environment
+ window.AudioContext = undefined as any;
+ (window as any).webkitAudioContext = class MockWebkitAudioContext {
+ state = "suspended";
+
+ decodeAudioData(arrayBuffer: ArrayBuffer, successCallback: (buffer: AudioBuffer) => void) {
+ setTimeout(() => {
+ successCallback({ duration: 10 } as AudioBuffer);
+ }, 10);
+ }
+ };
+
+ // Import and run the actual polyfill implementation
+ const { Polyfill } = await import("@galacean/engine-core");
+ Polyfill.registerPolyfill();
+
+ // Verify AudioContext was properly polyfilled
+ expect(window.AudioContext).to.equal((window as any).webkitAudioContext);
+
+ // Test the Promise-based decodeAudioData
+ const context = new window.AudioContext();
+ const arrayBuffer = new ArrayBuffer(10);
+ const result = await context.decodeAudioData(arrayBuffer);
+ expect(result).to.have.property("duration", 10);
+
+ // Restore original AudioContext
+ window.AudioContext = OriginalAudioContext;
});This version:
- Properly tests the actual Polyfill implementation rather than reimplementing it
- Uses a more specific function signature for callbacks
- Removes the unnecessary constructor
- Avoids the unnecessary this aliasing
- Restores the original AudioContext after the test
- Addresses most of the formatting issues flagged by the static analyzers
🌐 Web query:
What's the best practice for mocking browser objects in JavaScript/TypeScript tests?
💡 Result:
When testing JavaScript/TypeScript code that interacts with browser objects, proper mocking techniques are essential for creating reliable and maintainable tests. Here are the best practices for mocking browser objects in your tests:
Mocking the Window Object
The window object presents unique challenges due to its read-only properties and global nature. There are several recommended approaches:
Using Object.defineProperty
One effective technique is using Object.defineProperty to mock window properties:
Object.defineProperty(window, 'location', {
value: { href: 'https://example.com/en' },
writable: true
})This approach allows you to define custom properties on the window object specifically for testing purposes[5].
Creating a Window Module
A cleaner approach involves:
- Exporting a copy of the window object from a module
- Using that copy in your code
- Mocking the exported window in your tests
This pattern provides consistency regardless of whether the property is read-only or not[8].
Using Jest's Mock Capabilities
Jest provides robust functionality for mocking:
beforeEach(() => {
jest.spyOn(window, 'performance').mockImplementation(() => ({
measure: jest.fn(),
getEntries: jest.fn()
}));
});
afterEach(() => {
jest.restoreAllMocks();
});This approach is preferred for its versatility and built-in functions for restoring mocks[5].
General Mocking Best Practices
Only Mock What's Necessary
Don't write more mocking code than needed. Mocking code still requires creation and maintenance time that could be spent adding functionality[2].
Mock Objects with Logic, Not Value Objects
Only mock objects that contain logic whose impact on the component under test might be unpredictable. Don't mock objects that only carry values (like DTOs)[2].
Be Clear About Test Objectives
Define precisely what you want the test to prove. In unit testing, you typically want to isolate the component under test by mocking its dependencies[2].
Use Declarative Mocking Code
Prefer mocking tools that let you declaratively state what you want your mock to do without requiring complex logic:
Mock.Arrange(() => MyStaticUtility.CalcCost(Arg.IsAny<Destination>())).Returns(42);This approach is more maintainable than imperative mocking code[2].
Choose the Right Mocking Tool
Different situations call for different approaches:
- jest.fn(): Most commonly used for mocking function parameters or function props[1]
- jest.spyOn(): Preferred when you need to mock only specific elements of an exported module[1]
- jest.mock(): Useful for "static" mocks and replacing entire modules[1]
Specialized Mocking Approaches
Using Testing Libraries
Libraries like Mock Service Worker (MSW) facilitate type-safe API mocking through generic arguments in TypeScript, allowing you to annotate path parameters, request and response body types, and more[6].
JSON Server for API Mocking
For REST API mocking, JSON Server is popular due to its efficiency and simplicity. It supports basic validations and can create demo REST APIs with minimal configuration[3].
Consider Environment-Specific Mocking
When testing browser-specific functionality, consider using testing environments like jsdom or happy-dom that provide browser-like environments for your tests[7].
When to Avoid Mocking
While mocking is valuable, it shouldn't be overused. Mocks should primarily be used when:
- You need to verify multiple specific behaviors on a dependency
- Testing with actual dependencies would be impractical (like external APIs)
- You need to isolate the component under test from unpredictable dependencies[4][9]
Remember that mocks can make tests overly specific, leading to brittle tests that break easily when code changes. Balance the need for isolation with the risk of creating tests that are too tightly coupled to implementation details[4].
Citations:
- 1: https://dev.to/alexpladev/js-tests-mocking-best-practices-10kp
- 2: https://www.telerik.com/blogs/mocking-best-practices
- 3: https://www.syncfusion.com/blogs/post/javascript-api-mocking-techniques
- 4: https://semaphoreci.com/community/tutorials/best-practices-for-spies-stubs-and-mocks-in-sinon-js
- 5: https://claritydev.net/blog/mock-window-object-jest
- 6: https://mswjs.io/docs/best-practices/typescript/
- 7: Help mocking an extended `window` object. vitest-dev/vitest#2369
- 8: https://dev.to/grug/mocking-the-window-object-in-unit-tests-1h2e
- 9: https://www.browserstack.com/guide/javascript-testing-best-practices
Improve AudioContext polyfill test to use the real Polyfill and proper window mocking
Use best practices for mocking global browser objects and test the actual Polyfill.registerPolyfill() implementation rather than re-implementing its logic.
• Mock window.AudioContext and window.webkitAudioContext via Object.defineProperty instead of delete
• Call Polyfill.registerPolyfill() to install the polyfill under test
• Use a strongly-typed callback signature (buffer: AudioBuffer) => void
• Leverage Jest’s beforeEach/afterEach (or similar) to restore originals and avoid side effects
• Remove unnecessary constructor and this aliasing
Suggested refactor:
- it("AudioContext polyfill", async () => {
- delete window.AudioContext;
- (window as any).webkitAudioContext = class MockWebkitAudioContext {
- state = "suspended";
- constructor() { }
- decodeAudioData(arrayBuffer: ArrayBuffer, successCallback: Function) {
- setTimeout(() => successCallback({ duration: 10 } as AudioBuffer), 10);
- }
- };
- window.AudioContext = (window as any).webkitAudioContext;
- expect(window.AudioContext).to.equal((window as any).webkitAudioContext);
- const context = new window.AudioContext();
- const originalDecodeAudioData = context.decodeAudioData;
- AudioContext.prototype.decodeAudioData = function(arrayBuffer: ArrayBuffer): Promise<AudioBuffer> {
- const self = this;
- return new Promise(resolve => {
- originalDecodeAudioData.apply(self, [arrayBuffer, resolve]);
- });
- };
- const arrayBuffer = new ArrayBuffer(10);
- const result = await context.decodeAudioData(arrayBuffer);
- expect(result).to.have.property("duration", 10);
- });
+ describe("AudioContext polyfill", () => {
+ let OriginalAudioContext: typeof AudioContext;
+
+ beforeEach(() => {
+ OriginalAudioContext = window.AudioContext;
+ Object.defineProperty(window, "AudioContext", { value: undefined, writable: true });
+ Object.defineProperty(window, "webkitAudioContext", {
+ value: class MockWebkitAudioContext {
+ state = "suspended";
+ decodeAudioData(buffer: ArrayBuffer, cb: (buf: AudioBuffer) => void) {
+ setTimeout(() => cb({ duration: 10 } as AudioBuffer), 10);
+ }
+ },
+ writable: true
+ });
+
+ // Install the real polyfill
+ const { Polyfill } = require("@galacean/engine-core");
+ Polyfill.registerPolyfill();
+ });
+
+ afterEach(() => {
+ // Restore original global
+ Object.defineProperty(window, "AudioContext", { value: OriginalAudioContext, writable: true });
+ delete (window as any).webkitAudioContext;
+ });
+
+ it("should polyfill AudioContext.decodeAudioData as Promise", async () => {
+ expect(window.AudioContext).toBe((window as any).webkitAudioContext);
+ const ctx = new window.AudioContext();
+ const buffer = new ArrayBuffer(10);
+ const result = await ctx.decodeAudioData(buffer);
+ expect(result).toHaveProperty("duration", 10);
+ });
+ });Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 40-40: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 45-45: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 47-47: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 63-63: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
🪛 ESLint
[error] 41-41: Delete ······
(prettier/prettier)
[error] 44-44: Delete ······
(prettier/prettier)
[error] 45-45: Delete ·
(prettier/prettier)
[error] 46-46: Delete ······
(prettier/prettier)
[error] 53-53: Delete ····
(prettier/prettier)
[error] 55-55: Delete ····
(prettier/prettier)
[error] 57-57: Delete ······
(prettier/prettier)
[error] 59-59: Delete ····
(prettier/prettier)
[error] 61-61: Delete ····
(prettier/prettier)
[error] 62-62: Insert ·
(prettier/prettier)
[error] 64-64: Replace resolve with (resolve)
(prettier/prettier)
[error] 68-68: Delete ····
(prettier/prettier)
packages/core/src/Utils.ts
Outdated
| @@ -79,7 +79,7 @@ export class Utils { | |||
| } | |||
|
|
|||
| if (!/^https?:/.test(baseUrl)) { | |||
| const fileSchema = "files://"; | |||
| const fileSchema = "file://"; | |||
There was a problem hiding this comment.
Fixed URL scheme to standard format.
Changed the file scheme from non-standard "files://" to the standard "file://" format, ensuring proper URL resolution for local files according to web standards.
| ResourceManager, | ||
| Texture2D | ||
| } from "@galacean/engine-core"; | ||
| import { RequestConfig } from "@galacean/engine-core/types/asset/request"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid deep import paths – rely on public API instead
Importing RequestConfig from an internal path (@galacean/engine-core/types/asset/request) couples the loader to package internals and may break on refactors or bundler configurations that strip types/.
Prefer re-exporting the type from the public entry of @galacean/engine-core (or adding such a re-export) and importing from there instead.
| override restoreContent(): AssetPromise<Texture2D> { | ||
| const texture = this.resource; | ||
| const engine = texture.engine; | ||
| return ( | ||
| engine.resourceManager | ||
| // @ts-ignore | ||
| ._request<ArrayBuffer>(this.url, this.requestConfig) | ||
| .then((data) => decode<Texture2D>(data, engine, texture)) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
restoreContent returns the wrong promise type
_request().then() returns a plain Promise, not an AssetPromise, violating the declared return type AssetPromise<Texture2D> and potentially losing engine-specific behaviour embedded in AssetPromise.
Replicate the pattern used in MeshContentRestorer:
- override restoreContent(): AssetPromise<Texture2D> {
- const texture = this.resource;
- const engine = texture.engine;
- return (
- engine.resourceManager
- // @ts-ignore
- ._request<ArrayBuffer>(this.url, this.requestConfig)
- .then((data) => decode<Texture2D>(data, engine, texture))
- );
- }
+ override restoreContent(): AssetPromise<Texture2D> {
+ const texture = this.resource;
+ const engine = texture.engine;
+ return new AssetPromise((resolve, reject) => {
+ engine.resourceManager
+ // @ts-ignore
+ ._request<ArrayBuffer>(this.url, this.requestConfig)
+ .then((data) => decode<Texture2D>(data, engine, texture))
+ .then(resolve)
+ .catch(reject);
+ });
+ }This keeps the type contract intact and ensures proper error propagation.
📝 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.
| override restoreContent(): AssetPromise<Texture2D> { | |
| const texture = this.resource; | |
| const engine = texture.engine; | |
| return ( | |
| engine.resourceManager | |
| // @ts-ignore | |
| ._request<ArrayBuffer>(this.url, this.requestConfig) | |
| .then((data) => decode<Texture2D>(data, engine, texture)) | |
| ); | |
| } | |
| override restoreContent(): AssetPromise<Texture2D> { | |
| const texture = this.resource; | |
| const engine = texture.engine; | |
| return new AssetPromise((resolve, reject) => { | |
| engine.resourceManager | |
| // @ts-ignore | |
| ._request<ArrayBuffer>(this.url, this.requestConfig) | |
| .then((data) => decode<Texture2D>(data, engine, texture)) | |
| .then(resolve) | |
| .catch(reject); | |
| }); | |
| } |
| ._request<ArrayBuffer>(url, requestConfig) | ||
| .then((data) => { | ||
| decode<Texture2D>(data, resourceManager.engine).then((textur 6D40 e) => { | ||
| resourceManager.addContentRestorer(new EditorTexture2DContentRestorer(texture, url, requestConfig)); | ||
| resolve(texture); | ||
| }); | ||
| }) | ||
| .catch(reject); |
There was a problem hiding this comment.
Propagate decoding errors and flatten the promise chain
decode() returns a promise, but calling it inside a nested then() means its rejection is not caught by the outer .catch(reject), causing an unhandled-promise rejection and an AssetPromise that never settles.
A minimal fix is to chain decode() directly:
- resourceManager
- // @ts-ignore
- ._request<ArrayBuffer>(url, requestConfig)
- .then((data) => {
- decode<Texture2D>(data, resourceManager.engine).then((texture) => {
- resourceManager.addContentRestorer(new EditorTexture2DContentRestorer(texture, url, requestConfig));
- resolve(texture);
- });
- })
+ resourceManager
+ // @ts-ignore – _request returns an AssetPromise
+ ._request<ArrayBuffer>(url, requestConfig)
+ .then((data) => decode<Texture2D>(data, resourceManager.engine))
+ .then((texture) => {
+ resourceManager.addContentRestorer(
+ new EditorTexture2DContentRestorer(texture, url, requestConfig)
+ );
+ resolve(texture);
+ })
.catch(reject);This mirrors the pattern already used in MeshLoader.
📝 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.
| ._request<ArrayBuffer>(url, requestConfig) | |
| .then((data) => { | |
| decode<Texture2D>(data, resourceManager.engine).then((texture) => { | |
| resourceManager.addContentRestorer(new EditorTexture2DContentRestorer(texture, url, requestConfig)); | |
| resolve(texture); | |
| }); | |
| }) | |
| .catch(reject); | |
| resourceManager | |
| // @ts-ignore – _request returns an AssetPromise | |
| ._request<ArrayBuffer>(url, requestConfig) | |
| .then((data) => decode<Texture2D>(data, resourceManager.engine)) | |
| .then((texture) => { | |
| resourceManager.addContentRestorer( | |
| new EditorTexture2DContentRestorer(texture, url, requestConfig) | |
| ); | |
| resolve(texture); | |
| }) | |
| .catch(reject); |
| const pxRaycastCallback = this._physXPhysics._physX.PxQueryFilterCallback.implement(raycastCallback); | ||
| const result = this._pxScene.raycastSingle( | ||
| ray.origin, | ||
| ray.direction, | ||
| distance, | ||
| pxHitResult, | ||
| this._pxFilterData, | ||
| this._physXPhysics._physX.PxQueryFilterCallback.implement(raycastCallback) | ||
| pxRaycastCallback | ||
| ); | ||
|
|
||
| pxRaycastCallback.delete(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guarantee native callback deletion even on exceptions
pxRaycastCallback.delete() is called after raycastSingle, but if the PhysX call throws, the deletion is skipped, leaking wasm memory.
Wrap the call in try…finally:
- const pxRaycastCallback = this._physXPhysics._physX.PxQueryFilterCallback.implement(raycastCallback);
- const result = this._pxScene.raycastSingle(
- ray.origin,
- ray.direction,
- distance,
- pxHitResult,
- this._pxFilterData,
- pxRaycastCallback
- );
- pxRaycastCallback.delete();
+ const pxRaycastCallback = this._physXPhysics._physX.PxQueryFilterCallback.implement(raycastCallback);
+ let result: boolean;
+ try {
+ result = this._pxScene.raycastSingle(
+ ray.origin,
+ ray.direction,
+ distance,
+ pxHitResult,
+ this._pxFilterData,
+ pxRaycastCallback
+ );
+ } finally {
+ pxRaycastCallback.delete();
+ }📝 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.
| const pxRaycastCallback = this._physXPhysics._physX.PxQueryFilterCallback.implement(raycastCallback); | |
| const result = this._pxScene.raycastSingle( | |
| ray.origin, | |
| ray.direction, | |
| distance, | |
| pxHitResult, | |
| this._pxFilterData, | |
| this._physXPhysics._physX.PxQueryFilterCallback.implement(raycastCallback) | |
| pxRaycastCallback | |
| ); | |
| pxRaycastCallback.delete(); | |
| const pxRaycastCallback = this._physXPhysics._physX.PxQueryFilterCallback.implement(raycastCallback); | |
| let result: boolean; | |
| try { | |
| result = this._pxScene.raycastSingle( | |
| ray.origin, | |
| ray.direction, | |
| distance, | |
| pxHitResult, | |
| this._pxFilterData, | |
| pxRaycastCallback | |
| ); | |
| } finally { | |
| pxRaycastCallback.delete(); | |
| } |
packages/loader/src/MeshLoader.ts
Outdated
| @resourceLoader(AssetType.Mesh, ["mesh"]) | ||
| class MeshLoader extends Loader<ModelMesh> { | ||
| load(item: LoadItem, resourceManager: ResourceManager): AssetPromise<ModelMesh> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Export the loader to ensure tree-shaking doesn’t remove it
Without the export keyword, the MeshLoader class can be stripped out by module bundlers in production builds, breaking mesh loading at runtime.
-@resourceLoader(AssetType.Mesh, ["mesh"])
-class MeshLoader extends Loader<ModelMesh> {
+@resourceLoader(AssetType.Mesh, ["mesh"])
+export class MeshLoader extends Loader<ModelMesh> {6ebf6cc to
92524de
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.5 #2659 +/- ##
===========================================
+ Coverage 68.68% 68.70% +0.01%
===========================================
Files 971 971
Lines 102088 102088
Branches 8781 8784 +3
===========================================
+ Hits 70119 70135 +16
+ Misses 31703 31687 -16
Partials 266 266
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 (4)
e2e/case/canvas-transparency.ts (4)
31-32: Consider adding error handling for engine creation.The engine creation promise has no error handling. While this is an E2E test, it would be better to include error handling for robustness.
-WebGLEngine.create({ canvas: "canvas", graphicDeviceOptions: { alpha: false } }).then((engine) => { +WebGLEngine.create({ canvas: "canvas", graphicDeviceOptions: { alpha: false } }) + .then((engine) => { engine.canvas.resizeByClientSize(2); + }) + .catch((error) => { + console.error("Failed to create WebGL engine:", error); + });
34-45: Add error handling for resource loading.Resource loading lacks error handling, which could lead to silent failures if the resources can't be loaded.
engine.resourceManager .load([ { url: "https://mdn.alipayobjects.com/huamei_kz4wfo/afts/img/A*3d2oSbam_wcAAAAAAAAAAAAAesp6AQ/original", type: AssetType.Texture2D }, { url: "https://mdn.alipayobjects.com/huamei_qbugvr/afts/img/A*j6MERb-exVgAAAAAAAAAAAAADtKFAQ/original", type: AssetType.Texture2D } ]) - .then((resources) => { + .then((resources) => { const transparencyTexture = resources[1] as Texture2D; const sceneColorTexture = createMainScene(engine, transparencyTexture); + }) + .catch((error) => { + console.error("Failed to load resources:", error); + });
108-110: Consider making render target dimensions configurable.The render target size is hardcoded to 512x512. Consider making this configurable or responsive to the canvas size for better flexibility and testing at different resolutions.
+const RENDER_TARGET_SIZE = 512; const sceneCamera = cameraEntity.addComponent(Camera); -const renderColorTexture = new Texture2D(engine, 512, 512, TextureFormat.R8G8B8A8, false, false); -const renderTarget = new RenderTarget(engine, 512, 512, renderColorTexture); +const renderColorTexture = new Texture2D(engine, RENDER_TARGET_SIZE, RENDER_TARGET_SIZE, TextureFormat.R8G8B8A8, false, false); +const renderTarget = new RenderTarget(engine, RENDER_TARGET_SIZE, RENDER_TARGET_SIZE, renderColorTexture);
121-124: This is the key part testing the fixed additive blend mode.This section applies the transparent texture with
BlendMode.Additiveto the plane material, which directly tests the fix for the additive blend mode bug mentioned in the PR. Consider adding an explanatory comment to highlight the connection to the bug being fixed.planeMaterial.baseTexture = transparencyTexture; planeMaterial.isTransparent = true; +// Testing the fixed additive blend mode with transparent canvas planeMaterial.blendMode = BlendMode.Additive;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
e2e/fixtures/originImage/Advance_canvas-transparency.jpgis excluded by!**/*.jpgpackages/core/src/postProcess/shaders/FinalAntiAliasing.fs.glslis excluded by!**/*.glsl
📒 Files selected for processing (2)
e2e/case/canvas-transparency.ts(1 hunks)e2e/config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- e2e/config.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
e2e/case/canvas-transparency.ts (8)
packages/core/src/index.ts (5)
AssetType(29-29)BackgroundMode(36-36)BackgroundTextureFillMode(46-46)Camera(16-16)AntiAliasing(77-77)packages/core/src/shader/index.ts (2)
BlendFactor(1-1)BlendOperation(2-2)packages/rhi-webgl/src/WebGLGraphicDevice.ts (1)
renderer(126-128)packages/core/src/mesh/MeshRenderer.ts (1)
MeshRenderer(13-190)packages/core/src/mesh/PrimitiveMesh.ts (1)
PrimitiveMesh(23-1506)e2e/case/.mockForE2E.ts (2)
updateForE2E(3-15)initScreenshot(20-113)packages/core/src/texture/index.ts (1)
TextureFormat(5-5)packages/core/src/material/index.ts (1)
BlendMode(3-3)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: e2e (22.x)
- GitHub Check: codecov
🔇 Additional comments (4)
e2e/case/canvas-transparency.ts (4)
1-4: Good documentation headers with title and category.Clear and descriptive documentation header that properly categorizes this test case under "Canvas" with the title "Transparency".
5-28: Thorough and appropriate imports for this test case.All necessary imports are included from the Galacean engine for rendering, materials, geometry, and texture handling. The imports are well-organized and include everything needed for the test case.
70-82: Well-documented blending configuration for transparency simulation.The blending setup is correctly configured to simulate canvas transparency blending with the browser. The comments clearly explain the rationale for the specific blend factors, particularly why
Zerois used for source alpha blending andOnefor destination alpha blending.
97-137: Well-implemented main scene creation with appropriate camera and object setup.The
createMainScenefunction properly sets up a scene with a semi-transparent red background, a camera rendering to an offscreen texture, and test objects (plane and cube) with appropriate materials. The key part of this test is the plane material usingBlendMode.Additivewith a transparent texture, which directly tests the fix for the additive blend mode bug mentioned in the PR.
| updateForE2E(engine); | ||
| initScreenshot(engine, canvasCamera); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding assertions to verify the test outcome.
The test uses updateForE2E and initScreenshot but doesn't have explicit assertions to verify the outcome. While visual inspection of screenshots is valuable, adding programmatic verification would enhance test reliability.
🌐 Web query:
How to add assertions to Galacean Engine E2E tests?
💡 Result:
To add assertions to your Galacean Engine end-to-end (E2E) tests, you can utilize the Cypress testing framework, which is integrated into the Galacean Engine for E2E testing. Cypress provides a rich set of commands and assertions to verify the behavior of your application. Here's how you can incorporate assertions into your E2E tests:
-
Set Up Your E2E Test Environment:
- Ensure that Cypress is installed and configured in your project.
- Create a test case file in the
e2e/casedirectory. For example, you can refer toe2e/case/animator-play.tsas a template. (github.com)
-
Write Your Test Case:
-
In your test case file, use Cypress commands to interact with your application and perform assertions.
-
For example, to verify that a specific element is visible on the page:
it('should display the login button', () => { cy.visit('/login'); cy.get('button#login').should('be.visible'); });
In this example,
cy.visit('/login')navigates to the login page, andcy.get('button#login').should('be.visible')asserts that the login button is visible.
-
-
Run and Debug Your Tests:
-
Launch the Cypress test runner to execute your tests:
npm run e2e:case
-
If you need to debug your test cases, you can use the Cypress interactive mode:
npm run e2e:debug
This will open the Cypress client, allowing you to interactively debug your tests. (github.com)
-
-
Review and Store Baseline Images:
- Cypress captures screenshots of your test cases. Review these screenshots to ensure they match the expected outcomes.
- If the screenshots are correct, store them in the
e2e/fixtures/originImagedirectory. (github.com)
-
Run the Complete E2E Tests:
-
After setting up and debugging your individual test cases, you can run the entire suite of E2E tests:
npm run e2e
-
By following these steps, you can effectively add assertions to your Galacean Engine E2E tests, ensuring that your application behaves as expected.
Citations:
- 1: https://github.com/galacean/engine/wiki/How-to-write-an-e2e-Test-for-runtime?utm_source=openai
- 2: https://github.com/galacean/engine/wiki/How-to-write-an-e2e-Test-for-runtime?utm_source=openai
- 3: https://github.com/galacean/engine/wiki/How-to-write-an-e2e-Test-for-runtime?utm_source=openai
Add programmatic assertions to the Canvas Transparency E2E test
The current test only captures a screenshot but doesn’t fail automatically when the canvas output is wrong. You can leverage Cypress (already integrated in our E2E setup) to assert both DOM visibility and pixel‐level transparency. For example, immediately after updateForE2E(engine) and initScreenshot(engine, canvasCamera), you could:
• Verify the canvas rendered and is in the DOM:
cy.get('canvas').should('be.visible');• Inspect a pixel’s alpha value to confirm transparency:
cy.window().then(win => {
const canvas = win.document.querySelector('canvas')!;
const ctx = (canvas as HTMLCanvasElement).getContext('2d')!;
const [r, g, b, a] = ctx.getImageData(0, 0, 1, 1).data;
expect(a).to.equal(0);
});• (Optional) Use a snapshot plugin for visual regression:
cy.get('canvas').toMatchImageSnapshot();Adding these assertions will cause your CI to fail on regressions instead of relying solely on manual screenshot review.
Please check if the PR fulfills these requirements
The commit message follows our guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)
Fix material additive bug when canvas is transparency
Fix FXAA bug when camera has render target
Beofre:
Fixed:
Summary by CodeRabbit