Merge origin/dev into origin/main #795
Conversation
* test: 完善部分单测类型，优化帧对比失败提示 * fix: 根据机器人建议优化代码
perf: opt sprite shader alpha clip performance
WalkthroughThis pull request introduces modifications across multiple packages, focusing on test infrastructure, rendering components, and file structure reorganization. The changes span test cases, rendering logic, and module imports, with key updates in file paths, type annotations, and component behaviors. The modifications aim to enhance type safety, streamline test configurations, and refine rendering and hit testing mechanisms. Changes
Sequence DiagramsequenceDiagram
participant BaseRenderComponent
participant Renderer
participant HitTestParams
BaseRenderComponent->>Renderer: Initialize with parameters
BaseRenderComponent->>BaseRenderComponent: Check texParams.x
alt texParams.x is zero
BaseRenderComponent->>Renderer: Enable ALPHA_CLIP macro
else
BaseRenderComponent->>Renderer: Disable ALPHA_CLIP macro
end
BaseRenderComponent->>HitTestParams: Generate hit test parameters
HitTestParams-->>BaseRenderComponent: Return hit test configuration
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
web-packages/test/unit/src/effects-webgl/gl-frame-buffer.spec.ts (2)
Line range hint
28-33: Consider removing arbitrary sleep.The
sleep(3000)seems arbitrary and could make tests flaky. Consider:
- Using proper async setup if waiting for specific initialization
- Removing if not strictly necessary
- Adding a comment explaining why the sleep is needed if it must stay
Line range hint
261-279: Enhance error case testing.The error case test for unsupported depth texture could be more comprehensive:
- Consider testing other error scenarios
- Add assertions for the specific error message
- Consider testing the state after the error
🧹 Nitpick comments (15)
web-packages/test/memory/src/memory-leak.spec.ts (3)
22-26: Consider usingundefinedinstead ofnullfor cleanup.While the current implementation works, using
undefinedinstead ofnullwould be more idiomatic in TypeScript and wouldn't require type error suppression:- // @ts-expect-error - memoryTool = null; + memoryTool = undefined; - // @ts-expect-error - canvas = null; + canvas = undefined;Also applies to: 74-79
36-36: Consider refactoring repeated type assertions.The type assertion
as keyof typeof sceneListis repeated multiple times. Consider creating a helper function or type-safe accessor:function getScene(key: string) { return sceneList[key as keyof typeof sceneList]; }This would reduce repetition and maintain type safety.
Also applies to: 96-96, 109-109
29-31: Consider adding deterministic test cases.While random testing is valuable, consider adding a few deterministic test cases with fixed seeds and speeds to ensure consistent baseline testing. This would help in reproducing and debugging issues when they occur.
Also applies to: 81-83
web-packages/test/memory/src/scene-list.ts (1)
193-225: Add unit tests for new WuFu scenes.
The newly introduced scenes (WuFu1_2 through WuFu1_9) look fine, but each scene should have test coverage if utilized in production code or user-facing features, ensuring that the rendering or animations work as expected.Need help adding or updating test files for these new scenes?
plugin-packages/orientation-transformer/demo/src/simple.ts (1)
4-4: Consider externalizing the scene URLThe hardcoded URL should be moved to a configuration file or environment variable for better maintainability and flexibility across different environments.
-const json = 'https://mdn.alipayobjects.com/mars/afts/file/A*GbiuQIresOsAAAAAAAAAAAAADlB4AQ'; +const json = process.env.SCENE_URL || 'https://mdn.alipayobjects.com/mars/afts/file/A*GbiuQIresOsAAAAAAAAAAAAADlB4AQ';web-packages/test/case/src/common/test-controller.ts (1)
7-7: Consider version management improvementsThe hardcoded version comment is outdated and could lead to confusion. Also, relying solely on unpkg.com without a fallback could cause reliability issues.
Consider:
- Using a version constant or configuration file
- Adding a fallback CDN
- Documenting version compatibility requirements
packages/effects-core/src/plugins/sprite/sprite-mesh.ts (1)
Line range hint
77-81: Remove or implement TODO commentThe TODO comment indicates this is only used in tests, but there's no tracking issue or timeline for addressing it.
Consider either:
- Creating a GitHub issue to track this
- Removing if no longer needed
- Adding more context about the test-only usage
web-packages/test/case/spine.html (1)
Line range hint
47-47: Document the test file reorganization in READMEThe changes consistently reorganize test files into subdirectories (2d, 3d, spine) which improves code organization. Consider adding documentation about this new structure and any implications for running tests.
Consider adding or updating the test documentation to:
- Explain the new directory structure
- Document any changes to test running procedures
- Clarify the usage of different file extensions (.ts vs .spec.ts)
Also applies to: 52-52, 63-63
web-packages/test/unit/src/effects-webgl/math-utils.ts (1)
68-69: Consider using early returns for boundary conditionsThe if-statements in
smoothstepandsmootherstepcould be simplified using early returns for better readability.- if (x <= min) { return 0; } - if (x >= max) { return 1; } + if (x <= min) return 0; + if (x >= max) return 1;Also applies to: 77-78
web-packages/test/case/src/2d/dynamic.spec.ts (1)
53-53: Consider adding frame timing information to diff reportsWhile collecting all differences for a final assertion is consistent with other test files, for dynamic tests with hit testing, it would be valuable to include timing information in the diffRatioList to help identify patterns in rendering discrepancies.
- diffRatioList.push(diffCountRatio); + diffRatioList.push({ time, ratio: diffCountRatio });Also applies to: 120-120, 124-124
web-packages/test/case/src/2d/interact.spec.ts (1)
54-54: Consider memory usage optimization for diffRatioListThe new approach of collecting all differences before failing provides better error reporting but could consume more memory with many differences. Consider adding a maximum limit to the number of differences collected.
const diffRatioList = []; +const MAX_DIFF_COUNT = 10; // Limit memory usage // ... in the comparison loop if (diffCountRatio > accumRatioThreshold) { + if (diffRatioList.length < MAX_DIFF_COUNT) { diffRatioList.push(diffCountRatio); + } }Also applies to: 120-120, 124-124
web-packages/test/case/src/3d/case.spec.ts (1)
Line range hint
91-95: Consider moving time list to a constantThe time list for full tests could be moved to a constant to improve maintainability and reusability.
+const FULL_TIME_TEST_POINTS = [ + 0.11, 0.22, 0.34, 0.45, 0.57, 0.65, 0.71, 0.83, 0.96, 1.0, + 1.1, 1.2, 1.3, 1.4, 1.5, 1.7, 1.9, 2.0, 2.2, 2.5, 2.7, 3.0, 3.3, 3.8, + 4.1, 4.7, 5.2, 5.9, 6.8, 7.5, 8.6, 9.7, 9.99, 11.23, 12.5, 15.8, 18.9, +]; if (isFullTimeTest(name)) { - timeList.push( - 0.11, 0.22, 0.34, 0.45, 0.57, 0.65, 0.71, 0.83, 0.96, 1.0, - 1.1, 1.2, 1.3, 1.4, 1.5, 1.7, 1.9, 2.0, 2.2, 2.5, 2.7, 3.0, 3.3, 3.8, - 4.1, 4.7, 5.2, 5.9, 6.8, 7.5, 8.6, 9.7, 9.99, 11.23, 12.5, 15.8, 18.9, - ); + timeList.push(...FULL_TIME_TEST_POINTS); }web-packages/test/unit/src/effects-webgl/gl-frame-buffer.spec.ts (3)
11-19: Consider adding type annotation for colorTexOptions.While the type annotations for the function parameters are good, consider explicitly typing the
colorTexOptionsobject for better type safety.- const colorTexOptions = { + const colorTexOptions: TextureSourceOptions = { sourceType: TextureSourceType.framebuffer, format: glContext.RGBA, internalFormat: glContext.RGBA, type: glContext.UNSIGNED_BYTE, };
314-314: Consider extracting repeated GLTexture instantiation pattern.The pattern of creating a new GLTexture with colorTexOptions is repeated across multiple test cases. Consider extracting this into a helper function to reduce duplication and improve maintainability.
function createColorTexture(engine: Engine, options: Partial<TextureSourceOptions> = {}): GLTexture { return new GLTexture(engine, { ...colorTexOptions, ...options, } as TextureSourceOptions); }Also applies to: 343-343, 427-427, 454-454
Line range hint
1-485: Consider organizing tests by feature category.The test suite is comprehensive but could benefit from better organization:
- Consider grouping related tests using
describeblocks (e.g., "Color Attachments", "Depth/Stencil", "Error Cases")- Consider adding more descriptive test names that follow the given-when-then pattern
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/effects-core/src/shader/item.frag.glslis excluded by!**/*.glslpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
packages/effects-core/src/components/base-render-component.ts(1 hunks)packages/effects-core/src/plugins/sprite/sprite-mesh.ts(1 hunks)plugin-packages/orientation-transformer/demo/src/simple.ts(1 hunks)web-packages/test/case/2d.html(1 hunks)web-packages/test/case/3d.html(1 hunks)web-packages/test/case/all.html(1 hunks)web-packages/test/case/index.html(1 hunks)web-packages/test/case/spine.html(1 hunks)web-packages/test/case/src/2d/dynamic.spec.ts(3 hunks)web-packages/test/case/src/2d/inspire.spec.ts(3 hunks)web-packages/test/case/src/2d/interact.spec.ts(3 hunks)web-packages/test/case/src/3d/case.spec.ts(3 hunks)web-packages/test/case/src/3d/gltf.spec.ts(3 hunks)web-packages/test/case/src/all/index.ts(1 hunks)web-packages/test/case/src/common/image-comparator.ts(0 hunks)web-packages/test/case/src/common/test-controller.ts(1 hunks)web-packages/test/case/src/common/test-player.ts(0 hunks)web-packages/test/case/src/spine/index.spec.ts(3 hunks)web-packages/test/memory/src/memory-leak.spec.ts(8 hunks)web-packages/test/memory/src/scene-list.ts(1 hunks)web-packages/test/package.json(1 hunks)web-packages/test/unit/src/effects-webgl/gl-frame-buffer.spec.ts(22 hunks)web-packages/test/unit/src/effects-webgl/gl-state.spec.ts(5 hunks)web-packages/test/unit/src/effects-webgl/math-utils.ts(4 hunks)
💤 Files with no reviewable changes (2)
- web-packages/test/case/src/common/image-comparator.ts
- web-packages/test/case/src/common/test-player.ts
🔇 Additional comments (25)
web-packages/test/package.json (1)
9-14: LGTM! Dependencies are properly configured.
The new workspace dependencies are correctly added with workspace:* version specifier, which is appropriate for monorepo internal dependencies.
web-packages/test/memory/src/memory-leak.spec.ts (2)
1-2: LGTM! Import simplification and type safety improvements.
The changes properly add type annotations and simplify imports while maintaining functionality.
Also applies to: 11-12
Line range hint 39-55: Verify memory leak testing coverage across packages.
The memory leak testing implementation looks solid, but let's verify if similar tests exist in other packages.
✅ Verification successful
Memory leak testing coverage is comprehensive and well-implemented
The verification reveals that memory leak testing is properly implemented across the codebase:
- Main memory leak test suite in
web-packages/test/memory/src/memory-leak.spec.tscovers both single and multiple scene scenarios - Plugin-specific memory leak test in
plugin-packages/model/test/src/memory-leak.spec.tsreuses the same testing utilities - Both test suites use
GPUMemoryToolfrom a shared location and verify WebGL leaks withcheckWebGLLeak()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar memory leak tests in other packages
echo "Searching for memory leak test files..."
fd -e spec.ts -e test.ts memory-leak web-packages
echo "Searching for GPUMemoryTool usage..."
rg "GPUMemoryTool" -t typescript
Length of output: 340
Script:
#!/bin/bash
# Search for memory leak related tests with ripgrep
echo "Searching for memory leak related tests..."
rg -l "memory.*leak|leak.*memory" --type-add 'test:*.{spec,test}.*' --type test
echo "Searching for memory tool usage..."
rg -l "memoryTool|MemoryTool" --type-add 'test:*.{spec,test}.*' --type test
echo "Checking content of found memory leak test..."
cat web-packages/test/memory/src/memory-leak.spec.ts
Length of output: 3904
Script:
#!/bin/bash
# Let's check the content of the model plugin's memory leak test
cat plugin-packages/model/test/src/memory-leak.spec.ts
# Search for any other memory-related testing utilities
rg -l "memory.*tool|memorytool" --type ts
# Check if there are any other WebGL-related memory checks
rg "checkWebGLLeak|WebGLLeak" --type ts
Length of output: 2480
web-packages/test/memory/src/scene-list.ts (2)
317-317: No functional changes beyond the bracket adjustments.
These lines are likely closing braces or structural adjustments. Nothing critical to address here.
Also applies to: 329-329, 333-333, 353-353
4-5: Confirm scene name usage consistency.
The "spring" scene's name has changed to "春促-主页互动". Ensure all references across the codebase, test files, or documentation align with this updated naming.
✅ Verification successful
Let me search for any potential previous or different names for this scene to ensure complete verification.
Let me modify the search pattern to match the scene name format in the files:
Scene name '春促-主页互动' is consistently used across relevant files
The scene name '春促-主页互动' is correctly and consistently used in all relevant scene list files:
- web-packages/test/memory/src/scene-list.ts
- web-packages/test/case/src/2d/assets/inspire.ts
- web-packages/imgui-demo/src/asset/inspire-list.ts
- web-packages/demo/src/assets/inspire-list.ts
- web-packages/demo/src/assets/post-processing-list.ts
Note: The other scenes with "春" in their names ('新春烟花' and '春风') are different scenes and not related to this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify usage of the new scene name "春促-主页互动" across the codebase.
rg -A 3 $'春促-主页互动'
Length of output: 1564
Script:
#!/bin/bash
# Search for potential previous scene names or variations
rg -B 2 -A 2 '"name":\s*"[^"]*春[^"]*"' --type ts
Length of output: 50
Script:
#!/bin/bash
# Search for potential previous scene names or variations
rg -B 2 -A 2 "name:\s*'[^']*春[^']*'" --type ts
Length of output: 3894
web-packages/test/case/all.html (1)
9-9: Verify new script path.
Ensure that "./src/all/index.ts" exists, is correctly exported, and matches the updated test structure. Also confirm that all modules imported in this file are reachable.
✅ Verification successful
Let me verify the imports in the index.ts file to ensure all modules are reachable.
Let me verify if the referenced HTML files in urlList exist.
Script path and dependencies verified successfully
The file web-packages/test/case/src/all/index.ts exists and contains valid code. All referenced HTML files (2d.html, 3d.html, and spine.html) are present in the correct location at web-packages/test/case/. The script is a simple test utility that opens these files in popup windows, with no external module dependencies to verify.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence and usage of src/all/index.ts
fd index.ts | grep 'src/all/index.ts'
Length of output: 77
Script:
#!/bin/bash
# Check the content of the index.ts file and its imports
cat web-packages/test/case/src/all/index.ts
Length of output: 603
Script:
#!/bin/bash
# Check if the referenced HTML files exist
fd -e html | grep -E '2d\.html|3d\.html|spine\.html'
Length of output: 198
web-packages/test/case/src/all/index.ts (1)
2-4: Check updated HTML paths.
The references to "/case/2d.html", "/case/3d.html", and "/case/spine.html" now bypass subdirectories. Confirm these files exist at the expected locations.
✅ Verification successful
HTML paths are correctly referenced
All three files exist at the expected locations in web-packages/test/case/:
web-packages/test/case/2d.htmlweb-packages/test/case/3d.htmlweb-packages/test/case/spine.html
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Ensure that the 2d.html, 3d.html, and spine.html files exist at the top level in /case/.
fd 2d.html
fd 3d.html
fd spine.html
Length of output: 185
web-packages/test/case/index.html (1)
13-13: Validate updated anchor links.
The links are flattened to "2d.html", "3d.html", "spine.html", "all.html". Confirm they match the new file structure and are accessible.
Also applies to: 17-17, 21-21, 25-25
packages/effects-core/src/plugins/sprite/sprite-mesh.ts (1)
70-72: Document rationale for disabling shader caching
The shader caching logic has been commented out without explanation. This could impact rendering performance.
Let's verify the impact of this change:
Consider:
- Adding a comment explaining why caching was disabled
- Documenting any performance implications
- Adding performance tests if not already present
✅ Verification successful
Let me gather more context about the shader caching implementation and its usage.
Let me gather more specific information about the shader caching implementation and its usage in the sprite mesh context.
Based on the context gathered, I can now provide a final response:
Shader caching removal is intentional for wireframe mode
The commented-out shader caching was specifically for non-wireframe mode, and its removal appears to be a deliberate design choice. The code shows that:
- Shader caching was only used when
wireframewas false - The shader is still marked as
shared, which provides a different level of reuse - The change maintains consistency with wireframe mode behavior
The performance impact should be minimal since:
- The shader is still marked as
shared, allowing shader reuse - The caching was conditional (wireframe-dependent) to begin with
- The change simplifies the codebase by removing special-case handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other shader caching implementations or related tests
rg -A 3 "shader.*cache"
# Look for performance test cases
ast-grep --pattern 'describe($_, function () {
$$$
performance($_, function () {
$$$
})
$$$
})'
Length of output: 4446
Script:
#!/bin/bash
# Look for the shader caching implementation and usage
ast-grep --pattern 'spriteMeshShaderIdFromRenderInfo($$$)'
# Check for recent changes or discussions around shader caching
rg -A 3 "spriteMeshShaderIdFromRenderInfo"
# Look for performance-related comments or documentation
rg -B 2 -A 2 "performance|perf" --type ts
Length of output: 42714
Script:
#!/bin/bash
# Look for the sprite mesh shader implementation and related code
ast-grep --pattern 'spriteMeshShaderFromRenderInfo($$$)'
# Check for any recent changes or related code in sprite-mesh.ts
rg -B 3 -A 3 "shader" packages/effects-core/src/plugins/sprite/sprite-mesh.ts
# Look for any performance-related tests for sprite mesh
rg -A 3 "sprite.*performance" --type ts
Length of output: 1445
web-packages/test/case/2d.html (1)
63-63: Verify the new module path exists
The script source has been updated to use a more specific path structure. This change is part of a broader reorganization of test files into subdirectories.
Let's verify the new path exists and no other files depend on the old location:
✅ Verification successful
Path change verified and consistent with codebase structure
The new path ./src/2d/index.ts exists at web-packages/test/case/src/2d/index.ts and is part of a well-organized directory structure where different test categories (2d, 3d, all, etc.) have their own subdirectories. The change aligns with the codebase organization, and the old path pattern src/index.ts is only used in other test suites (memory and unit) where it's still appropriate for their context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new module path exists and check for any remaining references to the old path
# Check if the new file exists
fd "index.ts" "web-packages/test/src/2d"
# Look for any remaining references to the old path that might need updating
rg -g "!*.html" "src/index.ts" "web-packages/test"
Length of output: 199
Script:
#!/bin/bash
# Let's first check the actual directory structure to understand the paths
fd . "web-packages/test" --type d
# Then look for any index.ts files
fd "index.ts" "web-packages/test"
# Check for any references to the old path pattern
rg "src/index.ts" "web-packages/test"
Length of output: 2696
web-packages/test/case/3d.html (1)
47-47: Verify the new module path exists
The script source has been updated to use a more specific path structure, consistent with the changes in other test files.
Let's verify the new path exists and check for any shared dependencies:
✅ Verification successful
Module path verified and exists
The new path ./src/3d/index.ts exists at web-packages/test/case/src/3d/index.ts relative to the test file location, confirming the path update is correct and consistent with the repository structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new module path and check for shared dependencies
# Check if the new file exists
fd "index.ts" "web-packages/test/src/3d"
# Look for any shared dependencies between 2d and 3d tests that might need updating
rg -g "*.ts" "import.*from.*\.\." "web-packages/test/src/3d"
Length of output: 276
Script:
#!/bin/bash
# Let's first check the actual directory structure to locate the test files
fd "3d.html" "web-packages"
# Then check the src directory structure
fd "src" "web-packages/test"
# Finally, let's look for any index.ts files that might be relevant
fd "index.ts" "web-packages"
Length of output: 1261
web-packages/test/case/spine.html (1)
52-52: Verify the new module path and file naming convention
The script source has been updated to use a more specific path structure. Note that this file uses a .spec.ts extension, different from the other test files.
Let's verify the path and check for consistency in test file naming:
✅ Verification successful
Path and naming convention are consistent with the codebase
The file path and naming convention are correct:
- The file exists at
web-packages/test/case/src/spine/index.spec.ts - The
.spec.tsextension is also used in other test files (e.g.,web-packages/test/unit/src/effects-core/fallback/model/index.spec.ts) - The directory structure follows the same pattern as other test modules (e.g.,
2d,3d,all, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new module path and check test file naming patterns
# Check if the new spec file exists
fd "index.spec.ts" "web-packages/test/src/spine"
# Check for consistency in test file naming patterns
fd -e ts "web-packages/test/src" -x echo "{//}: {}"
Length of output: 213
Script:
#!/bin/bash
# Let's first check if the directory structure exists and locate TypeScript files
fd -t d "spine" "web-packages/test"
# Find all TypeScript files in the test directory to check naming patterns
fd -e ts "web-packages/test"
# Check the actual path structure
fd -t f "index" "web-packages/test"
Length of output: 1530
web-packages/test/case/src/spine/index.spec.ts (2)
2-2: LGTM: Import path change looks good
The change to use a relative import path ../common is consistent with the module resolution best practices.
63-63: Verify the impact of the new diff ratio collection approach
The change from immediate assertion to collecting differences and asserting at the end could mask individual frame issues. While this approach provides a better summary, it might make debugging specific frames more difficult.
Let's analyze the test results history to see if this change impacts test reliability:
Also applies to: 92-92, 95-95
web-packages/test/unit/src/effects-webgl/math-utils.ts (1)
1-1: LGTM: Type annotations enhance code safety
The addition of TypeScript type annotations to all mathematical utility functions improves type safety and code maintainability. The types are accurate and appropriate for each function's purpose.
Also applies to: 27-27, 33-33, 38-38, 43-43, 52-52, 57-57, 62-62, 67-67, 76-76, 86-86, 92-92, 98-98, 104-104, 114-114, 118-118, 122-122, 126-126, 130-130
web-packages/test/case/src/2d/dynamic.spec.ts (2)
2-2: LGTM: Import path change is consistent
The change to use a relative import path ../common aligns with the changes in other test files.
Line range hint 83-95: Commented-out hit test ID comparisons should be addressed
There are commented-out hit test ID comparisons. These should either be removed or re-enabled with an explanation.
Let's check if these comparisons are needed:
web-packages/test/case/src/2d/interact.spec.ts (2)
1-2: LGTM: Import path simplification
The import path has been simplified to use the parent directory, making the code more maintainable.
Line range hint 82-84: Verify if hit test ID comparisons can be re-enabled
The commented-out hit test ID comparisons might indicate incomplete testing. These validations could be important for ensuring hit testing behavior remains consistent.
Also applies to: 95-97
web-packages/test/case/src/2d/inspire.spec.ts (1)
2-2: LGTM: Consistent implementation of diff collection
The changes follow the same pattern as other test files, providing better error reporting while maintaining test reliability. The empty line after the assertion improves readability.
Also applies to: 77-77, 106-106, 110-111
web-packages/test/case/src/3d/case.spec.ts (1)
2-2: LGTM: Consistent implementation of diff collection for 3D tests
The changes maintain consistency with other test files while properly handling 3D-specific requirements.
Also applies to: 88-88, 125-125, 129-130
web-packages/test/case/src/3d/gltf.spec.ts (1)
103-103: LGTM: Improved test reliability
The introduction of diffRatioList to collect all pixel differences before asserting improves test reliability by providing a comprehensive view of all differences found during the test.
Also applies to: 139-139, 143-144
packages/effects-core/src/components/base-render-component.ts (1)
209-214: LGTM: Proper handling of ALPHA_CLIP macro
The conditional logic correctly enables/disables the ALPHA_CLIP macro based on the texture parameters, which is essential for proper alpha handling in the shader.
web-packages/test/unit/src/effects-webgl/gl-frame-buffer.spec.ts (2)
1-1: LGTM! Well-structured type imports.
The type imports are properly organized and follow TypeScript best practices.
Line range hint 470-485: LGTM! Well-structured helper function.
The checkColorAttachment helper function is well-typed and properly encapsulates the common verification logic.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores