Merge main to dev/1.4 #2450
Merge main to dev/1.4 #2450
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe changes in this pull request primarily enhance the functionality of the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (14)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1)
123-178: Handle potential null or undefined depth formatsIn the logic for creating internal color textures and depth formats, there's a possibility that
depthFormatcould benull. Ensure that the code gracefully handles cases where depth or stencil buffers are not available to prevent any rendering issues.packages/rhi-webgl/src/WebGLGraphicDevice.ts (1)
140-142: Clarify the naming of the 'context' getter methodThe
get context()method returns the_contextAttributes. To avoid confusion with the WebGL rendering context, consider renaming this getter tocontextAttributesfor clearer intent.Apply this diff to rename the method:
- get context(): WebGLGraphicDeviceOptions { + get contextAttributes(): WebGLContextAttributes { return this._contextAttributes; }Make sure to update all references to this getter accordingly.
packages/core/src/Transform.ts (1)
279-285: Use epsilon tolerance when comparing floating-point valuesDirect comparison of floating-point values can be unreliable due to precision errors. When checking for uniform scaling, use an epsilon value to determine equality.
Apply this diff to implement tolerance in comparisons:
- const localUniformScaling = scale.x === scale.y && scale.y === scale.z; + const epsilon = 1e-6; + const localUniformScaling = Math.abs(scale.x - scale.y) < epsilon && Math.abs(scale.y - scale.z) < epsilon;Similarly, update the comparison in the
_onScaleChangedmethod:- const localUniformScaling = x == y && y == z; + const epsilon = 1e-6; + const localUniformScaling = Math.abs(x - y) < epsilon && Math.abs(y - z) < epsilon;Also applies to: 858-862
packages/core/src/Polyfill.ts (2)
6-36: Avoid using a class with only static membersThe
Polyfillclass contains only static methods and properties. It's recommended to refactor this into a module with standalone functions to simplify the code and adhere to best practices.Apply this diff to refactor the class into functions:
- export class Polyfill { - static registerPolyfill(): void { - Polyfill._registerMatchAll(); - } - - private static _registerMatchAll(): void { - // Implementation - } - } + export function registerPolyfill(): void { + _registerMatchAll(); + } + + function _registerMatchAll(): void { + // Implementation + }Ensure to update any imports or calls to
Polyfill.registerPolyfill()accordingly.🧰 Tools
🪛 Biome (1.9.4)
[error] 6-36: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 14-14: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead(lint/complexity/noBannedTypes)
14-14: Use lowercase 'string' instead of 'String' for type annotationsFor consistency and to avoid using the boxed primitive type, use the lowercase
stringwhen specifying type annotations.Apply this diff to correct the type:
- String.prototype.matchAll = function (pattern: RegExp): ReturnType<String["matchAll"]> { + String.prototype.matchAll = function (pattern: RegExp): Iterable<RegExpMatchArray> {This change also explicitly specifies the return type as
Iterable<RegExpMatchArray>, which is appropriate for thematchAllmethod.🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead(lint/complexity/noBannedTypes)
tests/src/core/Transform.test.ts (1)
29-39: LGTM! Consider adding more test casesThe World Scale test effectively validates the compound effects of scale and rotation transformations. The test structure is clear and the assertions are well-defined.
Consider adding these test cases to improve coverage:
- Edge cases with zero or negative scale values
- Additional rotation angles (45°, 180°, 270°)
- Compound rotations on multiple axes
- Scale changes after rotation to verify order independence
Example additional test:
// Test edge cases transform.setScale(0, -2, 3); expect(transform.lossyWorldScale).to.deep.equal(new Vector3(0, -4, 9)); // Test compound rotations transform.setRotation(45, 45, 45); expect(transform.lossyWorldScale).to.deep.approximately(/* expected values */);e2e/case/multi-scene-clear.ts (2)
27-28: Consider removing commented-out code.The commented line appears to be test code that's no longer needed.
- // initScreenshot(engine, [secondCamera, firstCamera]);
52-85: Consider extracting post-processing configuration.The second scene setup mixes camera initialization with post-processing configuration. Consider extracting the post-processing setup into a separate function for better maintainability.
Example refactor:
function setupPostProcessing(scene: Scene) { // @ts-ignore - TODO: Create proper public APIs const bloomEffect = scene._postProcessManager._bloomEffect; // @ts-ignore - TODO: Create proper public APIs const tonemappingEffect = scene._postProcessManager._tonemappingEffect; bloomEffect.enabled = true; tonemappingEffect.enabled = true; bloomEffect.threshold = 0.1; bloomEffect.intensity = 2; } function initSecondScene(engine: Engine): Camera { const scene = new Scene(engine); engine.sceneManager.addScene(scene); const rootEntity = scene.createRootEntity(); const cameraEntity = rootEntity.createChild("window-camera"); const camera = cameraEntity.addComponent(Camera); camera.enablePostProcess = true; cameraEntity.transform.setPosition(0, 0, 20); setupPostProcessing(scene); // Create cube... return camera; }e2e/case/multi-scene-no-clear.ts (2)
34-34: Remove commented codeThe commented line appears to be testing code that's no longer needed. Remove it to maintain code cleanliness.
- // initScreenshot(engine, [secondCamera, firstCamera]);
37-56: Consider extracting common scene setup logicBoth
initFirstSceneandinitSecondSceneshare similar cube creation logic. Consider extracting this into a helper function to follow DRY principles.function createCube(rootEntity: Entity, position: Vector3, material: BlinnPhongMaterial): void { const cubeEntity = rootEntity.createChild("cube"); cubeEntity.transform.setPosition(position.x, position.y, position.z); const renderer = cubeEntity.addComponent(MeshRenderer); renderer.mesh = PrimitiveMesh.createCuboid(engine, 2, 2, 2); renderer.setMaterial(material); }tests/src/core/resource/ResourceManager.test.ts (2)
89-90: Implement the TODO test caseThe TODO comment indicates a missing test case for GLTF loader error handling. This should be implemented to ensure proper error handling coverage.
Would you like me to help implement the missing test case for invalid query URL handling in the GLTF loader?
80-87: Enhance test case documentation and assertionsThe test case for invalid query parameters could benefit from better documentation and more specific assertions.
- it("invalid q case", async () => { + it("should return undefined when loading a project with invalid query parameters", async () => { const loadRes = await engine.resourceManager.load({ // contains invalid q value cdn url. url: "https://mdn.alipayobjects.com/huamei_aftkdx/afts/file/A*_Ao1QZtL9fMAAAAAAAAAAAAADteEAQ/mock-project.json", type: AssetType.Project }); - expect(loadRes).to.equal(undefined); + expect(loadRes).to.be.undefined; + // Verify no errors were logged + expect(console.error).not.toHaveBeenCalled(); });e2e/case/multi-camera-no-clear.ts (1)
39-42: Remove commented render target code.These commented lines about render target setup should be removed if they're not needed. If they're for future reference, consider documenting them elsewhere.
- // const renderColorTexture = new Texture2D(engine, 1024, 1024); - // const depthTexture = new Texture2D(engine, 1024, 1024, TextureFormat.Depth24Stencil8, false); - // const renderTarget = new RenderTarget(engine, 1024, 1024, renderColorTexture, TextureFormat.Depth); - // const renderTarget = new RenderTarget(engine, 1024, 1024, renderColorTexture, depthTexture);packages/core/src/RenderPipeline/RenderQueue.ts (1)
109-124: Improve code indentation and structure.The code logic is correct, but the indentation is inconsistent. Consider fixing the indentation to improve readability.
Apply this diff to fix the indentation:
- let renderState = shaderPass._renderState; - if (!needMaskType) { - let passQueueType: RenderQueueType; - if (renderState) { - passQueueType = renderState._getRenderQueueByShaderData(shaderPass._renderStateDataMap, materialData); - } else { - renderState = renderStates[j]; - passQueueType = renderState.renderQueueType; } - if (passQueueType !== renderQueueType) { - continue; - } - } + let renderState = shaderPass._renderState; + if (!needMaskType) { + let passQueueType: RenderQueueType; + if (renderState) { + passQueueType = renderState._getRenderQueueByShaderData(shaderPass._renderStateDataMap, materialData); + } else { + renderState = renderStates[j]; + passQueueType = renderState.renderQueueType; + } + + if (passQueueType !== renderQueueType) { + continue; + } + }🧰 Tools
🪛 eslint
[error] 109-110: Delete
⏎(prettier/prettier)
[error] 112-112: Insert
··(prettier/prettier)
[error] 113-113: Insert
··(prettier/prettier)
[error] 114-114: Replace
··········with············(prettier/prettier)
[error] 115-115: Insert
··(prettier/prettier)
[error] 116-116: Insert
··(prettier/prettier)
[error] 117-117: Replace
··········with············(prettier/prettier)
[error] 118-118: Insert
··(prettier/prettier)
[error] 120-120: Insert
··(prettier/prettier)
[error] 121-121: Replace
··········with············(prettier/prettier)
[error] 122-122: Insert
··(prettier/prettier)
[error] 123-123: Insert
··(prettier/prettier)
🪛 GitHub Check: lint
[failure] 109-109:
Delete⏎
[failure] 112-112:
Insert··
[failure] 113-113:
Insert··
[failure] 114-114:
Replace··········with············
[failure] 115-115:
Insert··
[failure] 116-116:
Insert··
[failure] 117-117:
Replace··········with············
[failure] 118-118:
Insert··
[failure] 120-120:
Insert··
[failure] 121-121:
Replace··········with············
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (5)
e2e/fixtures/originImage/Advance_multi-camera-no-clear.jpgis excluded by!**/*.jpge2e/fixtures/originImage/Advance_multi-scene-clear.jpgis excluded by!**/*.jpge2e/fixtures/originImage/Advance_multi-scene-no-clear.jpgis excluded by!**/*.jpge2e/fixtures/originImage/Material_shaderLab-renderState.jpgis excluded by!**/*.jpgpackages/core/src/shaderlib/extra/Blit.fs.glslis excluded by!**/*.glsl
📒 Files selected for processing (22)
e2e/case/.mockForE2E.ts(4 hunks)e2e/case/multi-camera-no-clear.ts(1 hunks)e2e/case/multi-scene-clear.ts(1 hunks)e2e/case/multi-scene-no-clear.ts(1 hunks)e2e/case/shaderLab-renderState.ts(1 hunks)e2e/config.ts(2 hunks)packages/core/src/Camera.ts(2 hunks)packages/core/src/Polyfill.ts(1 hunks)packages/core/src/RenderPipeline/BasicRenderPipeline.ts(7 hunks)packages/core/src/RenderPipeline/PipelineUtils.ts(5 hunks)packages/core/src/RenderPipeline/RenderQueue.ts(1 hunks)packages/core/src/Transform.ts(13 hunks)packages/core/src/asset/ResourceManager.ts(2 hunks)packages/core/src/index.ts(1 hunks)packages/core/src/particle/ParticleGenerator.ts(1 hunks)packages/core/src/particle/ParticleRenderer.ts(1 hunks)packages/core/src/shader/state/RenderState.ts(1 hunks)packages/loader/src/ShaderChunkLoader.ts(1 hunks)packages/rhi-webgl/src/WebGLGraphicDevice.ts(8 hunks)tests/src/core/Polyfill.test.ts(1 hunks)tests/src/core/Transform.test.ts(1 hunks)tests/src/core/resource/ResourceManager.test.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/Polyfill.ts
[error] 6-36: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 14-14: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
🪛 GitHub Check: codecov
tests/src/core/Polyfill.test.ts
[failure] 3-3: src/core/Polyfill.test.ts
ReferenceError: describe is not defined
❯ src/core/Polyfill.test.ts:3:0
🪛 eslint
packages/core/src/RenderPipeline/RenderQueue.ts
[error] 109-110: Delete ⏎
(prettier/prettier)
[error] 112-112: Insert ··
(prettier/prettier)
[error] 113-113: Insert ··
(prettier/prettier)
[error] 114-114: Replace ·········· with ············
(prettier/prettier)
[error] 115-115: Insert ··
(prettier/prettier)
[error] 116-116: Insert ··
(prettier/prettier)
[error] 117-117: Replace ·········· with ············
(prettier/prettier)
[error] 118-118: Insert ··
(prettier/prettier)
[error] 120-120: Insert ··
(prettier/prettier)
[error] 121-121: Replace ·········· with ············
(prettier/prettier)
[error] 122-122: Insert ··
(prettier/prettier)
[error] 123-123: Insert ··
(prettier/prettier)
🪛 GitHub Check: lint
packages/core/src/RenderPipeline/RenderQueue.ts
[failure] 109-109:
Delete ⏎
[failure] 112-112:
Insert ··
[failure] 113-113:
Insert ··
[failure] 114-114:
Replace ·········· with ············
[failure] 115-115:
Insert ··
[failure] 116-116:
Insert ··
[failure] 117-117:
Replace ·········· with ············
[failure] 118-118:
Insert ··
[failure] 120-120:
Insert ··
[failure] 121-121:
Replace ·········· with ············
🔇 Additional comments (18)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1)
89-90:
Ensure all method calls are updated to reflect signature changes
The parameters in the render method have changed from ignoreClear to finalClearFlags. Verify that all calls to render and _drawRenderPass methods have been updated accordingly throughout the codebase to prevent runtime errors.
Run the following script to find any outdated method calls:
packages/loader/src/ShaderChunkLoader.ts (1)
15-15: Verify the impact of relaxed shader include pattern
The regex pattern change removes the requirement for leading whitespace before #include directives. While this allows for more flexible shader code formatting, we should verify that this change:
- Is intentional and aligns with the shader specification
- Doesn't break existing shader includes
Let's verify the impact:
✅ Verification successful
Regex change is safe and consistent with codebase usage
Based on the examination of actual shader files, the removal of required leading whitespace in the regex pattern is correct and matches the codebase's usage patterns. The shader files show both styles of #include directives:
- Direct includes without leading whitespace:
#include <PostCommon> - Includes with leading whitespace (if any) are still matched by
\s*
The change makes the regex more flexible while maintaining compatibility with existing shader code. The previous regex was actually more restrictive than necessary.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing shader includes that might be affected
echo "Searching for shader includes without leading whitespace..."
rg --type glsl "^#include"
echo "Searching for shader includes with leading whitespace..."
rg --type glsl "^\s+#include"
Length of output: 370
Script:
#!/bin/bash
# Search for shader files and their includes using common shader file extensions
echo "Searching for shader files and their includes..."
# First find all potential shader files
fd -e glsl -e vert -e frag -e shader
echo "Searching for #include patterns in found files..."
# Use ripgrep without file type filter
rg "^#include|^\s+#include" -l $(fd -e glsl -e vert -e frag -e shader)
# Also search for test files related to shader loading
echo "Searching for shader loader tests..."
rg -l "ShaderChunkLoader"
Length of output: 14790
Script:
#!/bin/bash
# Let's examine the actual content of some shader files to see the #include patterns
echo "Examining shader files with #include directives..."
rg -A 1 "^#include|^\s+#include" packages/core/src/shaderlib/pbr/pbr_helper.glsl packages/core/src/postProcess/shaders/UberPost.glsl
# Check the tests for ShaderChunkLoader
echo "Checking ShaderChunkLoader tests..."
rg -A 5 "ShaderChunkLoader" packages/loader/src/ShaderChunkLoader.ts
# Look for test files that might validate shader loading
echo "Looking for shader loading tests..."
rg -l "ShaderChunkLoader.*test"
Length of output: 1754
packages/core/src/index.ts (1)
68-68: LGTM! Verify polyfill implementation.
The Polyfill import and registration are appropriately placed after all exports. This ensures polyfills are available before module usage.
Let's verify the polyfill implementation:
Also applies to: 74-74
✅ Verification successful
Let me gather more information about the Polyfill implementation to ensure it's complete.
LGTM! Polyfill implementation is well-implemented and tested.
The Polyfill implementation for String.prototype.matchAll is properly implemented with:
- Correct feature detection and fallback
- Proper error handling for non-global RegExp
- Generator function implementation that matches native behavior
- Comprehensive test coverage including:
- Error cases (non-global RegExp)
- Matching functionality verification against native implementation
- Multiple match cases with capture groups
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Polyfill implementation and usage
# Check if Polyfill is properly implemented
ast-grep --pattern 'class Polyfill {
$$$
static registerPolyfill() {
$$$
}
$$$
}'
# Check for any direct usage of String.prototype.matchAll
rg "String\.prototype\.matchAll"
Length of output: 783
Script:
#!/bin/bash
# Get the full implementation of Polyfill class
cat packages/core/src/Polyfill.ts
# Check test coverage
cat tests/src/core/Polyfill.test.ts
Length of output: 2763
e2e/case/multi-scene-clear.ts (1)
31-50: LGTM! First scene setup is well-structured.
The first scene initialization is well-organized with clear separation of camera and cube setup.
packages/core/src/shader/state/RenderState.ts (1)
62-71: LGTM! Improved function purity.
The refactoring improves the function by:
- Returning a value instead of modifying state
- Using a more accurate function name
- Providing appropriate default fallback
e2e/case/multi-camera-no-clear.ts (2)
1-33: LGTM! Note about antialiasing configuration.
The setup is well-structured with appropriate imports and engine initialization. Antialiasing is explicitly disabled which is good for consistent screenshot testing.
92-97: LGTM! Multi-camera screenshot initialization.
The test properly initializes both cameras for screenshot testing, aligning with the new multi-camera support mentioned in the changes.
e2e/config.ts (1)
123-126: LGTM! Configuration entries for new test cases.
The new configuration entries follow the established pattern and use appropriate thresholds consistent with similar test cases.
Also applies to: 241-254
packages/core/src/RenderPipeline/PipelineUtils.ts (3)
4-4: LGTM! Clean shader macro addition.
The new shader macro for Y-axis flipping follows the existing pattern of macro definitions.
Also applies to: 18-18
Line range hint 154-164: LGTM! Backward-compatible parameter addition.
The new flipYOfSource parameter is well-documented and maintains backward compatibility with its default value.
183-197: Verify macro collection union order.
While the implementation looks correct, please verify that the order of macro collection unions (camera → renderer → material) is intentional and doesn't affect other rendering scenarios.
✅ Verification successful
Let me analyze the macro collection order across the codebase. I'll need to check the implementation of ShaderMacroCollection.unionCollection to understand how it combines the collections.
Let me try a different approach to find the implementation details.
Based on the implementation found in ShaderMacroCollection.ts and the usage patterns across the codebase, I can now provide the final response:
The macro collection union order is correct and consistent.
The implementation in PipelineUtils.ts follows the established pattern across the codebase where:
- Camera's global shader macros form the base
- Renderer/material-specific macros are added on top
- The order (camera → renderer → material) matches other rendering scenarios like Sky rendering and the main render queue
The union operation properly combines the macro collections by performing a bitwise union of their internal masks, making the order of operations safe and consistent with the rest of the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other uses of ShaderMacroCollection.unionCollection to verify consistency
ast-grep --pattern 'ShaderMacroCollection.unionCollection($$$)'
Length of output: 2103
Script:
#!/bin/bash
# Search for ShaderMacroCollection class implementation
ast-grep --pattern 'class ShaderMacroCollection {
$$$
static unionCollection($$$) {
$$$
}
$$$
}'
Length of output: 110
Script:
#!/bin/bash
# Search for ShaderMacroCollection class and its implementation with a simpler pattern
ast-grep --pattern 'class ShaderMacroCollection'
# Also search for unionCollection method implementation
rg "unionCollection" -A 5
Length of output: 4208
packages/core/src/particle/ParticleRenderer.ts (1)
215-222: Performance optimization: Skip rendering when no particles are alive.
Good optimization! The check for aliveParticleCount before rendering prevents unnecessary draw calls.
packages/core/src/asset/ResourceManager.ts (2)
517-517: Improved query parameter handling by prioritizing last occurrence.
The reverse iteration ensures that the last occurrence of the 'q=' parameter takes precedence, which is a more robust approach for handling duplicate query parameters.
576-578: Simplified URL construction with optional query parameter.
The URL construction is now more straightforward and only adds the query parameter when a key is provided.
packages/core/src/Camera.ts (3)
86-86: Documentation improvement looks good!
The clarification about msaaSamples dependency on independentCanvasEnabled helps prevent potential confusion.
168-177: Improved logic for determining independent canvas requirement
The condition has been refined to check the actual format differences between internal color texture and render target, making the determination more precise.
Line range hint 243-244: Improved particle count check logic
The condition has been updated to use _getNotRetiredParticleCount() which provides a more accurate count of active particles, preventing potential issues with particle emission limits.
packages/core/src/particle/ParticleGenerator.ts (1)
243-244: Improved particle count check logic
The condition has been updated to use _getNotRetiredParticleCount() which provides a more accurate count of active particles, preventing potential issues with particle emission limits.
|
|
||
| // Cuboid | ||
| { | ||
| const cuboid = rootEntity.createChild("sphere"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Correct the entity name for the cuboid
The cuboid object is created with the name "sphere", which is inconsistent and may cause confusion or errors in entity management. It should be named "cuboid" to reflect its true nature.
Apply this diff to fix the entity name:
- const cuboid = rootEntity.createChild("sphere");
+ const cuboid = rootEntity.createChild("cuboid");📝 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 cuboid = rootEntity.createChild("sphere"); | |
| const cuboid = rootEntity.createChild("cuboid"); |
| blitInternalRTByBlitFrameBuffer( | ||
| srcRT: RenderTarget, | ||
| destRT: RenderTarget, | ||
| clearFlags: CameraClearFlags, | ||
| viewport: Vector4 | ||
| ) { | ||
| if (!this._isWebGL2) { | ||
| Logger.warn("WebGL1.0 not support blit frame buffer."); | ||
| return; | ||
| } | ||
| const gl = this._gl; | ||
| // @ts-ignore | ||
| const srcFrameBuffer = srcRT ? srcRT._platformRenderTarget._frameBuffer : null; | ||
| // @ts-ignore | ||
| const destFrameBuffer = destRT ? destRT._platformRenderTarget._frameBuffer : null; | ||
| const bufferWidth = this.getMainFrameBufferWidth(); | ||
| const bufferHeight = this.getMainFrameBufferHeight(); | ||
| const srcWidth = srcRT ? srcRT.width : bufferWidth; | ||
| const srcHeight = srcRT ? srcRT.height : bufferHeight; | ||
| const blitWidth = destRT.width; | ||
| const blitHeight = destRT.height; | ||
| const needFlipY = !srcRT; | ||
| const needBlitColor = (clearFlags & CameraClearFlags.Color) === 0; | ||
| const needBlitDepth = (clearFlags & CameraClearFlags.Depth) === 0; | ||
| const needBlitStencil = (clearFlags & CameraClearFlags.Stencil) === 0; | ||
|
|
||
| gl.bindFramebuffer(gl.READ_FRAMEBUFFER, srcFrameBuffer); | ||
| gl.bindFramebuffer(gl.DRAW_FRAMEBUFFER, destFrameBuffer); | ||
|
|
||
| let blitMask = needBlitColor ? gl.COLOR_BUFFER_BIT : 0; | ||
|
|
||
| if (needBlitDepth || needBlitStencil) { | ||
| // @ts-ignore | ||
| const depthFormat = destRT._depthFormat; | ||
|
|
||
| if (needBlitDepth) { | ||
| if ( | ||
| depthFormat === TextureFormat.Depth || | ||
| (depthFormat >= TextureFormat.DepthStencil && depthFormat <= TextureFormat.Depth32Stencil8) | ||
| ) { | ||
| blitMask |= gl.DEPTH_BUFFER_BIT; | ||
| } else { | ||
| Logger.warn(`Do not clear depth, or set depth format of target which is ${TextureFormat[depthFormat]} now.`); | ||
| } | ||
| } | ||
|
|
||
| if (needBlitStencil) { | ||
| if ( | ||
| depthFormat === TextureFormat.Stencil || | ||
| depthFormat === TextureFormat.DepthStencil || | ||
| depthFormat >= TextureFormat.Depth24Stencil8 || | ||
| depthFormat >= TextureFormat.Depth32Stencil8 | ||
| ) { | ||
| blitMask |= gl.STENCIL_BUFFER_BIT; | ||
| } else { | ||
| Logger.warn( | ||
| `Do not clear stencil, or set stencil format of target which is ${TextureFormat[depthFormat]} now.` | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const xStart = viewport.x * srcWidth; | ||
| const xEnd = xStart + blitWidth; | ||
| const yStart = needFlipY ? srcHeight - viewport.y * srcHeight : srcHeight - viewport.y * srcHeight - blitHeight; | ||
| const yEnd = needFlipY ? yStart - blitHeight : yStart + blitHeight; | ||
|
|
||
| gl.blitFramebuffer(xStart, yStart, xEnd, yEnd, 0, 0, blitWidth, blitHeight, blitMask, gl.NEAREST); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
WebGL version check missing in copyRenderTargetToSubTexture method
The copyRenderTargetToSubTexture method is missing the WebGL 2.0 compatibility check, while being called from BasicRenderPipeline. This could lead to runtime errors on WebGL 1.0 platforms. The method should include a version check similar to blitInternalRTByBlitFrameBuffer.
packages/rhi-webgl/src/WebGLGraphicDevice.ts: Add WebGL 2.0 check at the start ofcopyRenderTargetToSubTexturemethodpackages/core/src/RenderPipeline/BasicRenderPipeline.ts: Add WebGL version check before callingcopyRenderTargetToSubTexture
🔗 Analysis chain
Check for WebGL version compatibility in framebuffer methods
The newly added methods blitInternalRTByBlitFrameBuffer and copyRenderTargetToSubTexture include WebGL 2.0 specific functionality. Ensure that appropriate checks are in place to prevent issues on platforms that only support WebGL 1.0.
Verify that all usages of these methods safeguard against WebGL 1.0 contexts:
If any usages are found without version checks, consider wrapping them with conditions that verify this._isWebGL2 is true.
Let me verify the implementation of copyRenderTargetToSubTexture to ensure it has WebGL 2.0 checks similar to blitInternalRTByBlitFrameBuffer.
Let me try a different approach to get the implementation.
Let me try one more time with a simpler ripgrep command.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Ensure methods are only called when WebGL 2.0 is available.
# Check for calls to 'blitInternalRTByBlitFrameBuffer' without WebGL version checks
rg -A2 'blitInternalRTByBlitFrameBuffer' packages | rg -B2 -v '_isWebGL2'
# Check for calls to 'copyRenderTargetToSubTexture' without WebGL version checks
rg -A2 'copyRenderTargetToSubTexture' packages | rg -B2 -v '_isWebGL2'
Length of output: 1464
Script:
#!/bin/bash
# Get the implementation of copyRenderTargetToSubTexture
ast-grep --pattern 'copyRenderTargetToSubTexture($_) {
$$$
}'
Length of output: 65
Script:
#!/bin/bash
# Get the implementation of copyRenderTargetToSubTexture using ripgrep with context
rg -U --multiline --context 20 "copyRenderTargetToSubTexture.*?\{.*?\}" packages/rhi-webgl/src/WebGLGraphicDevice.ts
Length of output: 118
Script:
#!/bin/bash
# Get the implementation of copyRenderTargetToSubTexture
rg -A 30 "^ copyRenderTargetToSubTexture" packages/rhi-webgl/src/WebGLGraphicDevice.ts
Length of output: 1226
|
|
||
| return (function* () { | ||
| const matchResult = bindThis.match(pattern) as string[]; | ||
| if (matchResult == null) return null; |
There was a problem hiding this comment.
Fix incorrect return of 'null' from generator function
Returning null from a generator function results in a TypeError when iterated over. Instead, the generator should simply return without yielding any values.
Apply this diff to correct the return statement:
- if (matchResult == null) return null;
+ if (matchResult == null) return;📝 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.
| if (matchResult == null) return null; | |
| if (matchResult == null) return; |
| it("String.prototype.matchAll", async () => { | ||
| const regex = /#include\s+"([./][^\\"]+)"/gm; | ||
| const content = `#include "./f1.glsl" | ||
| #include "./f2.glsl" | ||
| xxx | ||
| #include "/f3.glsl" | ||
| ooo`; | ||
| const originMatchResult = content.matchAll(regex); | ||
| const originResultArray = Array.from(originMatchResult); | ||
|
|
||
| String.prototype.matchAll = null; | ||
| import("@galacean/engine-core").then(() => { | ||
| const regexTest = /noGlobal/; | ||
| const contentTest = "test"; | ||
| expect( | ||
| String.prototype.matchAll.bind(contentTest, regexTest), | ||
| "Should throw error caused by absence of g flag" | ||
| ).to.throw(TypeError); | ||
|
|
||
| const matchResult = content.matchAll(regex); | ||
| expect(!!matchResult.next, "result should be iterable").to.be.true; | ||
| const resultArray = Array.from(matchResult); | ||
| expect(resultArray.length).to.equal(3).to.equal(originResultArray.length); | ||
|
|
||
| expect(resultArray[0][0]).to.equal(originResultArray[0][0]).to.equal('#include "./f1.glsl"'); | ||
| expect(resultArray[0][1]).to.equal(originResultArray[0][1]).to.equal("./f1.glsl"); | ||
|
|
||
| expect(resultArray[1][0]).to.equal(originResultArray[1][0]).to.equal('#include "./f2.glsl"'); | ||
| expect(resultArray[1][1]).to.equal(originResultArray[1][1]).to.equal("./f2.glsl"); | ||
|
|
||
| expect(resultArray[2][0]).to.equal(originResultArray[2][0]).to.equal('#include "/f3.glsl"'); | ||
| expect(resultArray[2][1]).to.equal(originResultArray[2][1]).to.equal("/f3.glsl"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Improve async test reliability and error handling
The async test implementation has several areas for improvement:
- The Promise chain lacks error handling
- The test may complete before the assertions run
- Static analysis shows 'describe' is undefined
Apply these improvements:
- it("String.prototype.matchAll", async () => {
+ it("String.prototype.matchAll", () => {
- const regex = /#include\s+"([./][^\\"]+)"/gm;
+ const regex = /#include\s+"([./][^\\"]+)"/gm;
const content = `#include "./f1.glsl"
#include "./f2.glsl"
xxx
#include "/f3.glsl"
ooo`;
const originMatchResult = content.matchAll(regex);
const originResultArray = Array.from(originMatchResult);
String.prototype.matchAll = null;
- import("@galacean/engine-core").then(() => {
+ return import("@galacean/engine-core").then(() => {
const regexTest = /noGlobal/;
const contentTest = "test";
expect(
String.prototype.matchAll.bind(contentTest, regexTest),
"Should throw error caused by absence of g flag"
).to.throw(TypeError);
const matchResult = content.matchAll(regex);
expect(!!matchResult.next, "result should be iterable").to.be.true;
const resultArray = Array.from(matchResult);
expect(resultArray.length).to.equal(3).to.equal(originResultArray.length);
expect(resultArray[0][0]).to.equal(originResultArray[0][0]).to.equal('#include "./f1.glsl"');
expect(resultArray[0][1]).to.equal(originResultArray[0][1]).to.equal("./f1.glsl");
expect(resultArray[1][0]).to.equal(originResultArray[1][0]).to.equal('#include "./f2.glsl"');
expect(resultArray[1][1]).to.equal(originResultArray[1][1]).to.equal("./f2.glsl");
expect(resultArray[2][0]).to.equal(originResultArray[2][0]).to.equal('#include "/f3.glsl"');
expect(resultArray[2][1]).to.equal(originResultArray[2][1]).to.equal("/f3.glsl");
- });
+ }).catch((error) => {
+ throw error;
+ });
});📝 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.
| it("String.prototype.matchAll", async () => { | |
| const regex = /#include\s+"([./][^\\"]+)"/gm; | |
| const content = `#include "./f1.glsl" | |
| #include "./f2.glsl" | |
| xxx | |
| #include "/f3.glsl" | |
| ooo`; | |
| const originMatchResult = content.matchAll(regex); | |
| const originResultArray = Array.from(originMatchResult); | |
| String.prototype.matchAll = null; | |
| import("@galacean/engine-core").then(() => { | |
| const regexTest = /noGlobal/; | |
| const contentTest = "test"; | |
| expect( | |
| String.prototype.matchAll.bind(contentTest, regexTest), | |
| "Should throw error caused by absence of g flag" | |
| ).to.throw(TypeError); | |
| const matchResult = content.matchAll(regex); | |
| expect(!!matchResult.next, "result should be iterable").to.be.true; | |
| const resultArray = Array.from(matchResult); | |
| expect(resultArray.length).to.equal(3).to.equal(originResultArray.length); | |
| expect(resultArray[0][0]).to.equal(originResultArray[0][0]).to.equal('#include "./f1.glsl"'); | |
| expect(resultArray[0][1]).to.equal(originResultArray[0][1]).to.equal("./f1.glsl"); | |
| expect(resultArray[1][0]).to.equal(originResultArray[1][0]).to.equal('#include "./f2.glsl"'); | |
| expect(resultArray[1][1]).to.equal(originResultArray[1][1]).to.equal("./f2.glsl"); | |
| expect(resultArray[2][0]).to.equal(originResultArray[2][0]).to.equal('#include "/f3.glsl"'); | |
| expect(resultArray[2][1]).to.equal(originResultArray[2][1]).to.equal("/f3.glsl"); | |
| }); | |
| }); | |
| it("String.prototype.matchAll", () => { | |
| const regex = /#include\s+"([./][^\\"]+)"/gm; | |
| const content = `#include "./f1.glsl" | |
| #include "./f2.glsl" | |
| xxx | |
| #include "/f3.glsl" | |
| ooo`; | |
| const originMatchResult = content.matchAll(regex); | |
| const originResultArray = Array.from(originMatchResult); | |
| String.prototype.matchAll = null; | |
| return import("@galacean/engine-core").then(() => { | |
| const regexTest = /noGlobal/; | |
| const contentTest = "test"; | |
| expect( | |
| String.prototype.matchAll.bind(contentTest, regexTest), | |
| "Should throw error caused by absence of g flag" | |
| ).to.throw(TypeError); | |
| const matchResult = content.matchAll(regex); | |
| expect(!!matchResult.next, "result should be iterable").to.be.true; | |
| const resultArray = Array.from(matchResult); | |
| expect(resultArray.length).to.equal(3).to.equal(originResultArray.length); | |
| expect(resultArray[0][0]).to.equal(originResultArray[0][0]).to.equal('#include "./f1.glsl"'); | |
| expect(resultArray[0][1]).to.equal(originResultArray[0][1]).to.equal("./f1.glsl"); | |
| expect(resultArray[1][0]).to.equal(originResultArray[1][0]).to.equal('#include "./f2.glsl"'); | |
| expect(resultArray[1][1]).to.equal(originResultArray[1][1]).to.equal("./f2.glsl"); | |
| expect(resultArray[2][0]).to.equal(originResultArray[2][0]).to.equal('#include "/f3.glsl"'); | |
| expect(resultArray[2][1]).to.equal(originResultArray[2][1]).to.equal("/f3.glsl"); | |
| }).catch((error) => { | |
| throw error; | |
| }); | |
| }); |
| const cameraEntity = rootEntity.createChild("window-camera"); | ||
| const camera = cameraEntity.addComponent(Camera); | ||
| camera.enablePostProcess = true; | ||
| // camera.clearFlags = CameraClearFlags.None; |
There was a problem hiding this comment.
Remove commented code and fix TypeScript ignores.
There are multiple issues in this section:
- Commented-out camera configuration
- TypeScript ignores for accessing private members
Consider:
- Either remove or implement the camera clear flags configuration
- Create proper public APIs instead of accessing private members with @ts-ignore
Also applies to: 63-66
| // @ts-ignore | ||
| const bloomEffect = scene._postProcessManager._bloomEffect; | ||
| // @ts-ignore | ||
| const tonemappingEffect = scene._postProcessManager._tonemappingEffect; |
There was a problem hiding this comment.
Remove @ts-ignore and use proper type assertions
Using @ts-ignore suppresses TypeScript checks and can hide potential issues. Consider properly typing the _postProcessManager or using type assertions if the properties are intentionally internal.
- // @ts-ignore
- const bloomEffect = scene._postProcessManager._bloomEffect;
- // @ts-ignore
- const tonemappingEffect = scene._postProcessManager._tonemappingEffect;
+ const postProcessManager = (scene as any)._postProcessManager;
+ const bloomEffect = postProcessManager._bloomEffect;
+ const tonemappingEffect = postProcessManager._tonemappingEffect;📝 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.
| // @ts-ignore | |
| const bloomEffect = scene._postProcessManager._bloomEffect; | |
| // @ts-ignore | |
| const tonemappingEffect = scene._postProcessManager._tonemappingEffect; | |
| const postProcessManager = (scene as any)._postProcessManager; | |
| const bloomEffect = postProcessManager._bloomEffect; | |
| const tonemappingEffect = postProcessManager._tonemappingEffect; |
| @@ -94,8 +104,7 @@ export function initScreenshot( | |||
| // window.URL.revokeObjectURL(url); | |||
|
|
|||
| // revert | |||
| camera.renderTarget = originalTarget; | |||
| camera.resetAspectRatio(); | |||
| callbacks.forEach((cb) => cb()); | |||
There was a problem hiding this comment.
Ensure resource cleanup on error
The callbacks are only executed after successful screenshot creation. Consider ensuring cleanup runs even if the blob creation fails.
+ const cleanup = () => {
+ callbacks.forEach((cb) => cb());
+ !isPaused && engine.resume();
+ renderTarget.destroy();
+ renderColorTexture.destroy();
+ };
+
+ try {
canvas.toBlob(
(blob) => {
// ... existing code ...
- callbacks.forEach((cb) => cb());
- !isPaused && engine.resume();
+ cleanup();
},
isPNG ? "image/png" : "image/jpeg",
!isPNG && jpgQuality
);
+ } catch (error) {
+ cleanup();
+ throw error;
+ }Committable suggestion skipped: line range outside the PR's diff.
| const cameras = Array.isArray(camera) ? camera : [camera]; | ||
| const callbacks = []; | ||
| const renderColorTexture = new Texture2D(engine, width, height); | ||
| const renderTargetData = new Uint8Array(width * height * 4); | ||
| const renderTarget = new RenderTarget(engine, width, height, renderColorTexture, undefined, 1); | ||
| const renderTarget = new RenderTarget(engine, width, height, renderColorTexture, TextureFormat.Depth24Stencil8, 1); | ||
|
|
||
| // render to off-screen | ||
| camera.renderTarget = renderTarget; | ||
| camera.aspectRatio = width / height; | ||
| camera.render(); | ||
| cameras.forEach((camera) => { | ||
| const originalTarget = camera.renderTarget; | ||
|
|
||
| // render to off-screen | ||
| camera.renderTarget = renderTarget; | ||
| camera.aspectRatio = width / height; | ||
| camera.render(); | ||
|
|
||
| callbacks.push(() => { | ||
| camera.renderTarget = originalTarget; | ||
| camera.resetAspectRatio(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Add error handling for render target creation
The code creates resources without error handling. Consider adding try-catch blocks and proper cleanup if resource creation fails.
+ try {
const cameras = Array.isArray(camera) ? camera : [camera];
const callbacks = [];
const renderColorTexture = new Texture2D(engine, width, height);
const renderTargetData = new Uint8Array(width * height * 4);
const renderTarget = new RenderTarget(engine, width, height, renderColorTexture, TextureFormat.Depth24Stencil8, 1);
+ } catch (error) {
+ console.error("Failed to create render resources:", error);
+ renderColorTexture?.destroy();
+ renderTarget?.destroy();
+ throw error;
+ }Committable suggestion skipped: line range outside the PR's diff.
| // @ts-ignore | ||
| const bloomEffect = scene._postProcessManager._bloomEffect; | ||
| // @ts-ignore | ||
| const tonemappingEffect = scene._postProcessManager._tonemappingEffect; |
There was a problem hiding this comment.
Avoid using @ts-ignore and accessing private members.
Direct access to private members (_postProcessManager, _bloomEffect, _tonemappingEffect) with @ts-ignore is not recommended. Consider:
- Using public APIs if available
- Adding accessor methods if needed
- Documenting why private access is necessary if unavoidable
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.4 #2450 +/- ##
===========================================
+ Coverage 67.12% 67.69% +0.57%
===========================================
Files 897 902 +5
Lines 93218 93861 +643
Branches 7581 7821 +240
===========================================
+ Hits 62572 63541 +969
+ Misses 30401 30071 -330
- Partials 245 249 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
tests/src/core/resource/ResourceManager.test.ts (3)
71-74: Fix formatting issuesRemove unnecessary whitespace and improve URL string formatting for better readability.
Apply this diff:
- - engine.resourceManager.load("https://gw.alipayobjects.com/os/bmw-prod/5e3c1e4e-496e-45f8-8e05-f89f2bd5e4a4.glb"); - engine.resourceManager.load("https://gw.alipayobjects.com/os/bmw-prod/5e3c1e4e-496e-45f8-8e05-f89f2bd5e4a4.glb"); - engine.resourceManager.load("https://gw.alipayobjects.com/os/bmw-prod/5e3c1e4e-496e-45f8-8e05-f89f2bd5e4a4.glb?q=materials[0]"); + const baseUrl = "https://gw.alipayobjects.com/os/bmw-prod/5e3c1e4e-496e-45f8-8e05-f89f2bd5e4a4.glb"; + engine.resourceManager.load(baseUrl); + engine.resourceManager.load(baseUrl); + engine.resourceManager.load(`${baseUrl}?q=materials[0]`);🧰 Tools
🪛 eslint
[error] 71-71: Delete
······(prettier/prettier)
[error] 74-74: Replace
"https://gw.alipayobjects.com/os/bmw-prod/5e3c1e4e-496e-45f8-8e05-f89f2bd5e4a4.glb?q=materials[0]"with⏎········"https://gw.alipayobjects.com/os/bmw-prod/5e3c1e4e-496e-45f8-8e05-f89f2bd5e4a4.glb?q=materials[0]"⏎······(prettier/prettier)
80-87: Improve test case documentation and implementationThe test case could be more specific about what makes the query invalid and should consider using a mock URL instead of a real endpoint.
Consider this improvement:
- it("invalid q case", async () => { + it("should return undefined when loading a project with invalid query parameter", async () => { + const mockUrl = "mock://invalid-project.json"; const loadRes = await engine.resourceManager.load({ - // contains invalid q value cdn url. - url: "https://mdn.alipayobjects.com/huamei_aftkdx/afts/file/A*_Ao1QZtL9fMAAAAAAAAAAAAADteEAQ/mock-project.json", + url: mockUrl, type: AssetType.Project }); expect(loadRes).to.equal(undefined); });
89-89: Enhance TODO comment with implementation detailsThe TODO comment should specify the expected error type and conditions.
Consider updating the comment:
- // TODO: case for gltf loader load invalid q url, expect to throw + // TODO: Add test case to verify that the GLTF loader throws an appropriate error + // when loading a URL with an invalid query parameter format (e.g., malformed q= value)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests/src/core/resource/ResourceManager.test.ts(2 hunks)
🧰 Additional context used
🪛 eslint
tests/src/core/resource/ResourceManager.test.ts
[error] 71-71: Delete ······
(prettier/prettier)
[error] 74-74: Replace "https://gw.alipayobjects.com/os/bmw-prod/5e3c1e4e-496e-45f8-8e05-f89f2bd5e4a4.glb?q=materials[0]" with ⏎········"https://gw.alipayobjects.com/os/bmw-prod/5e3c1e4e-496e-45f8-8e05-f89f2bd5e4a4.glb?q=materials[0]"⏎······
(prettier/prettier)
🔇 Additional comments (1)
tests/src/core/resource/ResourceManager.test.ts (1)
1-1: LGTM: Import changes are well-organized
The addition of AssetType import and reordering of vitest imports maintain good code organization.
Also applies to: 4-4
| describe("gltf subAsset load", () => { | ||
| it("invalid q case", async () => { | ||
| const loadRes = await engine.resourceManager.load({ | ||
| // contains invalid q value cdn url. | ||
| url: "https://mdn.alipayobjects.com/huamei_aftkdx/afts/file/A*_Ao1QZtL9fMAAAAAAAAAAAAADteEAQ/mock-project.json", | ||
| type: AssetType.Project | ||
| }); | ||
| expect(loadRes).to.equal(undefined); | ||
| }); | ||
|
|
||
| // TODO: case for gltf loader load invalid q url, expect to throw | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
Test coverage for GLTF loading needs improvement
The current test coverage for GLTF loading has gaps in error handling:
- While there are tests for successful GLTF loading scenarios and resource management
- The TODO comment about testing invalid GLTF URL handling is not implemented
- No explicit error handling tests for GLTF loading failures
- Missing edge cases for malformed GLTF content
Recommended test additions:
- Implement the pending test case for invalid GLTF URL handling
- Add test cases for malformed GLTF content
- Add test cases for network failures during GLTF loading
- Add test cases for invalid GLTF schema validation
🔗 Analysis chain
Verify test coverage for GLTF subAsset loading
Let's ensure we have comprehensive test coverage for GLTF subAsset loading scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing GLTF loader tests
echo "Searching for existing GLTF loader tests..."
rg -l "gltf.*load" tests/
rg -l "ResourceManager.*load.*gltf" tests/
# Search for related error handling tests
echo "Searching for related error handling tests..."
rg -l "invalid.*url" tests/
rg -l "throw.*error.*load" tests/
Length of output: 432
Script:
#!/bin/bash
# Let's examine the actual test content for GLTF loading
echo "Checking GLTF loader test implementations..."
rg -B 2 -A 5 "gltf.*load" tests/src/core/resource/ResourceManager.test.ts
echo "\nChecking error handling test implementations..."
rg -B 2 -A 5 "invalid.*url" tests/src/core/resource/ResourceManager.test.ts
echo "\nChecking for GLTF type definitions and related code..."
rg "AssetType\.GLTF|AssetType\.gltf" tests/
Length of output: 1594
Script:
#!/bin/bash
# Let's check the GLTFLoader test file content
echo "Examining GLTFLoader test cases..."
rg -B 2 -A 10 "AssetType\.GLTF" tests/src/loader/GLTFLoader.test.ts
# Let's also check if there are any other error handling patterns in GLTFLoader tests
echo "\nChecking error handling in GLTFLoader tests..."
rg -B 2 -A 5 "error|throw|invalid|fail" tests/src/loader/GLTFLoader.test.ts
Length of output: 1963
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
Release Notes
New Features
String.prototype.matchAll.Bug Fixes
Documentation
CameraandTransform.Tests
Polyfill,Transform, andResourceManagerfunctionalities.