Support GUI #2375
Support GUI #2375
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/ui/README.md (4)
3-3: Make the experimental status warning more prominentConsider making the experimental status warning more prominent by moving it to a dedicated "Warning" or "Note" section at the top, using blockquote syntax for better visibility:
-`@galacean/engine-ui` is a UI library designed for the `@galacean/engine`, this library enables developers to create and manage user interfaces efficiently within their **Galacean-based** applications. It is important to note that it is currently in an **experimental version**. +`@galacean/engine-ui` is a UI library designed for the `@galacean/engine`, this library enables developers to create and manage user interfaces efficiently within their **Galacean-based** applications. + +> **Warning**: This library is currently in an experimental version and the API may change without notice.
14-18: Add peer dependency informationConsider adding information about peer dependencies, particularly the required version of
@galacean/engine:To use `@galacean/engine-ui` in your project, install it via npm: ```bash npm install @galacean/engine-ui + +# Install peer dependencies if not already installed +npm install @galacean/engine--- `53-58`: **Improve the example code** 1. Replace the placeholder URL with a valid example image URL or use a relative path to a sample image. 2. Add error handling for texture loading: ```diff engine.resourceManager .load({ - url: "https://xxx.png", + url: "./assets/sample-image.png", // or a valid example URL type: AssetType.Texture2D }) .then((texture) => { image.sprite = new Sprite(engine, <Texture2D>texture); - }); + }) + .catch((error) => { + console.error("Failed to load texture:", error); + });
63-67: Enhance documentation links sectionConsider adding more specific links to relevant documentation sections:
Easier operations in the [Editor](https://galacean.antgroup.com/editor/). ## Documentation -For detailed documentation, visit [the official documentation site](https://galacean.antgroup.com/engine). +For detailed documentation, visit [the official documentation site](https://galacean.antgroup.com/engine). You can find: + +- [UI Components Guide](https://galacean.antgroup.com/engine/docs/ui-components) +- [Event Handling Guide](https://galacean.antgroup.com/engine/docs/ui-events) +- [API Reference](https://galacean.antgroup.com/engine/api/ui)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ui/README.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x, windows-latest)
| // Create Image | ||
| const imageEntity = canvasEntity.createChild("image"); | ||
| const image = imageEntity.addComponent(Image); | ||
| engine.resourceManager | ||
| .load({ | ||
| url: "https://xxx.png", | ||
| type: AssetType.Texture2D | ||
| }) | ||
| .then((texture) => { | ||
| image.sprite = new Sprite(engine, <Texture2D>texture); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add size management example
Based on the PR objectives regarding UI element size management, consider adding an example that demonstrates the new size management features:
// Create Image with size
const imageEntity = canvasEntity.createChild("image");
const image = imageEntity.addComponent(Image);
// Set initial size
image.setWidth(200); // Using the new convenient method
image.setHeight(150);
// Load and display image
engine.resourceManager
.load({
url: "./assets/sample-image.png",
type: AssetType.Texture2D
})
.then((texture) => {
image.sprite = new Sprite(engine, <Texture2D>texture);
})
.catch((error) => {
console.error("Failed to load texture:", error);
});| - **Rendering components**: Includes `Image` and `Text`. | ||
| - **Interactive Components**: Include `Button`, as well as other planned basic rendering components to be added in the future. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add UI prefix to component names for consistency
Based on the PR objectives regarding UI component naming consistency, component names should include the "UI" prefix:
-**Rendering components**: Includes `Image` and `Text`.
-**Interactive Components**: Include `Button`, as well as other planned basic rendering components to be added in the future.
+**Rendering components**: Includes `UIImage` and `UIText`.
+**Interactive Components**: Include `UIButton`, as well as other planned basic UI components to be added in the future.Also, consider listing the planned components or removing the mention if they're not yet finalized.
📝 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.
| - **Rendering components**: Includes `Image` and `Text`. | |
| - **Interactive Components**: Include `Button`, as well as other planned basic rendering components to be added in the future. | |
| **Rendering components**: Includes `UIImage` and `UIText`. | |
| **Interactive Components**: Include `UIButton`, as well as other planned basic UI components to be added in the future. |
* refactor: `PostProcessParameter` to strong type
* feat: support callback props in parser
* refactor: remove unused physx function * feat: update downgrade version
* fix: iridescence bug
* fix: ci
* feat: add built-in LUT
* refactor: update physX wasm without assertions
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (1)
packages/core/src/postProcess/effects/BloomEffect.ts (1)
Line range hint
102-104: Consider updating isValid() to check dirtIntensity.The current implementation only checks
intensity.value, but the bloom effect could still be visible through the dirt texture whendirtIntensity.value > 0. Consider updating the method to account for both parameters:override isValid(): boolean { - return this.enabled && this.intensity.value > 0; + return this.enabled && (this.intensity.value > 0 || (this.dirtTexture.value && this.dirtIntensity.value > 0)); }
♻️ Duplicate comments (1)
packages/core/src/Engine.ts (1)
359-359:⚠️ Potential issueRestore physics initialization check for pointer events.
The removal of the
physicsInitializedcheck is problematic because pointer events depend on the physics engine for raycasting. This could lead to null pointer exceptions and undefined behavior.Apply this diff to restore the check:
- inputManager._firePointerScript(scenes); + if (physicsInitialized) { + inputManager._firePointerScript(scenes); + }🧰 Tools
🪛 GitHub Actions: CI
[warning] Circular dependency detected between Engine.ts, BasicResources.ts, 2d/index.ts, sprite/index.ts, sprite/Sprite.ts
🧹 Nitpick comments (15)
packages/core/src/postProcess/effects/TonemappingEffect.ts (1)
3-3: Improved type safety with enum-specific parameter!The change from
PostProcessEffectParametertoPostProcessEffectEnumParameterenhances type safety and self-documentation. The new implementation:
- Provides compile-time validation of tonemapping mode values
- Makes the parameter's purpose clearer through its specialized type
- Eliminates the need for the boolean flag parameter
Consider applying this pattern to other effect parameters where specific types (float, boolean, texture) would provide similar benefits.
Also applies to: 31-31
packages/core/src/postProcess/PostProcess.ts (1)
134-137: Consider implementing thorough cleanup of effectsWhile setting array length to 0 clears the array, it doesn't ensure proper cleanup of individual
PostProcessEffectinstances. Consider explicitly cleaning up each effect before clearing the array.Here's a suggested implementation:
override _onDestroy(): void { super._onDestroy(); + // Clean up each effect + for (const effect of this._effects) { + effect.destroy?.(); // Call destroy if available + } this._effects.length = 0; }tests/src/core/postProcess/PostProcess.test.ts (1)
242-244: Consider consolidating duplicate test cases.
p2andp3are testing the same scenario (both initialize withtrue). Consider replacing one of these with a different test case to improve test coverage.- const p2 = new PostProcessEffectBoolParameter(true); - const p3 = new PostProcessEffectBoolParameter(true); + const p2 = new PostProcessEffectBoolParameter(true); + // Test value reassignment + const p3 = new PostProcessEffectBoolParameter(false); + p3.value = true;packages/physics-physx/src/PhysXPhysics.ts (2)
93-95: Consider adding integrity checks for CDN scriptsThe PhysX scripts are loaded from a CDN without integrity checks, which could pose a security risk. Consider adding integrity checks using the
integrityattribute and implementing fallback mechanisms.Example implementation:
if (runtimeMode == PhysXRuntimeMode.JavaScript) { - script.src = `https://mdn.alipayobjects.com/rms/afts/file/A*CfV8RrDQk5oAAAAAAAAAAAAAARQnAQ/physx.release.downgrade.js`; + script.src = `https://mdn.alipayobjects.com/rms/afts/file/A*CfV8RrDQk5oAAAAAAAAAAAAAARQnAQ/physx.release.downgrade.js`; + script.integrity = "sha384-[hash]"; // Add appropriate hash + script.crossOrigin = "anonymous"; } else if (runtimeMode == PhysXRuntimeMode.WebAssembly) { script.src = `https://mdn.alipayobjects.com/rms/afts/file/A*ZDDgR4ERdfwAAAAAAAAAAAAAARQnAQ/physx.release.js`; + script.integrity = "sha384-[hash]"; // Add appropriate hash + script.crossOrigin = "anonymous"; }
93-96: Enhance error handling for script loading failuresWhile basic error handling exists through the Promise rejection, consider implementing a more robust error handling strategy:
- Add specific error types for different failure scenarios
- Implement retry logic with exponential backoff
- Consider fallback CDNs
Example implementation:
private async _loadScript(url: string, retries = 3): Promise<void> { for (let i = 0; i < retries; i++) { try { await new Promise((resolve, reject) => { const script = document.createElement("script"); script.src = url; script.async = true; script.onload = resolve; script.onerror = () => reject(new Error(`Failed to load PhysX from ${url}`)); document.body.appendChild(script); }); return; } catch (error) { if (i === retries - 1) throw error; await new Promise(resolve => setTimeout(resolve, Math.pow(2, i) * 1000)); } } }packages/core/src/postProcess/effects/BloomEffect.ts (1)
75-79: Consider enhancing parameter documentation.While the JSDoc comments are good, they could be more comprehensive by including:
- Valid value ranges where applicable (e.g., for
threshold,scatter,intensity)- Default values in the documentation
- Impact of different values on the bloom effect
Example for the threshold parameter:
/** * Set the level of brightness to filter out pixels under this level. * @remarks This value is expressed in gamma-space. * @default 0.9 * @min 0 * @example * // Lower values (e.g., 0.5) will make the bloom effect more pronounced * // Higher values (e.g., 0.95) will make it more subtle */Also applies to: 82-84, 87-89, 92-94, 97-99
tests/vitest.config.ts (1)
19-21: Document the purpose of GPU-related launch arguments.The browser launch configuration includes specific GPU and graphics-related flags (
--use-gl=egl,--ignore-gpu-blocklist,--use-gl=angle). Consider adding comments to explain why these flags are necessary, especially in the context of GUI testing.providerOptions: { launch: { + // Enable hardware acceleration and use ANGLE for consistent GPU behavior across platforms args: ["--use-gl=egl", "--ignore-gpu-blocklist", "--use-gl=angle"] }tests/src/core/resource/ResourceManager.test.ts (1)
Line range hint
93-93: Consider implementing the TODO test case.The TODO comment indicates a missing test case for GLTF loader with invalid query URLs. Since this PR introduces GUI components that may interact with GLTF resources, it would be beneficial to implement this test case now.
Would you like me to help generate the test case for invalid GLTF loader URLs?
e2e/case/project-loader.ts (2)
25-28: Consider adding error handling for camera entity lookup.The camera entity lookup assumes the entity named "Camera" exists. Consider adding error handling to provide a better debugging experience in case the camera entity is not found.
- const cameraEntity = engine.sceneManager.activeScene.findEntityByName("Camera"); - const camera = cameraEntity.getComponent(Camera); + const cameraEntity = engine.sceneManager.activeScene.findEntityByName("Camera"); + if (!cameraEntity) { + throw new Error("Camera entity not found in the scene"); + } + const camera = cameraEntity.getComponent(Camera); + if (!camera) { + throw new Error("Camera component not found on camera entity"); + } initScreenshot(engine, camera);
15-28: Consider using async/await for better readability.The nested promise chain could be simplified using async/await syntax for improved readability and error handling.
-WebGLEngine.create({ canvas: "canvas", shaderLab }).then((engine) => { - engine.canvas.resizeByClientSize(2); - engine.resourceManager - .load({ - type: AssetType.Project, - url: "https://mdn.alipayobjects.com/oasis_be/afts/file/A*o15SSopTBh0AAAAAAAAAAAAADkp5AQ/project.json" - }) - .then(() => { - updateForE2E(engine); - const cameraEntity = engine.sceneManager.activeScene.findEntityByName("Camera"); - const camera = cameraEntity.getComponent(Camera); - initScreenshot(engine, camera); - }); -}); +async function initEngine() { + try { + const engine = await WebGLEngine.create({ canvas: "canvas", shaderLab }); + engine.canvas.resizeByClientSize(2); + + await engine.resourceManager.load({ + type: AssetType.Project, + url: "https://mdn.alipayobjects.com/oasis_be/afts/file/A*o15SSopTBh0AAAAAAAAAAAAADkp5AQ/project.json" + }); + + updateForE2E(engine); + const cameraEntity = engine.sceneManager.activeScene.findEntityByName("Camera"); + if (!cameraEntity) { + throw new Error("Camera entity not found in the scene"); + } + const camera = cameraEntity.getComponent(Camera); + if (!camera) { + throw new Error("Camera component not found on camera entity"); + } + initScreenshot(engine, camera); + } catch (error) { + Logger.error("Failed to initialize engine:", error); + } +} + +initEngine();e2e/case/material-LUT.ts (1)
22-26: Consider using quaternions for rotation.While using
setRotationwith Euler angles works, consider using quaternions to avoid potential gimbal lock issues:-entity.transform.setRotation(90, 0, 0); +entity.transform.rotationQuaternion = Quaternion.FromEulerAngles(Math.PI/2, 0, 0);packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts (1)
170-176: Use consistent property access notation in type guardsIn the
_isClassand_isClassTypemethods, bracket notation is used to access object properties. For consistency and readability, consider using dot notation unless bracket notation is necessary.Apply this diff to update the property access:
private static _isClass(value: any): value is IClass { - return value["class"] !== undefined; + return value.class !== undefined; } private static _isClassType(value: any): value is IClassType { - return value["classType"] !== undefined; + return value.classType !== undefined; }packages/core/src/BasicResources.ts (1)
180-189: Refactor_initializemethod usingasync/awaitfor better readabilityTo improve readability and maintainability, consider refactoring the
_initializemethod withasync/awaitsyntax instead of manual Promise handling.Refactored code:
- _initialize(): Promise<BasicResources> { - return new Promise((resolve, reject) => { - PrefilteredDFG.create(this.engine) - .then((texture) => { - this._prefilteredDFGTexture = texture; - resolve(this); - }) - .catch(reject); - }); - } + async _initialize(): Promise<BasicResources> { + try { + this._prefilteredDFGTexture = await PrefilteredDFG.create(this.engine); + return this; + } catch (error) { + throw error; + } + }packages/core/src/material/utils/PrefilteredDFG.ts (1)
7-46: Refactor static-only class to module-level functionsThe
PrefilteredDFGclass contains only static members and methods. According to best practices, consider refactoring it into a module with exported functions to simplify the code and reduce unnecessary class scaffolding.Refactored code:
// Module-level constants const _size = 256; const _base64 = `data:image/png;base64,...`; // Exported function export function createPrefilteredDFG(engine: Engine): Promise<Texture2D> { return new Promise((resolve, reject) => { const texture = new Texture2D(engine, _size, _size, TextureFormat.R8G8B8, false); texture.wrapModeU = texture.wrapModeV = TextureWrapMode.Clamp; texture.isGCIgnored = true; const image = new Image(); image.onload = () => { texture.setImageSource(image); resolve(texture); }; image.onerror = image.onabort = () => { const message = "Failed to load prefiltered LUT image."; Logger.error(message); reject(message); }; image.src = _base64; engine.resourceManager.addContentRestorer( new (class extends ContentRestorer<Texture2D> { constructor() { super(texture); } restoreContent() { texture.setImageSource(image); } })() ); }); }Update usage accordingly:
- PrefilteredDFG.create(this.engine) + createPrefilteredDFG(this.engine)🧰 Tools
🪛 Biome (1.9.4)
[error] 7-46: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/core/src/Engine.ts (1)
42-42: Address circular dependency warning.The import of
UIUtilscontributes to a circular dependency betweenEngine.ts,BasicResources.ts,2d/index.ts,sprite/index.ts, andsprite/Sprite.ts. This could lead to initialization issues and complicate the build process.Consider restructuring the imports to break the circular dependency:
- Extract common interfaces/types into a separate module
- Use dependency injection
- Consider implementing an event-based communication pattern
🧰 Tools
🪛 GitHub Actions: CI
[warning] Circular dependency detected between Engine.ts, BasicResources.ts, 2d/index.ts, sprite/index.ts, sprite/Sprite.ts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
e2e/fixtures/originImage/Material_material-LUT.jpgis excluded by!**/*.jpgpackages/physics-physx/libs/physx.release.wasmis excluded by!**/*.wasmpackages/shader-shaderlab/src/shaders/shadingPBR/BRDF.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/FragmentPBR.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/LightIndirectPBR.glslis excluded by!**/*.glsl
📒 Files selected for processing (32)
.github/workflows/release.yml(3 hunks)e2e/case/material-LUT.ts(1 hunks)e2e/case/material-shaderLab.ts(1 hunks)e2e/case/postProcess-customPass.ts(2 hunks)e2e/case/project-loader.ts(1 hunks)e2e/case/shaderLab-mrt.ts(1 hunks)e2e/case/shaderLab-renderState.ts(1 hunks)e2e/config.ts(1 hunks)examples/buffer-mesh-particle-shader-effect.ts(3 hunks)examples/shader-lab-multi-pass.ts(1 hunks)examples/shader-lab-simple.ts(3 hunks)examples/shader-lab.ts(3 hunks)examples/shader-water.ts(5 hunks)packages/core/src/BasicResources.ts(3 hunks)packages/core/src/Engine.ts(5 hunks)packages/core/src/Scene.ts(3 hunks)packages/core/src/material/utils/PrefilteredDFG.ts(1 hunks)packages/core/src/postProcess/PostProcess.ts(1 hunks)packages/core/src/postProcess/PostProcessEffectParameter.ts(2 hunks)packages/core/src/postProcess/effects/BloomEffect.ts(2 hunks)packages/core/src/postProcess/effects/TonemappingEffect.ts(2 hunks)packages/core/src/postProcess/index.ts(1 hunks)packages/design/src/physics/IDynamicCollider.ts(0 hunks)packages/loader/src/SceneLoader.ts(2 hunks)packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts(5 hunks)packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts(4 hunks)packages/physics-lite/src/LiteDynamicCollider.ts(0 hunks)packages/physics-physx/src/PhysXDynamicCollider.ts(0 hunks)packages/physics-physx/src/PhysXPhysics.ts(1 hunks)tests/src/core/postProcess/PostProcess.test.ts(4 hunks)tests/src/core/resource/ResourceManager.test.ts(1 hunks)tests/vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (3)
- packages/physics-lite/src/LiteDynamicCollider.ts
- packages/physics-physx/src/PhysXDynamicCollider.ts
- packages/design/src/physics/IDynamicCollider.ts
✅ Files skipped from review due to trivial changes (8)
- examples/shader-water.ts
- e2e/case/shaderLab-mrt.ts
- e2e/case/material-shaderLab.ts
- examples/shader-lab-multi-pass.ts
- examples/shader-lab.ts
- e2e/case/shaderLab-renderState.ts
- examples/shader-lab-simple.ts
- examples/buffer-mesh-particle-shader-effect.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/Scene.ts
🧰 Additional context used
🪛 GitHub Actions: CI
packages/core/src/Engine.ts
[warning] Circular dependency detected between Engine.ts, BasicResources.ts, 2d/index.ts, sprite/index.ts, sprite/Sprite.ts
🪛 Biome (1.9.4)
packages/core/src/material/utils/PrefilteredDFG.ts
[error] 7-46: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (35)
packages/core/src/postProcess/PostProcess.ts (1)
Line range hint
1-137: Well-structured implementation with good type safetyThe overall implementation is solid with:
- Proper type safety using generics in effect management methods
- Good error handling and logging
- Clear and consistent API design
- Well-structured component lifecycle management
e2e/config.ts (1)
132-136: LGTM! Verify the test file existence.The new LUT configuration entry follows the established patterns and maintains consistency with other Material category entries.
Let's verify that the corresponding test file exists:
✅ Verification successful
Test file verification successful
The material-LUT test file exists at the expected location:
e2e/case/material-LUT.ts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the material-LUT test file exists # Expected: The test file should exist at a location similar to other material test files # Find the material-LUT test file fd -t f "material-LUT.ts"Length of output: 50
tests/src/core/postProcess/PostProcess.test.ts (3)
10-11: LGTM! Enhanced type safety with specialized parameter classes.The introduction of dedicated parameter classes for boolean and float values improves type safety and code clarity.
30-30: LGTM! Proper migration to type-specific parameter.The
intensityparameter has been correctly migrated to usePostProcessEffectFloatParameterwith appropriate range constraints.
226-231: LGTM! Comprehensive test coverage for float parameters.The test cases effectively cover various scenarios including basic values, range constraints, and clamping behavior.
packages/physics-physx/src/PhysXPhysics.ts (1)
93-95: Verify inclusion of PhysX changes in GUI PRThese changes to PhysX script URLs appear unrelated to the PR's stated objective of adding GUI functionality. Please clarify if these changes should be part of a separate PR focused on physics engine updates.
packages/core/src/postProcess/effects/BloomEffect.ts (2)
6-12: LGTM! Well-organized parameter type imports.The imports are properly structured and follow TypeScript best practices, with clear naming conventions for the specific parameter types.
63-63: LGTM! Excellent type safety improvements.The transition from generic
PostProcessEffectParameterto specific parameter types (PostProcessEffectBoolParameter,PostProcessEffectEnumParameter, etc.) significantly improves type safety and makes the code more maintainable.Also applies to: 68-68, 73-73, 79-79, 84-84, 89-89, 94-94, 99-99
tests/vitest.config.ts (1)
22-22: Verify the impact of removing headless mode on CI/CD pipelines.The removal of
headless: truemeans tests will run in non-headless mode by default. While this is beneficial for visual inspection of GUI components during development, it might impact CI/CD pipelines that require headless testing.Run this script to check if there are any CI/CD configurations that might be affected:
✅ Verification successful
Removing headless mode appears safe for CI/CD pipeline
The CI pipeline runs tests on MacOS runners which support GUI operations, and the coverage tests using Playwright should work correctly even without explicit headless mode. However, consider documenting this intentional configuration choice in the Vitest config file for future maintainers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for CI/CD configurations that might require headless testing # Look for CI configuration files echo "Checking CI configuration files..." fd -e yml -e yaml -e json . .github/workflows/ # Look for references to headless testing in documentation echo "Checking documentation for headless testing references..." rg -i "headless.*test|test.*headless" -g '!node_modules'Length of output: 431
Script:
#!/bin/bash # Check CI workflow content and environment variables related to testing echo "Checking CI workflow content..." cat .github/workflows/ci.yml echo -e "\nSearching for test-related environment variables..." rg -i "env.*test|test.*env" .github/workflows/Length of output: 3058
tests/src/core/resource/ResourceManager.test.ts (1)
36-36: Verify the new texture count expectation.The test now expects 5 textures instead of 4. This change likely reflects the addition of a new texture resource due to GUI-related changes.
Let's verify the texture resources being created:
✅ Verification successful
The texture count expectation is correct.
The test expects 5 textures which matches the default textures initialized by the engine in WebGL2 mode:
- whiteTexture2D
- whiteTextureCube
- whiteTexture2DArray
- uintWhiteTexture2D
- _depthTexture2D
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Texture2D instantiations across the codebase rg "new Texture2D\(" --type ts # Search for texture resource registrations rg "registerTexture|addTexture" --type tsLength of output: 8738
Script:
#!/bin/bash # View the ResourceManager test content cat tests/src/core/resource/ResourceManager.test.ts # Search for default texture initialization in Engine and ResourceManager rg -A 5 "defaultTexture|createDefault.*Texture" --type tsLength of output: 7308
Script:
#!/bin/bash # View BasicResources implementation cat packages/core/src/BasicResources.ts # Search for texture initialization in Engine rg -A 5 "initialize.*texture|create.*texture" --type tsLength of output: 39272
e2e/case/postProcess-customPass.ts (1)
12-12: LGTM! Import added for new parameter type.The addition of
PostProcessEffectFloatParameterimport aligns with the shift towards property-based parameter management..github/workflows/release.yml (2)
68-68: Reconsider disabling frozen lockfile.Adding
--frozen-lockfile=falsecould lead to inconsistent builds if dependencies have breaking changes. Consider:
- Using a lockfile to ensure reproducible builds
- Explicitly updating dependencies when needed
- Documenting why this change was necessary
✅ Verification successful
Disabling frozen lockfile appears intentional and scoped.
The
--frozen-lockfile=falseis specifically used for the engine-spine repository's dev/4.2 branch, while other components maintain strict dependency controls. This seems to be an intentional choice to allow flexibility during development of this specific component.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if package.json and lockfile exist test -f engine-spine/package.json && echo "package.json exists" || echo "package.json missing" test -f engine-spine/pnpm-lock.yaml && echo "lockfile exists" || echo "lockfile missing" # Check if there are any dependency version ranges that could cause issues cat engine-spine/package.json | jq '.dependencies, .devDependencies'Length of output: 296
Script:
#!/bin/bash # Find all package.json and pnpm-lock.yaml files echo "=== Package Files ===" fd -t f "package.json|pnpm-lock.yaml" echo -e "\n=== Release Workflow Context ===" # Get context around the pnpm install command cat .github/workflows/release.ymlLength of output: 7729
124-126: Verify WASM file path and integration.The addition of WASM files to the adapter bundle is a significant change. Please ensure:
- The WASM file path is correct and consistent
- The file is properly generated during the build process
- The bundling process handles WASM files correctly
✅ Verification successful
WASM integration verified successfully.
- WASM file exists with correct permissions and size
- JS loader is present and properly configured
- Bundling configuration in workflow correctly references the WASM file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the WASM file exists test -f "packages/physics-physx/libs/physx.release.wasm" && echo "WASM file exists" || echo "WASM file missing" # Verify file permissions ls -l "packages/physics-physx/libs/physx.release.wasm" 2>/dev/null # Check if the corresponding JS loader exists test -f "packages/physics-physx/libs/physx.release.js" && echo "JS loader exists" || echo "JS loader missing"Length of output: 351
Script:
#!/bin/bash # Search for build scripts and configurations handling WASM echo "=== Build Scripts ===" rg -g "package.json" -g "*.config.js" -g "*.config.ts" "physx.*wasm" -A 3 echo -e "\n=== WASM Imports ===" # Look for WASM imports or references in code rg "import.*physx.*wasm" -g "*.ts" -g "*.js" rg "require.*physx.*wasm" -g "*.ts" -g "*.js" echo -e "\n=== Build Configurations ===" # Check for webpack or other bundler configs fd -e config.js -e config.ts -e webpack.config.js | xargs rg "wasm|WASM" -lLength of output: 130166
packages/loader/src/SceneLoader.ts (1)
13-13: Verify the impact of type definition changes.The change from
IClassObjecttoIClasstype might affect other parts of the codebase.Let's verify the consistency of this type change:
Also applies to: 15-15
✅ Verification successful
Type definition change is package-specific and correct
The change from
IClassObjecttoIClassinSceneLoader.tsis intentional and correct. Each package maintains its own type definitions:
- The loader package defines and uses
IClass- The UI package uses
IClassObjectfrom the core engine🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usages of IClassObject to ensure complete migration rg "IClassObject" --type ts # Search for the definition and usage of the new IClass type rg "interface IClass" --type ts -A 5 rg "IClass[^a-zA-Z]" --type tsLength of output: 1289
Script:
#!/bin/bash # Check the definition of IClassObject and its relationship with IClass ast-grep --pattern 'interface IClassObject' -l ts ast-grep --pattern 'type IClassObject' -l ts # Check imports to understand the relationship between packages rg "from.*@galacean/.*ui" --type ts rg "from.*@galacean/.*loader" --type tsLength of output: 1712
Script:
#!/bin/bash # Check exports and type definitions rg "export.*IClassObject" --type ts -A 3 rg "import.*IClassObject" --type ts # Check the content of the UI package's index file cat packages/ui/src/index.tsLength of output: 3891
e2e/case/material-LUT.ts (3)
1-7: LGTM! Well-structured test file with clear documentation.The file is properly documented with JSDoc comments and has clean, organized imports.
17-21: LGTM! Clear camera setup with appropriate positioning.The camera setup is well-structured with a descriptive entity name and suitable positioning for viewing the test plane.
8-15: Verify the commented engine.run() and add error handling.
The commented
engine.run()needs clarification:
- If it's required for the test, it should be uncommented.
- If it's not needed, the comment should be removed.
Consider adding error handling for the canvas resize operation.
engine.canvas.resizeByClientSize(2); +if (!engine.canvas.width || !engine.canvas.height) { + throw new Error("Failed to resize canvas"); +}packages/core/src/postProcess/index.ts (2)
5-15: Well-organized parameter exportsThe consolidated export statement is well-organized, with parameters listed alphabetically. This improves code maintainability and makes it easier to find specific parameter types.
5-15: Verify the relationship between post-process changes and GUI implementationWhile the code changes look well-structured and improve type safety by providing specific parameter types, they appear to be in the core post-processing module rather than directly related to the GUI implementation mentioned in the PR objectives.
Let's verify if these post-process parameters are utilized by the new GUI system:
packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts (2)
57-59: Approve the updatedmodificationstype inIRefEntityThe
modificationsproperty now accurately represents modification instances with an associated target, enhancing clarity and type safety. This change improves the structure for tracking instance modifications.
87-88: Approve addition ofIClassTypeThe new
IClassTypetype adds clarity for representing class types, improving the overall type structure and readability.packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts (1)
Line range hint
170-192: Good use of type guards to enhance type safetyThe addition of private static methods
_isClass,_isClassType, and_isMethodObjectimproves type safety and code readability by providing clear type guards. This helps in ensuring that the code handles different types appropriately.packages/core/src/postProcess/PostProcessEffectParameter.ts (9)
Line range hint
9-45: Refactoring to an abstract base class enhances extensibilityRefactoring
PostProcessEffectParameterto an abstract class without specific type constraints onTimproves the extensibility and flexibility of the post-process effect parameters. This allows for a wider range of parameter types in derived classes.
47-80: Well-implementedPostProcessEffectFloatParameterwith clamping logicThe
PostProcessEffectFloatParameterclass correctly overrides thevaluegetter and setter to include clamping betweenminandmaxvalues, ensuring the float parameters remain within valid ranges. The_lerpmethod is appropriately overridden to provide interpolation when needed.
85-93: Proper definition ofPostProcessEffectBoolParameterThe
PostProcessEffectBoolParameterclass is straightforward and correctly initializes the boolean parameter without the need for interpolation.
98-106: Appropriate handling of texture parameters inPostProcessEffectTextureParameterThe
PostProcessEffectTextureParameterclass correctly manages texture parameters, ensuring they are handled without interpolation, which is suitable for texture types.
111-129: Effective interpolation inPostProcessEffectColorParameterThe
PostProcessEffectColorParameterclass properly overrides the_lerpmethod to interpolate between colors when necessary, utilizingColor.lerp.
133-150: Accurate implementation ofPostProcessEffectVector2ParameterThe class accurately provides interpolation for
Vector2parameters, improving parameter control in post-processing effects.
155-172: Correct handling ofVector3parameters inPostProcessEffectVector3ParameterInterpolation is properly implemented for
Vector3parameters, and the class structure follows the established pattern.
177-194: Consistent implementation inPostProcessEffectVector4ParameterThe
PostProcessEffectVector4Parameterclass correctly implements interpolation forVector4parameters, maintaining consistency across different vector types.
199-211: Effective use of generics inPostProcessEffectEnumParameterThe
PostProcessEffectEnumParameterclass handles enum types appropriately, capturing the enum definition and value. The use of generics enhances type safety.packages/core/src/BasicResources.ts (2)
12-12: Question regarding dependency onPrefilteredDFGin core packageImporting
PrefilteredDFGfrom./material/utils/PrefilteredDFGintroduces a dependency on material utilities within the core package. This could lead to increased coupling between core and material modules. Consider whetherPrefilteredDFGshould reside within the core package or if there's a way to restructure to minimize cross-package dependencies.
126-126: Use of access modifier in constructor enhances code clarityAdding the
publicmodifier to theengineparameter in the constructor streamlines property initialization and enhances code readability.packages/core/src/Engine.ts (2)
Line range hint
572-605: Well-structured UI rendering implementation.The UI rendering logic is well-integrated into the render loop with proper sorting of overlay canvases before rendering.
🧰 Tools
🪛 GitHub Actions: CI
[warning] Circular dependency detected between Engine.ts, BasicResources.ts, 2d/index.ts, sprite/index.ts, sprite/Sprite.ts
647-648: Verify and document the dependency chain.The initialization sequence needs careful review due to the circular dependency warning.
✅ Verification successful
Circular dependency is safely managed
The initialization sequence is properly handled through Promise-based async initialization, with BasicResources being initialized as part of the engine startup sequence. No issues were found with the dependency chain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze the dependency chain to identify circular references # Find all imports between the affected files echo "Analyzing imports in affected files..." for pattern in "from.*Engine" "from.*BasicResources" "from.*sprite" "from.*2d/index"; do echo "=== $pattern ===" rg "$pattern" packages/core/src/ -A 5 done # Find any potential dependency injection patterns that could be applied echo "Looking for existing dependency injection patterns..." ast-grep --pattern 'class $_ { constructor($_) { $$$ } }'Length of output: 42537
Script:
#!/bin/bash # Verify initialization order and usage patterns # Check BasicResources initialization echo "=== Checking BasicResources initialization ===" ast-grep --pattern 'class BasicResources { $_initialize() { $$$ } }' # Check Engine initialization sequence echo "=== Checking Engine initialization sequence ===" ast-grep --pattern 'class Engine { $$$ _initialize() { $$$ } $$$ }' # Look for any direct usage before initialization echo "=== Checking for early usage patterns ===" rg "this\._basicResources\." packages/core/src/Engine.ts -B 5 -A 5Length of output: 893
🧰 Tools
🪛 GitHub Actions: CI
[warning] Circular dependency detected between Engine.ts, BasicResources.ts, 2d/index.ts, sprite/index.ts, sprite/Sprite.ts
e2e/case/postProcess-customPass.ts
Outdated
| set intensity(value) { | ||
| this._blitMaterial.shaderData.setFloat("intensity", value); | ||
| } | ||
| intensity = new PostProcessEffectFloatParameter(0.7, 0, 1); |
There was a problem hiding this comment.
💡 Codebase verification
The intensity parameter is not being utilized in the shader or render pass
The review comment is correct. While the intensity parameter is properly declared, it's not being used in either:
- The shader code, which currently only outputs the red channel values
- The
onRendermethod, which doesn't set the intensity uniform
The suggested implementation in the review comment would correctly integrate the intensity parameter into the effect.
🔗 Analysis chain
Verify the implementation of the intensity parameter.
While the addition of the intensity parameter follows the engine's pattern for post-processing effects, it appears that the parameter isn't being used in the onRender method. This might lead to the intensity setting having no effect on the output.
Consider updating the shader and onRender method to utilize this parameter. Example implementation:
void main(){
- gl_FragColor = texture2D(renderer_BlitTexture, v_uv).rrra;
+ vec4 color = texture2D(renderer_BlitTexture, v_uv);
+ gl_FragColor = mix(color, vec4(color.rrr, color.a), intensity);
} onRender(_, srcTexture: Texture2D, dst: RenderTarget): void {
const engine = this.engine;
+ this._blitMaterial.shaderData.setFloat("intensity", this.intensity.value);
Blitter.blitTexture(engine, srcTexture, dst, undefined, undefined, this._blitMaterial, 0);
}🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check the complete implementation in the file
cat e2e/case/postProcess-customPass.ts
# Search for any other files that might contain related shader code
fd -e glsl -e frag
# Look for other implementations that might use intensity
rg "intensity" -A 5 -B 5
Length of output: 79887
e2e/case/project-loader.ts
Outdated
| @@ -3,27 +3,27 @@ | |||
| * @category Advance | |||
| */ | |||
| import { Logger, WebGLEngine, AssetType, Camera } from "@galacean/engine"; | |||
| import { ShaderLab } from "@galacean/engine-shader-lab"; | |||
| import { ShaderLab } from "@galacean/engine-shaderlab"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Import change is correct, but documentation needs updating
The new package name @galacean/engine-shaderlab is correct and matches the package structure in the monorepo. However, the documentation in docs/en/basics/overview.md and docs/zh/basics/overview.md still references the old package name and should be updated.
🔗 Analysis chain
Verify the new package name is correct.
The import path has changed from @galacean/engine-shader-lab to @galacean/engine-shaderlab. Let's verify this change is consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old package name
# and verify the new package exists in package.json
echo "Checking for old package references..."
rg -l "engine-shader-lab"
echo "Verifying package.json dependencies..."
if [ -f "package.json" ]; then
jq '.dependencies["@galacean/engine-shaderlab"] // .devDependencies["@galacean/engine-shaderlab"]' package.json
fi
Length of output: 398
Script:
#!/bin/bash
# Check documentation references
echo "=== Checking documentation references ==="
rg -A 5 -B 5 "engine-shader-lab" docs/
# Look for workspace configuration files
echo -e "\n=== Looking for workspace files ==="
fd -e yaml -e json
# Search for the new package name in configuration files
echo -e "\n=== Searching for new package name ==="
rg -l "engine-shaderlab" --type-add 'config:*.{json,yaml,yml}' -t config
Length of output: 4523
.github/workflows/release.yml
Outdated
| @@ -46,7 +46,7 @@ jobs: | |||
| run: | | |||
| git clone https://github.com/galacean/engine-toolkit.git | |||
| git clone https://github.com/galacean/engine-lottie.git | |||
| git clone https://github.com/galacean/engine-spine.git | |||
| git clone https://github.com/galacean/engine-spine.git -b dev/4.2 | |||
There was a problem hiding this comment.
💡 Codebase verification
Consider using a stable tag instead of the dev branch
The repository has a stable 4.2.0 tag available, which would be more reliable than the development branch. Note that there are also newer releases (v0.0.3) that might be more appropriate depending on your compatibility requirements.
- Replace
git clone https://github.com/galacean/engine-spine.git -b dev/4.2withgit clone https://github.com/galacean/engine-spine.git -b 4.2.0
🔗 Analysis chain
Verify the engine-spine branch dependency.
The workflow now specifically checks out the dev/4.2 branch of engine-spine. Please confirm if this is intended to be a permanent change or if it should track a stable branch/tag instead.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the dev/4.2 branch exists and its latest commit
curl -s "https://api.github.com/repos/galacean/engine-spine/branches/dev/4.2" | jq '.commit.sha, .commit.commit.message'
# Check available release tags
curl -s "https://api.github.com/repos/galacean/engine-spine/tags" | jq '.[].name'
Length of output: 780
packages/loader/src/SceneLoader.ts
Outdated
| ReflectionParser.registerCustomParseComponent("TextRenderer", async (instance: any, item: Omit<IClass, "class">) => { | ||
| const { props } = item; | ||
| if (!props.font) { | ||
| // @ts-ignore | ||
| instance.font = Font.createFromOS(instance.engine, props.fontFamily || "Arial"); | ||
| } | ||
| ); | ||
| return instance; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Several improvements needed in TextRenderer registration.
- The
@ts-ignorecomment suppresses type checking. Consider fixing the type issue instead. - Hardcoded "Arial" font might not be available on all systems.
- Missing error handling for font creation failure.
- Props structure is not documented.
Consider this improved implementation:
ReflectionParser.registerCustomParseComponent("TextRenderer", async (instance: any, item: Omit<IClass, "class">) => {
const { props } = item;
+ // Document expected props structure
+ interface TextRendererProps {
+ font?: Font;
+ fontFamily?: string;
+ }
if (!props.font) {
- // @ts-ignore
- instance.font = Font.createFromOS(instance.engine, props.fontFamily || "Arial");
+ try {
+ // Use a list of fallback fonts
+ const defaultFonts = ["Arial", "Helvetica", "sans-serif"];
+ const fontFamily = props.fontFamily || defaultFonts.join(",");
+ instance.font = await Font.createFromOS(instance.engine, fontFamily);
+ } catch (error) {
+ Logger.warn(`Failed to create font: ${error.message}. Using default font.`);
+ // Provide a basic fallback font
+ instance.font = Font.getDefaultFont(instance.engine);
+ }
}
return instance;
});Committable suggestion skipped: line range outside the PR's diff.
e2e/case/material-LUT.ts
Outdated
| const shader = Shader.create( | ||
| "LUT-test", | ||
| ` | ||
| attribute vec3 POSITION; | ||
| attribute vec2 TEXCOORD_0; | ||
| uniform mat4 renderer_MVPMat; | ||
| varying vec2 v_uv; | ||
|
|
||
| void main(){ | ||
| gl_Position = renderer_MVPMat * vec4(POSITION, 1.0); | ||
| v_uv = TEXCOORD_0; | ||
| }`, | ||
| ` | ||
| varying vec2 v_uv; | ||
| uniform sampler2D scene_PrefilteredDFG; | ||
|
|
||
| void main(){ | ||
| gl_FragColor = texture2D(scene_PrefilteredDFG, v_uv); | ||
| } | ||
| ` | ||
| ); | ||
| const material = new Material(engine, shader); | ||
| renderer.setMaterial(material); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add shader precision and resource cleanup.
- Add precision specifiers to the fragment shader:
`
+ precision highp float;
varying vec2 v_uv;
uniform sampler2D scene_PrefilteredDFG;- Consider adding shader compilation error handling and resource cleanup:
const shader = Shader.create(
"LUT-test",
// ... shader code ...
);
+ if (!shader.valid) {
+ throw new Error("Failed to compile shader: " + shader.errorMessage);
+ }
+
+ engine.on("beforeDestroy", () => {
+ shader.destroy();
+ material.destroy();
+ });Committable suggestion skipped: line range outside the PR's diff.
e2e/case/material-LUT.ts
Outdated
| updateForE2E(engine); | ||
| initScreenshot(engine, camera); | ||
| }) | ||
| .catch((e) => { | ||
| console.log(e); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and add test cleanup.
- Improve error handling with more detailed logging:
.catch((e) => {
- console.log(e);
+ console.error("LUT Test failed:", e);
+ throw e; // Re-throw to ensure test failure
});- Consider adding test cleanup code:
updateForE2E(engine);
initScreenshot(engine, camera);
+ return () => {
+ // Cleanup resources
+ engine.destroy();
+ };Committable suggestion skipped: line range outside the PR's diff.
| export type IMethod = { | ||
| params: Array<IBasicType>; | ||
| result?: IInstance; | ||
| }; | ||
|
|
||
| export type IMethodParams = Array<IBasicType> | IMethod; |
There was a problem hiding this comment.
Resolve recursive type definitions to prevent potential type-checking issues
The current type definitions introduce recursive references that can lead to infinite recursion during type-checking. Specifically:
IBasicTypeincludesIClass,IClassType, andIMethodParams.IMethodParamsis defined asArray<IBasicType> | IMethod, andIMethodincludes aresult?: IInstance.IInstanceincludespropsandmethodsthat useIBasicTypeandIMethodParams, respectively.
This creates a circular dependency:
IBasicType➔IMethodParams➔IMethod➔IInstance➔props/methods➔IBasicType
Such unbounded recursive types can cause TypeScript to fail during compilation or lead to unexpected behaviors.
Consider refactoring the type definitions to break the circular dependencies. Possible solutions include:
- Redefine
IBasicType: ExcludeIClass,IClassType, andIMethodParamsfromIBasicTypeor replace them with more specific types that do not introduce recursion. - Adjust
IMethodParams: If possible, avoid includingIMethodwithinIMethodParams, or restructureIMethodto prevent the recursion. - Limit Recursion Depth: Use techniques like type aliases or conditional types to limit the depth of recursion.
- Separate Interfaces: Break down the interfaces into smaller, non-recursive parts or use separate interfaces for different use cases to avoid circular references.
Also applies to: 75-75, 77-81, 82-86, 98-99
| if (ReflectionParser._isClassType(value)) { | ||
| return Promise.resolve(Loader.getClass(value["classType"])); | ||
| } else if (ReflectionParser._isClass(value)) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for undefined classes in parseBasicType
When Loader.getClass(value["classType"]) is called, it might return undefined if the class type does not exist. This could cause issues later in the code. Consider adding error handling to manage cases where the class is not found.
Apply this diff to include error handling:
if (ReflectionParser._isClassType(value)) {
- return Promise.resolve(Loader.getClass(value["classType"]));
+ const Class = Loader.getClass(value["classType"]);
+ if (!Class) {
+ return Promise.reject(new Error(`Class not found for classType: ${value["classType"]}`));
+ }
+ return Promise.resolve(Class);
} else if (ReflectionParser._isClass(value)) {
// class object
return this.parseClassObject(value);
}📝 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 (ReflectionParser._isClassType(value)) { | |
| return Promise.resolve(Loader.getClass(value["classType"])); | |
| } else if (ReflectionParser._isClass(value)) { | |
| if (ReflectionParser._isClassType(value)) { | |
| const Class = Loader.getClass(value["classType"]); | |
| if (!Class) { | |
| return Promise.reject(new Error(`Class not found for classType: ${value["classType"]}`)); | |
| } | |
| return Promise.resolve(Class); | |
| } else if (ReflectionParser._isClass(value)) { |
| parseMethod(instance: any, methodName: string, methodParams: IMethodParams) { | ||
| const isMethodObject = ReflectionParser._isMethodObject(methodParams); | ||
| const params = isMethodObject ? methodParams.params : methodParams; | ||
|
|
||
| return Promise.all(params.map((param) => this.parseBasicType(param))).then((result) => { | ||
| const methodResult = instance[methodName](...result); | ||
| if (isMethodObject && methodParams.result) { | ||
| return this.parsePropsAndMethods(methodResult, methodParams.result); | ||
| } else { | ||
| return methodResult; | ||
| } |
There was a problem hiding this comment.
Ensure params is defined before using in parseMethod
In the parseMethod function, the params variable might be undefined if methodParams is neither an array nor an object with a params property. This could lead to a runtime error when calling params.map. Consider adding a check to ensure that params is an array before proceeding.
Apply this diff to handle cases where params is not an array:
const isMethodObject = ReflectionParser._isMethodObject(methodParams);
const params = isMethodObject ? methodParams.params : methodParams;
+ if (!Array.isArray(params)) {
+ return Promise.resolve();
+ }
return Promise.all(params.map((param) => this.parseBasicType(param))).then((result) => {
const methodResult = instance[methodName](...result);
if (isMethodObject && methodParams.result) {
return this.parsePropsAndMethods(methodResult, methodParams.result);
} else {
return methodResult;
}
});📝 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.
| parseMethod(instance: any, methodName: string, methodParams: IMethodParams) { | |
| const isMethodObject = ReflectionParser._isMethodObject(methodParams); | |
| const params = isMethodObject ? methodParams.params : methodParams; | |
| return Promise.all(params.map((param) => this.parseBasicType(param))).then((result) => { | |
| const methodResult = instance[methodName](...result); | |
| if (isMethodObject && methodParams.result) { | |
| return this.parsePropsAndMethods(methodResult, methodParams.result); | |
| } else { | |
| return methodResult; | |
| } | |
| parseMethod(instance: any, methodName: string, methodParams: IMethodParams) { | |
| const isMethodObject = ReflectionParser._isMethodObject(methodParams); | |
| const params = isMethodObject ? methodParams.params : methodParams; | |
| if (!Array.isArray(params)) { | |
| return Promise.resolve(); | |
| } | |
| return Promise.all(params.map((param) => this.parseBasicType(param))).then((result) => { | |
| const methodResult = instance[methodName](...result); | |
| if (isMethodObject && methodParams.result) { | |
| return this.parsePropsAndMethods(methodResult, methodParams.result); | |
| } else { | |
| return methodResult; | |
| } |
| return texture; | ||
| }); |
There was a problem hiding this comment.
Remove unnecessary return texture; statement inside Promise
The return texture; statement inside the Promise executor does not affect the resolved value of the Promise and can cause confusion. The value to resolve should be provided via the resolve function.
Remove the redundant line:
- return texture;📝 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.
| return texture; | |
| }); | |
| }); |
Fix: Merge code and fix unit test error
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/ui/src/index.ts (3)
33-62: 🛠️ Refactor suggestionImprove type safety and documentation in UI material setup.
The material configuration is thorough but needs improvements:
- The
@ts-ignoreon line 44 should be replaced with proper typing- Add JSDoc explaining the blend state configuration
- Consider making the material configuration parameters customizable
Apply this diff to improve the implementation:
export class EngineExtension { + /** Default material used for UI rendering with alpha blending */ _uiDefaultMaterial: Material; + /** + * Gets or creates the default UI material with alpha blending configuration. + * @returns Material configured for UI rendering with transparency + */ _getUIDefaultMaterial(): Material { if (!this._uiDefaultMaterial) { const shader = Shader.find("ui") ?? Shader.create("ui", [ new ShaderPass("Forward", uiDefaultVs, uiDefaultFs, { pipelineStage: PipelineStage.Forward }) ]); - // @ts-ignore - const material = new Material(this, shader); + const material = new Material(this as unknown as Engine, shader); const renderState = material.renderState; + this._configureBlendState(renderState.blendState.targetBlendState); + this._configureRenderState(renderState); + material.isGCIgnored = true; + this._uiDefaultMaterial = material; } return this._uiDefaultMaterial; } + + private _configureBlendState(target: any) { target.enabled = true; target.sourceColorBlendFactor = BlendFactor.SourceAlpha; target.destinationColorBlendFactor = BlendFactor.OneMinusSourceAlpha; target.sourceAlphaBlendFactor = BlendFactor.One; target.destinationAlphaBlendFactor = BlendFactor.OneMinusSourceAlpha; target.colorBlendOperation = target.alphaBlendOperation = BlendOperation.Add; + } + + private _configureRenderState(renderState: any) { renderState.depthState.writeEnabled = false; renderState.rasterState.cullMode = CullMode.Off; renderState.renderQueueType = RenderQueueType.Transparent; - material.isGCIgnored = true; - this._uiDefaultMaterial = material; } }
64-73: 🛠️ Refactor suggestionImprove UI hierarchy version tracking and add size management API.
Based on your feedback about UI element size management, we should enhance this class:
- Add proper typing for parent reference
- Add size management methods
- Document the version tracking mechanism
Apply this diff to improve the implementation:
export class EntityExtension { + /** Version number for tracking UI hierarchy updates */ _uiHierarchyVersion = 0; + /** + * Updates the UI hierarchy version and propagates changes to parent. + * @param version - New version number + * @param depth - Internal parameter to prevent infinite recursion + */ - _updateUIHierarchyVersion(version: number): void { + _updateUIHierarchyVersion(version: number, depth: number = 0): void { + const MAX_DEPTH = 1000; + if (depth > MAX_DEPTH) { + console.warn('Maximum UI hierarchy depth exceeded'); + return; + } if (this._uiHierarchyVersion !== version) { this._uiHierarchyVersion = version; - // @ts-ignore - this.parent?._updateUIHierarchyVersion(version); + (this as any).parent?._updateUIHierarchyVersion(version, depth + 1); } } + + /** + * Sets the width of the UI element. + * @param width - New width value + */ + setWidth(width: number): void { + const transform = this.getComponent('UITransform'); + if (transform) { + transform.width = width; + this._updateUIHierarchyVersion(Date.now()); + } + } + + /** + * Sets the height of the UI element. + * @param height - New height value + */ + setHeight(height: number): void { + const transform = this.getComponent('UITransform'); + if (transform) { + transform.height = height; + this._updateUIHierarchyVersion(Date.now()); + } + } }
105-112: 🛠️ Refactor suggestionImprove type safety and error handling in Text component parser.
The current implementation needs improvements in error handling and type safety:
Apply this diff:
-ReflectionParser.registerCustomParseComponent("Text", async (instance: any, item: Omit<IClass, "class">) => { +interface TextInstance { + engine: Engine; + font: any; +} + +interface TextProps extends Omit<IClass, "class"> { + props: { + font?: any; + fontFamily?: string; + }; +} + +ReflectionParser.registerCustomParseComponent("Text", async ( + instance: TextInstance, + item: TextProps +) => { const { props } = item; if (!props.font) { - // @ts-ignore - instance.font = Font.createFromOS(instance.engine, props.fontFamily || "Arial"); + try { + instance.font = await Font.createFromOS( + instance.engine, + props.fontFamily || "Arial" + ); + } catch (error) { + console.error(`Failed to create font: ${error}`); + throw new Error(`Font initialization failed: ${error}`); + } } return instance; });
🧹 Nitpick comments (13)
packages/ui/src/Utils.ts (4)
28-42: Consider optimizing entity traversal.The entity traversal could be optimized by:
- Caching the root canvas reference
- Using a more efficient data structure for component lookup
Example optimization:
private static _cachedRootCanvas = new WeakMap<Entity, UICanvas>(); static searchRootCanvasInParents(element: IElement): UICanvas { const entity = element instanceof UICanvas ? element.entity.parent : element.entity; // Check cache first if (this._cachedRootCanvas.has(entity)) { return this._cachedRootCanvas.get(entity); } // Existing traversal logic let current = entity; while (current) { const canvas = current._components?.find( c => c.enabled && c instanceof UICanvas && c._isRootCanvas ) as UICanvas; if (canvas) { // Cache the result this._cachedRootCanvas.set(entity, canvas); return canvas; } current = current.parent; } this._cachedRootCanvas.set(entity, null); return null; }
69-86: Consider extracting common traversal logic.The traversal logic in
searchGroupInParentsis similar tosearchRootCanvasInParents. Consider extracting the common pattern into a reusable function.private static _traverseEntities<T>( startEntity: Entity, stopEntity: Entity | null, predicate: (component: any) => component is T ): T | null { let entity = startEntity; while (entity && entity !== stopEntity) { const component = entity._components?.find(predicate); if (component) return component; entity = entity.parent; } return null; } static searchGroupInParents(element: IGroupAble): UIGroup { const rootCanvas = element._getRootCanvas(); if (!rootCanvas) return null; const startEntity = element instanceof UIGroup ? element.entity.parent : element.entity; return this._traverseEntities( startEntity, rootCanvas.entity.parent, (c): c is UIGroup => c.enabled && c instanceof UIGroup ); }
92-94: Improve code clarity by avoiding assignments in expressions.The assignments within logical expressions make the code harder to read. Consider separating them:
- const replaced = preCanvas._disorderedElements.deleteByIndex(element._indexInRootCanvas); - replaced && (replaced._indexInRootCanvas = element._indexInRootCanvas); + const replaced = preCanvas._disorderedElements.deleteByIndex(element._indexInRootCanvas); + if (replaced) { + replaced._indexInRootCanvas = element._indexInRootCanvas; + }Also applies to: 109-111
🧰 Tools
🪛 Biome (1.9.4)
[error] 93-93: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
1-152: Consider adding JSDoc documentation.The file implements complex UI element management logic. Adding comprehensive JSDoc documentation would help developers understand:
- The purpose and usage of each method
- The relationships between root canvases and groups
- The significance of dirty flags and registration
🧰 Tools
🪛 Biome (1.9.4)
[error] 7-152: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 11-11: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 17-17: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 20-20: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 24-24: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 25-25: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 47-47: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 53-53: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 58-58: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 60-60: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 65-65: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 66-66: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 93-93: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 110-110: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
packages/ui/src/index.ts (2)
18-31: Consider reorganizing UI components based on developer feedback.Regarding your question about the directory structure: The current organization (
component/advanced/,component/interactive/transition/) provides good separation of concerns. However, if you find that placing UI components directly in the root directory would improve developer experience, consider:
- Moving frequently used components to the root
- Keeping specialized components (like transitions) in subdirectories
- Maintaining the "UI" prefix convention you suggested for clarity
Would you like me to propose a specific directory restructuring plan?
75-88: Consider exposing a public API for UI operations.While it's good practice to mark internal implementation details with
@internal, consider exposing public methods for common UI operations that developers need to perform, such as size management.Would you like me to propose a public API design that wraps these internal implementations?
packages/core/src/Entity.ts (2)
629-644: Document the modification flags and their usage.Consider adding JSDoc comments to explain:
- The purpose of the modification system
- When and how to use the modification listeners
- The meaning of different modification flags
🧰 Tools
🪛 Biome (1.9.4)
[error] 633-633: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Line range hint
802-806: Consider removing deprecated matrix calculation code.Since this code is already marked as deprecated, consider:
- Creating a migration guide for users
- Removing the deprecated code in the next major version
packages/ui/src/component/UIGroup.ts (2)
57-127: Add parameter validation documentation.While the implementation is solid, consider adding JSDoc documentation for the parameter ranges:
alpha: Document that values are automatically clamped between 0 and 1interactive: Document the implications of enabling/disabling interactivity
143-154: Enhance cleanup in _onDisableInScene.The cleanup is good but could be more thorough. Consider resetting all group modify flags:
override _onDisableInScene(): void { Utils.cleanRootCanvas(this); Utils.cleanGroup(this); const disorderedElements = this._disorderedElements; disorderedElements.forEach((element: IGroupAble) => { Utils.setGroupDirty(element); }); disorderedElements.length = 0; disorderedElements.garbageCollection(); this._isRootCanvasDirty = this._isGroupDirty = false; + this._groupDirtyFlags = GroupModifyFlags.None; + this._globalAlpha = 1; + this._globalInteractive = true; }packages/ui/src/component/interactive/UIInteractive.ts (3)
49-51: Address multi-touch support TODO.The TODO comment indicates missing multi-touch support. This could impact the user experience on touch devices.
Would you like me to help create a GitHub issue to track the implementation of multi-touch support?
110-121: Improve readability of transition removal logic.The assignment within the condition makes the code harder to read. Consider separating the swap operation:
removeTransition(transition: Transition): void { const transitions = this._transitions; const lastOneIndex = transitions.length - 1; for (let i = lastOneIndex; i >= 0; i--) { if (transitions[i] === transition) { - i !== lastOneIndex && (transitions[i] = transitions[lastOneIndex]); + if (i !== lastOneIndex) { + transitions[i] = transitions[lastOneIndex]; + } transitions.length = lastOneIndex; transition._interactive = null; break; } } }🧰 Tools
🪛 Biome (1.9.4)
[error] 115-115: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
271-276: Add documentation for InteractiveState enum.Consider adding JSDoc comments to describe when each state is active and its implications:
export enum InteractiveState { + /** Default state when no interaction is occurring */ Normal, + /** Active when the pointer is down and dragging */ Pressed, + /** Active when the pointer is over the element */ Hover, + /** Active when interaction is disabled */ Disable }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/physics-physx/libs/physx.release.wasmis excluded by!**/*.wasm
📒 Files selected for processing (7)
packages/core/src/Entity.ts(10 hunks)packages/core/src/enums/EntityModifyFlags.ts(1 hunks)packages/ui/src/Utils.ts(1 hunks)packages/ui/src/component/UIGroup.ts(1 hunks)packages/ui/src/component/UIRenderer.ts(1 hunks)packages/ui/src/component/interactive/UIInteractive.ts(1 hunks)packages/ui/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/enums/EntityModifyFlags.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/ui/src/component/interactive/UIInteractive.ts
[error] 115-115: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
packages/ui/src/component/UIRenderer.ts
[error] 203-205: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
packages/core/src/Entity.ts
[error] 633-633: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
packages/ui/src/Utils.ts
[error] 7-152: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 11-11: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 17-17: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 20-20: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 24-24: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 25-25: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 47-47: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 53-53: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 58-58: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 60-60: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 65-65: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 66-66: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 93-93: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 110-110: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x, windows-latest)
🔇 Additional comments (14)
packages/ui/src/Utils.ts (2)
7-7: Consider converting static class to namespace or standalone functions.The
Utilsclass contains only static methods, which is an anti-pattern making the code harder to test and maintain.🧰 Tools
🪛 Biome (1.9.4)
[error] 7-152: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
31-32: Improve type safety by properly typing internal APIs.The code uses @ts-ignore to bypass TypeScript checks. Consider creating proper interfaces for these internal APIs.
packages/ui/src/index.ts (1)
90-103: Improve type safety in mixin implementation.The current implementation uses
anytype which reduces type safety. Let's improve it:Apply this diff:
-function ApplyMixins(derivedCtor: any, baseCtors: any[]): void { +interface Constructor { + new (...args: any[]): any; + prototype: any; +} + +function ApplyMixins(derivedCtor: Constructor, baseCtors: Constructor[]): void { baseCtors.forEach((baseCtor) => { Object.getOwnPropertyNames(baseCtor.prototype).forEach((name) => { Object.defineProperty( derivedCtor.prototype, name, Object.getOwnPropertyDescriptor(baseCtor.prototype, name) || Object.create(null) ); }); }); }packages/ui/src/component/UIRenderer.ts (4)
112-113: Remove @ts-ignore by properly typing the Color class.The use of @ts-ignore suppresses type checking. Consider properly typing the
_onValueChangedproperty in the Color class.
239-261: Optimize raycast hit detection performance.The raycast method creates new Vector3 instances on every call. Consider reusing existing vectors from a pool to reduce garbage collection.
202-209:⚠️ Potential issueAdd a break statement to prevent unintended fallthrough.
The case for
EntityModifyFlags.Parentfalls through toEntityModifyFlags.Childwithout an explicit break statement. This could lead to unexpected behavior.switch (flag) { case EntityModifyFlags.Parent: Utils.setRootCanvasDirty(this); Utils.setGroupDirty(this); + break; case EntityModifyFlags.Child: entity._updateUIHierarchyVersion(UICanvas._hierarchyCounter); break; default: break; }Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 203-205: This case is falling through to the next case.
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.(lint/suspicious/noFallthroughSwitchClause)
30-36: Consider thread safety implications of static temporary objects.The use of static Vector3, Matrix, and Plane objects for temporary calculations could lead to race conditions if the renderer is used in concurrent contexts. Consider using an object pool pattern or instance-level temporary objects.
Let's verify if these objects are used in any concurrent contexts:
packages/core/src/Entity.ts (2)
624-627: LGTM! Well-structured entity modification system.The changes introduce a consistent way to notify listeners about entity hierarchy modifications. This is particularly useful for GUI systems that need to react to entity relationship changes.
Also applies to: 350-350, 594-594, 663-663, 714-714, 790-790
106-106: Verify integration with GUI system.The new modification system seems crucial for GUI updates. Let's verify its integration:
Also applies to: 624-644
✅ Verification successful
Modification system integration verified successfully
The modification system is properly integrated with the GUI components, with correct usage patterns found in UI utilities and canvas components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if GUI components are using the entity modification system # Expected: Find GUI components that register modification listeners # Search for GUI components using the modification system ast-grep --pattern 'class $_ { $$$ _registerModifyListener($$$) { $$$ } $$$ }' # Search for direct usage of the modification system rg "_registerModifyListener|_unRegisterModifyListener" -A 5Length of output: 3165
packages/ui/src/component/UIGroup.ts (2)
6-51: LGTM! Well-structured class with clear property management.The class is well-organized with appropriate use of decorators and clear separation of internal properties.
205-219: LGTM! Efficient flag management implementation.The bitwise operations for managing dirty flags are well-implemented and efficient.
packages/ui/src/component/interactive/UIInteractive.ts (3)
143-161: LGTM! Clean and consistent pointer event handling.The pointer event handlers are well-implemented with consistent state updates.
236-268: LGTM! Well-structured state management.The state management logic is well-organized with clear separation between global interactive state and local state updates.
187-194: 🛠️ Refactor suggestionRemove @ts-ignore by properly extending Script class.
The use of @ts-ignore suggests type system issues. Consider properly extending the Script class by declaring the overridden methods in the base class.
- // @ts-ignore - override _onEnableInScene(): void { - // @ts-ignore - super._onEnableInScene(); + override _onEnableInScene(): void { + super._onEnableInScene?.(); Utils.setRootCanvasDirty(this); Utils.setGroupDirty(this); this._updateState(true); }Likely invalid or redundant comment.
| get color(): Color { | ||
| return this._color; | ||
| } | ||
|
|
||
| set color(value: Color) { | ||
| if (this._color !== value) { | ||
| this._color.copyFrom(value); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add convenience methods for size manipulation.
Based on the PR objectives, developers find it cumbersome to modify UI element sizes at runtime. Consider adding convenience methods like setSize(width, height) or setWidth(width) to simplify size manipulation without directly accessing the transform.
Example implementation:
+ /**
+ * Sets the width of the UI element.
+ * @param width The new width value
+ */
+ setWidth(width: number): void {
+ const transform = <UITransform>this._transformEntity.transform;
+ transform.size.x = width;
+ }
+
+ /**
+ * Sets the height of the UI element.
+ * @param height The new height value
+ */
+ setHeight(height: number): void {
+ const transform = <UITransform>this._transformEntity.transform;
+ transform.size.y = height;
+ }
+
+ /**
+ * Sets both width and height of the UI element.
+ * @param width The new width value
+ * @param height The new height value
+ */
+ setSize(width: number, height: number): void {
+ const transform = <UITransform>this._transformEntity.transform;
+ transform.size.x = width;
+ transform.size.y = height;
+ }Committable suggestion skipped: line range outside the PR's diff.
| * @internal | ||
| */ | ||
| _registerModifyListener(onChange: (flag: EntityModifyFlags) => void): void { | ||
| (this._modifyFlagManager ||= new UpdateFlagManager()).addListener(onChange); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix assignment in expression.
The assignment within the expression can be confusing and should be split for better readability.
- (this._modifyFlagManager ||= new UpdateFlagManager()).addListener(onChange);
+ if (!this._modifyFlagManager) {
+ this._modifyFlagManager = new UpdateFlagManager();
+ }
+ this._modifyFlagManager.addListener(onChange);📝 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.
| (this._modifyFlagManager ||= new UpdateFlagManager()).addListener(onChange); | |
| if (!this._modifyFlagManager) { | |
| this._modifyFlagManager = new UpdateFlagManager(); | |
| } | |
| this._modifyFlagManager.addListener(onChange); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 633-633: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
refactor: UIRenderer.raycastEnable -> UIRenderer.raycastEnabled
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
packages/ui/src/component/advanced/Image.ts (1)
259-299: Consider refactoring nested switch statements for better maintainability.The
_onSpriteChangemethod contains nested switch statements that handle different sprite modification flags. Consider extracting the inner switch cases into separate methods for better readability and maintainability.+ private _handleDrawModeUpdate(drawMode: SpriteDrawMode): void { + switch (drawMode) { + case SpriteDrawMode.Sliced: + this._dirtyUpdateFlag |= RendererUpdateFlags.WorldVolume; + break; + case SpriteDrawMode.Tiled: + this._dirtyUpdateFlag |= ImageUpdateFlags.WorldVolumeUVAndColor; + break; + } + } private _onSpriteChange(type: SpriteModifyFlags): void { switch (type) { case SpriteModifyFlags.size: - switch (this._drawMode) { - case SpriteDrawMode.Sliced: - this._dirtyUpdateFlag |= RendererUpdateFlags.WorldVolume; - break; - case SpriteDrawMode.Tiled: - this._dirtyUpdateFlag |= ImageUpdateFlags.WorldVolumeUVAndColor; - break; - } + this._handleDrawModeUpdate(this._drawMode); break; // ... rest of the cases } }packages/ui/src/component/advanced/Text.ts (4)
32-61: Consider enhancing type safety and documentationThe class properties are well-organized, but consider these improvements:
- Add
readonlymodifier to static properties- Add validation for numeric properties like
fontSizeandlineSpacing- Consider using an interface for the configuration options
309-354: Consider optimizing render methodThe render method handles multiple concerns. Consider splitting it into smaller, focused methods:
- State validation
- Position updates
- Color updates
- Render element creation
Also, add error handling for the texture assignment:
- subRenderElement.shaderData.setTexture(Text._textTextureProperty, texture); + if (!texture) { + console.warn("Attempted to set null texture in Text renderer"); + return; + } + subRenderElement.shaderData.setTexture(Text._textTextureProperty, texture);
655-659: Enhance TextChunk class type safetyThe TextChunk class could benefit from better type definitions and documentation.
class TextChunk { + /** Array of character render information for this chunk */ charRenderInfos = new Array<CharRenderInfo>(); + /** Texture used for this chunk */ texture: Texture2D; - subChunk; + /** Sub-chunk information */ + subChunk: SubChunk | null; }
664-672: Document DirtyFlag enum valuesAdd documentation for each flag value to explain its purpose and when it's used.
enum DirtyFlag { + /** Flag indicating the sub-font needs to be updated */ SubFont = 0x4, + /** Flag indicating local position and bounds need recalculation */ LocalPositionBounds = 0x8, + /** Flag indicating world position needs updating */ WorldPosition = 0x10, // LocalPositionBounds | WorldPosition | WorldVolume Position = 0x19, Font = SubFont | Position }packages/ui/src/component/UICanvas.ts (1)
597-619: Document intentional switch case fall-throughs.The switch cases in
_setRealRenderModehave intentional fall-throughs that should be documented to make the code more maintainable.switch (preRealMode) { case CanvasRenderMode.ScreenSpaceOverlay: this._removeCanvasListener(); + // Intentional fall-through case CanvasRenderMode.ScreenSpaceCamera: case CanvasRenderMode.WorldSpace: componentsManager.removeUICanvas(this, preRealMode === CanvasRenderMode.ScreenSpaceOverlay); break; default: break; } switch (curRealMode) { case CanvasRenderMode.ScreenSpaceOverlay: this._addCanvasListener(); + // Intentional fall-through case CanvasRenderMode.ScreenSpaceCamera: this._adapterPoseInScreenSpace(); this._adapterSizeInScreenSpace(); + // Intentional fall-through case CanvasRenderMode.WorldSpace: componentsManager.addUICanvas(this, curRealMode === CanvasRenderMode.ScreenSpaceOverlay); break; default: break; }🧰 Tools
🪛 Biome (1.9.4)
[error] 598-599: This case is falling through to the next case.
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.(lint/suspicious/noFallthroughSwitchClause)
[error] 608-609: This case is falling through to the next case.
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.(lint/suspicious/noFallthroughSwitchClause)
[error] 610-612: This case is falling through to the next case.
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.(lint/suspicious/noFallthroughSwitchClause)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/physics-physx/libs/physx.release.wasmis excluded by!**/*.wasm
📒 Files selected for processing (5)
packages/ui/src/component/UICanvas.ts(1 hunks)packages/ui/src/component/UIRenderer.ts(1 hunks)packages/ui/src/component/advanced/Image.ts(1 hunks)packages/ui/src/component/advanced/Text.ts(1 hunks)packages/ui/src/interface/IGraphics.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui/src/interface/IGraphics.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/ui/src/component/advanced/Text.ts
[error] 249-249: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 509-509: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 510-510: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 520-520: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 522-522: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 608-608: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 610-610: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
packages/ui/src/component/UICanvas.ts
[error] 189-189: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 262-262: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 277-277: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 468-468: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 598-599: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 608-609: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 610-612: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
packages/ui/src/component/UIRenderer.ts
[error] 208-210: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
🔇 Additional comments (10)
packages/ui/src/component/advanced/Image.ts (4)
27-38: LGTM! Well-structured class declaration and properties.The class is well-organized with appropriate use of decorators for cloning behavior and proper encapsulation of properties.
107-126: Remove @ts-ignore and improve type safety for sprite updates.The code uses
@ts-ignoreto bypass type checking for_updateFlagManager.This is a duplicate of a previous review comment. The suggestion to create proper interfaces for type safety remains valid:
+interface IUpdateFlagManager { + addListener(callback: (type: number) => void): void; + removeListener(callback: (type: number) => void): void; +} +interface ISpriteInternal extends Sprite { + _updateFlagManager: IUpdateFlagManager; +} - // @ts-ignore - lastSprite._updateFlagManager.removeListener(this._onSpriteChange); + (lastSprite as ISpriteInternal)._updateFlagManager.removeListener(this._onSpriteChange); - // @ts-ignore - value._updateFlagManager.addListener(this._onSpriteChange); + (value as ISpriteInternal)._updateFlagManager.addListener(this._onSpriteChange);
187-190: Address TODO comment and improve material handling.The code silently handles destroyed materials by falling back to default material.
This is a duplicate of a previous review comment. The suggestion to improve material handling remains valid:
- // @todo: This question needs to be raised rather than hidden. - if (material.destroyed) { - material = this._engine._getUIDefaultMaterial(); - } + if (material.destroyed) { + console.warn('Material was destroyed, falling back to default material'); + material = this._engine._getUIDefaultMaterial(); + this.setMaterial(material); // Update the reference to prevent future warnings + }
302-316: LGTM! Well-structured enum declaration.The
ImageUpdateFlagsenum is well-documented and follows proper bit flag patterns for update flags.packages/ui/src/component/UIRenderer.ts (4)
1-26: Well-structured class with appropriate dependencies!The class is properly organized with clear dependencies and implements the correct interface. The use of
@dependentComponentsensures thatUITransformis automatically added when needed.Also applies to: 27-28
244-266: Optimize raycast hit detection performance.The raycast method creates new Vector3 instances on every call. Consider reusing existing vectors from a pool to reduce garbage collection.
207-217:⚠️ Potential issueFix unintended switch case fall-through.
The case for
EntityModifyFlags.Parentfalls through toEntityModifyFlags.Childwithout abreakstatement, which appears to be unintentional and could lead to unexpected behavior.switch (flag) { case EntityModifyFlags.Parent: Utils.setRootCanvasDirty(this); Utils.setGroupDirty(this); + break; case EntityModifyFlags.Child: entity._updateUIHierarchyVersion(UICanvas._hierarchyCounter); break; default: break; }Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 208-210: This case is falling through to the next case.
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.(lint/suspicious/noFallthroughSwitchClause)
106-114: 🛠️ Refactor suggestionImprove type safety in the constructor.
The constructor uses
@ts-ignoreand directly accesses a protected property_onValueChanged. Consider creating a proper interface or type for the Color class to avoid using type assertions.- //@ts-ignore - this._color._onValueChanged = this._onColorChanged; + interface IObservableColor extends Color { + _onValueChanged: () => void; + } + (this._color as IObservableColor)._onValueChanged = this._onColorChanged;Likely invalid or redundant comment.
packages/ui/src/component/UICanvas.ts (2)
466-469: 🛠️ Refactor suggestionImprove readability by avoiding assignment in logical expression.
The assignment within the logical AND expression in the
_walkmethod makes the code harder to read and maintain.for (let i = 0, n = children.length; i < n; i++) { const child = children[i]; - child.isActive && (depth = this._walk(child, renderers, depth, group)); + if (child.isActive) { + depth = this._walk(child, renderers, depth, group); + } }Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 468-468: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
187-190: 🛠️ Refactor suggestionImprove readability by avoiding assignment in logical expression.
The assignment within the logical AND expression makes the code harder to read and maintain.
if (this._sortOrder !== value) { this._sortOrder = value; - this._realRenderMode === CanvasRenderMode.ScreenSpaceOverlay && - // @ts-ignore - (this.scene._componentsManager._overlayCanvasesSortingFlag = true); + if (this._realRenderMode === CanvasRenderMode.ScreenSpaceOverlay) { + // @ts-ignore + this.scene._componentsManager._overlayCanvasesSortingFlag = true; + } }Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 189-189: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
| this._freeTextChunks(); | ||
| this._textChunks = null; | ||
|
|
||
| this._subFont && (this._subFont = null); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor assignments in expressions
Multiple instances of assignments within expressions make the code less readable and maintainable.
Apply these changes:
- this._subFont && (this._subFont = null);
+ if (this._subFont) {
+ this._subFont = null;
+ }
- firstRow < 0 && (firstRow = j);
+ if (firstRow < 0) {
+ firstRow = j;
+ }
- const charRenderInfo = (charRenderInfos[renderElementCount++] = charRenderInfoPool.get());
+ const charRenderInfo = charRenderInfoPool.get();
+ charRenderInfos[renderElementCount] = charRenderInfo;
+ renderElementCount++;
- i === firstLine && (maxY = Math.max(maxY, top));
+ if (i === firstLine) {
+ maxY = Math.max(maxY, top);
+ }
- j === firstRow && (minX = Math.min(minX, left));
+ if (j === firstRow) {
+ minX = Math.min(minX, left);
+ }Also applies to: 509-509, 510-510, 520-520, 522-522
🧰 Tools
🪛 Biome (1.9.4)
[error] 249-249: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
| set fontSize(value: number) { | ||
| if (this._fontSize !== value) { | ||
| this._fontSize = value; | ||
| this._setDirtyFlagTrue(DirtyFlag.Font); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for numeric properties
The fontSize setter should validate the input value to prevent invalid states.
set fontSize(value: number) {
+ if (value < 0) {
+ throw new Error("Font size cannot be negative");
+ }
if (this._fontSize !== value) {
this._fontSize = value;
this._setDirtyFlagTrue(DirtyFlag.Font);
}
}📝 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.
| set fontSize(value: number) { | |
| if (this._fontSize !== value) { | |
| this._fontSize = value; | |
| this._setDirtyFlagTrue(DirtyFlag.Font); | |
| } | |
| } | |
| set fontSize(value: number) { | |
| if (value < 0) { | |
| throw new Error("Font size cannot be negative"); | |
| } | |
| if (this._fontSize !== value) { | |
| this._fontSize = value; | |
| this._setDirtyFlagTrue(DirtyFlag.Font); | |
| } | |
| } |
| switch (mode) { | ||
| case CanvasRenderMode.ScreenSpaceOverlay: | ||
| const { min, max } = renderer.bounds; | ||
| if (min.x > width || max.x < 0 || min.y > height || max.y < 0) { | ||
| continue; | ||
| } | ||
| break; | ||
| case CanvasRenderMode.ScreenSpaceCamera: | ||
| case CanvasRenderMode.WorldSpace: | ||
| if (!frustum.intersectsBox(renderer.bounds)) { | ||
| continue; | ||
| } | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap switch case variable declarations in blocks.
Variables declared in switch cases can be accidentally accessed by other cases. Wrap the declarations in blocks to prevent scope leakage.
switch (mode) {
case CanvasRenderMode.ScreenSpaceOverlay: {
const { min, max } = renderer.bounds;
if (min.x > width || max.x < 0 || min.y > height || max.y < 0) {
continue;
}
break;
}
case CanvasRenderMode.ScreenSpaceCamera:
case CanvasRenderMode.WorldSpace:
if (!frustum.intersectsBox(renderer.bounds)) {
continue;
}
break;
default:
break;
}📝 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.
| switch (mode) { | |
| case CanvasRenderMode.ScreenSpaceOverlay: | |
| const { min, max } = renderer.bounds; | |
| if (min.x > width || max.x < 0 || min.y > height || max.y < 0) { | |
| continue; | |
| } | |
| break; | |
| case CanvasRenderMode.ScreenSpaceCamera: | |
| case CanvasRenderMode.WorldSpace: | |
| if (!frustum.intersectsBox(renderer.bounds)) { | |
| continue; | |
| } | |
| break; | |
| default: | |
| break; | |
| } | |
| } | |
| switch (mode) { | |
| case CanvasRenderMode.ScreenSpaceOverlay: { | |
| const { min, max } = renderer.bounds; | |
| if (min.x > width || max.x < 0 || min.y > height || max.y < 0) { | |
| continue; | |
| } | |
| break; | |
| } | |
| case CanvasRenderMode.ScreenSpaceCamera: | |
| case CanvasRenderMode.WorldSpace: | |
| if (!frustum.intersectsBox(renderer.bounds)) { | |
| continue; | |
| } | |
| break; | |
| default: | |
| break; | |
| } | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 277-277: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
Fix: fix conflicts
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
packages/ui/src/component/interactive/UIInteractive.ts (2)
178-185:⚠️ Potential issueRemove @ts-ignore and fix type definitions.
Using @ts-ignore suppresses TypeScript errors which could hide potential issues. This appears to be a recurring issue based on past reviews.
Consider:
- Properly declaring the lifecycle methods in the base Script class
- Using proper method visibility modifiers
- Adding proper type definitions for the overridden methods
87-95: 🛠️ Refactor suggestionAdd validation in addTransition method.
The method should validate the input transition.
addTransition(transition: Transition): void { + if (!transition) { + throw new Error("Transition cannot be null or undefined"); + } const interactive = transition._interactive; if (interactive !== this) { interactive?.removeTransition(transition); this._transitions.push(transition); transition._interactive = this; transition._setState(this._state, true); } }
🧹 Nitpick comments (13)
packages/shader-lab/src/lexer/Utils.ts (1)
37-38: LGTM! Consider enhancing the comments.The addition of carriage return handling improves cross-platform compatibility. A minor suggestion would be to make the comments more descriptive:
- charCode === 10 || // Line break - /n - charCode === 13 || // Carriage return -/r + charCode === 10 || // Line Feed (LF/\n) - Unix style line ending + charCode === 13 || // Carriage Return (CR/\r) - Classic Mac style line endingpackages/shader-lab/src/common/BaseScanner.ts (1)
120-124: Consider optimizing the comment scanning loop.While the current implementation correctly handles both line endings, it could be simplified using a more idiomatic approach.
- let curChar = this.getCurChar(); - while (curChar !== "\n" && curChar !== "\r" && !this.isEnd()) { - this._advance(); - curChar = this.getCurChar(); - } + while (!this.isEnd()) { + const curChar = this.getCurChar(); + if (curChar === "\n" || curChar === "\r") break; + this._advance(); + }This refactoring:
- Reduces variable reassignment
- Improves readability with early termination
- Maintains the same functionality
tests/src/shader-lab/ShaderLab.test.ts (2)
21-41: Consider moving macro definitions to a test fixture file.While the macro definitions are well-structured, moving them to a separate test fixture file would improve maintainability and reusability across tests.
- const commonMacros = [ - { name: "RENDERER_IS_RECEIVE_SHADOWS" }, - // ... other macros - ] - .map((item) => `#define ${item.name} ${item.value ?? ""}`) - .join("\n"); + import { commonMacros } from "./fixtures/shader-macros";
150-151: Add error handling for registerIncludes.Consider wrapping the registration in a try-catch block to handle potential initialization failures gracefully.
beforeAll(() => { - registerIncludes(); + try { + registerIncludes(); + } catch (error) { + console.error('Failed to register shader includes:', error); + throw error; + } });packages/ui/src/component/interactive/UIInteractive.ts (5)
49-51: Track multi-touch support requirement.The TODO comment indicates that multi-touch points are not yet supported. This limitation should be tracked and addressed in future updates.
Would you like me to create a GitHub issue to track the multi-touch support requirement?
13-36: Document internal state tracking properties.Consider adding JSDoc comments to describe the purpose and behavior of these internal state tracking properties. This would help other developers understand:
- The relationship between canvas and group indices
- The purpose of dirty flags
- The role of listening entities arrays
125-132: Consider performance optimization in onUpdate.The update loop iterates through all transitions even when there might be no active transitions.
override onUpdate(deltaTime: number): void { if (this._getGlobalInteractive()) { const transitions = this._transitions; + if (transitions.length === 0) return; for (let i = 0, n = transitions.length; i < n; i++) { transitions[i]._onUpdate(deltaTime); } } }
117-123: Rename clearShapes to clearTransitions for consistency.The method name
clearShapesdoesn't match its functionality of clearing transitions. This could be confusing for developers.- clearShapes(): void { + clearTransitions(): void { const transitions = this._transitions; for (let i = 0, n = transitions.length; i < n; i++) { transitions[i]._interactive = null; } transitions.length = 0; }
106-106: Simplify the conditional assignment.The current implementation uses assignment within an expression, which can be confusing and was flagged by static analysis.
- i !== lastOneIndex && (transitions[i] = transitions[lastOneIndex]); + if (i !== lastOneIndex) { + transitions[i] = transitions[lastOneIndex]; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 106-106: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
packages/ui/src/component/advanced/Button.ts (2)
1-5: Consider adding "UI" prefix to the class name for consistency.Based on the PR objectives discussing naming conventions, consider renaming the class to
UIButtonto maintain consistency with other UI components.-export class Button extends UIInteractive { +export class UIButton extends UIInteractive {
23-30: Consider using a more functional approach for listener iteration.While the current implementation works correctly, it could be more concise using functional programming patterns.
override onPointerClick(event: PointerEventData): void { if (!this._getGlobalInteractive()) return; - const listeners = this._listeners.getLoopArray(); - for (let i = 0, n = listeners.length; i < n; i++) { - const listener = listeners[i]; - !listener.destroyed && listener.fn(event); - } + this._listeners.getLoopArray() + .filter(listener => !listener.destroyed) + .forEach(listener => listener.fn(event)); }packages/shader-lab/src/parser/builtin/functions.ts (1)
35-41: Clarify the distinction between_returnTypeand_realReturnTypeBoth
_returnTypeand_realReturnTypeare used within theBuiltinFunctionclass to represent return types. To improve code clarity and maintainability, consider:
- Renaming variables: Use more descriptive names to differentiate the intended purposes of these properties. For example,
_genericReturnTypeand_actualReturnType.- Adding comments: Provide comments explaining the roles of each property and how they differ.
This will help future developers understand the code more easily.
packages/shader-lab/src/parser/AST.ts (1)
54-56: Consider makingsemanticAnalyzean abstract methodThe
semanticAnalyzemethod is defined in the baseTreeNodeclass but has an empty implementation. To enforce implementation in all subclasses that require semantic analysis, consider:
- Making
semanticAnalyzeabstract: If using TypeScript'sabstractkeyword, declaresemanticAnalyzeas an abstract method. This will ensure all subclasses provide their own implementation.- Providing a default implementation: If an empty implementation is acceptable, document the intended usage so that subclasses override it as needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
packages/physics-physx/libs/physx.release.wasmis excluded by!**/*.wasmpnpm-lock.yamlis excluded by!**/pnpm-lock.yamltests/src/shader-lab/shaders/builtin-function.shaderis excluded by!**/*.shadertests/src/shader-lab/shaders/demo.shaderis excluded by!**/*.shader
📒 Files selected for processing (14)
packages/shader-lab/src/common/BaseScanner.ts(2 hunks)packages/shader-lab/src/common/types.ts(1 hunks)packages/shader-lab/src/lalr/CFG.ts(1 hunks)packages/shader-lab/src/lexer/Utils.ts(1 hunks)packages/shader-lab/src/parser/AST.ts(5 hunks)packages/shader-lab/src/parser/GrammarSymbol.ts(1 hunks)packages/shader-lab/src/parser/TargetParser.y(1 hunks)packages/shader-lab/src/parser/builtin/functions.ts(4 hunks)packages/shader-lab/src/preprocessor/PpParser.ts(1 hunks)packages/ui/src/component/advanced/Button.ts(1 hunks)packages/ui/src/component/interactive/UIInteractive.ts(1 hunks)tests/package.json(1 hunks)tests/src/shader-lab/ShaderLab.test.ts(3 hunks)tests/src/shader-lab/ShaderValidate.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
packages/ui/src/component/interactive/UIInteractive.ts
[error] 106-106: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
packages/ui/src/component/advanced/Button.ts
[error] 20-20: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 34-34: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (11)
packages/shader-lab/src/common/BaseScanner.ts (1)
14-14: LGTM! Consistent with lexer changes.The addition of '\r' to _spaceCharsWithBreak aligns with the lexer's enhanced whitespace handling, ensuring consistent behavior across the codebase.
tests/src/shader-lab/ShaderLab.test.ts (1)
12-12: LGTM!The import statement follows the project's conventions and is appropriately placed with other imports.
packages/ui/src/component/advanced/Button.ts (3)
7-13: LGTM! Well-documented and type-safe implementation.The method provides a clean interface for adding click listeners with proper typing and documentation.
38-41: LGTM! Clean interface definition.The interface is well-structured with appropriate types and optionality.
4-4: Verify transition-related functionality.Based on previous review comments about transitions, it seems that transition-related code might be missing or moved elsewhere. Could you clarify:
- Whether multiple transition types are still needed?
- Where the transition-related properties are currently implemented?
✅ Verification successful
Transition functionality is properly implemented
The Button class inherits comprehensive transition support from UIInteractive, including:
- Multiple transition types (Color, Scale, Sprite)
- Full transition lifecycle management
- Ability to combine different transitions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for transition-related code in UI components echo "Searching for transition-related code..." rg -i "transition" "packages/ui/src/"Length of output: 6342
packages/shader-lab/src/common/types.ts (1)
137-137: LGTM! The SEMICOLON token is a logical addition.The addition of the SEMICOLON token type is well-placed among other punctuation tokens and will support the new variable declaration syntax in the shader language parser.
Let's verify the usage of this token in the grammar rules:
✅ Verification successful
SEMICOLON token is properly integrated in the grammar
The token is correctly used in variable declarations, struct declarations, and other statement types, following standard language grammar patterns. The implementation is complete and consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if SEMICOLON token is properly integrated in grammar rules # Expected: Find references to SEMICOLON in grammar/parser files # Search for SEMICOLON usage in grammar definitions rg -A 5 "SEMICOLON" "packages/shader-lab/src" # Search for variable declaration related grammar rules ast-grep --pattern 'variable_declaration_$$$'Length of output: 5786
Script:
#!/bin/bash # Search for variable declaration related patterns in grammar rg -A 3 "init_declarator" packages/shader-lab/src rg -A 3 "declaration_list" packages/shader-lab/srcLength of output: 8159
packages/shader-lab/src/parser/builtin/functions.ts (2)
258-259: Corrected return types fortexture2DfunctionsThe
texture2Dfunctions now returnEKeyword.VEC4, which is consistent with the expectedvec4output in GLSL. This change aligns with the GLSL specifications and enhances the correctness of texture sampling functions.
295-296: Verify correctness of newly added texture functionsThe functions
texture2DLodEXTandtextureCubeLodEXThave been added with specific parameter and return types. To ensure they conform to GLSL specifications:
- Confirm parameter types: Verify that the parameter types (
EGenType.GSampler2D,EGenType.GSampler3D, etc.) match the expected sampler and coordinate types in GLSL.- Validate return types: Ensure that the return types (
EGenType.GVec4) accurately represent the expected output of these functions.Refer to the official GLSL documentation for
texture2DLodEXTandtextureCubeLodEXTto confirm the correctness of these function definitions.Also applies to: 301-301
packages/shader-lab/src/parser/AST.ts (1)
1150-1171: Review the handling of variable declarations inVariableDeclarationListWhen creating new variables in
VariableDeclarationList, ensure that:
- Pooled objects are properly initialized: After calling
VariableDeclaration.pool.get(), thesetmethod should fully initialize the object.- Semantic analysis is correctly applied: The
semanticAnalyzemethod is called on the new variable, but ensure it operates correctly with the newly assigned children.Double-check that reusing pooled objects does not introduce unintended side effects or state from previous usages.
packages/shader-lab/src/parser/TargetParser.y (1)
69-69: LGTM! Well-structured grammar changes.The changes improve the grammar by:
- Separating variable declarations from statements
- Supporting multiple variable declarations in a single statement
- Maintaining a clean separation of concerns
The new structure aligns with common programming language grammars and provides better flexibility.
Also applies to: 75-76, 79-87
packages/shader-lab/src/lalr/CFG.ts (1)
19-19: LGTM! Well-implemented CFG changes.The changes correctly implement the grammar rules from TargetParser.y while:
- Maintaining consistency with the grammar specification
- Preserving the AST node pool pattern
- Following the established production structure
Also applies to: 27-28, 33-45
| it("builtin-function", async () => { | ||
| let shaderSource = await readFile("./shaders/builtin-function.shader"); | ||
| shaderSource = shaderSource.replace("__$$insert_maros$$__", commonMacros); | ||
| glslValidate(shaderSource, shaderLabVerbose, {}); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertions to verify shader validation result.
The test should include assertions to verify that the shader validation succeeds and produces the expected output.
it("builtin-function", async () => {
let shaderSource = await readFile("./shaders/builtin-function.shader");
shaderSource = shaderSource.replace("__$$insert_maros$$__", commonMacros);
- glslValidate(shaderSource, shaderLabVerbose, {});
+ const result = glslValidate(shaderSource, shaderLabVerbose, {});
+ expect(result).toBeTruthy();
+ expect(shaderLabVerbose.errors).toHaveLength(0);
});📝 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("builtin-function", async () => { | |
| let shaderSource = await readFile("./shaders/builtin-function.shader"); | |
| shaderSource = shaderSource.replace("__$$insert_maros$$__", commonMacros); | |
| glslValidate(shaderSource, shaderLabVerbose, {}); | |
| it("builtin-function", async () => { | |
| let shaderSource = await readFile("./shaders/builtin-function.shader"); | |
| shaderSource = shaderSource.replace("__$$insert_maros$$__", commonMacros); | |
| const result = glslValidate(shaderSource, shaderLabVerbose, {}); | |
| expect(result).toBeTruthy(); | |
| expect(shaderLabVerbose.errors).toHaveLength(0); | |
| }); |
| @@ -439,7 +439,7 @@ export class PpParser { | |||
| return !!this._definedMacros.get(macro.lexeme); | |||
| } else { | |||
| const macro = this._definedMacros.get(id.lexeme); | |||
| if (!macro) { | |||
| if (!macro || !macro.body) { | |||
There was a problem hiding this comment.
💡 Codebase verification
Add tests for macro body validation
The null check is crucial as macro.body is used extensively in subsequent operations (numeric value extraction, macro expansion, and replacements). While the validation is correct, there are no dedicated tests found for the preprocessor.
Consider adding tests for:
- Macro definition without a body
- Invalid macro body for numeric constants
- Function macro body validation
🔗 Analysis chain
LGTM! Additional null check improves robustness.
The additional validation !macro.body prevents potential runtime errors when a macro exists but has no body defined. This check is particularly important as the code subsequently uses macro.body.lexeme.
Consider adding a test case to verify this edge case. Here's a script to check if there are any existing tests for this scenario:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files related to PpParser and macro body validation
rg -l "PpParser.*macro.*body" --type ts "test|spec|__tests__"
Length of output: 123
Script:
#!/bin/bash
# Find test files related to PpParser
fd -e ts -e test.ts "PpParser" packages/shader-lab
# If we find test files, let's examine their contents for macro-related tests
rg "macro.*body|body.*macro" packages/shader-lab --type ts -C 2
Length of output: 1883
| override onDestroy(): void { | ||
| super.onDestroy(); | ||
| this._listeners.findAndRemove((value) => (value.destroyed = true)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply consistent cleanup pattern.
Similar to the removeClicked method, simplify the cleanup logic for better readability.
override onDestroy(): void {
super.onDestroy();
- this._listeners.findAndRemove((value) => (value.destroyed = true));
+ this._listeners.findAndRemove((value) => {
+ value.destroyed = true;
+ return true;
+ });
}📝 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 onDestroy(): void { | |
| super.onDestroy(); | |
| this._listeners.findAndRemove((value) => (value.destroyed = true)); | |
| } | |
| override onDestroy(): void { | |
| super.onDestroy(); | |
| this._listeners.findAndRemove((value) => { | |
| value.destroyed = true; | |
| return true; | |
| }); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 34-34: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
| /** | ||
| * Remove a listening function of click. | ||
| * @param listener - The listening function | ||
| */ | ||
| removeClicked(listener: (event: PointerEventData) => void): void { | ||
| this._listeners.findAndRemove((value) => (value.fn === listener ? (value.destroyed = true) : false)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the removal logic for better readability.
The current implementation uses an assignment within an expression, which can be confusing and is flagged by static analysis.
- removeClicked(listener: (event: PointerEventData) => void): void {
- this._listeners.findAndRemove((value) => (value.fn === listener ? (value.destroyed = true) : false));
+ removeClicked(listener: (event: PointerEventData) => void): void {
+ this._listeners.findAndRemove((value) => {
+ if (value.fn === listener) {
+ value.destroyed = true;
+ return true;
+ }
+ return false;
+ });
+ }📝 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.
| /** | |
| * Remove a listening function of click. | |
| * @param listener - The listening function | |
| */ | |
| removeClicked(listener: (event: PointerEventData) => void): void { | |
| this._listeners.findAndRemove((value) => (value.fn === listener ? (value.destroyed = true) : false)); | |
| } | |
| /** | |
| * Remove a listening function of click. | |
| * @param listener - The listening function | |
| */ | |
| removeClicked(listener: (event: PointerEventData) => void): void { | |
| this._listeners.findAndRemove((value) => { | |
| if (value.fn === listener) { | |
| value.destroyed = true; | |
| return true; | |
| } | |
| return false; | |
| }); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 20-20: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
| static getFn(ident: string, parameterTypes: NonGenericGalaceanType[]): BuiltinFunction | undefined { | ||
| const list = BuiltinFunctionTable.get(ident); | ||
| let realType = TypeAny; | ||
| if (list?.length) { | ||
| const fun = list.find((item) => { | ||
| if (item.args.length !== args.length) return false; | ||
| let genType = 0; | ||
| for (let i = 0; i < args.length; i++) { | ||
| if (args[i] === TypeAny) continue; | ||
| realType = args[i]; | ||
| if (isGenericType(item.args[i])) { | ||
| if (genType === 0) { | ||
| genType = args[i]; | ||
| continue; | ||
| } else { | ||
| realType = genType; | ||
| if (list) { | ||
| for (let length = list.length, i = 0; i < length; i++) { | ||
| const fn = list[i]; | ||
| const fnArgs = fn.args; | ||
| const argLength = fnArgs.length; | ||
| if (argLength !== parameterTypes.length) continue; | ||
| // Try to match generic parameter type. | ||
| let returnType = TypeAny; | ||
| let found = true; | ||
| for (let i = 0; i < argLength; i++) { | ||
| const curFnArg = fnArgs[i]; | ||
| if (isGenericType(curFnArg)) { | ||
| if (returnType === TypeAny) returnType = parameterTypes[i]; | ||
| } else { | ||
| if (curFnArg !== parameterTypes[i] && parameterTypes[i] !== TypeAny) { | ||
| found = false; | ||
| break; | ||
| } | ||
| } | ||
| if (args[i] === TypeAny) continue; | ||
| if (args[i] !== realType) return false; | ||
| } | ||
| return true; | ||
| }); | ||
| if (fun) return { fun, genType: realType }; | ||
| if (found) { | ||
| fn._realReturnType = returnType; | ||
| return fn; | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure valid assignment of _realReturnType in getFn method
In the getFn method, returnType is initially set to TypeAny. If no generic type is found, returnType may remain TypeAny, which is not a valid NonGenericGalaceanType. Assigning TypeAny to _realReturnType (which expects a NonGenericGalaceanType) could lead to type inconsistencies or runtime errors.
Apply this diff to ensure _realReturnType is assigned only when it is a valid type:
if (found) {
- fn._realReturnType = returnType;
- return fn;
+ if (returnType !== TypeAny) {
+ fn._realReturnType = returnType;
+ return fn;
+ }
}This adjustment ensures that _realReturnType is assigned only when returnType is a valid NonGenericGalaceanType.
📝 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.
| static getFn(ident: string, parameterTypes: NonGenericGalaceanType[]): BuiltinFunction | undefined { | |
| const list = BuiltinFunctionTable.get(ident); | |
| let realType = TypeAny; | |
| if (list?.length) { | |
| const fun = list.find((item) => { | |
| if (item.args.length !== args.length) return false; | |
| let genType = 0; | |
| for (let i = 0; i < args.length; i++) { | |
| if (args[i] === TypeAny) continue; | |
| realType = args[i]; | |
| if (isGenericType(item.args[i])) { | |
| if (genType === 0) { | |
| genType = args[i]; | |
| continue; | |
| } else { | |
| realType = genType; | |
| if (list) { | |
| for (let length = list.length, i = 0; i < length; i++) { | |
| const fn = list[i]; | |
| const fnArgs = fn.args; | |
| const argLength = fnArgs.length; | |
| if (argLength !== parameterTypes.length) continue; | |
| // Try to match generic parameter type. | |
| let returnType = TypeAny; | |
| let found = true; | |
| for (let i = 0; i < argLength; i++) { | |
| const curFnArg = fnArgs[i]; | |
| if (isGenericType(curFnArg)) { | |
| if (returnType === TypeAny) returnType = parameterTypes[i]; | |
| } else { | |
| if (curFnArg !== parameterTypes[i] && parameterTypes[i] !== TypeAny) { | |
| found = false; | |
| break; | |
| } | |
| } | |
| if (args[i] === TypeAny) continue; | |
| if (args[i] !== realType) return false; | |
| } | |
| return true; | |
| }); | |
| if (fun) return { fun, genType: realType }; | |
| if (found) { | |
| fn._realReturnType = returnType; | |
| return fn; | |
| } | |
| } | |
| static getFn(ident: string, parameterTypes: NonGenericGalaceanType[]): BuiltinFunction | undefined { | |
| const list = BuiltinFunctionTable.get(ident); | |
| if (list) { | |
| for (let length = list.length, i = 0; i < length; i++) { | |
| const fn = list[i]; | |
| const fnArgs = fn.args; | |
| const argLength = fnArgs.length; | |
| if (argLength !== parameterTypes.length) continue; | |
| // Try to match generic parameter type. | |
| let returnType = TypeAny; | |
| let found = true; | |
| for (let i = 0; i < argLength; i++) { | |
| const curFnArg = fnArgs[i]; | |
| if (isGenericType(curFnArg)) { | |
| if (returnType === TypeAny) returnType = parameterTypes[i]; | |
| } else { | |
| if (curFnArg !== parameterTypes[i] && parameterTypes[i] !== TypeAny) { | |
| found = false; | |
| break; | |
| } | |
| } | |
| } | |
| if (found) { | |
| if (returnType !== TypeAny) { | |
| fn._realReturnType = returnType; | |
| return fn; | |
| } | |
| } | |
| } |
| type: FullySpecifiedType; | ||
|
|
||
| override semanticAnalyze(sa: SematicAnalyzer): void { | ||
| const type = this.children[0] as FullySpecifiedType; | ||
| const ident = this.children[1] as Token; | ||
| let sm: VarSymbol; | ||
| sm = new VarSymbol(ident.lexeme, new SymbolType(type.type, type.typeSpecifier.lexeme), true, this); | ||
| const children = this.children; | ||
| const type = children[0] as FullySpecifiedType; | ||
| const ident = children[1] as Token; | ||
| this.type = type; | ||
| const sm = new VarSymbol(ident.lexeme, new SymbolType(type.type, type.typeSpecifier.lexeme), true, this); | ||
|
|
||
| sa.symbolTableStack.insert(sm); | ||
| } | ||
|
|
||
| override codeGen(visitor: CodeGenVisitor): string { | ||
| return visitor.visitGlobalVariableDeclaration(this); | ||
| return visitor.visitGlobalVariableDeclaration(this) + ";"; | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure type is properly initialized in VariableDeclaration
In the VariableDeclaration class, the type property is assigned from children[0]:
this.type = children[0] as FullySpecifiedType;Ensure that:
children[0]is always aFullySpecifiedType: Add type checks or assertions if necessary.- Null references are handled: Prevent potential runtime errors by verifying
childrenis not null or undefined.
Consider adding checks or handling to ensure this.type is correctly assigned.
| const builtinFn = BuiltinFunction.getFn(fnIdent, paramSig); | ||
| if (builtinFn) { | ||
| this.type = BuiltinFunction.getReturnType(builtinFn.fun, builtinFn.genType); | ||
| this.type = builtinFn.realReturnType; |
There was a problem hiding this comment.
Handle potential undefined realReturnType in function calls
In the FunctionCallGeneric class, when assigning:
this.type = builtinFn.realReturnType;Ensure that builtinFn.realReturnType is defined. If realReturnType could be undefined, this may lead to runtime errors.
Consider adding a check:
if (builtinFn.realReturnType !== undefined) {
this.type = builtinFn.realReturnType;
} else {
// Handle the undefined case appropriately
}This will prevent potential issues when realReturnType is not properly assigned.
| variable_declaration_list, | ||
| variable_declaration_statement, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure parser handles new non-terminal symbols
New non-terminals variable_declaration_list and variable_declaration_statement have been added to the NoneTerminal enum. To fully integrate these additions:
- Update parser logic: Ensure that the parser recognizes and correctly handles these new symbols in the grammar rules.
- Add corresponding productions: Modify the grammar to include the rules for how these non-terminals are derived.
- Test parsing scenarios: Create test cases to verify that variable declarations are parsed as expected, including multiple declarations in a single statement.
| if (shaderLab.errors) { | ||
| for (const error of shaderLab.errors) { | ||
| console.error(error.toString()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling mechanism.
While logging shader errors to console is helpful for debugging, consider these improvements:
- Aggregate errors and throw or return them for better error handling in tests.
- Use a proper test assertion to verify expected errors.
- Consider using a structured logging format for better parsing in CI/CD pipelines.
Apply this diff to improve error handling:
if (shaderLab.errors) {
- for (const error of shaderLab.errors) {
- console.error(error.toString());
- }
+ const errorMessages = shaderLab.errors.map(error => error.toString());
+ throw new Error(`Shader validation failed:\n${errorMessages.join('\n')}`);
}📝 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 (shaderLab.errors) { | |
| for (const error of shaderLab.errors) { | |
| console.error(error.toString()); | |
| } | |
| } | |
| if (shaderLab.errors) { | |
| const errorMessages = shaderLab.errors.map(error => error.toString()); | |
| throw new Error(`Shader validation failed:\n${errorMessages.join('\n')}`); | |
| } |
Merge code from dev/1.4
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/loader/src/SpriteAtlasLoader.ts (1)
107-108:⚠️ Potential issueFix critical logical error in width/height assignment.
The current conditions will not set the sprite dimensions as intended. The logical OR with assignment will always evaluate to true without assigning values.
Apply this fix:
- width === undefined || (sprite.width = width); - height === undefined || (sprite.height = height); + if (width !== undefined) sprite.width = width; + if (height !== undefined) sprite.height = height;🧰 Tools
🪛 Biome (1.9.4)
[error] 107-107: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 108-108: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🧹 Nitpick comments (4)
packages/math/src/Matrix.ts (1)
1069-1069: LGTM! Clean improvement using optional chaining.The change from
&&to the optional chaining operator?.is a good modernization that makes the code more idiomatic and readable while maintaining the same safe behavior.Consider applying this pattern consistently across the codebase. Run this script to find similar patterns that could be modernized:
#!/bin/bash # Find similar patterns of existence checks with immediate calls rg --type ts "(\w+)\.(\w+)\s*&&\s*\1\.\2\(\)"packages/math/src/Vector4.ts (1)
Line range hint
1-999: Overall Assessment: Improved Error Handling for GUI Math OperationsThe consistent update to use optional chaining for
_onValueChangedcallbacks across all math classes enhances the reliability of GUI-related calculations. This improvement is particularly relevant for:
- UI element positioning and layout (Vector2, Rect)
- Visual styling and rendering (Color)
- Mathematical transformations (Vector4)
The changes align well with the PR's GUI enhancement objectives while maintaining a uniform approach to error handling throughout the math module.
packages/math/src/Vector3.ts (1)
27-27: LGTM! Consider extracting the change notification to a private method.The consistent use of optional chaining for
_onValueChangedcalls improves code safety and readability. However, since this pattern is repeated multiple times, consider extracting it to a private method to reduce duplication.export class Vector3 implements IClone<Vector3>, ICopy<Vector3Like, Vector3> { + private _notifyValueChanged(): void { + this._onValueChanged?.(); + }Then use it throughout the class:
- this._onValueChanged?.(); + this._notifyValueChanged();Also applies to: 40-40, 53-53, 66-66, 146-146, 159-159, 172-172, 184-184, 211-211, 232-232, 248-248, 264-264, 289-289, 312-312, 333-333, 345-345, 357-357, 383-383, 396-396, 409-409, 422-422, 435-435, 465-465, 487-487, 560-560, 586-586
packages/math/src/Quaternion.ts (1)
27-27: LGTM! Consider the same optimization as Vector3.The consistent use of optional chaining for
_onValueChangedcalls improves code safety and readability. To maintain consistency with Vector3, consider extracting the change notification to a private method.export class Quaternion implements IClone<Quaternion>, ICopy<QuaternionLike, Quaternion> { + private _notifyValueChanged(): void { + this._onValueChanged?.(); + }Then use it throughout the class:
- this._onValueChanged?.(); + this._notifyValueChanged();Also applies to: 50-50, 63-63, 106-106, 146-146, 159-159, 172-172, 184-184, 211-211, 232-232, 248-248, 264-264, 289-289, 312-312, 333-333, 345-345, 357-357, 383-383, 396-396, 409-409, 422-422, 435-435, 465-465, 487-487, 560-560, 586-586, 729-729, 757-757
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
packages/core/src/particle/modules/ParticleGradient.ts(3 hunks)packages/loader/src/SpriteAtlasLoader.ts(1 hunks)packages/math/src/Color.ts(15 hunks)packages/math/src/Matrix.ts(1 hunks)packages/math/src/Quaternion.ts(27 hunks)packages/math/src/Rect.ts(6 hunks)packages/math/src/Vector2.ts(21 hunks)packages/math/src/Vector3.ts(26 hunks)packages/math/src/Vector4.ts(25 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/loader/src/SpriteAtlasLoader.ts
[error] 107-107: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 108-108: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x, windows-latest)
🔇 Additional comments (8)
packages/math/src/Rect.ts (1)
26-26: LGTM! Improved error handling for value change notifications.The consistent use of optional chaining (
?.) for invoking_onValueChangedacross all setters and methods improves the robustness of the rectangle calculations, which is crucial for UI layout operations.Also applies to: 38-38, 50-50, 62-62, 92-92, 114-114
packages/math/src/Color.ts (1)
66-66: LGTM! Consistent error handling for color transformations.The changes maintain a uniform approach to value change notifications across all color manipulation methods, which is essential for reliable sprite rendering and UI updates.
Also applies to: 82-82, 97-97, 115-115, 140-140, 152-152, 164-164, 176-176, 206-206, 220-220, 234-234, 257-257, 285-285, 310-310, 323-323
packages/math/src/Vector2.ts (1)
23-23: LGTM! Enhanced reliability for UI positioning calculations.The improved error handling in Vector2 operations ensures more reliable UI element positioning and transformations, which directly supports the PR's GUI enhancement objectives.
Also applies to: 35-35, 47-47, 59-59, 117-117, 129-129, 141-141, 152-152, 167-167, 180-180, 199-199, 211-211, 233-233, 245-245, 257-257, 269-269, 281-281, 310-310, 331-331, 351-351, 375-375
packages/math/src/Vector4.ts (1)
27-27: LGTM! Consistent error handling across vector operations.The changes maintain a uniform approach to value change notifications across all vector operations, ensuring consistency with other math classes.
Also applies to: 41-41, 55-55, 69-69, 138-138, 152-152, 166-166, 179-179, 196-196, 211-211, 227-227, 254-254, 277-277, 289-289, 301-301, 313-313, 343-343, 357-357, 371-371, 385-385, 399-399, 430-430, 453-453, 476-476, 504-504
packages/core/src/particle/modules/ParticleGradient.ts (4)
248-248: LGTM! Clean callback invocation using optional chaining.The change to use the optional chaining operator (
?.) improves code readability while maintaining the same functionality.
296-296: LGTM! Consistent callback handling.The optional chaining operator usage maintains consistency with the GradientColorKey implementation.
Line range hint
1-324: Verify PR objectives alignment.The changes in this file are related to particle system gradient functionality, but the PR objectives mention GUI-related changes. Consider updating the PR description to include these particle system improvements for better documentation and clarity.
308-308: LGTM! Consistent pattern application.The optional chaining operator is consistently applied across all property setters.
Let's verify if all similar callback patterns have been updated consistently across the codebase:
✅ Verification successful
Callback pattern verification successful
The optional chaining operator for
_onValueChangedcallbacks is consistently implemented across the entire codebase. No remaining conditional callback invocations were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for remaining conditional callback invocations that might need updating rg "if.*_onValueChanged.*\)" --type tsLength of output: 40
Script:
#!/bin/bash # Search for all occurrences of _onValueChanged with context rg "_onValueChanged" --type ts -B 1 -A 1Length of output: 39141
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the high-level release notes for end-users:
New Features
@galacean/engine-uiwith comprehensive rendering and interaction capabilitiesUICanvas,UIGroup,UITransform,Image,Text, andButtonImprovements
Breaking Changes
Fixes