Add GUI #2375
Add GUI #2375
Conversation
| worldBounds.max.set(0, 0, 0); | ||
| const { x, y, z } = this._transformEntity.transform.worldPosition; | ||
| worldBounds.min.set(x, y, z); | ||
| worldBounds.max.set(x, y, z); |
There was a problem hiding this comment.
const { worldPosition } = this._transformEntity.transform;
worldBounds.min.copyFrom(worldPosition);
worldBounds.max.copyFrom(worldPosition);
Fix: fix conflicts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/ui/src/component/advanced/Image.ts (2)
111-112:⚠️ Potential issueFix type safety issues in sprite management.
The code uses @ts-ignore to bypass TypeScript checks for
_updateFlagManager. This was previously flagged in reviews.Consider creating an interface for the Sprite class that includes the update flag manager:
interface SpriteWithUpdateManager extends Sprite { _updateFlagManager: { addListener(callback: (type: SpriteModifyFlags) => void): void; removeListener(callback: (type: SpriteModifyFlags) => void): void; }; }Then update the sprite property type:
-private _sprite: Sprite = null; +private _sprite: SpriteWithUpdateManager = null;Also applies to: 117-119
232-234:⚠️ Potential issueFix type safety issue in _onDestroy method.
Similar type safety issue with
_updateFlagManagerexists here.This will be resolved by implementing the
SpriteWithUpdateManagerinterface suggested earlier.
🧹 Nitpick comments (2)
packages/ui/src/component/advanced/Image.ts (2)
106-125: Refactor sprite setter for better resource management.The sprite setter handles multiple concerns: reference counting, event listening, and shader data updates.
Consider extracting the resource management logic into separate methods:
private _detachSprite(sprite: Sprite): void { if (sprite) { this._addResourceReferCount(sprite, -1); sprite._updateFlagManager.removeListener(this._onSpriteChange); } } private _attachSprite(sprite: Sprite): void { if (sprite) { this._addResourceReferCount(sprite, 1); sprite._updateFlagManager.addListener(this._onSpriteChange); this.shaderData.setTexture(UIRenderer._textureProperty, sprite.texture); } else { this.shaderData.setTexture(UIRenderer._textureProperty, null); } }
239-280: Consider simplifying sprite change handling.The sprite change handler has multiple nested switch statements which could be simplified.
Consider using a map-based approach:
private readonly SPRITE_CHANGE_HANDLERS = new Map<SpriteModifyFlags, () => void>([ [SpriteModifyFlags.texture, () => { this.shaderData.setTexture(UIRenderer._textureProperty, this.sprite.texture); }], [SpriteModifyFlags.destroy, () => { this.sprite = null; }], // ... other handlers ]); private _onSpriteChange(type: SpriteModifyFlags): void { const handler = this.SPRITE_CHANGE_HANDLERS.get(type); if (handler) { handler(); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ui/src/component/advanced/Image.ts(1 hunks)
🔇 Additional comments (1)
packages/ui/src/component/advanced/Image.ts (1)
1-37: LGTM! Well-structured class declaration and properties.
The class is well-organized with appropriate decorators and clear property declarations.
opt code
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/ui/src/component/UIRenderer.ts (1)
204-206:⚠️ Potential issueAdd a break or return statement to prevent unintended fallthrough.
According to static analysis, the
case EntityModifyFlags.Parent:section falls through tocase EntityModifyFlags.SiblingIndex:without an explicit break. If the fallthrough is intentional, add a comment to clarify; otherwise, insert abreakorreturnstatement.case EntityModifyFlags.Parent: Utils.setRootCanvasDirty(this); Utils.setGroupDirty(this); + break; case EntityModifyFlags.SiblingIndex: entity._updateUIHierarchyVersion(this.engine.time.frameCount); break;🧰 Tools
🪛 Biome (1.9.4)
[error] 204-206: This case is falling through to the next case.
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.(lint/suspicious/noFallthroughSwitchClause)
🧹 Nitpick comments (6)
packages/ui/src/component/UIRenderer.ts (1)
88-92: Optimize color setter to avoid unnecessary copy operations.The current implementation always copies the value even if the colors are equal. Consider adding a check for equality before copying.
set color(value: Color) { - if (this._color !== value) { + if (this._color !== value && !this._color.equals(value)) { this._color.copyFrom(value); } }packages/ui/src/component/UIGroup.ts (3)
7-51: Consider enhancing type safety for internal properties.While the properties are well organized, consider adding stricter typing:
- _indexInGroup: number = -1; + _indexInGroup: -1 | number = -1; - _groupDirtyFlags: number = GroupModifyFlags.None; + _groupDirtyFlags: GroupModifyFlags = GroupModifyFlags.None;
86-110: Consider caching parent group reference for performance.The
globalAlphaandglobalInteractivegetters perform parent group lookups on each access. Consider caching the parent group reference when dirty flags are set and invalidating it when necessary.+ private _cachedParentGroup: UIGroup | null = null; private _getGroup(): UIGroup { this._isGroupDirty && Utils.setGroup(this, Utils.searchGroupInParents(this)); + if (this._isGroupDirty) { + this._cachedParentGroup = this._group; + } return this._group; }
181-188: Add documentation for the _onGroupModify method.The method handles group modification propagation but lacks documentation explaining the flags parameter and isPass behavior.
Add JSDoc comments:
/** * Handles group modification and propagates changes to child elements * @internal * @param flags - Bit flags indicating which properties were modified * @param isPass - Whether this is a pass-through modification from a parent */ _onGroupModify(flags: GroupModifyFlags, isPass: boolean = false): void {packages/ui/src/component/UICanvas.ts (2)
34-85: Add explicit type annotations to improve type safety.Consider adding explicit type annotations to the following properties:
_renderElement: anyshould have a more specific type_rootCanvasListeningEntities: Entity[]_orderedRenderers: UIRenderer[]
557-586: Document intentional switch case fall-throughs in _setRealRenderMode.The switch statements have intentional fall-throughs that should be documented for clarity.
Apply this diff:
switch (curRealMode) { case CanvasRenderMode.ScreenSpaceOverlay: this._addCanvasListener(); + // intentional fall-through: cascade setup for screen space modes case CanvasRenderMode.ScreenSpaceCamera: this._adapterPoseInScreenSpace(); this._adapterSizeInScreenSpace(); + // intentional fall-through: common setup for all modes case CanvasRenderMode.WorldSpace: componentsManager.addUICanvas(this, curRealMode === CanvasRenderMode.ScreenSpaceOverlay); break;🧰 Tools
🪛 Biome (1.9.4)
[error] 564-565: 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] 574-575: 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] 576-578: 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 selected for processing (5)
packages/core/src/Transform.ts(0 hunks)packages/ui/src/component/UICanvas.ts(1 hunks)packages/ui/src/component/UIGroup.ts(1 hunks)packages/ui/src/component/UIRenderer.ts(1 hunks)packages/ui/src/component/UITransform.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/core/src/Transform.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui/src/component/UITransform.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/ui/src/component/UICanvas.ts
[error] 164-164: 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] 237-237: 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] 252-252: 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] 434-434: 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] 564-565: 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] 574-575: 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] 576-578: 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] 204-206: 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)
🪛 GitHub Check: codecov/patch
packages/ui/src/component/UICanvas.ts
[warning] 200-213: packages/ui/src/component/UICanvas.ts#L200-L213
Added lines #L200 - L213 were not covered by tests
[warning] 219-220: packages/ui/src/component/UICanvas.ts#L219-L220
Added lines #L219 - L220 were not covered by tests
[warning] 226-227: packages/ui/src/component/UICanvas.ts#L226-L227
Added lines #L226 - L227 were not covered by tests
[warning] 233-270: packages/ui/src/component/UICanvas.ts#L233-L270
Added lines #L233 - L270 were not covered by tests
🔇 Additional comments (13)
packages/ui/src/component/UIRenderer.ts (4)
1-28: LGTM! Well-structured class definition with appropriate dependencies.
The imports are well-organized and the class is properly set up with the required dependencies and interfaces.
240-262: 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.
- const hitPointWorld = ray.getPoint(curDistance, UIRenderer._tempVec30);
- const worldMatrixInv = UIRenderer._tempMat;
- const localPosition = UIRenderer._tempVec31;
+ // Use a vector pool to avoid allocations
+ const hitPointWorld = this._getVectorFromPool();
+ const worldMatrixInv = this._getMatrixFromPool();
+ const localPosition = this._getVectorFromPool();
try {
Matrix.invert(transform.worldMatrix, worldMatrixInv);
Vector3.transformCoordinate(hitPointWorld, worldMatrixInv, localPosition);
if (this._hitTest(localPosition)) {
// ... rest of the code
}
} finally {
// Return vectors to pool
this._returnVectorToPool(hitPointWorld);
this._returnMatrixToPool(worldMatrixInv);
this._returnVectorToPool(localPosition);
}290-295: LGTM! Clear and well-documented enum definition.
The UIRendererUpdateFlags enum is properly documented and follows the extension pattern of RendererUpdateFlags.
112-113: 🛠️ Refactor suggestion
Remove @ts-ignore by properly typing the _onValueChanged property.
The use of @ts-ignore suppresses type checking which could hide potential issues. Consider adding proper type definitions for the Color class's _onValueChanged property.
- //@ts-ignore
- this._color._onValueChanged = this._onColorChanged;
+ interface ColorWithCallback extends Color {
+ _onValueChanged?: () => void;
+ }
+ (this._color as ColorWithCallback)._onValueChanged = this._onColorChanged;Likely invalid or redundant comment.
packages/ui/src/component/UIGroup.ts (4)
1-6: LGTM! Class hierarchy and imports are well structured.
The class extends Component and implements IGroupAble, which is appropriate for a UI group component that needs to be part of the entity component system.
126-137: Implementation matches previous review suggestion.
The _onDisableInScene method follows the cleanup pattern suggested in the previous review.
207-212: LGTM! Well-structured flags enum.
The GroupModifyFlags enum is properly defined for bitwise operations with appropriate values.
6-205: Consider adding comprehensive unit tests.
The UIGroup class handles complex state management and propagation. To ensure reliability, consider adding unit tests covering:
- State propagation through the group hierarchy
- Dirty flag management
- Lifecycle method behavior
- Edge cases in alpha and interactive property inheritance
Let's check for existing tests:
packages/ui/src/component/UICanvas.ts (5)
1-33: LGTM! Class structure and dependencies are well-organized.
The class extends Component and implements IElement appropriately, with clear dependency declarations using decorators.
199-213: Add test coverage for the raycast method.
The raycast method is crucial for hit-testing UI elements but lacks test coverage. Consider adding tests to verify:
- Ray intersection with UI elements
- Proper handling of hit results
- Edge cases (no hits, out of bounds)
Would you like me to help generate test cases for the raycast method?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 200-213: packages/ui/src/component/UICanvas.ts#L200-L213
Added lines #L200 - L213 were not covered by tests
589-602: LGTM! Enums are well-documented and properly structured.
The enums clearly extend existing enums with proper documentation.
159-166: 🛠️ Refactor suggestion
Refactor assignment in sortOrder setter for better readability.
The current implementation uses assignment within a logical expression which reduces code clarity.
Apply this diff:
set sortOrder(value: number) {
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] 164-164: 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)
232-270: 🛠️ Refactor suggestion
Refactor _prepareRender method for better readability and safety.
- The renderElement assignment should be separated:
- const renderElement = (this._renderElement = engine._renderElementPool.get());
+ this._renderElement = engine._renderElementPool.get();
+ const renderElement = this._renderElement;- The switch case variable declaration should be wrapped in a block:
case CanvasRenderMode.ScreenSpaceOverlay:
+ {
const { min, max } = renderer.bounds;
if (min.x > width || max.x < 0 || min.y > height || max.y < 0) {
continue;
}
break;
+ }Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 237-237: 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] 252-252: 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)
🪛 GitHub Check: codecov/patch
[warning] 233-270: packages/ui/src/component/UICanvas.ts#L233-L270
Added lines #L233 - L270 were not covered by tests
Opt: unified coordinate system
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (12)
packages/ui/src/component/UITransform.ts (1)
6-10: Consider introducing a dedicated Size classBased on the previous discussion, using Vector2 for size might not provide the most intuitive API. A dedicated Size class could offer better semantics and more relevant methods.
Consider creating a Size class:
export class Size { constructor(public width: number = 1, public height: number = 1) {} copyFrom(other: Size): void { this.width = other.width; this.height = other.height; } equals(other: Size): boolean { return this.width === other.width && this.height === other.height; } }Then update the UITransform class:
- private _size: Vector2 = new Vector2(1, 1); + private _size: Size = new Size(1, 1);packages/ui/src/component/advanced/Label.ts (3)
248-248: Improve code readability by avoiding assignments in expressions.The code contains multiple instances of assignments within expressions, which can make the code harder to read and maintain.
Consider refactoring these assignments into separate statements:
- 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++; - const subChunk = (textChunk.subChunk = this._getChunkManager().allocateSubChunk(count * 4)); + const subChunk = this._getChunkManager().allocateSubChunk(count * 4); + textChunk.subChunk = subChunk; - const indices = (subChunk.indices = []); + const indices = []; + subChunk.indices = indices;Also applies to: 494-495, 594-594, 596-596
🧰 Tools
🪛 Biome (1.9.4)
[error] 248-248: 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)
417-436: Consider performance optimization for text measurement.The text measurement logic in
_updateLocalDatacould be optimized by caching results when text and size haven't changed.Consider adding a caching mechanism:
private _cachedTextMetrics: { text: string; width: number; height: number; metrics: TextMetrics; } | null = null; private _getTextMetrics(width: number, height: number): TextMetrics { const cacheKey = `${this._text}_${width}_${height}`; if (this._cachedTextMetrics?.text === cacheKey) { return this._cachedTextMetrics.metrics; } const metrics = this.enableWrapping ? TextUtils.measureTextWithWrap(...) : TextUtils.measureTextWithoutWrap(...); this._cachedTextMetrics = { text: cacheKey, width, height, metrics }; return metrics; }
580-588: Consider extracting visibility checks into separate methods.The
_isTextNoVisiblemethod combines multiple conditions. Breaking these into separate methods would improve readability and maintainability.Consider refactoring:
private _hasValidContent(): boolean { return this._text !== '' && this._fontSize !== 0; } private _hasValidDimensions(): boolean { const size = (<UITransform>this._transformEntity.transform).size; return !( (this.enableWrapping && size.x <= 0) || (this.overflowMode === OverflowMode.Truncate && size.y <= 0) ); } private _isTextNoVisible(): boolean { return !this._hasValidContent() || !this._hasValidDimensions(); }packages/core/src/2d/sprite/SpriteRenderer.ts (3)
475-495: Enhance documentation for update flags.While the flags are well-organized, consider adding more detailed documentation for composite flags to explain:
- Which flags are combined
- When each combination is used
- The impact of each flag on the rendering pipeline
421-437: Consider simplifying draw mode specific updates.The switch statements for handling size and border changes could be simplified using a lookup table to map draw modes to update flags.
private readonly MODE_TO_SIZE_FLAGS = { [SpriteDrawMode.Simple]: this._customWidth === undefined || this._customHeight === undefined ? SpriteRendererUpdateFlags.Position : 0, [SpriteDrawMode.Sliced]: SpriteRendererUpdateFlags.Position, [SpriteDrawMode.Tiled]: SpriteRendererUpdateFlags.PositionUVAndColor }; private readonly MODE_TO_BORDER_FLAGS = { [SpriteDrawMode.Sliced]: SpriteRendererUpdateFlags.PositionAndUV, [SpriteDrawMode.Tiled]: SpriteRendererUpdateFlags.PositionUVAndColor };Also applies to: 441-450
Line range hint
333-358: Enhance error handling in render method.Consider adding more descriptive error handling:
- Log a warning when sprite texture is missing
- Log a warning when material is destroyed
const { _sprite: sprite } = this; if (!sprite?.texture || !this.width || !this.height) { + if (sprite && !sprite.texture) { + console.warn(`SpriteRenderer: Missing texture for sprite "${sprite.name}"`); + } return; }packages/ui/src/component/interactive/transition/Transition.ts (2)
8-11: Consider adding constraints to generic types.The generic type
Tshould be constrained to ensure it has the necessary properties for transitions.-export abstract class Transition< - T extends TransitionValueType = TransitionValueType, - K extends UIRenderer = UIRenderer -> { +export abstract class Transition< + T extends TransitionValueType & { clone?(): T } = TransitionValueType, + K extends UIRenderer = UIRenderer +> {
167-171: Add error handling for edge cases in _updateValue.The method should handle cases where
_durationis 0 or when transitions are interrupted.protected _updateValue() { + if (this._duration < 0) { + console.warn('Invalid transition duration'); + this._duration = 0; + } const weight = this._duration ? 1 - this._countDown / this._duration : 1; this._updateCurrentValue(this._initialValue, this._finalValue, weight); this._target?.enabled && this._applyValue(this._currentValue); }packages/ui/src/component/interactive/UIInteractive.ts (1)
111-121: Optimize transition removal for better performance.The current implementation could be optimized by avoiding array element swapping.
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]); - transitions.length = lastOneIndex; - transition._interactive = null; - break; - } - } + const index = transitions.indexOf(transition); + if (index !== -1) { + transitions.splice(index, 1); + transition._interactive = null; + } }🧰 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)
packages/ui/src/component/advanced/Image.ts (1)
238-279: Optimize sprite change handling with a lookup map.The nested switch statements could be replaced with a more efficient lookup approach.
private static readonly SPRITE_UPDATE_FLAGS = new Map<SpriteModifyFlags, number>([ [SpriteModifyFlags.texture, 0], [SpriteModifyFlags.size, ImageUpdateFlags.Position], [SpriteModifyFlags.border, ImageUpdateFlags.PositionAndUV], [SpriteModifyFlags.region, ImageUpdateFlags.PositionAndUV], [SpriteModifyFlags.atlasRegionOffset, ImageUpdateFlags.PositionAndUV], [SpriteModifyFlags.atlasRegion, ImageUpdateFlags.UV], [SpriteModifyFlags.destroy, -1] ]); private _onSpriteChange(type: SpriteModifyFlags): void { const flag = Image.SPRITE_UPDATE_FLAGS.get(type); if (flag === undefined) return; if (flag === -1) { this.sprite = null; return; } if (flag === 0) { this.shaderData.setTexture(UIRenderer._textureProperty, this.sprite.texture); return; } this._dirtyUpdateFlag |= flag; }packages/ui/src/component/UICanvas.ts (1)
557-584: Document intentional switch case fall-throughs.The switch statements contain intentional fall-throughs that should be documented for clarity.
Apply this diff to improve maintainability:
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] 564-565: 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] 574-575: 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] 576-578: 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 (7)
tests/src/core/2d/text/__screenshots__/TextRenderer.test.ts/TextRenderer-bounds-1.pngis excluded by!**/*.pngtests/src/core/__screenshots__/Sprite.test.ts/Sprite-get-set-size-1.pngis excluded by!**/*.pngtests/src/core/__screenshots__/SpriteMask.test.ts/SpriteMask--render-1.pngis excluded by!**/*.pngtests/src/core/__screenshots__/SpriteMask.test.ts/SpriteMask-get-set-size-1.pngis excluded by!**/*.pngtests/src/core/__screenshots__/SpriteRenderer.test.ts/SpriteRenderer--render-1.pngis excluded by!**/*.pngtests/src/core/__screenshots__/SpriteRenderer.test.ts/SpriteRenderer-get-set-size-1.pngis excluded by!**/*.pngtests/src/core/audio/__screenshots__/AudioSource.test.ts/AudioSource-load-1.pngis excluded by!**/*.png
📒 Files selected for processing (17)
packages/core/src/2d/assembler/ISpriteAssembler.ts(1 hunks)packages/core/src/2d/assembler/SlicedSpriteAssembler.ts(5 hunks)packages/core/src/2d/assembler/TiledSpriteAssembler.ts(7 hunks)packages/core/src/2d/sprite/Sprite.ts(5 hunks)packages/core/src/2d/sprite/SpriteMask.ts(9 hunks)packages/core/src/2d/sprite/SpriteRenderer.ts(13 hunks)packages/core/src/2d/text/ITextRenderer.ts(1 hunks)packages/ui/src/component/UICanvas.ts(1 hunks)packages/ui/src/component/UITransform.ts(1 hunks)packages/ui/src/component/advanced/Image.ts(1 hunks)packages/ui/src/component/advanced/Label.ts(1 hunks)packages/ui/src/component/interactive/UIInteractive.ts(1 hunks)packages/ui/src/component/interactive/transition/ColorTransition.ts(1 hunks)packages/ui/src/component/interactive/transition/ScaleTransition.ts(1 hunks)packages/ui/src/component/interactive/transition/Transition.ts(1 hunks)tests/src/core/Sprite.test.ts(0 hunks)tests/src/ui/UIInteractive.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- tests/src/core/Sprite.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/src/ui/UIInteractive.test.ts
- packages/ui/src/component/interactive/transition/ColorTransition.ts
- packages/core/src/2d/assembler/ISpriteAssembler.ts
- packages/ui/src/component/interactive/transition/ScaleTransition.ts
👮 Files not reviewed due to content moderation or server errors (3)
- packages/core/src/2d/assembler/SlicedSpriteAssembler.ts
- packages/core/src/2d/sprite/SpriteMask.ts
- packages/core/src/2d/assembler/TiledSpriteAssembler.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/ui/src/component/UICanvas.ts
[error] 164-164: 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] 237-237: 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] 252-252: 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] 434-434: 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] 564-565: 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] 574-575: 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] 576-578: 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/advanced/Label.ts
[error] 248-248: 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] 494-494: 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] 495-495: 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] 505-505: 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] 507-507: 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] 594-594: 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] 596-596: 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/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/core/src/2d/assembler/TiledSpriteAssembler.ts
[error] 79-79: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 79-79: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 84-84: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 84-84: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 248-248: 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] 260-260: 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] 261-261: 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] 262-262: 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] 290-290: 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] 291-291: 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] 292-292: 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)
🪛 GitHub Check: codecov/patch
packages/ui/src/component/UICanvas.ts
[warning] 200-213: packages/ui/src/component/UICanvas.ts#L200-L213
Added lines #L200 - L213 were not covered by tests
[warning] 219-220: packages/ui/src/component/UICanvas.ts#L219-L220
Added lines #L219 - L220 were not covered by tests
[warning] 226-227: packages/ui/src/component/UICanvas.ts#L226-L227
Added lines #L226 - L227 were not covered by tests
[warning] 233-270: packages/ui/src/component/UICanvas.ts#L233-L270
Added lines #L233 - L270 were not covered by tests
packages/core/src/2d/sprite/SpriteRenderer.ts
[warning] 326-328: packages/core/src/2d/sprite/SpriteRenderer.ts#L326-L328
Added lines #L326 - L328 were not covered by tests
packages/core/src/2d/sprite/SpriteMask.ts
[warning] 251-253: packages/core/src/2d/sprite/SpriteMask.ts#L251-L253
Added lines #L251 - L253 were not covered by tests
🔇 Additional comments (16)
packages/ui/src/component/UITransform.ts (3)
19-23: Improve setter logic for size property
The current implementation has readability issues and potential bugs with reference equality checks.
32-36: Improve setter logic for pivot property
The current implementation has readability issues and potential bugs with reference equality checks.
66-71: LGTM!
The UITransformModifyFlags enum is well-defined with appropriate values for bitwise operations and clear documentation.
packages/core/src/2d/text/ITextRenderer.ts (1)
1-9: LGTM! Well-designed interface for text rendering.
The interface is clean, well-structured, and follows TypeScript best practices.
packages/ui/src/component/advanced/Label.ts (2)
228-231:
Remove @ts-ignore comments and add proper type definitions.
The code uses @ts-ignore to bypass type checking for engine properties. This can lead to runtime errors and makes the code harder to maintain.
Consider adding proper type definitions:
- // @ts-ignore
- this.font = engine._textDefaultFont;
- // @ts-ignore
- this.setMaterial(engine._basicResources.textDefaultMaterial);
+ interface IEngineInternal {
+ _textDefaultFont: Font;
+ _basicResources: {
+ textDefaultMaterial: Material;
+ };
+ }
+ this.font = (engine as IEngineInternal)._textDefaultFont;
+ this.setMaterial((engine as IEngineInternal)._basicResources.textDefaultMaterial);Likely invalid or redundant comment.
100-105: 🛠️ Refactor suggestion
Add input validation for fontSize.
The fontSize setter lacks validation for negative or extremely large values that could impact performance.
Add validation:
set fontSize(value: number) {
+ if (value < 0) {
+ throw new Error("Font size cannot be negative");
+ }
+ if (value > 1000) {
+ console.warn("Large font sizes may impact performance");
+ }
if (this._fontSize !== value) {
this._fontSize = value;
this._setDirtyFlagTrue(DirtyFlag.Font);
}
}Likely invalid or redundant comment.
packages/core/src/2d/sprite/Sprite.ts (1)
Line range hint 372-404: LGTM! Correct usage of @ignoreClone decorator.
The addition of @ignoreClone decorator to the event handlers is appropriate since these methods are bound in the constructor and should not be cloned.
packages/core/src/2d/sprite/SpriteRenderer.ts (3)
318-324: LGTM! Correct bounds calculation with flip handling.
The bounds calculation properly accounts for sprite flipping by adjusting the pivot points accordingly.
25-25: LGTM! Correct implementation of ISpriteRenderer interface.
The class properly implements all required members of the ISpriteRenderer interface.
326-328: Add test coverage for null sprite bounds calculation.
The fallback bounds calculation for null sprite cases needs test coverage.
✅ Verification successful
Test coverage exists for null sprite bounds calculation
The test coverage for null sprite bounds calculation already exists in the test file. Specifically, in tests/src/core/SpriteRenderer.test.ts, there are test cases that verify:
- Initial state (no sprite):
expect(Vector3.equals(spriteRenderer.bounds.min, new Vector3(0, 0, 0))).to.eq(true);
expect(Vector3.equals(spriteRenderer.bounds.max, new Vector3(0, 0, 0))).to.eq(true);- Another test case also verifies the null sprite bounds:
expect(spriteRenderer.bounds.min).to.deep.eq(new Vector3(0, 0, 0));
expect(spriteRenderer.bounds.max).to.deep.eq(new Vector3(0, 0, 0));These test cases adequately cover the fallback bounds calculation for null sprite cases, which sets both min and max bounds to the world position of the transform entity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for null sprite bounds calculation
# Test: Search for test cases covering null sprite bounds. Expect: Test cases that verify bounds for null sprite.
rg -A 5 "describe.*bounds.*null" "test/**/*.test.ts"
Length of output: 112
Script:
#!/bin/bash
# First, let's find all test files in the repository
fd "\.test\.(ts|js)$"
# Then, let's search for any test cases related to sprite bounds or world bounds
rg -i "describe.*sprite.*bounds|describe.*world.*bounds" -A 5
Length of output: 3536
Script:
#!/bin/bash
# Let's check the SpriteRenderer test file specifically for bounds-related tests
rg -A 10 "bounds|worldBounds" "tests/src/core/SpriteRenderer.test.ts"
# Also check the implementation to understand the bounds calculation logic better
rg -A 10 "worldBounds" "packages/core/src/2d/sprite/SpriteRenderer.ts"
Length of output: 5102
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 326-328: packages/core/src/2d/sprite/SpriteRenderer.ts#L326-L328
Added lines #L326 - L328 were not covered by tests
packages/ui/src/component/interactive/transition/Transition.ts (1)
156-165:
Improve type safety in resource management.
The code uses @ts-ignore to bypass type checking when accessing _addReferCount. This could lead to runtime errors.
Create an interface for reference-counted resources:
+interface IReferenceCountable {
+ _addReferCount(count: number): void;
+}
-protected _onStateValueDirty(state: InteractiveState, preValue: T, curValue: T): void {
+protected _onStateValueDirty(
+ state: InteractiveState,
+ preValue: T & Partial<IReferenceCountable>,
+ curValue: T & Partial<IReferenceCountable>
+): void {
preValue instanceof ReferResource && preValue._addReferCount(-1);
curValue instanceof ReferResource && curValue._addReferCount(1);
if (this._finalState === state) {
this._finalValue = curValue;
this._updateValue();
}
}Likely invalid or redundant comment.
packages/ui/src/component/interactive/UIInteractive.ts (1)
187-194:
Remove @ts-ignore comments and fix type safety.
The code uses @ts-ignore to bypass TypeScript checks in lifecycle methods.
- // @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.
packages/ui/src/component/advanced/Image.ts (1)
167-170:
Address TODO comment regarding destroyed materials.
The code silently handles destroyed materials by falling back to default material.
- // @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 reference to prevent future warnings
+ }Likely invalid or redundant comment.
packages/ui/src/component/UICanvas.ts (3)
1-33: LGTM! Well-structured class with appropriate dependencies.
The class structure is well-organized with clear dependency management through decorators and proper interface implementation.
589-602: LGTM! Well-documented enum extensions.
The enums properly extend existing enums with clear documentation and appropriate values.
159-166: 🛠️ Refactor suggestion
Refactor assignment in logical expression for better readability.
The current implementation uses an assignment within a logical expression, which can be confusing and harder to maintain.
Apply this diff to improve readability:
set sortOrder(value: number) {
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] 164-164: 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)
| private _onSizeChange(): void { | ||
| // @ts-ignore | ||
| this._entity._updateFlagManager.dispatch(UITransformModifyFlags.Size); | ||
| } | ||
|
|
||
| @ignoreClone | ||
| private _onPivotChange(): void { | ||
| // @ts-ignore | ||
| this._entity._updateFlagManager.dispatch(UITransformModifyFlags.Pivot); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve protected member access pattern
The change handlers directly access protected members with @ts-ignore. This is fragile and could break with engine updates.
Consider:
- Using proper accessor methods from the base class
- Creating a protected interface for entity updates
- Moving the update flag management to a dedicated service
protected notifyTransformUpdate(flag: UITransformModifyFlags): void {
if (this._entity?.updateManager) {
this._entity.updateManager.dispatch(flag);
}
}| for (let i = 0, n = children.length; i < n; i++) { | ||
| const child = children[i]; | ||
| child.isActive && (depth = this._walk(child, renderers, depth, group)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor assignment in logical expression within _walk method.
The method uses an assignment within a logical expression which reduces readability.
Apply this diff to improve clarity:
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);
+ }
}📝 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.
| for (let i = 0, n = children.length; i < n; i++) { | |
| const child = children[i]; | |
| child.isActive && (depth = this._walk(child, renderers, depth, group)); | |
| } | |
| for (let i = 0, n = children.length; i < n; i++) { | |
| const child = children[i]; | |
| if (child.isActive) { | |
| depth = this._walk(child, renderers, depth, group); | |
| } | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 434-434: 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)
Feat: add `referenceResolutionPerUnit` Fix: fix hierarchy dirty bug
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/core/src/2d/assembler/TiledSpriteAssembler.ts (1)
Line range hint
250-302: Wrap switch clause declarations in blocks.Variables declared in switch clauses can be erroneously accessed by other clauses. This can lead to subtle bugs.
Apply this diff to fix the issue:
switch (rType) { case TiledType.Compressed: { const scale = width / fixedLR; rPos.add(expectWidth * left * scale), rPos.add(fixedL * scale); rPos.add(width - expectWidth * (1 - right) * scale); rUV.add(spriteUV0.x), rUV.add(spriteUV1.x), rUV.add(spriteUV2.x), rUV.add(spriteUV3.x); break; + } case TiledType.WithTiled: { const uv1 = spriteUV1.x; const uv2 = spriteUV2.x; const repeatWidth = (width - fixedLR) / rTiledCount; // ... rest of the case break; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 250-250: 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] 262-262: 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] 263-263: 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] 264-264: 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)
♻️ Duplicate comments (3)
packages/ui/src/component/advanced/Image.ts (3)
105-124:⚠️ Potential issueFix multiple type safety suppressions in sprite setter.
The sprite setter uses @ts-ignore to bypass type checking for
_updateFlagManager. Consider creating proper interfaces for better type safety.+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);
167-170:⚠️ Potential issueAddress TODO comment and improve material handling.
The code silently handles destroyed materials by falling back to default material.
- // @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 + }
241-250:⚠️ Potential issueRemove @ts-ignore in _onDestroy method.
Similar to the sprite setter, the _onDestroy method uses @ts-ignore for type checking.
+interface IUpdateFlagManager { + addListener(callback: (type: number) => void): void; + removeListener(callback: (type: number) => void): void; +} +interface ISpriteInternal extends Sprite { + _updateFlagManager: IUpdateFlagManager; +} - // @ts-ignore - sprite._updateFlagManager.removeListener(this._onSpriteChange); + (sprite as ISpriteInternal)._updateFlagManager.removeListener(this._onSpriteChange);
🧹 Nitpick comments (9)
packages/ui/src/component/advanced/Label.ts (5)
248-248: Avoid assignments in expressions.The code contains multiple instances of assignments within expressions, which can make the code harder to read and maintain.
Consider refactoring these assignments into separate statements:
- 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++; - const subChunk = (textChunk.subChunk = this._getChunkManager().allocateSubChunk(count * 4)); + const subChunk = this._getChunkManager().allocateSubChunk(count * 4); + textChunk.subChunk = subChunk; - const indices = (subChunk.indices = []); + const indices = []; + subChunk.indices = indices;Also applies to: 502-502, 503-503, 513-513, 515-515, 602-602, 604-604
🧰 Tools
🪛 Biome (1.9.4)
[error] 248-248: 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)
588-596: Consider adding validation for fontSize.The
_isTextNoVisiblemethod checks for zero fontSize but doesn't validate negative values.Add validation in the fontSize setter:
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); } }
421-444: Consider caching text measurements for performance.The
_updateLocalDatamethod performs text measurements on every update. For static text, these measurements could be cached.Consider adding a cache mechanism for text measurements when the text content and style properties remain unchanged.
535-538: Consider using a more efficient sorting algorithm.The code sorts CharRenderInfo arrays by texture instanceId. For small arrays, the current approach is fine, but for large text blocks, a more efficient sorting algorithm or pre-sorting during character processing might be beneficial.
Consider maintaining a sorted structure during character processing to avoid the need for sorting afterward, especially for large text blocks.
649-654: Add documentation for TextChunk class.The
TextChunkclass lacks documentation explaining its purpose and usage.Add JSDoc comments explaining the purpose of the TextChunk class and its properties:
+/** + * Represents a chunk of text that shares the same texture and can be rendered together. + * Used for efficient batching of text rendering operations. + */ class TextChunk { + /** Array of character rendering information for this chunk */ charRenderInfos = new Array<CharRenderInfo>(); + /** Texture shared by all characters in this chunk */ texture: Texture2D; + /** Rendering data for this chunk */ subChunk; }packages/core/src/2d/assembler/TiledSpriteAssembler.ts (1)
35-44: Consider encapsulating transform parameters.The method signature has grown with multiple related parameters. Consider grouping them into a transform configuration object for better maintainability.
Example refactor:
interface TransformConfig { width: number; height: number; pivot: Vector2; flipX?: boolean; flipY?: boolean; referenceResolutionPerUnit?: number; } static updatePositions( renderer: ISpriteRenderer, worldMatrix: Matrix, config: TransformConfig ): voidpackages/ui/src/component/UICanvas.ts (3)
178-180: Improve readability by avoiding assignments in expressions.Assignments within logical expressions reduce code clarity. Consider using an explicit if statement.
- this._realRenderMode === CanvasRenderMode.ScreenSpaceOverlay && - (this.scene._componentsManager._overlayCanvasesSortingFlag = true); + if (this._realRenderMode === CanvasRenderMode.ScreenSpaceOverlay) { + // @ts-ignore + this.scene._componentsManager._overlayCanvasesSortingFlag = true; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 180-180: 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)
581-601: Document intentional switch case fallthroughs.The switch cases in
_setRealRenderModehave intentional fallthroughs, but this should be explicitly documented to prevent confusion.case CanvasRenderMode.ScreenSpaceOverlay: this._removeCanvasListener(); + // Intentional fallthrough to handle common cleanup case CanvasRenderMode.ScreenSpaceCamera: case CanvasRenderMode.WorldSpace: case CanvasRenderMode.ScreenSpaceOverlay: this._addCanvasListener(); + // Intentional fallthrough to handle common setup case CanvasRenderMode.ScreenSpaceCamera: this._adapterPoseInScreenSpace(); this._adapterSizeInScreenSpace(); + // Intentional fallthrough to handle common initialization case CanvasRenderMode.WorldSpace:🧰 Tools
🪛 Biome (1.9.4)
[error] 581-582: 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] 591-592: 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] 593-595: This case is falling through to the next case.
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.(lint/suspicious/noFallthroughSwitchClause)
267-273: Limit variable scope in switch cases.Variables declared in switch cases are accessible to all subsequent cases. Consider wrapping the case body in a block to restrict variable scope.
case CanvasRenderMode.ScreenSpaceOverlay: + { const { min, max } = renderer.bounds; if (min.x > width || max.x < 0 || min.y > height || max.y < 0) { continue; } break; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 268-268: 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/core/src/2d/assembler/ISpriteAssembler.ts(1 hunks)packages/core/src/2d/assembler/SlicedSpriteAssembler.ts(5 hunks)packages/core/src/2d/assembler/TiledSpriteAssembler.ts(7 hunks)packages/ui/src/component/UICanvas.ts(1 hunks)packages/ui/src/component/UIRenderer.ts(1 hunks)packages/ui/src/component/UITransform.ts(1 hunks)packages/ui/src/component/advanced/Image.ts(1 hunks)packages/ui/src/component/advanced/Label.ts(1 hunks)packages/ui/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ui/src/component/UITransform.ts
- packages/core/src/2d/assembler/SlicedSpriteAssembler.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/ui/src/component/UICanvas.ts
[error] 180-180: 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] 253-253: 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] 268-268: 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] 451-451: 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] 581-582: 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] 591-592: 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] 593-595: 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/advanced/Label.ts
[error] 248-248: 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] 502-502: 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] 503-503: 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] 513-513: 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] 515-515: 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] 602-602: 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] 604-604: 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/core/src/2d/assembler/TiledSpriteAssembler.ts
[error] 80-80: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 80-80: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 85-85: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 85-85: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 250-250: 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] 262-262: 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] 263-263: 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] 264-264: 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] 292-292: 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] 293-293: 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] 294-294: 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)
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)
🪛 GitHub Check: codecov/patch
packages/ui/src/component/UICanvas.ts
[warning] 93-94: packages/ui/src/component/UICanvas.ts#L93-L94
Added lines #L93 - L94 were not covered by tests
[warning] 97-100: packages/ui/src/component/UICanvas.ts#L97-L100
Added lines #L97 - L100 were not covered by tests
[warning] 216-229: packages/ui/src/component/UICanvas.ts#L216-L229
Added lines #L216 - L229 were not covered by tests
[warning] 235-236: packages/ui/src/component/UICanvas.ts#L235-L236
Added lines #L235 - L236 were not covered by tests
🔇 Additional comments (13)
packages/ui/src/component/advanced/Image.ts (3)
1-21: LGTM! Well-organized imports and class structure.The imports are properly organized and the class correctly extends UIRenderer while implementing ISpriteRenderer.
26-35: LGTM! Well-defined properties with appropriate cloning behavior.Properties are correctly marked as private with appropriate clone decorators based on their usage.
252-316: LGTM! Well-implemented event handler and enum.The _onSpriteChange method properly handles all sprite modification cases, and the ImageUpdateFlags enum is well-documented.
packages/ui/src/component/advanced/Label.ts (2)
1-27: LGTM! Well-organized imports.The imports are well-structured and properly grouped from the engine package and local modules.
228-231: 🛠️ Refactor suggestionRemove @ts-ignore comments and add proper type assertions.
The code uses @ts-ignore to bypass type checking for engine properties. Consider adding proper type assertions or interfaces.
- // @ts-ignore - this.font = engine._textDefaultFont; - // @ts-ignore - this.setMaterial(engine._basicResources.textDefaultMaterial); + interface IEngineInternal { + _textDefaultFont: Font; + _basicResources: { + textDefaultMaterial: Material; + }; + } + this.font = (engine as IEngineInternal)._textDefaultFont; + this.setMaterial((engine as IEngineInternal)._basicResources.textDefaultMaterial);Likely invalid or redundant comment.
packages/core/src/2d/assembler/ISpriteAssembler.ts (1)
1-20: LGTM! Interface changes align with PR objectives.The interface updates improve type safety by using
ISpriteRendererand enhance flexibility by adding parameters for better UI element size management.packages/ui/src/index.ts (3)
105-112:⚠️ Potential issueFix type safety in Label component registration.
The
@ts-ignorecomment on line 108 indicates a type mismatch that should be properly handled.Apply this diff to fix the type safety issue:
-ReflectionParser.registerCustomParseComponent("Label", async (instance: any, item: Omit<IClassObject, "class">) => { +ReflectionParser.registerCustomParseComponent("Label", async ( + instance: { engine: Engine; font?: any }, + item: Omit<IClassObject, "class"> & { props: { font?: any; fontFamily?: string } } +) => { const { props } = item; if (!props.font) { - // @ts-ignore - instance.font = Font.createFromOS(instance.engine, props.fontFamily || "Arial"); + instance.font = await Font.createFromOS(instance.engine, props.fontFamily || "Arial"); } return instance; });Likely invalid or redundant comment.
64-73: 🛠️ Refactor suggestionImprove UI hierarchy version tracking implementation.
The current implementation has two issues:
- Missing documentation for the version tracking mechanism
- Type safety issue with parent reference
Apply this diff to improve the implementation:
export class EntityExtension { _uiHierarchyVersion = 0; + /** + * Updates the UI hierarchy version and propagates the change to parent. + * @param version - The new version number + */ _updateUIHierarchyVersion(version: number): void { if (this._uiHierarchyVersion !== version) { this._uiHierarchyVersion = version; - // @ts-ignore - this.parent?._updateUIHierarchyVersion(version); + (this as any).parent?._updateUIHierarchyVersion?.(version); } } }Likely invalid or redundant comment.
33-62:⚠️ Potential issueFix type safety issues in UI material initialization.
The
@ts-ignorecomment on line 44 indicates a type mismatch that should be properly handled.Apply this diff to fix the type safety issue:
- // @ts-ignore - const material = new Material(this, shader); + const material = new Material(this as unknown as Engine, shader);Likely invalid or redundant comment.
packages/ui/src/component/UIRenderer.ts (4)
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.
87-95: Well-implemented color management.The color property implementation with proper change detection and dirty flag updates is clean and efficient.
Also applies to: 229-231
208-210:⚠️ Potential issueAdd a break statement to prevent unintended fallthrough.
The case for
EntityModifyFlags.Parentfalls through to the next case without a break statement, which could lead to unexpected behavior.case EntityModifyFlags.Parent: Utils.setRootCanvasDirty(this); Utils.setGroupDirty(this); + break; case EntityModifyFlags.SiblingIndex: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)
115-116: 🛠️ Refactor suggestionImprove type safety by properly typing the color change callback.
The
@ts-ignorecomment suppresses type checking. Consider defining proper types for the_onValueChangedproperty in theColorclass.- //@ts-ignore - this._color._onValueChanged = this._onColorChanged; + interface ColorWithCallback extends Color { + _onValueChanged: () => void; + } + (this._color as ColorWithCallback)._onValueChanged = this._onColorChanged;Likely invalid or redundant comment.
| if (isNaN(cUV.get(doubleJ)) || isNaN(cUV.get(doubleJ + 1))) { | ||
| continue; | ||
| } | ||
| for (let i = 0; i < rowLength; i++) { | ||
| const uvL = uvRow.get(2 * i); | ||
| const uvR = uvRow.get(2 * i + 1); | ||
| const uvT = uvColumn.get(doubleJ + 1); | ||
| if (isNaN(uvL) || isNaN(uvR) || isNaN(uvT)) { | ||
| const doubleI = i << 1; | ||
| if (isNaN(rUV.get(doubleI)) || isNaN(rUV.get(doubleI + 1))) { |
There was a problem hiding this comment.
Replace unsafe isNaN with Number.isNaN.
The global isNaN function performs type coercion which can lead to unexpected behavior.
Apply this diff to use the safer Number.isNaN:
- if (isNaN(cUV.get(doubleJ)) || isNaN(cUV.get(doubleJ + 1))) {
+ if (Number.isNaN(cUV.get(doubleJ)) || Number.isNaN(cUV.get(doubleJ + 1))) {
continue;
}
for (let i = 0; i < rowLength; i++) {
const doubleI = i << 1;
- if (isNaN(rUV.get(doubleI)) || isNaN(rUV.get(doubleI + 1))) {
+ if (Number.isNaN(rUV.get(doubleI)) || Number.isNaN(rUV.get(doubleI + 1))) {📝 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 (isNaN(cUV.get(doubleJ)) || isNaN(cUV.get(doubleJ + 1))) { | |
| continue; | |
| } | |
| for (let i = 0; i < rowLength; i++) { | |
| const uvL = uvRow.get(2 * i); | |
| const uvR = uvRow.get(2 * i + 1); | |
| const uvT = uvColumn.get(doubleJ + 1); | |
| if (isNaN(uvL) || isNaN(uvR) || isNaN(uvT)) { | |
| const doubleI = i << 1; | |
| if (isNaN(rUV.get(doubleI)) || isNaN(rUV.get(doubleI + 1))) { | |
| if (Number.isNaN(cUV.get(doubleJ)) || Number.isNaN(cUV.get(doubleJ + 1))) { | |
| continue; | |
| } | |
| for (let i = 0; i < rowLength; i++) { | |
| const doubleI = i << 1; | |
| if (Number.isNaN(rUV.get(doubleI)) || Number.isNaN(rUV.get(doubleI + 1))) { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 80-80: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 80-80: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 85-85: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 85-85: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
There was a problem hiding this comment.
@cptbtptpbcptdtptp, glad to hear that you found it helpful! Feel free to let me know if there's anything else you need.
| height: number, | ||
| pivot: Vector2, | ||
| flipX: boolean = false, | ||
| flipY: boolean = false |
| @@ -74,15 +78,16 @@ export class SimpleSpriteAssembler { | |||
| vertices[offset + 28] = top; | |||
| } | |||
|
|
|||
| static updateColor(renderer: SpriteRenderer): void { | |||
| static updateColor(renderer: ISpriteRenderer, alpha: number = 1): void { | |||
| @@ -83,35 +67,35 @@ export class TiledSpriteAssembler { | |||
| const wE13 = (wE[13] = pWE[13] - localTransX * wE[1] - localTransY * wE[5]); | |||
| const wE14 = (wE[14] = pWE[14] - localTransX * wE[2] - localTransY * wE[6]); | |||
| // Assemble position and uv | |||
| const rowLength = posRow.length - 1; | |||
| const columnLength = posColumn.length - 1; | |||
| const rowLength = rPos.length - 1; | |||
| static _uvRow = new DisorderedArray<number>(); | ||
| static _uvColumn = new DisorderedArray<number>(); | ||
| private static _matrix = new Matrix(); | ||
| private static _posRow = new DisorderedArray<number>(); |
| /** | ||
| * @internal | ||
| */ | ||
| _getBounds(): BoundingBox { |
| @@ -76,7 +77,7 @@ export class SpriteMask extends Renderer { | |||
| set width(value: number) { | |||
| if (this._customWidth !== value) { | |||
| this._customWidth = value; | |||
| this._dirtyUpdateFlag |= RendererUpdateFlags.WorldVolume; | |||
| this._dirtyUpdateFlag |= SpriteMaskUpdateFlags.WorldVolumeAndPosition; | |||
| @@ -647,11 +648,10 @@ export class TextRenderer extends Renderer { | |||
| this._buildChunk(curTextChunk, charLength); | |||
| } | |||
| charRenderInfos.length = 0; | |||
| this._setDirtyFlagFalse(DirtyFlag.LocalPositionBounds); | |||
packages/core/src/Camera.ts
Outdated
| @@ -417,6 +433,7 @@ export class Camera extends Component { | |||
| value && this._addResourceReferCount(value, 1); | |||
| this._renderTarget = value; | |||
| this._onPixelViewportChanged(); | |||
| this._checkMainCanvasAntialiasWaste(); | |||
packages/core/src/Camera.ts
Outdated
| @@ -130,6 +132,10 @@ export class Camera extends Component { | |||
| private _enableHDR = false; | |||
| private _enablePostProcess = false; | |||
|
|
|||
| /** @internal */ | |||
| @ignoreClone | |||
| _updateFlagManager: UpdateFlagManager; | |||
packages/core/src/Entity.ts
Outdated
| @@ -97,6 +98,8 @@ export class Entity extends EngineObject { | |||
| _isTemplate: boolean = false; | |||
| /** @internal */ | |||
| _updateFlagManager: UpdateFlagManager = new UpdateFlagManager(); | |||
| /** @internal */ | |||
| _modifyFlagManager: UpdateFlagManager; | |||
packages/core/src/Entity.ts
Outdated
| /** | ||
| * @internal | ||
| */ | ||
| _dispatchModify(flag: EntityModifyFlags, param?: any): void { |
packages/core/src/Script.ts
Outdated
| */ | ||
| onPointerDown(pointer: Pointer): void {} | ||
| onPointerDown(event: PointerEventData): void {} |
| onPointerDrag = "onPointerDrag", | ||
| onPointerEndDrag = "onPointerEndDrag", | ||
| onPointerDrop = "onPointerDrop" | ||
| } |
packages/core/src/input/index.ts
Outdated
| export { InputManager } from "./InputManager"; | ||
| export { Keys } from "./enums/Keys"; | ||
| export { PointerButton } from "./enums/PointerButton"; | ||
| export { PointerMethods } from "./enums/PointerMethods"; |
| /** @internal */ | ||
| _frameEvents: PointerEventType = PointerEventType.None; | ||
| /** @internal */ | ||
| _emitters: PointerEventEmitter[] = []; |
There was a problem hiding this comment.
Add comments and consider to restruct
| /** The position of the event trigger (in world space). */ | ||
| position: Vector3 = new Vector3(); | ||
|
|
||
| dispose() { |
| * Pointer event data. | ||
| */ | ||
| export class PointerEventData implements IPoolElement { | ||
| /** The entity that listens to this event. */ |
| /** The entity that listens to this event. */ | ||
| target: Entity; | ||
| /** The entity currently handling this event. */ | ||
| currentTarget: Entity; |
| */ | ||
| export class PointerEventData implements IPoolElement { | ||
| /** The entity that listens to this event. */ | ||
| target: Entity; |
| /** The pointer that triggers this event. */ | ||
| pointer: Pointer; | ||
| /** The position of the event trigger (in world space). */ | ||
| position: Vector3 = new Vector3(); |
| @@ -65,55 +68,68 @@ export class PointerManager implements IInput { | |||
| * @internal | |||
| */ | |||
| _update(): void { | |||
| const { _pointers: pointers, _nativeEvents: nativeEvents, _htmlCanvas: htmlCanvas } = this; | |||
| const { _pointers, _nativeEvents, _htmlCanvas, _engine, _eventPool: eventPool } = this; | |||
There was a problem hiding this comment.
_position:position is better!
| pointer._firePointerExitAndEnter(null); | ||
| emitters.forEach((emitter) => { | ||
| emitter.processLeave(pointer); | ||
| }); |
| import { Pointer } from "../Pointer"; | ||
| import { PointerEventEmitter } from "./PointerEventEmitter"; | ||
|
|
||
| export class PhysicsPointerEventEmitter extends PointerEventEmitter { |
| @@ -16,4 +17,6 @@ export class HitResult { | |||
| normal: Vector3 = new Vector3(); | |||
| /** The shape of the collider that was hit. */ | |||
| shape: ColliderShape = null; | |||
| /** The component that was hit. */ | |||
| component: Component = null; | |||
There was a problem hiding this comment.
Maybe should't add in physic struct
| @@ -320,6 +320,7 @@ export class PhysicsScene { | |||
| const result = this._nativePhysicsScene.raycast(ray, distance, onRaycast, (idx, distance, position, normal) => { | |||
| const hitShape = Engine._physicalObjectsMap[idx]; | |||
| hitResult.entity = hitShape._collider.entity; | |||
| hitResult.component = hitShape._collider; | |||
packages/core/src/ui/UIUtils.ts
Outdated
| private static _virtualCamera: VirtualCamera; | ||
| private static _viewport: Vector4; | ||
|
|
||
| static render(engine: Engine, uiCanvases: DisorderedArray<IUICanvas>): void { |
There was a problem hiding this comment.
Only overlay mode? why not named renderOverlay?
| @@ -1,6 +1,6 @@ | |||
| export type { IClone } from "./IClone"; | |||
| export * from "./input/index"; | |||
There was a problem hiding this comment.
Forgot to export this beofre?
packages/core/src/ui/UIUtils.ts
Outdated
| private static _virtualCamera: VirtualCamera; | ||
| private static _viewport: Vector4; | ||
|
|
||
| static render(engine: Engine, uiCanvases: DisorderedArray<IUICanvas>): void { |
| return this._resolutionAdaptationStrategy; | ||
| } | ||
|
|
||
| set resolutionAdaptationStrategy(value: ResolutionAdaptationStrategy) { |
There was a problem hiding this comment.
Why not call resolutionAdaptationMode?
| @ignoreClone | ||
| _groupDirtyFlags: number = GroupModifyFlags.None; | ||
|
|
||
| get ignoreParentGroup(): boolean { |
There was a problem hiding this comment.
??? where is the comments with this class?
| /** | ||
| * The transitions of this interactive. | ||
| */ | ||
| get transitions(): Readonly<Transition[]> { |
| /** | ||
| * UI component used to render text. | ||
| */ | ||
| export class Label extends UIRenderer implements ITextRenderer { |
* opt: optimize code based on CR * opt: replace isNaN to Number.isNaN
There was a problem hiding this comment.
Actionable comments posted: 17
♻️ Duplicate comments (2)
packages/core/src/Entity.ts (1)
631-643: 🛠️ Refactor suggestionRefactor assignment in expression for better readability.
The assignment within the expression can be confusing and should be split for better readability.
Apply this diff to improve readability:
- (this._modifyFlagManager ||= new UpdateFlagManager()).addListener(onChange); + if (!this._modifyFlagManager) { + this._modifyFlagManager = new UpdateFlagManager(); + } + this._modifyFlagManager.addListener(onChange);🧰 Tools
🪛 Biome (1.9.4)
[error] 632-632: 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/core/src/Camera.ts (1)
737-749: 🛠️ Refactor suggestionRefactor assignment in expression for better readability.
The assignment within the expression should be split for better readability, matching the Entity.ts pattern.
Apply this diff to improve readability:
- (this._updateFlagManager ||= new UpdateFlagManager()).addListener(onChange); + if (!this._updateFlagManager) { + this._updateFlagManager = new UpdateFlagManager(); + } + this._updateFlagManager.addListener(onChange);🧰 Tools
🪛 Biome (1.9.4)
[error] 741-741: 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 (39)
packages/math/src/BoundingBox.ts (1)
148-152: Improved precision of NaN checks in getExtent.Similar to the change in
getCenter, this update enhances type safety by using the more preciseNumber.isNaN. This is particularly important for extent calculations as they involve arithmetic operations that could produce NaN values.Consider adding unit tests to verify the behavior with edge cases:
test("should handle NaN values correctly", () => { const box = new BoundingBox(); box.min.set(NaN, 1, 2); box.max.set(3, NaN, 4); const extent = new Vector3(); box.getExtent(extent); expect(extent.x).toBe(0); expect(extent.y).toBe(0); expect(extent.z).toBe(1); });packages/ui/src/enums/ResolutionAdaptationMode.ts (1)
1-16: LGTM! Consider enhancing documentation with examples.The enum is well-structured and provides a comprehensive set of resolution adaptation modes. The documentation is clear and follows TypeScript conventions.
Consider adding code examples or visual diagrams in the documentation to illustrate how each adaptation mode behaves with different screen sizes and aspect ratios. This would help developers choose the most appropriate mode for their use case.
tests/src/ui/Text.test.ts (1)
58-82: Enhance text wrapping tests with realistic dimensions.The wrap tests use arbitrary size values (2x3) which may not represent realistic UI dimensions. Consider using more practical dimensions that match common UI scenarios.
- size1.x = 2; - size1.y = 3; + size1.x = 200; // Width in pixels + size1.y = 100; // Height in pixelspackages/ui/src/interface/IGraphics.ts (1)
9-9: Consider adding parameter documentation for _raycast method.The method signature would benefit from JSDoc comments explaining the purpose of each parameter and the return value.
+ /** + * Performs raycasting for UI hit detection. + * @param ray - The ray to test intersection with + * @param out - The hit result object to store intersection details + * @param distance - The maximum distance to test for intersection + * @returns true if the ray intersects with the UI element + */ _raycast(ray: Ray, out: UIHitResult, distance: number): boolean;packages/core/src/input/pointer/PointerEventData.ts (2)
12-12: Consider lazy initialization forworldPosition.The
Vector3instance is always created even if not used. Consider lazy initialization to optimize memory usage.-worldPosition: Vector3 = new Vector3(); +private _worldPosition: Vector3 | null = null; +get worldPosition(): Vector3 { + return this._worldPosition ||= new Vector3(); +}
17-19: Ensure complete cleanup indispose.The
disposemethod only resets thepointerproperty. Consider also resettingworldPositionfor complete cleanup.dispose() { this.pointer = null; + if (this._worldPosition) { + this._worldPosition.set(0, 0, 0); + } }packages/core/src/2d/assembler/ISpriteAssembler.ts (1)
9-18: Consider splittingupdatePositionsinto smaller focused methods.The method has many parameters which could make it harder to maintain. Consider introducing a parameter object or splitting the functionality.
+interface SpriteUpdateOptions { + worldMatrix: Matrix; + width: number; + height: number; + pivot: Vector2; + flipX: boolean; + flipY: boolean; + referenceResolutionPerUnit?: number; +} -updatePositions( - renderer: ISpriteRenderer, - worldMatrix: Matrix, - width: number, - height: number, - pivot: Vector2, - flipX: boolean, - flipY: boolean, - referenceResolutionPerUnit?: number -): void; +updatePositions(renderer: ISpriteRenderer, options: SpriteUpdateOptions): void;packages/loader/src/SpriteLoader.ts (1)
51-52: Improve readability by avoiding assignments in expressions.The current syntax using logical OR assignments makes the code harder to read and understand.
-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] 51-51: 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] 52-52: 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/core/src/ui/UIUtils.ts (3)
10-14: Consider refactoring to a module with standalone functions.The class contains only static members. Consider using simple functions or a module instead.
-export class UIUtils { - private static _renderQueue: RenderQueue; - private static _virtualCamera: VirtualCamera; - private static _viewport: Vector4; +let _renderQueue: RenderQueue; +let _virtualCamera: VirtualCamera; +let _viewport: Vector4; + +export function renderOverlay(engine: Engine, uiCanvases: DisorderedArray<IUICanvas>): void { // Implementation }🧰 Tools
🪛 Biome (1.9.4)
[error] 10-42: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
22-22: Add validation for matrix operations.Matrix operations could throw errors if the values are invalid (NaN, Infinity).
+ if (!Number.isFinite(canvas.width) || !Number.isFinite(canvas.height)) { + throw new Error("Invalid canvas dimensions"); + } (projectE[0] = 2 / canvas.width), (projectE[5] = 2 / canvas.height), (projectE[10] = 0); + if (!Number.isFinite(transform.position.x) || !Number.isFinite(transform.position.y)) { + throw new Error("Invalid transform position"); + } (viewE[12] = -transform.position.x), (viewE[13] = -transform.position.y);Also applies to: 28-28
24-40: Add error handling for render operations.The render loop should handle potential errors to prevent crashes.
for (let i = 0, n = uiCanvases.length; i < n; i++) { const uiCanvas = uiCanvases.get(i); if (uiCanvas) { + try { const transform = uiCanvas.entity.transform; // ... rendering logic ... uiRenderQueue.render(renderContext, "Forward"); + } catch (error) { + console.error(`Failed to render UI canvas at index ${i}:`, error); + // Optionally: continue rendering other canvases + } } }packages/core/src/input/pointer/Pointer.ts (1)
40-43: Add explicit access modifiers to internal properties.Consider marking internal properties as
privateorprotectedfor better encapsulation.-/** @internal */ -_frameEvents: PointerEventType = PointerEventType.None; -/** @internal */ -_emitters: PointerEventEmitter[] = []; +/** @internal */ +private _frameEvents: PointerEventType = PointerEventType.None; +/** @internal */ +private _emitters: PointerEventEmitter[] = [];packages/core/src/2d/assembler/SimpleSpriteAssembler.ts (1)
11-12: Consider making_matrixan instance property for thread safety.The static
_matrixproperty could lead to race conditions if multiple sprite assemblers are processing concurrently. Consider making it an instance property to ensure thread safety.- private static _matrix = new Matrix(); + private readonly _matrix = new Matrix();packages/ui/src/Utils.ts (2)
92-93: Avoid assignments in expressions.The code uses assignments within expressions, which can be confusing and harder to maintain.
Separate the assignments for better readability:
// Instead of const replaced = preCanvas._disorderedElements.deleteByIndex(element._indexInRootCanvas); replaced && (replaced._indexInRootCanvas = element._indexInRootCanvas); // Use const replaced = preCanvas._disorderedElements.deleteByIndex(element._indexInRootCanvas); if (replaced) { replaced._indexInRootCanvas = element._indexInRootCanvas; }Also applies to: 109-110
🧰 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)
123-143: Add error handling for entity registration.The
_registerListenermethod lacks error handling for invalid entities or listeners.Add validation and error handling:
private static _registerListener( entity: Entity, root: Entity, listener: (flag: number, param?: any) => void, listeningEntities: Entity[] ): void { if (!listener) { throw new Error("Listener cannot be null or undefined"); } let count = 0; while (entity && entity !== root && count < MAX_ENTITY_DEPTH) { const preEntity = listeningEntities[count]; if (preEntity !== entity) { preEntity?._unRegisterModifyListener(listener); listeningEntities[count] = entity; entity._registerModifyListener(listener); } entity = entity.parent; count++; } listeningEntities.length = count; }packages/core/src/Script.ts (2)
123-173: Improve pointer event documentation.The pointer event method documentation could be more descriptive and consistent.
Add more detailed documentation:
/** * Called when the pointer is down while over the ColliderShape. * @param eventData - The pointer event data containing: * - position: The current pointer position * - button: The button that was pressed * - pressure: The pressure of the pointer */ onPointerDown(eventData: PointerEventData): void {}
225-229: Consider using a more efficient iteration method.Using
Object.values()on an enum can be inefficient. Consider using a predefined array of methods.private static readonly POINTER_METHODS = [ "onPointerDown", "onPointerUp", "onPointerClick", // ... other methods ] as const; // In _onEnableInScene: for (const method of Script.POINTER_METHODS) { if (this[method] === prototype[method]) { this[method] = null; } }packages/ui/src/component/interactive/UIInteractive.ts (1)
45-51: Consider using a state machine for interactive states.The current state management using boolean flags could be improved using a proper state machine pattern.
enum InteractiveStateFlags { None = 0, PointerInside = 1 << 0, PointerDragging = 1 << 1 } class InteractiveStateMachine { private _flags: InteractiveStateFlags = InteractiveStateFlags.None; setFlag(flag: InteractiveStateFlags): void { this._flags |= flag; } clearFlag(flag: InteractiveStateFlags): void { this._flags &= ~flag; } hasFlag(flag: InteractiveStateFlags): boolean { return (this._flags & flag) !== 0; } }tests/src/core/SpriteMask.test.ts (2)
229-238: Document the relationship between update flags.The
SpriteMaskUpdateFlagsenum extendsRendererUpdateFlagsbut this relationship isn't clearly documented.Add more detailed documentation:
/** * Defines update flags specific to SpriteMask, extending the base RendererUpdateFlags. * @remarks * - Extends `RendererUpdateFlags` by adding sprite-specific flags * - UV (0x2): Indicates UV coordinates need updating * - AutomaticSize (0x4): Indicates automatic size calculation is needed * - WorldVolumeAndUV (0x3): Combined flag for both world volume and UV updates * - All (0x7): Indicates all aspects need updating */ enum SpriteMaskUpdateFlags { // ... existing implementation }
121-148: Improve test readability with helper functions.The dirty flag tests are repetitive and could be made more readable with helper functions.
function testUpdateFlag(spriteMask: SpriteMask, flag: number, action: () => void): void { spriteMask._dirtyUpdateFlag &= ~flag; action(); expect(!!(spriteMask._dirtyUpdateFlag & flag)).to.eq(true); } // Usage: testUpdateFlag(spriteMask, RendererUpdateFlags.WorldVolume, () => { sprite.width = 10; }); testUpdateFlag(spriteMask, SpriteMaskUpdateFlags.WorldVolumeAndUV, () => { sprite.region = new Rect(); });packages/ui/src/component/UIRenderer.ts (3)
158-164: Consider adding explicit ordering mechanism for drag processing.While the comment indicates that drag must be processed first, there's no explicit mechanism to ensure this ordering is maintained. Future modifications might accidentally break this requirement.
Consider introducing a priority queue or explicit event ordering system to enforce the processing sequence.
165-191: Add error handling for event processing.The event processing loop lacks try-catch blocks, which could lead to unhandled exceptions if an event handler fails.
Add error handling to make the code more robust:
for (let j = 0; j < length; j++) { + try { const event = events[j]; pointer.button = _pointerDec2BinMap[event.button] || PointerButton.None; pointer.pressedButtons = event.buttons; switch (event.type) { // ... existing cases ... } + } catch (error) { + console.error('Error processing pointer event:', error); + // Consider adding telemetry or error reporting here + } }
Line range hint
306-313: Enhance decorator documentation and consider thread safety.The decorator implementation could benefit from:
- More detailed documentation about usage and examples
- Thread safety considerations when modifying the static array
Consider adding comprehensive documentation and implementing thread-safe registration:
/** * Declare pointer event emitter decorator. + * @example + * ```typescript + * @registerPointerEventEmitter() + * class CustomPointerEventEmitter extends PointerEventEmitter { + * // Implementation + * } + * ``` + * @remarks + * This decorator registers a pointer event emitter class to be used with pointers. + * The registered emitter will be instantiated for each pointer instance. */ export function registerPointerEventEmitter() { return <T extends PointerEventEmitter>(Target: { new (pool: ClearableObjectPool<PointerEventData>): T }) => { + // Consider using a thread-safe collection if needed PointerManager._pointerEventEmitters.push(Target); }; }🧰 Tools
🪛 Biome (1.9.4)
[error] 205-207: This case is falling through to the next case.
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.(lint/suspicious/noFallthroughSwitchClause)
packages/core/src/2d/assembler/TiledSpriteAssembler.ts (1)
244-251: Enhance vertex count limit handling.The warning message could be more informative, and the fallback behavior should be better documented.
Improve error handling and documentation:
if (vertexCount > maxVertexCount) { + const message = `Vertex count (${vertexCount}) exceeds the limit (${maxVertexCount}). ` + + 'Falling back to simple quad rendering. Consider:' + + '\n1. Reducing tile size' + + '\n2. Using a different draw mode' + + '\n3. Splitting the sprite into smaller parts'; + Logger.warn(message); - Logger.warn(`The number of vertices exceeds the upper limit(${maxVertexCount}).`); rPos.add(width * left), rPos.add(width * right); cPos.add(height * bottom), cPos.add(height * top); rUV.add(spriteUV0.x), rUV.add(spriteUV3.x); cUV.add(spriteUV0.y), cUV.add(spriteUV3.y); return 4; }packages/ui/src/component/UICanvas.ts (4)
187-189: Refactor assignment in logical expressionThe assignment within a logical expression reduces readability. Consider using an explicit if statement.
- this._realRenderMode === CanvasRenderMode.ScreenSpaceOverlay && - (this.scene._componentsManager._overlayCanvasesSortingFlag = true); + if (this._realRenderMode === CanvasRenderMode.ScreenSpaceOverlay) { + // @ts-ignore + this.scene._componentsManager._overlayCanvasesSortingFlag = true; + }🧰 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)
598-618: Document intentional switch case fall-throughThe switch cases are falling through without breaks. While this appears intentional for combining initialization steps, it should be documented.
switch (curRealMode) { case CanvasRenderMode.ScreenSpaceOverlay: this._addCanvasListener(); + // Intentional fall-through to initialize screen space case CanvasRenderMode.ScreenSpaceCamera: this._adapterPoseInScreenSpace(); this._adapterSizeInScreenSpace(); + // Intentional fall-through to add canvas case CanvasRenderMode.WorldSpace: componentsManager.addUICanvas(this, curRealMode === CanvasRenderMode.ScreenSpaceOverlay); 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)
262-262: Refactor assignment in expressionThe assignment within the expression makes the code harder to read and maintain.
- const renderElement = (this._renderElement = engine._renderElementPool.get()); + this._renderElement = engine._renderElementPool.get(); + const renderElement = this._renderElement;🧰 Tools
🪛 Biome (1.9.4)
[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)
468-468: Refactor assignment in logical expressionThe assignment within a logical expression reduces readability.
- child.isActive && (depth = this._walk(child, renderers, depth, group)); + if (child.isActive) { + depth = this._walk(child, renderers, depth, group); + }🧰 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)
tests/src/core/SpriteRenderer.test.ts (4)
902-908: Enhance edge case testing for sprite sizesWhile the test covers extreme sprite sizes, consider adding assertions to verify:
- No loss of precision in vertex positions for very small sizes
- Proper handling of floating-point arithmetic
- Correct aspect ratio preservation
198-198: Enhance render pipeline mockingThe current mock for render pipeline is minimal and doesn't verify if render elements are correctly pushed. Consider adding spy/mock assertions to validate the rendering pipeline interactions.
const pushRenderElementSpy = vi.fn(); const context = { camera: { engine: engine, _renderPipeline: { pushRenderElement: pushRenderElementSpy } } }; // After render call expect(pushRenderElementSpy).toHaveBeenCalled();
1583-1600: Improve enum documentationWhile the enum is well-structured, consider adding:
- Documentation for combined flags explaining when they should be used
- Examples of typical usage scenarios
- Clear explanation of how it extends
RendererUpdateFlags/** * Extends `RendererUpdateFlags` to provide specialized update flags for sprite rendering. */ enum SpriteRendererUpdateFlags { /** Update UV coordinates only */ UV = 0x2, /** Update vertex colors only */ Color = 0x4, /** Update size based on sprite's natural dimensions */ AutomaticSize = 0x8, /** * Update both world volume and UV coordinates. * Typically used when sprite's transform or texture region changes. */ WorldVolumeAndUV = 0x3, /** * Update world volume, UV coordinates and colors. * Typically used during full sprite refresh. */ WorldVolumeUVAndColor = 0x7, /** Update all sprite renderer properties */ All = 0xf }
1554-1555: Enhance bounds testingConsider adding test cases for:
- Rotated sprites
- Scaled sprites
- Nested sprite hierarchies
- Bounds after sprite modifications
packages/core/src/input/pointer/emitter/PhysicsPointerEventEmitter.ts (4)
60-60: Avoid multiple assignments in expressionsThe multiple assignments in a single expression make the code harder to read and maintain.
-const entity = (this._pressedEntity = this._draggedEntity = this._enteredEntity); +const entity = this._enteredEntity; +this._pressedEntity = entity; +this._draggedEntity = entity;🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: 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] 60-60: 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)
26-26: Consider using a private field for internal stateThe
_componentsManageris accessed directly, which might violate encapsulation.Consider using a getter method or properly exposing this through the Scene class API.
40-48: Add early return for optimizationThe nested conditions can be flattened for better readability and performance.
camera.screenPointToRay(pointer.position, ray); +if (!scenePhysics) return; if (scenePhysics.raycast(ray, camera.farClipPlane, camera.cullingMask, <HitResult>hitResult)) { this._updateRaycast(hitResult.entity, pointer); return; }
107-118: Consider extracting path composition logicThe path composition logic in
_updateRaycastis duplicated in multiple places.Consider extracting this into a reusable helper method to improve maintainability.
packages/core/src/2d/assembler/SlicedSpriteAssembler.ts (1)
65-69: Simplify complex assignment expressionsMultiple assignments in single expressions reduce readability.
- (row[0] = expectWidth * left * widthScale), (row[1] = row[2] = fixedLeft * widthScale); - row[3] = width - expectWidth * (1 - right) * widthScale; + row[0] = expectWidth * left * widthScale; + row[1] = fixedLeft * widthScale; + row[2] = fixedLeft * widthScale; + row[3] = width - expectWidth * (1 - right) * widthScale;Also applies to: 74-75
packages/ui/src/input/UIPointerEventEmitter.ts (1)
20-20: Consider making_MAX_PATH_DEPTHconfigurableThe hard-coded path depth limit might not be suitable for all use cases.
Consider making this configurable through a static setter or configuration object.
packages/core/src/input/pointer/PointerManager.ts (1)
296-302: Consider using Array.prototype.fill for bulk clearingMultiple array length assignments could be optimized.
- this._nativeEvents.length = 0; - this._pointers.length = 0; - this._downList.length = 0; - this._downMap.length = 0; - this._upList.length = 0; - this._upMap.length = 0; + [ + this._nativeEvents, + this._pointers, + this._downList, + this._downMap, + this._upList, + this._upMap + ].forEach(arr => arr.length = 0);
📜 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 (46)
packages/core/src/2d/assembler/ISpriteAssembler.ts(1 hunks)packages/core/src/2d/assembler/SimpleSpriteAssembler.ts(4 hunks)packages/core/src/2d/assembler/SlicedSpriteAssembler.ts(5 hunks)packages/core/src/2d/assembler/TiledSpriteAssembler.ts(7 hunks)packages/core/src/2d/sprite/Sprite.ts(6 hunks)packages/core/src/2d/sprite/SpriteMask.ts(5 hunks)packages/core/src/2d/sprite/SpriteRenderer.ts(13 hunks)packages/core/src/2d/text/TextRenderer.ts(3 hunks)packages/core/src/Camera.ts(12 hunks)packages/core/src/Engine.ts(4 hunks)packages/core/src/Entity.ts(8 hunks)packages/core/src/Script.ts(5 hunks)packages/core/src/input/index.ts(1 hunks)packages/core/src/input/pointer/Pointer.ts(2 hunks)packages/core/src/input/pointer/PointerEventData.ts(1 hunks)packages/core/src/input/pointer/PointerManager.ts(7 hunks)packages/core/src/input/pointer/emitter/IHitResult.ts(1 hunks)packages/core/src/input/pointer/emitter/PhysicsPointerEventEmitter.ts(1 hunks)packages/core/src/input/pointer/emitter/PointerEventEmitter.ts(1 hunks)packages/core/src/mesh/MeshRenderer.ts(2 hunks)packages/core/src/physics/HitResult.ts(1 hunks)packages/core/src/physics/PhysicsScene.ts(1 hunks)packages/core/src/ui/UIUtils.ts(1 hunks)packages/loader/src/SpriteAtlasLoader.ts(1 hunks)packages/loader/src/SpriteLoader.ts(1 hunks)packages/math/src/BoundingBox.ts(2 hunks)packages/ui/src/Utils.ts(1 hunks)packages/ui/src/component/UICanvas.ts(1 hunks)packages/ui/src/component/UIGroup.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/component/interactive/UIInteractive.ts(1 hunks)packages/ui/src/enums/ResolutionAdaptationMode.ts(1 hunks)packages/ui/src/index.ts(1 hunks)packages/ui/src/input/UIHitResult.ts(1 hunks)packages/ui/src/input/UIPointerEventEmitter.ts(1 hunks)packages/ui/src/interface/IElement.ts(1 hunks)packages/ui/src/interface/IGraphics.ts(1 hunks)tests/package.json(1 hunks)tests/src/core/SpriteMask.test.ts(4 hunks)tests/src/core/SpriteRenderer.test.ts(25 hunks)tests/src/ui/Text.test.ts(1 hunks)tests/src/ui/UICanvas.test.ts(1 hunks)tests/src/ui/UIGroup.test.ts(1 hunks)tests/src/ui/UIInteractive.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/core/src/physics/PhysicsScene.ts
- packages/core/src/physics/HitResult.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/package.json
- packages/core/src/input/index.ts
- tests/src/ui/UIGroup.test.ts
- tests/src/ui/UICanvas.test.ts
- tests/src/ui/UIInteractive.test.ts
- packages/core/src/mesh/MeshRenderer.ts
- packages/core/src/2d/sprite/SpriteMask.ts
- packages/core/src/2d/sprite/Sprite.ts
- packages/ui/src/component/UIGroup.ts
- packages/ui/src/interface/IElement.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/loader/src/SpriteLoader.ts
[error] 51-51: 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] 52-52: 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] 60-60: 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] 61-61: 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/core/src/ui/UIUtils.ts
[error] 10-42: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 16-16: 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] 16-16: 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: 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] 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] 18-18: 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] 18-18: 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/ui/src/input/UIPointerEventEmitter.ts
[error] 104-104: 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] 104-104: 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] 196-196: 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/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] 508-508: 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] 519-519: 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] 521-521: 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] 607-607: 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] 609-609: 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/core/src/Camera.ts
[error] 741-741: 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/core/src/Entity.ts
[error] 632-632: 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/core/src/2d/assembler/TiledSpriteAssembler.ts
[error] 67-67: 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] 68-68: 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] 69-69: 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] 256-256: 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] 268-268: 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] 269-269: 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] 270-270: 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] 298-298: 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] 299-299: 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] 300-300: 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)
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)
packages/ui/src/component/UIRenderer.ts
[error] 205-207: 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/input/pointer/emitter/PhysicsPointerEventEmitter.ts
[error] 60-60: 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] 60-60: 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/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)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x, windows-latest)
🔇 Additional comments (35)
packages/math/src/BoundingBox.ts (1)
130-134: Improved precision of NaN checks in getCenter.The change from
isNaNtoNumber.isNaNenhances type safety and provides more predictable behavior.Number.isNaNis more precise as it only returns true for the actualNaNvalue, whileisNaNwould coerce non-numbers before testing.Let's verify the impact of this change on the codebase:
✅ Verification successful
Verification successful: Codebase consistently uses Number.isNaN
The codebase already follows the best practice of using
Number.isNaNconsistently across all TypeScript files, with no instances of the less preciseisNaNfound.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances where isNaN might need similar updates rg '\bisNaN\s*\(' --type tsLength of output: 946
packages/ui/src/component/advanced/Text.ts (1)
609-609: 🛠️ Refactor suggestionAvoid assignments within expressions for clarity
Using assignments within expressions can be unclear. Consider refactoring to improve code readability.
Apply this diff:
- charRenderInfos.length = 0; + charRenderInfos = [];Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 609-609: 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/core/src/input/pointer/emitter/IHitResult.ts (1)
7-12: InterfaceIHitResultis well-definedThe
IHitResultinterface is clearly defined with appropriate properties, enhancing type safety for hit result data.packages/ui/src/input/UIHitResult.ts (1)
7-13: ClassUIHitResultinitializationThe
UIHitResultclass is properly initialized with default values, ensuring reliable usage in UI interactions.packages/ui/src/interface/IGraphics.ts (1)
6-7: Consider adding bounds validation for raycastPadding.The Vector4 padding could benefit from runtime validation to ensure non-negative values.
packages/core/src/input/pointer/PointerEventData.ts (1)
8-13: LGTM! Well-structured class implementing the pooling pattern.The class follows good practices by implementing the
IPoolElementinterface and providing clear property definitions.packages/core/src/2d/assembler/ISpriteAssembler.ts (2)
8-8: LGTM! Clear method signature forresetData.The method signature is well-defined with optional vertexCount parameter.
19-20: LGTM! Clear method signatures for UV and color updates.The methods have focused responsibilities and clear parameters.
packages/loader/src/SpriteLoader.ts (1)
60-61: Similar readability improvement needed here.🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: 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] 61-61: 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/core/src/ui/UIUtils.ts (1)
16-18: 🛠️ Refactor suggestionInitialize static fields eagerly to prevent race conditions.
Lazy initialization of static fields can lead to race conditions in concurrent scenarios.
-private static _renderQueue: RenderQueue; -private static _virtualCamera: VirtualCamera; -private static _viewport: Vector4; +private static _renderQueue: RenderQueue = new RenderQueue(RenderQueueType.Transparent); +private static _virtualCamera: VirtualCamera = new VirtualCamera(); +private static _viewport: Vector4 = new Vector4(0, 0, 1, 1);Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 16-16: 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] 16-16: 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: 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] 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] 18-18: 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] 18-18: 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/core/src/input/pointer/Pointer.ts (3)
82-88: LGTM! Well-structured enum for pointer event types.The enum values are clearly defined and follow a logical bit flag pattern.
65-67: 🛠️ Refactor suggestionReset all maps in
_resetOnFrameBegin.
_upMapand_downMapare not being reset, which could lead to data carryover between frames._resetOnFrameBegin(): void { this._frameEvents = PointerEventType.None; this._events.length = this._upList.length = this._downList.length = 0; + this._upMap.length = this._downMap.length = 0; }Likely invalid or redundant comment.
73-78: 🛠️ Refactor suggestionClear
_emittersarray after disposing to prevent memory leaks.After disposing of each emitter, clear the array to allow for garbage collection.
_dispose(): void { const emitters = this._emitters; for (let i = 0, n = emitters.length; i < n; i++) { emitters[i].dispose(); } + this._emitters.length = 0; this._events.length = this._upList.length = this._downList.length = 0; }Likely invalid or redundant comment.
packages/core/src/2d/assembler/SimpleSpriteAssembler.ts (2)
85-94: 🛠️ Refactor suggestionAdd alpha parameter validation.
The alpha parameter should be validated to ensure it's within the valid range [0, 1].
static updateColor(renderer: ISpriteRenderer, alpha: number): void { + if (alpha < 0 || alpha > 1) { + throw new Error("Alpha value must be between 0 and 1"); + } const subChunk = renderer._subChunk; const { r, g, b, a } = renderer.color; const finalAlpha = a * alpha;Likely invalid or redundant comment.
39-40: 🛠️ Refactor suggestionOptimize flip calculations using negation.
Based on performance testing from past discussions, using negation (
-) is more efficient than the ternary operator for flip calculations.- const sx = flipX ? -width : width; - const sy = flipY ? -height : height; + const sx = flipX ? -width : width; + const sy = flipY ? -height : height;Likely invalid or redundant comment.
packages/ui/src/index.ts (3)
66-71: 🛠️ Refactor suggestionImprove type safety and prevent infinite recursion in hierarchy updates.
- Remove @ts-ignore by properly typing the parent property
- Add recursion depth limit to prevent stack overflow
-_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); } }Likely invalid or redundant comment.
105-112: 🛠️ Refactor suggestionAdd error handling and improve type safety in font creation.
The font creation process lacks error handling and proper typing.
-// @ts-ignore -instance.font = Font.createFromOS(instance.engine, props.fontFamily || "Arial"); +try { + instance.font = await Font.createFromOS( + instance.engine as Engine, + props.fontFamily || "Arial" + ); +} catch (error) { + console.error(`Failed to create font: ${error}`); + throw error; +}Likely invalid or redundant comment.
44-58: 🛠️ Refactor suggestionImprove type safety and document blend state configuration.
- Remove @ts-ignore by properly typing the Material constructor parameter
- Add documentation for the blend state configuration
+/** + * Creates a default UI material with alpha blending configuration. + * - Source color blend factor: SourceAlpha + * - Destination color blend factor: OneMinusSourceAlpha + * - Alpha blend operation: Add + * - Depth write: Disabled + * - Cull mode: Off + */ -// @ts-ignore -const material = new Material(this, shader); +const material = new Material(this as unknown as Engine, shader);Likely invalid or redundant comment.
packages/ui/src/component/UIRenderer.ts (1)
204-213:⚠️ Potential issueAdd break statement to prevent unintended fallthrough.
The
EntityModifyFlags.Parentcase falls through toEntityModifyFlags.SiblingIndexwithout a break statement. This could lead to unintended behavior where both cases are executed sequentially.Apply this diff to fix the fallthrough:
case EntityModifyFlags.Parent: Utils.setRootCanvasDirty(this); Utils.setGroupDirty(this); + break; case EntityModifyFlags.SiblingIndex: entity._updateUIHierarchyVersion(this.engine.time.frameCount); break;Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 205-207: This case is falling through to the next case.
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.(lint/suspicious/noFallthroughSwitchClause)
packages/ui/src/component/advanced/Image.ts (2)
187-190: 🛠️ Refactor suggestionImprove material handling for destroyed materials.
The code silently falls back to default material when the material is destroyed. This could hide potential issues in material lifecycle management.
Consider logging warnings and implementing proper material lifecycle management:
- // @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(); + // Update the reference to prevent future warnings + this.setMaterial(material); + }Likely invalid or redundant comment.
112-113: 🛠️ Refactor suggestionRemove @ts-ignore and improve type safety.
The code uses @ts-ignore to bypass type checking for
_updateFlagManager. This could hide potential type safety issues.Create proper interfaces to handle the type checking:
+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);Likely invalid or redundant comment.
packages/core/src/Engine.ts (3)
42-42: LGTM!The addition of the
UIUtilsimport is appropriate for the new UI rendering functionality.
Line range hint
572-605: LGTM: UI canvas rendering implementationThe implementation correctly handles UI canvas rendering by:
- Checking for active cameras
- Sorting overlay UI canvases
- Using UIUtils for rendering
359-359: Verify the impact of removing physics initialization checkThe removal of the
_physicsInitializedcondition check for firing pointer events could lead to runtime errors if pointer events depend on physics components.Run this script to verify physics dependencies in pointer event handling:
packages/core/src/2d/text/TextRenderer.ts (2)
508-519: LGTM: Comprehensive text measurement implementationThe text measurement implementation correctly handles:
- Pixel per unit conversion
- Text wrapping with width/height constraints
- Line spacing adjustments
653-653: LGTM: Transform change handlingThe transform change handler correctly updates world position and bounds flags.
packages/core/src/input/pointer/emitter/PointerEventEmitter.ts (1)
11-11:⚠️ Potential issueConsider making
_tempRayinstance-based instead of staticUsing a static
Rayinstance could lead to race conditions in concurrent scenarios.-protected static _tempRay: Ray = new Ray(); +protected _tempRay: Ray = new Ray();Likely invalid or redundant comment.
packages/core/src/input/pointer/PointerManager.ts (1)
158-164: 🛠️ Refactor suggestionAdd error handling for event processing
Event processing lacks try-catch blocks which could lead to unhandled exceptions.
if (pointer._frameEvents & PointerEventType.Move) { pointer.phase = PointerPhase.Move; for (let k = 0; k < emittersLength; k++) { + try { emitters[k].processDrag(pointer); + } catch (error) { + console.error('Error processing drag event:', error); + } } }Likely invalid or redundant comment.
packages/core/src/Entity.ts (3)
15-15: LGTM! Well-structured modification flag system.The addition of
EntityModifyFlagsand_modifyFlagManagerestablishes a clean pattern for tracking entity modifications.Also applies to: 106-106
623-626: LGTM! Clean parent change handling.The method properly handles parent changes by:
- Updating the transform
- Dispatching the appropriate modification flag
788-788: LGTM! Proper sibling index change notification.The addition of
EntityModifyFlags.SiblingIndexdispatch ensures proper notification of sibling order changes.packages/core/src/Camera.ts (4)
15-15: LGTM! Consistent modification flag pattern.The implementation follows the same pattern as Entity.ts, maintaining consistency across the codebase.
Also applies to: 135-136
226-230: LGTM! Well-structured property change handling.The setters consistently:
- Check for value changes
- Update internal state
- Trigger necessary recalculations
- Dispatch appropriate modification flags
Also applies to: 245-245, 295-304, 316-320
715-715: LGTM! Proper scene lifecycle notifications.The addition of scene-related modification flags ensures proper notification of camera's scene lifecycle changes.
Also applies to: 723-723
765-765: LGTM! Proper cleanup and dispatch implementation.The implementation ensures:
- Proper cleanup of resources in _onDestroy
- Consistent pattern for dispatching modifications
Also applies to: 877-879
| const top = (startY + ascent + offsetHeight) * pixelsPerUnitReciprocal; | ||
| const bottom = (startY - descent + offsetHeight) * pixelsPerUnitReciprocal; | ||
| localPositions.set(left, top, right, bottom); | ||
| i === firstLine && (maxY = Math.max(maxY, top)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid assignments within expressions for clarity
Using assignments within expressions can make the code less readable. Refactor to an explicit if statement.
Apply this diff:
- i === firstLine && (maxY = Math.max(maxY, top));
+ if (i === firstLine) {
+ maxY = Math.max(maxY, top);
+ }📝 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.
| i === firstLine && (maxY = Math.max(maxY, top)); | |
| if (i === firstLine) { | |
| maxY = Math.max(maxY, top); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 519-519: 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)
| localPositions.set(left, top, right, bottom); | ||
| i === firstLine && (maxY = Math.max(maxY, top)); | ||
| minY = Math.min(minY, bottom); | ||
| j === firstRow && (minX = Math.min(minX, left)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid assignments within expressions for clarity
Assignments within expressions can lead to confusing code. Use explicit conditional statements instead.
Apply this diff:
- j === firstRow && (minX = Math.min(minX, left));
+ if (j === firstRow) {
+ minX = Math.min(minX, left);
+ }📝 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.
| j === firstRow && (minX = Math.min(minX, left)); | |
| if (j === firstRow) { | |
| minX = Math.min(minX, left); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 521-521: 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)
| const char = line[j]; | ||
| const charInfo = charFont._getCharInfo(char); | ||
| if (charInfo.h > 0) { | ||
| firstRow < 0 && (firstRow = j); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid assignments within expressions for clarity
Assignments within expressions can be confusing and reduce code readability. It's better to use explicit conditional statements.
Apply this diff:
- firstRow < 0 && (firstRow = j);
+ if (firstRow < 0) {
+ firstRow = j;
+ }📝 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.
| firstRow < 0 && (firstRow = j); | |
| if (firstRow < 0) { | |
| firstRow = j; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 508-508: 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
Avoid assignments within expressions for clarity
Using assignments within expressions can make code less readable and may introduce confusion. Consider refactoring to improve clarity.
Apply this diff to refactor the code:
- this._subFont && (this._subFont = null);
+ if (this._subFont) {
+ this._subFont = null;
+ }📝 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._subFont && (this._subFont = null); | |
| if (this._subFont) { | |
| this._subFont = null; | |
| } |
🧰 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)
| const charInfo = charFont._getCharInfo(char); | ||
| if (charInfo.h > 0) { | ||
| firstRow < 0 && (firstRow = j); | ||
| const charRenderInfo = (charRenderInfos[renderElementCount++] = charRenderInfoPool.get()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid complex assignments within expressions
Assigning and incrementing within the same expression can be hard to read and maintain. Consider splitting the statements for clarity.
Apply this diff:
- const charRenderInfo = (charRenderInfos[renderElementCount++] = charRenderInfoPool.get());
+ const newRenderInfo = charRenderInfoPool.get();
+ charRenderInfos[renderElementCount] = newRenderInfo;
+ const charRenderInfo = newRenderInfo;
+ renderElementCount++;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const charRenderInfo = (charRenderInfos[renderElementCount++] = charRenderInfoPool.get()); | |
| const newRenderInfo = charRenderInfoPool.get(); | |
| charRenderInfos[renderElementCount] = newRenderInfo; | |
| const charRenderInfo = newRenderInfo; | |
| renderElementCount++; |
🧰 Tools
🪛 Biome (1.9.4)
[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)
| const scale = width / fixedLR; | ||
| posRow.add(expectWidth * left * scale), posRow.add(fixedL * scale); | ||
| posRow.add(width - expectWidth * (1 - right) * scale); | ||
| uvRow.add(spriteUV0.x), uvRow.add(spriteUV1.x), uvRow.add(spriteUV2.x), uvRow.add(spriteUV3.x); | ||
| rowCount += 4; | ||
| rPos.add(expectWidth * left * scale), rPos.add(fixedL * scale); | ||
| rPos.add(width - expectWidth * (1 - right) * scale); | ||
| rUV.add(spriteUV0.x), rUV.add(spriteUV1.x), rUV.add(spriteUV2.x), rUV.add(spriteUV3.x); | ||
| break; | ||
| case TiledType.WithoutTiled: | ||
| posRow.add(expectWidth * left), posRow.add(fixedL), posRow.add(width - fixedR); | ||
| posRow.add(width - expectWidth * (1 - right)); | ||
| uvRow.add(spriteUV0.x), uvRow.add(spriteUV1.x), uvRow.add(NaN), uvRow.add(NaN); | ||
| uvRow.add(spriteUV2.x), uvRow.add(spriteUV3.x); | ||
| rowCount += 4; | ||
| rPos.add(expectWidth * left), rPos.add(fixedL), rPos.add(width - fixedR); | ||
| rPos.add(width - expectWidth * (1 - right)); | ||
| rUV.add(spriteUV0.x), rUV.add(spriteUV1.x), rUV.add(NaN), rUV.add(NaN); | ||
| rUV.add(spriteUV2.x), rUV.add(spriteUV3.x); | ||
| break; | ||
| case TiledType.WithTiled: | ||
| posRow.add(expectWidth * left), posRow.add(fixedL); | ||
| uvRow.add(spriteUV0.x), uvRow.add(spriteUV1.x), uvRow.add(spriteUV1.x); | ||
| rowCount += 3; | ||
| const countInteger = rRepeatCount | 0; | ||
| for (let i = 0; i < countInteger; i++) { | ||
| posRow.add(fixedL + (i + 1) * fixedCW); | ||
| uvRow.add(spriteUV2.x), uvRow.add(spriteUV1.x); | ||
| rowCount += 2; | ||
| const uv1 = spriteUV1.x; | ||
| const uv2 = spriteUV2.x; | ||
| const repeatWidth = (width - fixedLR) / rTiledCount; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix variable scoping in switch-case blocks.
Variables declared in case blocks can be accessed by other cases, which could lead to unintended behavior.
Wrap case blocks with curly braces to properly scope the variables:
switch (rType) {
case TiledType.Compressed: {
const scale = width / fixedLR;
rPos.add(expectWidth * left * scale), rPos.add(fixedL * scale);
rPos.add(width - expectWidth * (1 - right) * scale);
rUV.add(spriteUV0.x), rUV.add(spriteUV1.x), rUV.add(spriteUV2.x), rUV.add(spriteUV3.x);
break;
+ }
case TiledType.WithTiled: {
const uv1 = spriteUV1.x;
const uv2 = spriteUV2.x;
const repeatWidth = (width - fixedLR) / rTiledCount;
// ... rest of the code
break;
+ }Also applies to: 298-300
🧰 Tools
🪛 Biome (1.9.4)
[error] 256-256: 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] 268-268: 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] 269-269: 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] 270-270: 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)
| if (this._customWidth === undefined || this._customHeight === undefined) { | ||
| this._dirtyUpdateFlag |= RendererUpdateFlags.WorldVolume; | ||
| } else if (drawMode === SpriteDrawMode.Tiled) { | ||
| this._dirtyUpdateFlag |= SpriteRendererUpdateFlags.WorldVolumeAndUV; | ||
| } else { | ||
| // When the width and height of `SpriteRenderer` are `undefined`, | ||
| // the `size` of `Sprite` will affect the position of `SpriteRenderer`. | ||
| if (this._customWidth === undefined || this._customHeight === undefined) { | ||
| } | ||
| switch (this._drawMode) { | ||
| case SpriteDrawMode.Simple: | ||
| // When the width and height of `SpriteRenderer` are `undefined`, | ||
| // the `size` of `Sprite` will affect the position of `SpriteRenderer`. | ||
| if (this._customWidth === undefined || this._customHeight === undefined) { | ||
| this._dirtyUpdateFlag |= RendererUpdateFlags.WorldVolume; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate condition check.
The condition this._customWidth === undefined || this._customHeight === undefined is checked twice, which is redundant.
Refactor to avoid code duplication:
- if (this._customWidth === undefined || this._customHeight === undefined) {
- this._dirtyUpdateFlag |= RendererUpdateFlags.WorldVolume;
- }
switch (this._drawMode) {
case SpriteDrawMode.Simple:
- // When the width and height of `SpriteRenderer` are `undefined`,
- // the `size` of `Sprite` will affect the position of `SpriteRenderer`.
- if (this._customWidth === undefined || this._customHeight === undefined) {
- this._dirtyUpdateFlag |= RendererUpdateFlags.WorldVolume;
- }
+ // Update position when using sprite's default size
+ if (this._customWidth === undefined || this._customHeight === undefined) {
+ this._dirtyUpdateFlag |= RendererUpdateFlags.WorldVolume;
+ }
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.
| if (this._customWidth === undefined || this._customHeight === undefined) { | |
| this._dirtyUpdateFlag |= RendererUpdateFlags.WorldVolume; | |
| } else if (drawMode === SpriteDrawMode.Tiled) { | |
| this._dirtyUpdateFlag |= SpriteRendererUpdateFlags.WorldVolumeAndUV; | |
| } else { | |
| // When the width and height of `SpriteRenderer` are `undefined`, | |
| // the `size` of `Sprite` will affect the position of `SpriteRenderer`. | |
| if (this._customWidth === undefined || this._customHeight === undefined) { | |
| } | |
| switch (this._drawMode) { | |
| case SpriteDrawMode.Simple: | |
| // When the width and height of `SpriteRenderer` are `undefined`, | |
| // the `size` of `Sprite` will affect the position of `SpriteRenderer`. | |
| if (this._customWidth === undefined || this._customHeight === undefined) { | |
| this._dirtyUpdateFlag |= RendererUpdateFlags.WorldVolume; | |
| } | |
| switch (this._drawMode) { | |
| case SpriteDrawMode.Simple: | |
| // Update position when using sprite's default size | |
| if (this._customWidth === undefined || this._customHeight === undefined) { | |
| this._dirtyUpdateFlag |= RendererUpdateFlags.WorldVolume; | |
| } | |
| break; |
| @@ -10,7 +11,8 @@ import { | |||
| } from "@galacean/engine-core"; | |||
| import { Color, Rect, Vector2, Vector3, Vector4 } from "@galacean/engine-math"; | |||
| import { WebGLEngine } from "@galacean/engine-rhi-webgl"; | |||
| import { describe, beforeEach, expect, it } from "vitest"; | |||
| import { debug } from "console"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused import
The debug import from 'console' is not used in the test file.
-import { debug } from "console";| // @ts-ignore | ||
| entity._scripts.forEach( | ||
| (script: Script) => { | ||
| script.onPointerDown?.(eventData); | ||
| }, | ||
| (script: Script, index: number) => { | ||
| // @ts-ignore | ||
| script._entityScriptsIndex = index; | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in event firing methods
The event firing methods share similar patterns and could be refactored to reduce duplication.
Consider creating a generic event firing method:
protected _fireEvent<T extends keyof Script>(
entity: Entity,
eventData: PointerEventData,
methodName: T
): void {
entity._scripts.forEach(
(script: Script) => {
script[methodName]?.(eventData);
},
(script: Script, index: number) => {
script._entityScriptsIndex = index;
}
);
}Also applies to: 56-66, 69-79, 82-92, 95-105, 108-118, 121-131, 134-144, 147-157
| let entity = (path[0] = element.entity); | ||
| let i = 1; | ||
| const rootEntity = element._getRootCanvas().entity; | ||
| for (; i < UIPointerEventEmitter._MAX_PATH_DEPTH && !!entity && entity !== rootEntity; i++) { |
There was a problem hiding this comment.
Add boundary check for path depth
The path composition loop should check against _MAX_PATH_DEPTH before accessing the array.
-for (; i < UIPointerEventEmitter._MAX_PATH_DEPTH && !!entity && entity !== rootEntity; i++) {
+if (i >= UIPointerEventEmitter._MAX_PATH_DEPTH) {
+ console.warn('Maximum path depth reached');
+ break;
+}
+for (; !!entity && entity !== rootEntity; i++) {Committable suggestion skipped: line range outside the PR's diff.
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. |
Summary by CodeRabbit
Release Notes
New Features
UICanvas,UIGroup,UIRenderer, andUITransformcomponentsButton,Image, andTextPointerEventDatafor better event managementCanvasRenderModefor improved UI canvas rendering optionsImprovements
ResolutionAdaptationModefor better resolution handlingBreaking Changes
Bug Fixes
Performance