Add RectTool
#313
Add RectTool
#313
Conversation
WalkthroughThe pull request introduces several enhancements across multiple files in the Changes
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (7)
packages/gizmo/src/icon/Icon.ts (1)
18-126: Consider translating code comments to English for better accessibilityThe code contains comments in Chinese. Translating comments to English can improve maintainability and make the codebase more accessible to a wider range of developers.
packages/gizmo/src/Rect.ts (1)
18-997: Consider translating code comments to English for better accessibilityThe code contains comments in Chinese. For better maintainability and to cater to a wider developer audience, consider translating comments to English.
🧰 Tools
🪛 Biome (1.9.4)
[error] 130-130: 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] 165-165: 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] 199-199: 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] 585-585: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
[error] 588-588: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
[error] 591-591: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
[error] 612-612: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
[error] 331-331: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
packages/gizmo/src/Group.ts (1)
185-192: Consider adding null safety checks and documentation.The
getPrimaryEntitymethod could benefit from:
- JSDoc documentation explaining the return value could be null
- Explicit type annotation for null return
/** * Get the primary Entity (first selected Entity) * @returns The first entity in the group, or null if the group is empty - */ -getPrimaryEntity(): Entity { + */ +getPrimaryEntity(): Entity | null { const { _entities: entities } = this; return entities.length > 0 ? entities[0] : null; }packages/gizmo/src/Gizmo.ts (1)
3-20: Organize imports for better maintainability.The imports are not consistently organized. Consider grouping them:
- Core engine imports
- Toolkit imports
- Local imports
import { Camera, Component, Entity, Layer, MathUtil, Matrix, MeshRenderer, Pointer, PointerButton, PointerPhase, Ray, Script, Vector2, Vector3 } from "@galacean/engine"; + import { FramebufferPicker } from "@galacean/engine-toolkit-framebuffer-picker"; + import { Group, GroupDirtyFlag } from "./Group"; import { RectControl } from "./Rect"; import { RotateControl } from "./Rotate"; import { ScaleControl } from "./Scale"; import { TranslateControl } from "./Translate"; import { GizmoComponent } from "./Type"; import { Utils } from "./Utils"; import { State } from "./enums/GizmoState";packages/gizmo/src/icon/IconMaterial.ts (3)
3-6: Enhance class documentationWhile the documentation indicates the material is not affected by light and fog, it would be helpful to add:
- Example usage
- Parameters that can be configured
- Typical use cases in the context of gizmos
41-45: Consider adding null check for shaderThe constructor assumes the "icon" shader exists. Add a safety check to prevent runtime errors.
constructor(engine: Engine) { - super(engine, Shader.find("icon")); + const shader = Shader.find("icon"); + if (!shader) { + throw new Error("Icon shader not found"); + } + super(engine, shader); this.shaderData.setColor(IconMaterial._baseColorProp, new Color(1, 1, 1, 1)); this.renderState.rasterState.cullMode = CullMode.Off; }
54-109: Consider shader optimizationsThe shader implementation is functional but consider these optimizations:
- Cache the translation.w division result
- Combine the xFactor/yFactor calculations
- Consider using
highpprecision for critical calculationsExample optimization for the vertex shader:
vec4 translation = renderer_MVPMat[3]; - translation = translation / translation.w; + float invW = 1.0 / translation.w; + translation *= invW; - float xFactor = u_size.x / u_pixelViewport.z * 2.0; - float yFactor = u_size.y / u_pixelViewport.w * 2.0; + vec2 factor = (u_size / u_pixelViewport.zw) * 2.0; - gl_Position = vec4(translation.x + xFactor * position.x, translation.y + yFactor * position.y, translation.z, 1); + gl_Position = vec4(translation.xy + factor * position.xy, translation.z, 1);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
packages/gizmo/src/Gizmo.ts(2 hunks)packages/gizmo/src/Group.ts(6 hunks)packages/gizmo/src/Rect.ts(1 hunks)packages/gizmo/src/Utils.ts(3 hunks)packages/gizmo/src/enums/GizmoState.ts(1 hunks)packages/gizmo/src/icon/Icon.ts(1 hunks)packages/gizmo/src/icon/IconMaterial.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/gizmo/src/Utils.ts
[error] 86-86: 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] 87-87: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
packages/gizmo/src/Rect.ts
[error] 130-130: 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] 165-165: 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] 199-199: 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] 585-585: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
[error] 588-588: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
[error] 591-591: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
[error] 612-612: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
[error] 331-331: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
🔇 Additional comments (5)
packages/gizmo/src/Group.ts (2)
45-47: LGTM! Improved encapsulation with getter method.
The new entities getter provides controlled access to the private _entities array, following good encapsulation practices.
345-345: LGTM! Performance optimization in _getCenter.
Good optimization by initializing the renderers array outside the loop to prevent repeated allocations.
Also applies to: 348-348
packages/gizmo/src/Gizmo.ts (1)
145-145: Verify RectControl initialization and state handling.
The RectControl is added but we should verify:
- The
State.rectenum value exists - The control is properly handled in state changes
✅ Verification successful
Let me gather more information about the RectControl initialization and state handling.
Let me check one more aspect to verify the State enum definition and rect state handling.
RectControl initialization and state handling are properly implemented
The verification confirms:
State.rectis properly defined in the GizmoState enum with value0x8- The control is correctly initialized using
_createGizmoControlalongside other controls - RectControl implementation exists in
packages/gizmo/src/Rect.tsand properly uses the rect state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify State enum includes rect state and its handling
# Check if State enum includes rect
rg -A 10 "enum State"
# Check state handling for rect control
rg "State\.rect"
Length of output: 1174
Script:
#!/bin/bash
# Check RectControl class implementation and state handling
ast-grep --pattern 'class RectControl {
$$$
}'
# Check state handling in control creation and updates
rg "_createGizmoControl" -A 5
rg "updateControl.*State\.rect" -A 5
Length of output: 1159
Script:
#!/bin/bash
# Check complete State enum definition and rect state
cat packages/gizmo/src/enums/GizmoState.ts
# Check RectControl implementation
rg -l "class RectControl"
Length of output: 380
packages/gizmo/src/icon/IconMaterial.ts (2)
47-51: LGTM!
The clone implementation correctly creates a new instance and copies properties.
54-56: Verify shader name uniqueness
Ensure the "icon" shader name doesn't conflict with existing shaders in the codebase.
✅ Verification successful
The previous search might have been too strict. Let's broaden it to catch potential variations in shader definitions and also look for related files that might contain shader code.
The files found seem to be related to the same icon component. Let's examine their content to verify if there's any shader name conflict or if they're part of the same implementation.
No shader name conflicts found
After examining the codebase, the "icon" shader is only defined once in IconMaterial.ts and is properly scoped to the Icon component implementation. The Icon.ts file only consumes this shader through the IconMaterial class, and there are no other conflicting shader definitions with the same name.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other shader definitions using the name "icon"
rg -l "Shader\.create\(\s*[\"']icon[\"']" --type ts
Length of output: 57
Script:
#!/bin/bash
# Let's try a broader search for shader definitions to ensure we catch all variations
rg "Shader\.create.*icon" --type ts -A 2
# Also search for any shader-related files or references to icon shaders
fd -e ts -e glsl . | xargs rg -l "icon.*shader|shader.*icon" -i
Length of output: 180
Script:
#!/bin/bash
# Let's examine the content of both files to understand the shader usage
echo "=== Content of Icon.ts ==="
cat packages/gizmo/src/icon/Icon.ts
echo -e "\n=== Content of IconMaterial.ts ==="
cat packages/gizmo/src/icon/IconMaterial.ts
Length of output: 6426
| } | ||
| } | ||
| } else { | ||
| for (let i = 0, n = entities.length; i < n; i++) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary continue statements to simplify code flow
The continue statements at lines 585, 588, 591, and 612 are unnecessary and can be removed without affecting the logic. Eliminating them will simplify the code flow.
Apply this diff to remove the unnecessary continue statements:
585,588,591,612
- if (!entity.isActiveInHierarchy) continue;
+ if (!entity.isActiveInHierarchy) {
+ // Entity is not active; skip processing.
+ }Also applies to: 588-588, 591-591, 612-612
🧰 Tools
🪛 Biome (1.9.4)
[error] 585-585: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
| all = 0x7, | ||
| /** | ||
| * rect | ||
| */ | ||
| rect = 0x8 |
There was a problem hiding this comment.
Update State.all to include the new rect state
The State.all value currently does not include the newly added rect state. To ensure that State.all represents all possible gizmo states, update it to include rect.
Apply this diff to include the rect state in State.all:
- all = 0x7,
+ all = 0xF,This change combines all state flags (translate = 0x1, rotate = 0x2, scale = 0x4, rect = 0x8) so that State.all correctly represents all possible states.
📝 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.
| all = 0x7, | |
| /** | |
| * rect | |
| */ | |
| rect = 0x8 | |
| all = 0xF, | |
| /** | |
| * rect | |
| */ | |
| rect = 0x8 |
| Utils.visibleMaterialRect = this._createPlainColorMaterial(engine, State.rect, 163 / 255, 171 / 255, 180 / 255, 1); | ||
| Utils.invisibleMaterialRect = this._createPlainColorMaterial(engine, State.rect, 0, 0, 0, 0); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace this with Utils in static context for clarity
Using this in a static context can be confusing since it refers to the class Utils. It's clearer to use the class name explicitly.
Apply this diff to replace this with Utils:
- Utils.visibleMaterialRect = this._createPlainColorMaterial(engine, State.rect, 163 / 255, 171 / 255, 180 / 255, 1);
- Utils.invisibleMaterialRect = this._createPlainColorMaterial(engine, State.rect, 0, 0, 0, 0);
+ Utils.visibleMaterialRect = Utils._createPlainColorMaterial(engine, State.rect, 163 / 255, 171 / 255, 180 / 255, 1);
+ Utils.invisibleMaterialRect = Utils._createPlainColorMaterial(engine, State.rect, 0, 0, 0, 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.
| Utils.visibleMaterialRect = this._createPlainColorMaterial(engine, State.rect, 163 / 255, 171 / 255, 180 / 255, 1); | |
| Utils.invisibleMaterialRect = this._createPlainColorMaterial(engine, State.rect, 0, 0, 0, 0); | |
| Utils.visibleMaterialRect = Utils._createPlainColorMaterial(engine, State.rect, 163 / 255, 171 / 255, 180 / 255, 1); | |
| Utils.invisibleMaterialRect = Utils._createPlainColorMaterial(engine, State.rect, 0, 0, 0, 0); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 86-86: 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] 87-87: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
| // align to the primary entity | ||
| const primaryEntity = this.getPrimaryEntity(); | ||
| const worldE = primaryEntity.transform.worldMatrix.elements; |
There was a problem hiding this comment.
Verify primary entity existence before access.
Both _updateAnchor and _updateCoordinate methods access the primary entity's properties without checking if it exists, which could lead to null pointer exceptions.
-const primaryEntity = this.getPrimaryEntity();
-const worldE = primaryEntity.transform.worldMatrix.elements;
+const primaryEntity = this.getPrimaryEntity();
+if (!primaryEntity) {
+ return;
+}
+const worldE = primaryEntity.transform.worldMatrix.elements;Also applies to: 319-321
| get baseTexture(): Texture2D { | ||
| return <Texture2D>this.shaderData.getTexture(IconMaterial._baseTextureProp); | ||
| } | ||
|
|
||
| set baseTexture(value: Texture2D) { | ||
| this.shaderData.setTexture(IconMaterial._baseTextureProp, value); | ||
| if (value) { | ||
| this.shaderData.enableMacro(IconMaterial._baseTextureMacro); | ||
| } else { | ||
| this.shaderData.disableMacro(IconMaterial._baseTextureMacro); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add missing static properties for texture handling
The implementation references undefined static properties for texture and macro.
Add the following declarations at class level:
export class IconMaterial extends BaseMaterial {
+ private static _baseTextureProp = Shader.getPropertyByName("material_BaseTexture");
+ private static _baseTextureMacro = "MATERIAL_HAS_BASETEXTURE";Committable suggestion skipped: line range outside the PR's diff.
| get baseColor(): Color { | ||
| return this.shaderData.getColor(IconMaterial._baseColorProp); | ||
| } | ||
|
|
||
| set baseColor(value: Color) { | ||
| const baseColor = this.shaderData.getColor(IconMaterial._baseColorProp); | ||
| if (value !== baseColor) { | ||
| baseColor.copyFrom(value); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add missing static property declaration
The implementation references IconMaterial._baseColorProp but the static property declaration is missing.
Add the following declaration at class level:
export class IconMaterial extends BaseMaterial {
+ private static _baseColorProp = Shader.getPropertyByName("material_BaseColor");Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
packages/gizmo/src/icon/IconMaterial.ts (2)
10-19:⚠️ Potential issueAdd missing static property declaration
The implementation references
IconMaterial._baseColorPropbut the static property declaration is missing.
24-35:⚠️ Potential issueAdd missing static properties for texture handling
The implementation references undefined static properties for texture and macro.
🧹 Nitpick comments (8)
packages/gizmo/src/icon/IconMaterial.ts (2)
3-5: Enhance class documentation.The current documentation could be more descriptive. Consider adding:
- Usage examples
- Parameters that can be customized
- Typical use cases
54-105: Consider shader improvements for robustness.The shader implementation could benefit from several improvements:
- Add gamma correction handling for consistent color representation across different displays
- Add highp precision qualifier for viewport calculations to prevent floating-point precision issues
- Add texture coordinate validation to prevent artifacts from invalid UVs
Example improvements:
Shader.create( "icon", ` #include <common> +precision highp float; #include <common_vert> #include <uv_share> #include <blendShape_input> uniform vec2 u_size; uniform vec4 u_pixelViewport; `, ` #include <common> +precision highp float; #include <uv_share> uniform vec4 material_BaseColor; #ifdef MATERIAL_HAS_BASETEXTURE uniform sampler2D material_BaseTexture; #endif void main() { vec4 baseColor = material_BaseColor; #ifdef MATERIAL_HAS_BASETEXTURE + // Validate texture coordinates + if(v_uv.x < 0.0 || v_uv.x > 1.0 || v_uv.y < 0.0 || v_uv.y > 1.0) { + discard; + } vec4 textureColor = texture2D(material_BaseTexture, v_uv); baseColor *= textureColor; #endif #ifdef MATERIAL_IS_ALPHA_CUTOFF if( baseColor.a < material_AlphaCutoff ) { discard; } #endif + // Apply gamma correction + baseColor.rgb = pow(baseColor.rgb, vec3(1.0/2.2)); gl_FragColor = baseColor; } ` );packages/tween/package.json (1)
26-26: Consider version flexibility for peer dependencies.While standardizing on 1.4.0 is good for internal consistency, using an exact version for a peer dependency might cause installation conflicts in user projects. Consider using a version range (e.g.,
">=1.4.0 <2.0.0") to allow more flexibility while maintaining compatibility.packages/outline/package.json (1)
26-26: Consider a coordinated peer dependency strategy.The standardization of engine version across packages is good for internal consistency. However, since multiple packages (
stats,outline, and others) share this peer dependency, consider:
- Documenting this version requirement in the monorepo's root README
- Using consistent version range patterns across all packages
- Providing migration guides for users updating from beta versions
Also applies to: 26-26
packages/gizmo/src/Rect.ts (1)
167-170: Avoid assignments within expressions for better readability.Using assignments within expressions can make the code harder to read and maintain. Consider separating assignments to improve clarity.
Apply this diff to refactor the code:
- const entityXoY = (this._XoY = this._middleEntity.createChild("XoY")); + this._XoY = this._middleEntity.createChild("XoY"); + const entityXoY = this._XoY; - const entityXoZ = (this._XoZ = this._middleEntity.createChild("XoZ")); + this._XoZ = this._middleEntity.createChild("XoZ"); + const entityXoZ = this._XoZ; - const entityYoZ = (this._YoZ = this._middleEntity.createChild("YoZ")); + this._YoZ = this._middleEntity.createChild("YoZ"); + const entityYoZ = this._YoZ;Also applies to: 203-206, 238-241
🧰 Tools
🪛 Biome (1.9.4)
[error] 170-170: 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/auxiliary-lines/src/WireframePrimitive.ts (1)
759-795: Consider adding input validation.The method correctly implements rectangle wireframe creation with pivot support, but lacks input validation.
Consider adding:
static createRectWireframe( width: number, height: number, pivotX: number, pivotY: number, positions: Vector3[], positionOffset: number, indices: Uint16Array | Uint32Array, indicesOffset: number ): void { + if (width <= 0 || height <= 0) { + throw new Error("Width and height must be positive"); + } + if (pivotX < 0 || pivotX > 1 || pivotY < 0 || pivotY > 1) { + throw new Error("Pivot values must be between 0 and 1"); + } // Rest of the implementation...packages/gizmo/src/Group.ts (1)
416-423: Optimize renderer array reuse.The
renderersarray is reused in the loop but never cleared between iterations. This could lead to memory issues with a large number of entities.Apply this diff to clear the array in each iteration:
const renderers = []; for (let i = entities.length - 1; i >= 0; i--) { const entity = entities[i]; + renderers.length = 0; if (this._searchComponentType === SearchComponentType.CurrentEntity) { entity.getComponents(Renderer, renderers); } else { entity.getComponentsIncludeChildren(Renderer, renderers); }packages/skeleton-viewer/package.json (1)
26-26: Consider using a version range for smoother updates.The strict version requirement
"1.4.0"might make future updates more challenging. Consider using"~1.4.0"to allow patch updates or keeping the original"^1.4.0"to allow minor updates while ensuring breaking changes are avoided.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
package.json(1 hunks)packages/auxiliary-lines/package.json(1 hunks)packages/auxiliary-lines/src/WireframeManager.ts(2 hunks)packages/auxiliary-lines/src/WireframePrimitive.ts(2 hunks)packages/controls/package.json(1 hunks)packages/custom-gltf-parser/package.json(1 hunks)packages/custom-material/package.json(1 hunks)packages/draco/package.json(1 hunks)packages/dynamic-bone/package.json(1 hunks)packages/framebuffer-picker/package.json(1 hunks)packages/geometry-sketch/package.json(1 hunks)packages/gizmo/package.json(1 hunks)packages/gizmo/src/Gizmo.ts(4 hunks)packages/gizmo/src/Group.ts(9 hunks)packages/gizmo/src/Rect.ts(1 hunks)packages/gizmo/src/enums/GroupState.ts(1 hunks)packages/gizmo/src/icon/Icon.ts(1 hunks)packages/gizmo/src/icon/IconMaterial.ts(1 hunks)packages/input-logger/package.json(1 hunks)packages/lines/package.json(1 hunks)packages/navigation-gizmo/package.json(1 hunks)packages/outline/package.json(1 hunks)packages/skeleton-viewer/package.json(1 hunks)packages/stats/package.json(1 hunks)packages/tween/package.json(1 hunks)packages/way-point/package.json(1 hunks)packages/xr/package.json(1 hunks)tests/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/gizmo/src/icon/Icon.ts
🧰 Additional context used
🪛 eslint
packages/gizmo/src/Group.ts
[error] 241-243: Delete ⏎····⏎··
(prettier/prettier)
packages/auxiliary-lines/src/WireframePrimitive.ts
[error] 19-20: Delete ⏎
(prettier/prettier)
🪛 Biome (1.9.4)
packages/gizmo/src/Rect.ts
[error] 170-170: 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] 206-206: 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] 241-241: 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] 905-905: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
🔇 Additional comments (30)
packages/gizmo/src/icon/IconMaterial.ts (1)
47-51: Consider memory management in clone method.The
cloneTomethod might need to handle cleanup of existing resources in the destination material before copying.Run the following to check if
BaseMaterial.cloneTohandles resource cleanup:packages/draco/package.json (1)
23-23: Verify compatibility with the stable engine version.The change from
^1.4.0-beta.2to1.4.0is good for stability, but please ensure:
- All features used from the engine are compatible with 1.4.0
- The package has been tested against this exact version
Run this script to check for any breaking changes between beta.2 and stable:
✅ Verification successful
Version update to 1.4.0 appears safe to proceed
The codebase only uses stable core engine features and standard APIs, with no dependency on beta-specific functionality. The removal of the caret (^) also improves build reproducibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for breaking changes in the changelog between versions gh api repos/galacean/engine/releases | jq -r '.[] | select(.tag_name | contains("1.4.0")) | .body' | grep -i "breaking"Length of output: 120
Script:
#!/bin/bash # Find all imports from @galacean/engine rg --type js --type ts "@galacean/engine" -B 1 -A 1 # Find all files that might use the engine fd -e ts -e js --exec grep -l "@galacean/engine" {}Length of output: 23493
Script:
#!/bin/bash # Check for any version-specific code or version checks rg -i "version|1\.4\.0" --type ts --type js -g '!package*.json' -g '!CHANGELOG.md'Length of output: 433
tests/package.json (1)
17-20: Ensure test environment consistency.Good to see the test package aligning with the same engine version. The combination of fixed engine version and workspace dependencies will help maintain a consistent testing environment.
packages/controls/package.json (1)
27-27: Verify the impact of strict versioning.The change from
"^1.4.0-beta.2"to"1.4.0"across multiple packages indicates:
- An upgrade from beta to stable version
- A switch from flexible (^) to strict versioning
While this standardization is good for consistency, strict versioning might:
- Make it harder for consumers to resolve dependencies if they use a different version
- Increase maintenance overhead as every minor/patch update will require explicit version bumps
Consider:
- Whether strict versioning aligns with the project's dependency management strategy
- If there are breaking changes between beta.2 and stable that justify this constraint
- Adding a comment in the PR description explaining this significant change
Run this script to assess the impact:
✅ Verification successful
Strict versioning aligns with monorepo dependency strategy
The change to strict versioning for
@galacean/engineis appropriate as it:
- Ensures consistent engine version across all toolkit packages
- Follows the project's pattern of strict external dependencies while using workspace references for internal packages
- Provides better stability by preventing unintended version mismatches
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential dependency conflicts and breaking changes # List all package.json files echo "Checking package.json files..." fd -e json -x cat {} \; | jq -c 'select(.peerDependencies."@galacean/engine" != null) | {name: .name, version: .peerDependencies."@galacean/engine"}' # Check git history for breaking changes echo -e "\nChecking for breaking changes..." git log --oneline "@galacean/[email protected]"..HEAD | grep -i "break"Length of output: 560
Script:
#!/bin/bash # Check for documentation about versioning strategy echo "Checking for versioning documentation..." rg -i "version|dependency" -A 2 -B 2 "README.md" "CONTRIBUTING.md" "DEVELOPMENT.md" || true echo -e "\nChecking version patterns in package.json files..." fd package.json -x jq -r '.dependencies | to_entries | .[] | select(.key | startswith("@galacean")) | "\(.key): \(.value)"' {} \;Length of output: 2721
packages/auxiliary-lines/package.json (1)
27-27: Ensure version alignment with package version.The package is at version "1.4.0-beta.1" but requires exactly "@galacean/[email protected]". Consider aligning these versions to maintain consistency.
packages/navigation-gizmo/package.json (1)
27-27: Version mismatch with package version.packages/gizmo/package.json (1)
27-28: Verify UI dependency integration and version alignment.
- New dependency on "@galacean/engine-ui" suggests UI-related changes. This aligns with the addition of
RectTool.- Both engine dependencies are locked to "1.4.0" while the package is at "1.4.0-beta.1".
Run this script to check UI dependency compatibility:
✅ Verification successful
Dependencies are correctly aligned
The @galacean/[email protected] package requires @galacean/[email protected] as a peer dependency, which matches exactly with the versions specified in package.json.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if engine-ui is compatible with current engine version echo "Checking @galacean/engine-ui compatibility..." curl -s https://registry.npmjs.org/@galacean%2Fengine-ui/1.4.0 | jq -r '.peerDependencies["@galacean/engine"]'Length of output: 219
package.json (1)
77-77: Review impact of global engine version override.Using pnpm overrides to set "@galacean/engine" to exactly "1.4.0" ensures consistency across all packages but:
- This forces all packages to use 1.4.0 regardless of their individual compatibility requirements
- The override might conflict with the beta status of the packages themselves
Run this script to check for potential conflicts:
✅ Verification successful
Engine version override is correctly aligned with package requirements
All toolkit packages (currently at 1.4.0-beta.1) explicitly require @galacean/[email protected], which matches exactly with the override. This ensures consistent and compatible engine version across the monorepo.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if any package has specific version requirements that might conflict echo "Checking for potential version conflicts..." fd -e json -x jq -r 'select(.peerDependencies != null) | select(.peerDependencies["@galacean/engine"] != null) | "\(.name): \(.version) requires @galacean/engine@\(.peerDependencies["@galacean/engine"])"' {} \;Length of output: 1777
packages/gizmo/src/Gizmo.ts (3)
108-109: Properly adjustingsearchComponentTypebased on gizmo state.The implementation correctly sets
_group.searchComponentTypetoCurrentEntitywhen the state isState.rect, ensuring that the gizmo operates on the intended components.
147-147: Addition ofRectControlimproves functionality.Adding
RectControlto the gizmo controls expands the gizmo's capabilities to include rectangular manipulations.
171-176: Optimizing control updates with_gizmoTransformDirtycheck.The introduction of the
_gizmoTransformDirtyflag in theonUpdatemethod ensures that controls are updated only when necessary, improving performance.packages/gizmo/src/enums/GroupState.ts (1)
29-32: Addition ofSearchComponentTypeenumeration enhances flexibility.Introducing the
SearchComponentTypeenum provides better control over component searches inGroup, improving code clarity and functionality.packages/auxiliary-lines/src/WireframePrimitive.ts (2)
12-17: LGTM!The index count is correct for a rectangle wireframe (4 edges × 2 vertices = 8 indices).
20-25: LGTM!The position count is correct for a rectangle (4 corners).
packages/auxiliary-lines/src/WireframeManager.ts (1)
589-607: LGTM!The implementation correctly creates auxiliary mesh for rectangle shapes, following the same pattern as other shape methods in the class.
The method:
- Correctly manages indices and positions
- Uses the new WireframePrimitive.createRectWireframe
- Properly handles transform and wireframe elements
packages/stats/package.json (1)
26-26: Verify the compatibility with @galacean/engine 1.4.0The change from
^1.4.0-beta.2to1.4.0has two implications:
- Upgrading from beta.2 to the final 1.4.0 release
- Removing the caret (^) to enforce an exact version match
Let's verify the compatibility and potential breaking changes:
Consider the following architectural implications:
- Removing the caret (^) means packages won't automatically get minor version updates
- All toolkit packages should use the same engine version to prevent compatibility issues
- The package's own version (1.4.0-beta.1) should be updated to match the engine version
✅ Verification successful
Version change is safe and follows good practices
The change from
^1.4.0-beta.2to1.4.0is safe because:
- The final 1.4.0 release exists in npm registry
- Other packages in the monorepo are already using 1.4.0
- No breaking changes detected between beta.2 and 1.4.0
- Removing the caret (^) helps maintain consistent versions across the toolkit packages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for breaking changes between beta.2 and 1.4.0 # Check if 1.4.0 exists echo "Verifying @galacean/engine 1.4.0 exists..." curl -s https://registry.npmjs.org/@galacean%2Fengine/1.4.0 | jq '.version' # Check the package's current version echo "Current package version:" jq '.version' package.json # Look for potential breaking changes in git history echo "Checking for breaking changes in commit messages..." git log --grep="BREAKING CHANGE" --grep="breaking change" --grep="BREAKING CHANGES" --grep="breaking changes" -iLength of output: 392
Script:
#!/bin/bash # Find all package.json files and check for @galacean/engine dependency echo "Checking @galacean/engine versions across packages:" fd package.json --exec sh -c 'echo "=== $1 ==="; jq -r ".dependencies.\\"@galacean/engine\\" // empty" "$1"' sh {} # Find all imports of @galacean/engine echo -e "\nChecking @galacean/engine imports:" rg "from ['\"]\@galacean/engine['\"]" -A 2Length of output: 5959
Script:
#!/bin/bash # Check package versions with proper escaping echo "Checking @galacean/engine versions across packages:" fd package.json --exec sh -c 'echo "=== $1 ==="; cat "$1" | jq -r ".dependencies[\"@galacean/engine\"] // empty"' sh {} # Check imports with simpler patterns echo -e "\nChecking @galacean/engine imports:" rg "@galacean/engine" -t js -t ts -A 2Length of output: 25590
packages/lines/package.json (1)
27-27: Consider the implications of strict version pinning.The change from
^1.4.0-beta.2to1.4.0for@galacean/enginehas two significant implications:
- It removes flexibility in minor version updates that might include compatible features
- It ensures consistent behavior by preventing automatic updates to newer versions
While this change provides better stability and predictability, it may:
- Make it harder for users to adopt bug fixes or compatible features
- Increase maintenance overhead as each minor engine update will require explicit version bumps
Let's verify the compatibility requirements with other packages:
✅ Verification successful
Version pinning is aligned with monorepo strategy
The strict version pinning to
1.4.0is consistent with the entire monorepo's version management strategy, where all packages uniformly depend on the same exact version of@galacean/engine. This coordinated approach suggests intentional version alignment rather than a limitation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if other dependencies in the monorepo have conflicting engine version requirements fd -e package.json -x jq -r 'select(.peerDependencies."@galacean/engine" != null) | "\(.name): \(.peerDependencies."@galacean/engine")"'Length of output: 138
Script:
#!/bin/bash # Search for @galacean/engine in all dependency types across package.json files fd -e package.json -x jq -r ' [ {key: "dependencies", deps: (.dependencies // {})}, {key: "devDependencies", deps: (.devDependencies // {})}, {key: "peerDependencies", deps: (.peerDependencies // {})} ][] | select(.deps."@galacean/engine" != null) | "\(.key) in \(input_filename): @galacean/engine \(.deps."@galacean/engine")" 'Length of output: 351
Script:
#!/bin/bash # List all package.json files echo "=== Found package.json files ===" fd package.json # Show content of lines package.json echo -e "\n=== Content of packages/lines/package.json ===" cat packages/lines/package.jsonLength of output: 1620
Script:
#!/bin/bash # Check @galacean/engine dependencies across all packages fd -e package.json -x sh -c ' echo "=== $(dirname {}) ===" && jq -r " if .peerDependencies?.[\"@galacean/engine\"] then \"peerDependencies: \" + .peerDependencies.[\"@galacean/engine\"] elif .dependencies?.[\"@galacean/engine\"] then \"dependencies: \" + .dependencies.[\"@galacean/engine\"] elif .devDependencies?.[\"@galacean/engine\"] then \"devDependencies: \" + .devDependencies.[\"@galacean/engine\"] else empty end " {} 'Length of output: 481
Script:
#!/bin/bash # Simple grep to find any @galacean/engine version patterns echo "=== Searching for @galacean/engine in all package.json files ===" rg -A 1 "@galacean/engine" --type jsonLength of output: 7590
packages/gizmo/src/Group.ts (2)
207-210: Verify primary entity existence before access.The method correctly returns null when no entities exist, but its usage in
_updateAnchorand_updateCoordinatemethods could lead to null pointer exceptions.
221-221: LGTM! Good encapsulation.Using the
entitiesgetter instead of direct property access improves encapsulation.packages/skeleton-viewer/package.json (1)
26-26: Verify version compatibility across packages.The peer dependency update from
"^1.4.0-beta.2"to"1.4.0"is more restrictive and could cause compatibility issues. Consider the following:
- All packages still have version
"1.4.0-beta.1"but depend on the stable"1.4.0"of@galacean/engine- Removing the caret (^) means no minor version updates will be allowed
Run this script to check version compatibility:
packages/framebuffer-picker/package.json (1)
26-26: Same changes as other package.json files.packages/custom-gltf-parser/package.json (1)
26-26: Same changes as other package.json files.packages/way-point/package.json (1)
27-27: Same changes as other package.json files.packages/dynamic-bone/package.json (1)
27-27: Same changes as other package.json files.packages/custom-material/package.json (2)
Line range hint
2-2: Version mismatch between package and dependency.The package version is still
1.4.0-beta.1while depending on the stable engine version1.4.0. This could lead to confusion about the stability of this package.Consider either:
- Updating this package to a stable version
- Or keeping the engine dependency in beta to match the package version
27-27: Consider the implications of strict version dependency.While updating from beta to stable version is good, using an exact version (
1.4.0) instead of a caret version (^1.4.0) means:
- No minor version updates will be automatically included
- Bug fixes in patch versions won't be automatically included
- All packages must be manually updated for any engine updates
Consider using
^1.4.0to allow for compatible updates, unless there's a specific reason for the strict versioning.Let's verify the version compatibility:
packages/geometry-sketch/package.json (2)
27-27: Duplicate: Consider the implications of strict version dependency.
Line range hint
2-2: Duplicate: Version mismatch between package and dependency.packages/input-logger/package.json (2)
27-27: Duplicate: Consider the implications of strict version dependency.
Line range hint
2-2: Duplicate: Version mismatch between package and dependency.
packages/xr/package.json
Outdated
| "@galacean/engine": "1.4.0", | ||
| "@galacean/engine-xr": "^1.3.0-beta.6" |
There was a problem hiding this comment.
💡 Codebase verification
Update @galacean/engine-xr to stable version 1.4.1
The current setup using engine-xr@^1.3.0-beta.6 is incompatible with [email protected]. The beta version expects [email protected], which can cause runtime issues. Since a stable version (1.4.1) is now available, you should upgrade to it for better compatibility.
- Replace
"@galacean/engine-xr": "^1.3.0-beta.6"with"@galacean/engine-xr": "1.4.1"
🔗 Analysis chain
Review version compatibility across dependencies.
There are potential version compatibility concerns:
- This package is at version "1.4.0-beta.1" but requires exactly "@galacean/[email protected]"
- "@galacean/engine-xr" uses caret versioning ("^1.3.0-beta.6") while "@galacean/engine" uses exact versioning
Run this script to check version compatibility:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check version compatibility between engine and engine-xr
echo "Checking @galacean/engine version compatibility..."
curl -s https://registry.npmjs.org/@galacean%2Fengine-xr | jq -r '.versions | keys[]' | grep "1.3.0-beta" | while read -r version; do
echo "Checking engine-xr@$version dependencies..."
curl -s "https://registry.npmjs.org/@galacean%2Fengine-xr/$version" | jq -r '.peerDependencies["@galacean/engine"]'
done
Length of output: 1518
Script:
#!/bin/bash
# Check for newer versions of engine-xr that might be compatible with [email protected]
echo "Checking latest @galacean/engine-xr versions..."
curl -s https://registry.npmjs.org/@galacean%2Fengine-xr | jq -r '.versions | keys[]' | sort -V | tail -n 5
Length of output: 273
packages/gizmo/src/Group.ts
Outdated
| applyScale(from: Vector3, to: Vector3, towards: Vector3): void { | ||
|
|
||
| } |
There was a problem hiding this comment.
Implement the empty method.
The applyScale method is empty. Either implement it or remove it if not needed.
🧰 Tools
🪛 eslint
[error] 241-243: Delete ⏎····⏎··
(prettier/prettier)
packages/gizmo/src/Group.ts
Outdated
| applyScaleOrSize(from: Matrix, to: Matrix, refPivot: Vector3): void { | ||
| const { _entities: entities } = this; | ||
| if (entities.length <= 0) { | ||
| return; | ||
| } | ||
| if (Matrix.equals(from, to)) { | ||
| return; | ||
| } | ||
| // old worldMatrix. | ||
| const { _tempMat0: groupWorldInvMat, _tempMat1: nodeMat } = Group; | ||
| Matrix.invert(from, groupWorldInvMat); | ||
|
|
||
| // 给 Transform 用的 | ||
|
|
||
| // 给 UITransform 用的 | ||
| const fromElement = from.elements; | ||
| const toElement = to.elements; | ||
| const fromX = fromElement[0] + fromElement[1] + fromElement[2]; | ||
| const fromY = fromElement[4] + fromElement[5] + fromElement[6]; | ||
| const toX = toElement[0] + toElement[1] + toElement[2]; | ||
| const toY = toElement[4] + toElement[5] + toElement[6]; | ||
|
|
||
| // update entities worldMatrix | ||
| for (let i = entities.length - 1; i >= 0; i--) { | ||
| const nodeTrans = entities[i].transform; | ||
| if (nodeTrans instanceof UITransform) { | ||
| const { size, pivot, worldPosition } = <UITransform>nodeTrans; | ||
| size.set((size.x * toX) / fromX, (size.y * toY) / fromY); | ||
| } else if (nodeTrans instanceof Transform) { | ||
| // get entity's localMatrix. | ||
| Matrix.multiply(groupWorldInvMat, nodeTrans.worldMatrix, nodeMat); | ||
| // update entity's worldMatrix. | ||
| Matrix.multiply(to, nodeMat, nodeMat); | ||
| nodeTrans.worldMatrix = nodeMat; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation for complex matrix operations.
The method handles both Transform and UITransform with complex matrix calculations, but lacks documentation explaining:
- The purpose of the matrix calculations
- Why different handling is needed for different transform types
- The meaning of the scale factors calculation
| applyPivot(from: Vector3, to: Vector3): void { | ||
| const { _entities: entities } = this; | ||
| if (entities.length <= 0) { | ||
| return; | ||
| } | ||
| if (Vector3.equals(from, to)) { | ||
| return; | ||
| } | ||
| // update entities pivot | ||
| } | ||
|
|
There was a problem hiding this comment.
Complete the implementation.
The applyPivot method checks for validity but lacks implementation.
| set searchComponentType(value: SearchComponentType) { | ||
| if (this._searchComponentType !== value) { | ||
| this._searchComponentType = value; | ||
| this._dirtyFlag & GroupDirtyFlag.AnchorDirty; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix incorrect dirty flag operation in setter.
The dirty flag operation is incorrect. The current code only reads the flag without setting it.
Apply this diff to fix the dirty flag operation:
set searchComponentType(value: SearchComponentType) {
if (this._searchComponentType !== value) {
this._searchComponentType = value;
- this._dirtyFlag & GroupDirtyFlag.AnchorDirty;
+ this._dirtyFlag |= GroupDirtyFlag.AnchorDirty;
}
}📝 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 searchComponentType(value: SearchComponentType) { | |
| if (this._searchComponentType !== value) { | |
| this._searchComponentType = value; | |
| this._dirtyFlag & GroupDirtyFlag.AnchorDirty; | |
| } | |
| } | |
| set searchComponentType(value: SearchComponentType) { | |
| if (this._searchComponentType !== value) { | |
| this._searchComponentType = value; | |
| this._dirtyFlag |= GroupDirtyFlag.AnchorDirty; | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/auxiliary-lines/src/WireframePrimitive.ts (1)
759-796: Consider adding bounds validation for width and height parameters.The implementation looks good, but it would be safer to validate the input parameters to ensure positive width and height values.
static createRectWireframe( width: number, height: number, pivotX: number, pivotY: number, positions: Vector3[], positionOffset: number, indices: Uint16Array | Uint32Array, indicesOffset: number ): void { + if (width <= 0 || height <= 0) { + throw new Error("Width and height must be positive numbers"); + } + // 0 -- 3 // | | // 1 -- 2packages/gizmo/src/icon/Icon.ts (5)
17-19: Convert documentation to English for consistency.The class documentation is currently in Chinese. Consider translating it to English:
-/** - * Viewport Icon 只能被一个 Viewport Camera 观测 - */ +/** + * Viewport Icon that can only be observed by a single Viewport Camera + */
41-47: Extract color constants for better maintainability.The setGray method uses hardcoded color values. Consider extracting these as class constants:
+private static readonly GRAY_COLOR = new Color(0.75, 0.75, 0.75, 1); +private static readonly DEFAULT_COLOR = new Color(0, 157/255, 1, 1); setGray(value: boolean) { if (value) { - this._material.baseColor.set(0.75, 0.75, 0.75, 1); + this._material.baseColor.copyFrom(Icon.GRAY_COLOR); } else { - this._material.baseColor.set(0, 157/255, 1, 1); + this._material.baseColor.copyFrom(Icon.DEFAULT_COLOR); } }
91-92: Consider using more appropriate bounds values.Using
Number.MAX_SAFE_INTEGERfor bounds might cause floating-point precision issues. Consider using more appropriate values based on the expected usage range:-mesh.bounds.min.set(-Number.MAX_SAFE_INTEGER, -Number.MAX_SAFE_INTEGER, -Number.MAX_SAFE_INTEGER); -mesh.bounds.max.set(Number.MAX_SAFE_INTEGER, Number.MAX_SAFE_INTEGER, Number.MAX_SAFE_INTEGER); +const BOUND_LIMIT = 1e6; // Adjust based on your needs +mesh.bounds.min.set(-BOUND_LIMIT, -BOUND_LIMIT, -BOUND_LIMIT); +mesh.bounds.max.set(BOUND_LIMIT, BOUND_LIMIT, BOUND_LIMIT);
96-96: Document the significance of priority value.The renderer priority is set to 1 without explanation. Consider adding a comment explaining why this specific value is chosen:
-meshRenderer.priority = 1; +// Set priority to 1 to ensure icons render after/above regular scene geometry +meshRenderer.priority = 1;
133-136: Add documentation for IconScaleMode enum.Consider adding JSDoc comments to explain the purpose of each scale mode:
export enum IconScaleMode { + /** Icon size is interpreted in world units */ World, + /** Icon size is interpreted in screen pixels */ Screen }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/auxiliary-lines/src/WireframePrimitive.ts(2 hunks)packages/gizmo/src/icon/Icon.ts(1 hunks)packages/gizmo/src/icon/IconMaterial.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/gizmo/src/icon/IconMaterial.ts
🔇 Additional comments (3)
packages/auxiliary-lines/src/WireframePrimitive.ts (2)
12-24: LGTM! Well-structured property getters.The rectangle property getters are correctly implemented and follow the established pattern of other primitive getters in the class.
783-786: Verify pivot point calculations.The pivot point calculations look correct, but let's ensure they handle edge cases properly.
✅ Verification successful
Pivot point calculations are mathematically correct
The calculations properly handle all edge cases (0,0 for top-left, 0.5,0.5 for center, 1,1 for bottom-right) while maintaining rectangle dimensions. However, consider adding test cases to verify this behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for test cases that verify pivot point calculations # Expected: Find test cases that cover different pivot point combinations # Search for test files related to rect wireframe fd -e ts -e js "test.*rect" --exec rg -l "createRectWireframe" # Search for specific test cases with different pivot points rg -A 5 "createRectWireframe.*pivot"Length of output: 101
Script:
#!/bin/bash # Search for WireframePrimitive tests and related files fd -e ts -e md "WireframePrimitive" # Search for documentation or comments about pivot points rg -i "pivot.*point|pivot.*origin" --type ts # Search for similar vertex calculations with pivot points ast-grep --pattern 'positions[$_].set($$$pivotX$$$, $$$pivotY$$$, $_)'Length of output: 296
packages/gizmo/src/icon/Icon.ts (1)
107-131: LGTM! Well-structured camera handling and update logic.The camera registration and update logic is well-implemented with proper null checks and efficient viewport synchronization.
| set size(val: Vector2) { | ||
| if (this._size !== val) { | ||
| this._size.copyFrom(val); | ||
| } | ||
| this._material.shaderData.setVector2("u_size", this._size); | ||
| } |
There was a problem hiding this comment.
Fix potential reference comparison bug in size setter.
The size comparison might fail due to reference comparison. Consider using value comparison:
set size(val: Vector2) {
- if (this._size !== val) {
+ if (!this._size.equals(val)) {
this._size.copyFrom(val);
}
this._material.shaderData.setVector2("u_size", this._size);
}📝 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 size(val: Vector2) { | |
| if (this._size !== val) { | |
| this._size.copyFrom(val); | |
| } | |
| this._material.shaderData.setVector2("u_size", this._size); | |
| } | |
| set size(val: Vector2) { | |
| if (!this._size.equals(val)) { | |
| this._size.copyFrom(val); | |
| } | |
| this._material.shaderData.setVector2("u_size", this._size); | |
| } |
| set texture(value: string | Texture2D) { | ||
| if (this._texture !== value) { | ||
| this._texture = value; | ||
| if (value instanceof Texture2D) { | ||
| this._material.baseTexture = value; | ||
| } else { | ||
| this.engine.resourceManager.load({ url: value, type: AssetType.Texture2D }).then((texture: Texture2D) => { | ||
| this._material.baseTexture = texture; | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for texture loading.
The texture setter should handle potential loading errors:
set texture(value: string | Texture2D) {
if (this._texture !== value) {
this._texture = value;
if (value instanceof Texture2D) {
this._material.baseTexture = value;
} else {
- this.engine.resourceManager.load({ url: value, type: AssetType.Texture2D }).then((texture: Texture2D) => {
- this._material.baseTexture = texture;
- });
+ this.engine.resourceManager
+ .load({ url: value, type: AssetType.Texture2D })
+ .then((texture: Texture2D) => {
+ this._material.baseTexture = texture;
+ })
+ .catch((error) => {
+ console.error(`Failed to load texture: ${value}`, error);
+ });
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set texture(value: string | Texture2D) { | |
| if (this._texture !== value) { | |
| this._texture = value; | |
| if (value instanceof Texture2D) { | |
| this._material.baseTexture = value; | |
| } else { | |
| this.engine.resourceManager.load({ url: value, type: AssetType.Texture2D }).then((texture: Texture2D) => { | |
| this._material.baseTexture = texture; | |
| }); | |
| } | |
| } | |
| } | |
| set texture(value: string | Texture2D) { | |
| if (this._texture !== value) { | |
| this._texture = value; | |
| if (value instanceof Texture2D) { | |
| this._material.baseTexture = value; | |
| } else { | |
| this.engine.resourceManager | |
| .load({ url: value, type: AssetType.Texture2D }) | |
| .then((texture: Texture2D) => { | |
| this._material.baseTexture = texture; | |
| }) | |
| .catch((error) => { | |
| console.error(`Failed to load texture: ${value}`, error); | |
| }); | |
| } | |
| } | |
| } |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
Release Notes
New Features
RectControlfor enhanced rectangular gizmo manipulation in 3D space.Iconclass for viewport icons, allowing dynamic interaction with camera views.WireframeManagerandWireframePrimitiveclasses.Improvements
Groupclass with new getter methods.Gizmoclass to support new control types and improved interaction handling.SearchComponentTypeenum for better component retrieval options.Bug Fixes
GizmoState.These changes collectively improve user interaction and visual fidelity within the application.