Add RectTool
#313
Add RectTool
#313
Conversation
WalkthroughThe pull request introduces several enhancements across multiple files in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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.
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.Improvements
Groupclass with new getter methods.Gizmoclass to support new control types and improved interaction handling.Bug Fixes
GizmoState.These changes collectively improve user interaction and visual fidelity within the application.