Merge origin/dev into origin/main #769
Conversation
Merge origin/main into origin/dev
* fix: 修复播放器尺寸计算逻辑，确保在不同环境下正确获取宽高 * fix: 修复获取播放器尺寸时的可选链逻辑，确保在无窗口对象时不会抛出错误 * chore: 优化代码 --------- Co-authored-by: yiiqii <[email protected]>
fix: composition setVisible and texture load error
chore: add imgui widgets
fix: interact mesh rendering has offset
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on enhancing the functionality and performance of various components. Key changes include an optimized Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (9)
web-packages/imgui-demo/src/widgets/editor-gui-layout.ts (2)
12-12: Make alignWidth configurable.The hardcoded value of 150 for
alignWidthmight not be suitable for all UI scales or DPI settings.Consider making this configurable:
- static alignWidth = 150; + static alignWidth = 150; + static setAlignWidth(width: number) { + EditorGUILayout.alignWidth = Math.max(0, width); + }
1-53: Add error handling for ImGui operations.The current implementation doesn't handle potential errors from ImGui operations. Consider adding try-catch blocks and logging for better error tracking.
Key areas to consider:
- Handle ImGui initialization state
- Validate input parameters
- Add error logging for failed operations
- Consider adding an error callback mechanism for the consuming code
Would you like me to provide an example implementation of these improvements?
🧰 Tools
🪛 Biome (1.9.4)
[error] 11-53: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
web-packages/imgui-demo/src/object-inspectors/vfx-item-inspector.ts (4)
19-19: Consider adding JSDoc comment for defaultComponentEditorAdding documentation for this property would help explain its purpose and usage within the class.
+ /** Default editor used for rendering component properties when no custom editor is available */ private defaultComponentEditor = new Editor();
Line range hint
57-57: Fix typo in variable nameThe variable
componetshould becomponent.- for (const componet of activeObject.components) { + for (const component of activeObject.components) {
Line range hint
81-197: Consider refactoring drawMaterial methodThe method is quite long (>100 lines) and handles multiple responsibilities:
- Material property parsing
- UI rendering
- Drag and drop handling
- Serialization
Consider breaking this down into smaller, focused methods for better maintainability.
Suggested refactor approach:
private drawMaterial(material: Material) { if (!material || !material.shader.shaderData.properties) { return; } const glMaterial = material as GLMaterial; const serializedData = glMaterial.toData(); let dirtyFlag = false; dirtyFlag = this.drawMaterialTags(serializedData) || dirtyFlag; dirtyFlag = this.drawShaderProperties(glMaterial, serializedData) || dirtyFlag; this.applyMaterialChanges(glMaterial, serializedData, dirtyFlag); } private drawMaterialTags(serializedData: any): boolean { // Handle RenderType and RenderFace } private drawShaderProperties(material: GLMaterial, serializedData: any): boolean { // Handle shader properties } private applyMaterialChanges(material: GLMaterial, serializedData: any, dirtyFlag: boolean): void { // Handle serialization and dirty state }
Line range hint
116-127: Optimize shader property parsingThe current implementation parses shader properties on every frame using regular expressions. Consider caching the parsed properties to improve performance.
private cachedProperties = new Map<string, ParsedProperty>(); private parseShaderProperty(property: string): ParsedProperty | null { if (this.cachedProperties.has(property)) { return this.cachedProperties.get(property)!; } // Current parsing logic... const parsed = {/* parsed results */}; this.cachedProperties.set(property, parsed); return parsed; }packages/effects/src/player.ts (2)
362-370: LGTM! Consider adding error handling for texture loading.The texture handling logic is well-structured, supporting both pre-created textures and texture options. However, consider adding error handling for the loadGUID operation.
if (textureOptions instanceof Texture) { engine.addInstance(textureOptions); } else { + try { textureOptions = engine.assetLoader.loadGUID<Texture>(scene.textureOptions[i].id); scene.textureOptions[i] = textureOptions; + } catch (error) { + logger.error(`Failed to load texture: ${error.message}`); + throw error; + } }
906-915: Consider improving variable declaration consistency.The variable declarations could be more consistent and grouped better.
-let targetWidth; -let targetHeight; -let finalWidth = 0; -let finalHeight = 0; +let targetWidth = 0; +let targetHeight = 0; +let finalWidth = 0; +let finalHeight = 0; if (computedStyle) { finalWidth = parseInt(computedStyle.width, 10); finalHeight = parseInt(computedStyle.height, 10); } else { finalWidth = parentEle.clientWidth; finalHeight = parentEle.clientHeight; } if (displayAspect) { const parentAspect = finalWidth / finalHeight;Also applies to: 918-929
web-packages/imgui-demo/src/custom-editors/editor.ts (1)
52-53: Refactor duplicated code into a reusable functionThe drag-and-drop handling logic for updating
component[key]andcomponent.materialis similar. Consider extracting this logic into a reusable function to reduce code duplication and improve maintainability.Here's an example of how you might refactor the code:
Extract the shared logic into a separate asynchronous function:
async function handleDragDrop(payload: ImGui.Payload, component: Component, key: keyof Component) { const file = await (payload.Data as FileNode).getFile(); if (!file) { return; } const effectsPackage = await GalaceanEffects.assetDataBase.loadPackageFile(file); if (!effectsPackage || effectsPackage.exportObjects.length === 0) { return; } component[key] = effectsPackage.exportObjects[0] as Material; }Then, replace the duplicated code with calls to this function.
For the
EffectsObjectcase:- void (payload.Data as FileNode).getFile().then(async (file: File | undefined)=>{ - if (!file) { - return; - } - const effectsPackage = await GalaceanEffects.assetDataBase.loadPackageFile(file); - if (!effectsPackage) { - return; - } - //@ts-expect-error - component[key] = effectsPackage.exportObjects[0] as Material; - }); + await handleDragDrop(payload, component, key);For the
RendererComponentcase:- void (payload.Data as FileNode).getFile().then(async (file: File | undefined)=>{ - if (!file) { - return; - } - const effectsPackage = await GalaceanEffects.assetDataBase.loadPackageFile(file); - if (!effectsPackage) { - return; - } - component.material = effectsPackage.exportObjects[0] as Material; - }); + await handleDragDrop(payload, component, 'material');Also applies to: 78-78
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
packages/effects-core/src/composition.ts(1 hunks)packages/effects-core/src/plugins/interact/interact-item.ts(0 hunks)packages/effects/src/player.ts(2 hunks)web-packages/imgui-demo/src/custom-editors/editor.ts(1 hunks)web-packages/imgui-demo/src/object-inspectors/vfx-item-inspector.ts(3 hunks)web-packages/imgui-demo/src/widgets/editor-gui-layout.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/effects-core/src/plugins/interact/interact-item.ts
🧰 Additional context used
🪛 Biome (1.9.4)
web-packages/imgui-demo/src/widgets/editor-gui-layout.ts
[error] 11-53: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (4)
web-packages/imgui-demo/src/widgets/editor-gui-layout.ts (1)
36-40: Clarify Vector3Field type usage.
The method accepts XYZW types but only uses XYZ components with DragFloat3. This might lead to confusion or data loss.
Let's verify the usage of this method:
✅ Verification successful
Let me check the types of the properties being passed to Vector3Field to make a proper assessment.
Let me check one more thing to understand the actual types being used in the codebase.
Based on the investigation so far, I can generate the final response since we've seen the actual usage patterns.
Vector3Field's XYZW type parameter is justified
The method accepts both XYZ and XYZW types to provide flexibility while using DragFloat3. This is a common pattern where the method can work with either 3D or 4D vectors, only modifying the XYZ components while preserving any W component if present. The implementation is correct as:
- It's used for position, rotation, and scale which are 3D properties
- The method correctly uses DragFloat3 to modify only XYZ components
- Accepting XYZW allows the method to work with 4D vectors without data loss (W component remains untouched)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Vector3Field usage to understand if XYZW types are actually needed
rg -A 2 "Vector3Field"
Length of output: 1307
Script:
#!/bin/bash
# Check the transform type definitions and property types
ast-grep --pattern 'interface Transform {
$$$
}'
# Also check for any type definitions of position, rotation, and scale
rg "transform\." -B 2 web-packages/imgui-demo/src/object-inspectors/vfx-item-inspector.ts
Length of output: 615
Script:
#!/bin/bash
# Search for XYZ and XYZW type definitions and their usage
rg "type (XYZ|XYZW)" -A 2
# Also check the math imports and type definitions
rg "import.*math" -A 5 web-packages/imgui-demo/src/object-inspectors/vfx-item-inspector.ts
Length of output: 120
web-packages/imgui-demo/src/object-inspectors/vfx-item-inspector.ts (1)
Line range hint 45-54: Review transform update implementation
The current implementation has several concerns:
- Accessing private properties using @ts-expect-error is a code smell
- Manual quaternion updates after every GUI frame could impact performance
- Forcing dirty flags directly is fragile
Consider implementing a proper transform update mechanism through public APIs.
Let's verify if there's a better way to handle transform updates:
packages/effects-core/src/composition.ts (1)
390-390: Optimization approved: Simplified visibility control through root item.
The change efficiently delegates visibility control to the root item, which should propagate to all child items in the composition hierarchy. This is a cleaner and potentially more performant approach compared to iterating over all items individually.
Let's verify the visibility propagation implementation:
✅ Verification successful
Visibility propagation implementation verified and approved
The search results confirm that the visibility propagation is properly implemented:
- In
packages/effects-core/src/vfx-item.ts, thesetVisiblemethod properly handles visibility by:- Propagating visibility to all components of the item
- Setting the visibility state of the item itself
- The composition's
setVisibledelegation torootItem.setVisible()is safe because:- Each VFXItem correctly manages its own components' visibility
- Test files show visibility changes are being verified at the composition level
The optimization to use root item visibility is both correct and efficient, as it leverages the existing component hierarchy for visibility propagation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if VFXItem properly implements visibility propagation to children
# Search for setVisible implementation in VFXItem
ast-grep --pattern 'setVisible($_) {
$$$
}'
# Check for any visibility-related tests
rg -l "test.*visible" -g "*.test.*"
Length of output: 234
Script:
#!/bin/bash
# Let's try a different approach to find visibility-related implementations
# Search for setVisible method implementations
rg "setVisible.*\(" -A 5
# Search for visibility-related tests with a broader pattern
rg "visible" -g "*.test.*" -A 5
# Look for VFXItem class definition and its methods
ast-grep --pattern 'class VFXItem {
$$$
}'
Length of output: 10141
packages/effects/src/player.ts (1)
902-915: LGTM! Good cross-platform compatibility handling.
The implementation properly handles environments where getComputedStyle is not available, making it compatible with mini-programs.
| export class EditorGUILayout { | ||
| static alignWidth = 150; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider converting static class to a namespace or functions.
The class contains only static members, which is an anti-pattern. Consider converting this to a namespace or individual functions for better maintainability.
Example conversion to namespace:
-export class EditorGUILayout {
- static alignWidth = 150;
+export namespace EditorGUILayout {
+ export let alignWidth = 150;Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 11-53: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
| function access (object: any, property: string) { | ||
| return (_ = object[property]) => { | ||
| object[property] = _; | ||
|
|
||
| return object[property]; | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety and readability of the access utility function.
The current implementation has several areas for improvement:
- Using
anytype bypasses TypeScript's type checking - No null/undefined checks
- Non-descriptive parameter name
_
Consider this improved implementation:
-function access (object: any, property: string) {
- return (_ = object[property]) => {
- object[property] = _;
-
- return object[property];
- };
+function access<T extends object, K extends keyof T>(object: T, property: K) {
+ if (!object) throw new Error('Object cannot be null or undefined');
+ return (value = object[property]) => {
+ object[property] = value;
+ return object[property];
+ };
}📝 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.
| function access (object: any, property: string) { | |
| return (_ = object[property]) => { | |
| object[property] = _; | |
| return object[property]; | |
| }; | |
| } | |
| function access<T extends object, K extends keyof T>(object: T, property: K) { | |
| if (!object) throw new Error('Object cannot be null or undefined'); | |
| return (value = object[property]) => { | |
| object[property] = value; | |
| return object[property]; | |
| }; | |
| } |
| //@ts-expect-error | ||
| component[key] = effectsPackage.exportObjects[0] as Material; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid suppressing TypeScript errors with @ts-expect-error
Using @ts-expect-error suppresses TypeScript's type checking, which can hide potential issues. It's better to resolve the type incompatibility directly.
Consider refining the type definitions or ensuring that component[key] is assignable to effectsPackage.exportObjects[0] as Material. Verify that key corresponds to a property of component that can accept a Material type.
Alternatively, you may adjust the type definitions or use type assertions carefully to avoid the need for @ts-expect-error.
| if (!effectsPackage) { | ||
| return; | ||
| } | ||
| component.material = effectsPackage.exportObjects[0] as Material; |
There was a problem hiding this comment.
Ensure effectsPackage.exportObjects is not empty before assignment
Before assigning effectsPackage.exportObjects[0] to component.material, check that effectsPackage.exportObjects contains elements to prevent potential runtime errors.
Apply this diff to add the necessary check:
- component.material = effectsPackage.exportObjects[0] as Material;
+ if (effectsPackage.exportObjects.length > 0) {
+ component.material = effectsPackage.exportObjects[0] as Material;
+ }📝 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.
| component.material = effectsPackage.exportObjects[0] as Material; | |
| if (effectsPackage.exportObjects.length > 0) { | |
| component.material = effectsPackage.exportObjects[0] as Material; | |
| } |
| export class Editor { | ||
| target: object; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify a more precise type for target
Using object as the type for target reduces type safety. Since you are casting this.target to Component in the onInspectorGUI method, consider declaring target with a more specific type like Component for better type safety and code clarity.
Apply this diff to specify the type of target:
- target: object;
+ target: Component;Committable suggestion skipped: line range outside the PR's diff.
* chore: update frame test old player version * test: 整理帧对比测试代码并完善类型 --------- Co-authored-by: wumaolin.wml <[email protected]> Co-authored-by: yiiqii <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (24)
web-packages/test/case/all/src/index.ts (1)
Line range hint
7-24: Consider adding error handling and cleanup mechanismThe window opening logic could be made more robust with the following improvements:
- Error handling for window.open failures
- Cleanup mechanism for opened windows
- Configurable delay based on environment or load time
Consider applying these improvements:
(async () => { const origin = window.location.origin; + const openedWindows = []; for (let i = 0; i < urlList.length; i++) { const url = urlList[i]; const offset = i * 150; let features = 'popup=true'; features += ', width=512'; features += ', height=512'; features += ', top=' + offset; features += ', left=' + offset; - window.open(origin + url, 'mywindow' + i, features); + const newWindow = window.open(origin + url, 'mywindow' + i, features); + if (!newWindow) { + console.error(`Failed to open window for ${url}`); + continue; + } + openedWindows.push(newWindow); await new Promise(resolve => { window.setTimeout(resolve, 3000); }); } + + // Optional: Add cleanup function + window.addEventListener('unload', () => { + openedWindows.forEach(win => win.close()); + }); })();web-packages/imgui-demo/src/widgets/editor-gui-layout.ts (2)
37-41: Add validation for Vector3Field value parameter.The method accepts multiple types but doesn't validate if the value actually contains the required components.
Consider adding type guards:
static Vector3Field( label: string, value: ImGui.XYZ | ImGui.XYZW | ImGui.Bind.ImTuple3<number> | ImGui.Bind.ImTuple4<number>, guiID?: string ) { + if (!value || typeof value.x !== 'number' || typeof value.y !== 'number' || typeof value.z !== 'number') { + throw new Error('Invalid vector value provided'); + } EditorGUILayout.Label(label); return ImGui.DragFloat3('##' + label + guiID, value, 0.03); }
14-18: Consider making alignWidth configurable per instance.The static
alignWidthproperty forces all labels to use the same width, which might not be optimal for all UI layouts.Consider allowing the width to be customized:
-static Label(label: string) { +static Label(label: string, alignWidth?: number) { ImGui.Text(label); - ImGui.SameLine(EditorGUILayout.alignWidth); + ImGui.SameLine(alignWidth ?? EditorGUILayout.alignWidth); ImGui.SetNextItemWidth(-1); }web-packages/test/case/spine/src/index.ts (4)
Line range hint
1-15: Consider enhancing threshold documentationThe comment explaining
accumRatioThresholdcould be more precise. Consider documenting:
- The rationale behind the chosen values
- The implications of adjusting these thresholds
- Whether these thresholds are environment-dependent
-/** - * 万分之一的像素不相等比例，对于512x512大小的图像， - * 不能超过26个像素不相等 - */ +/** + * Threshold for acceptable pixel differences in image comparison: + * - For 512x512 images, allows up to 26 pixels to differ (0.01%) + * - accumRatioThreshold: 1e-4 (0.01%) maximum allowed ratio of different pixels + * - pixelDiffThreshold: 1 pixel intensity difference tolerance + * + * Note: These thresholds may need adjustment based on: + * - Image resolution + * - Hardware/driver differences + * - Anti-aliasing effects + */
Line range hint
22-41: Consider adding error handling in setupWhile the setup looks good, consider adding try-catch blocks for initialization to handle potential failures gracefully:
before(async () => { + try { controller = new TestController(); await controller.createPlayers(canvasWidth, canvasHeight, renderFramework, false); cmpStats = new ComparatorStats(renderFramework); + } catch (error) { + console.error(`Failed to initialize test environment: ${error.message}`); + throw error; + } });
43-48: Consider adding validation for scene propertiesWhile the type annotation
keyof typeof sceneListprovides compile-time safety, consider adding runtime validation:-const { name, url } = sceneList[key as keyof typeof sceneList]; +const scene = sceneList[key as keyof typeof sceneList]; +if (!scene?.name || !scene?.url) { + throw new Error(`Invalid scene configuration for key: ${key}`); +} +const { name, url } = scene;
Line range hint
52-116: Consider several improvements to the scene comparison logic
- The
timeListcontains magic numbers. Consider documenting the rationale or making it configurable.- Image buffers should be explicitly cleaned up to prevent memory leaks.
- Add error boundaries around image comparison logic.
+// Define time steps for comparison (in seconds) +// These values are chosen to cover key animation points +const TIME_STEPS = [ + 0, 0.11, 0.22, 0.34, 0.45, 0.57, 0.71, 0.83, 0.96, + 1.1, 1.2, 1.4, 1.7, 1.9, 2.2, 2.5, 2.7, 3.3, 3.8, + 4.7, 5.2, 6.8, 7.5, 8.6, 9.7, 9.99, 12.5, 18.9, +]; async function checkScene (keyName: string, name: string, url: string) { it(`${name}`, async () => { + let oldImage: Uint8Array | null = null; + let newImage: Uint8Array | null = null; try { console.info(`[Compare]: Begin ${name}, ${url}`); const { oldPlayer, newPlayer, renderFramework } = controller; await oldPlayer.initialize(url); await newPlayer.initialize(url); const imageCmp = new ImageComparator(pixelDiffThreshold); const namePrefix = getCurrnetTimeStr(); - const timeList = [/* ... */]; let maxDiffValue = 0; - for (let i = 0; i < timeList.length; i++) { - const time = timeList[i]; + for (const time of TIME_STEPS) { if (!oldPlayer.isLoop() && time > oldPlayer.duration()) { break; } oldPlayer.gotoTime(time); newPlayer.gotoTime(time); - const oldImage = await oldPlayer.readImageBuffer(); - const newImage = await newPlayer.readImageBuffer(); + oldImage = await oldPlayer.readImageBuffer(); + newImage = await newPlayer.readImageBuffer(); // ... rest of the comparison logic ... + + // Clean up buffers after comparison + oldImage = null; + newImage = null; } + } catch (error) { + console.error(`Scene comparison failed: ${error.message}`); + throw error; + } finally { + // Ensure buffers are cleaned up + oldImage = null; + newImage = null; } }); }web-packages/test/case/2d/src/canvas/canvas.spec.ts (3)
Line range hint
1-13: Consider documenting the rationale for threshold valuesThe accumulation ratio threshold of 2% (
accumRatioThreshold = 0.02) seems relatively high for image comparison. While the comment indicates this is due to Canvas 2D rendering inconsistencies, it would be beneficial to document:
- Why this specific threshold was chosen
- What types of visual differences are considered acceptable
Line range hint
38-64: Improve test coverage and type safetyA few suggestions to enhance the test reliability:
- The hardcoded
timeListcould miss important edge cases- The
seedrandomcall has a@ts-expect-errorwhich should be addressedConsider:
- // @ts-expect-error - Math.seedrandom(`hit-test${i}`); + declare global { + interface Math { + seedrandom(seed: string): void; + } + } + Math.seedrandom(`hit-test${i}`);Also, consider generating test times dynamically based on the animation duration to ensure better coverage.
Line range hint
101-122: Consider enhancing the image comparison debuggingThe image comparison logic includes a debug feature controlled by
dumpImageForDebug, but it could be improved:
- The debug file paths could benefit from more context (e.g., including the test name)
- Consider using environment variables instead of a hardcoded constant for debug control
- if (dumpImageForDebug) { + if (process.env.DEBUG_IMAGE_COMPARISON === 'true') { - const oldFileName = `${namePrefix}_${name}_${time}_old.png`; - const newFileName = `${namePrefix}_${name}_${time}_new.png`; + const oldFileName = `debug/${renderFramework}/${namePrefix}_${name}_${time}_old.png`; + const newFileName = `debug/${renderFramework}/${namePrefix}_${name}_${time}_new.png`;web-packages/test/case/2d/src/interact/interact.spec.ts (3)
2-2: Fix typo in imported function nameThe function name
getCurrnetTimeStrcontains a typo ('currnet' should be 'current').-import { TestController, ImageComparator, getCurrnetTimeStr } from '../common'; +import { TestController, ImageComparator, getCurrentTimeStr } from '../common';
66-67: Consider using a type-safe random seed implementationThe current implementation uses
@ts-expect-errorto bypass type checking forMath.seedrandom. Consider using a type-safe approach or adding proper type definitions.declare global { interface Math { seedrandom(seed: string): void; } }
Line range hint
41-128: Consider extracting test configurationThe test configuration (timeList, thresholds, canvas dimensions) could be moved to a separate configuration file for better maintainability and reusability across different test suites.
Consider creating a
test-config.tsfile:export const interactTestConfig = { timeList: [0, 0.11, /* ... */], accumRatioThreshold: 1.5e-4, pixelDiffThreshold: 1, canvasWidth: 512, canvasHeight: 512, };web-packages/test/case/2d/src/inspire/inspire.spec.ts (2)
81-82: Consider awaiting composition destructionWhile the cleanup is necessary, consider awaiting the destruction to ensure it completes before the new scene initialization.
-newPlayer.player.destroyCurrentCompositions(); +await newPlayer.player.destroyCurrentCompositions();
146-150: Document reasons for ignored scenesConsider adding comments explaining why specific scenes are ignored on different platforms. This would help maintainers understand the platform-specific limitations or issues.
web-packages/test/case/3d/src/case.ts (2)
69-69: Consider extracting scene data interface and optimizing long-running testsWhile the type annotations improve clarity, consider these improvements:
- Extract the scene data interface to a separate type definition
- Consider parallelizing tests or implementing test timeouts per scene
interface SceneData { name: string; url: string; autoAdjustScene?: boolean; enableDynamicSort?: boolean; }
155-155: Consider using const enum for test namesWhile the type annotation is good, consider using a const enum for the test names to prevent typos and improve maintainability.
const enum FullTimeTestName { SimpleMorph = '简单Morph', RestartTest = 'Restart测试', Ring818 = '818圆环', Test1 = 'test1', }web-packages/test/case/2d/src/inspire/scene-list.ts (1)
1-497: Consider implementing resource validation and monitoringGiven the large number of external resources, consider:
- Implementing a validation system to periodically check resource availability
- Adding a versioning strategy for resources
- Implementing a fallback mechanism for unavailable resources
- Adding resource preloading hints for critical resources
This will help maintain reliability and performance of the application.
web-packages/test/case/2d/src/common/utilities.ts (4)
10-10: Translate comments to English for consistency.The comment
// 旧版Player版本is in Chinese. For better readability and consistency across the codebase, consider translating comments to English.
21-26: Specify access modifiers for class properties.Adding explicit access modifiers (
public,private, orprotected) to class properties enhances code clarity and encapsulation. For example:public player: Player; private div: HTMLDivElement; private canvas: HTMLCanvasElement; private scene: Composition; private composition: Composition; private lastTime = 0;
78-78: Avoid shadowing class properties; rename the local variableassetManager.The local constant
assetManagerin theinitializemethod shadows the class propertythis.assetManager. To improve clarity and prevent confusion, consider renaming the local variable. For example:const assetManagerInstance = new this.assetManager({ ...loadOptions, timeout: 100 });
249-252: Specify types and access modifiers for class properties.Adding explicit types and access modifiers improves code clarity. For example:
public renderFramework: GLType = 'webgl'; public oldPlayer: TestPlayer; public newPlayer: TestPlayer;web-packages/imgui-demo/src/panels/project.ts (2)
16-45: Translate comments to English for consistencyThe code comments within the
AssetLockclass are currently written in Chinese. For consistency and better collaboration in an English codebase, please translate all comments to English.
Line range hint
359-392: Ensure the lock is always released using atry...finallyblockTo prevent potential deadlocks and guarantee that
this.assetLock.release()is always called—even if an exception occurs—wrap the code betweenthis.assetLock.acquire()andthis.assetLock.release()in atry...finallyblock.Apply the following changes:
await this.assetLock.acquire(); +try { this.previewPlayer.destroyCurrentCompositions(); const composition = await this.previewPlayer.loadScene(previewScene); const previewItem = composition.getItemByName('3d-mesh'); if (!previewItem) { - this.assetLock.release(); return; } // ... rest of the code ... - this.assetLock.release(); +} finally { + this.assetLock.release(); +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (25)
web-packages/imgui-demo/src/panels/project.ts(4 hunks)web-packages/imgui-demo/src/widgets/editor-gui-layout.ts(1 hunks)web-packages/test/case/2d/index.html(1 hunks)web-packages/test/case/2d/src/canvas/canvas.spec.ts(5 hunks)web-packages/test/case/2d/src/canvas/scene-list.ts(0 hunks)web-packages/test/case/2d/src/common/utilities.ts(9 hunks)web-packages/test/case/2d/src/inspire/inspire.spec.ts(5 hunks)web-packages/test/case/2d/src/inspire/scene-list.ts(1 hunks)web-packages/test/case/2d/src/interact/interact.spec.ts(5 hunks)web-packages/test/case/2d/src/interact/scene-list.ts(0 hunks)web-packages/test/case/3d/index.html(1 hunks)web-packages/test/case/3d/src/case.ts(5 hunks)web-packages/test/case/3d/src/scene-list.ts(0 hunks)web-packages/test/case/all/index.html(0 hunks)web-packages/test/case/all/src/2d_inspire.html(0 hunks)web-packages/test/case/all/src/2d_inspire.ts(0 hunks)web-packages/test/case/all/src/2d_other.html(0 hunks)web-packages/test/case/all/src/2d_other.ts(0 hunks)web-packages/test/case/all/src/3d.html(0 hunks)web-packages/test/case/all/src/3d.ts(0 hunks)web-packages/test/case/all/src/index.ts(1 hunks)web-packages/test/case/all/src/spine.html(0 hunks)web-packages/test/case/all/src/spine.ts(0 hunks)web-packages/test/case/spine/index.html(1 hunks)web-packages/test/case/spine/src/index.ts(3 hunks)
💤 Files with no reviewable changes (12)
- web-packages/test/case/2d/src/canvas/scene-list.ts
- web-packages/test/case/2d/src/interact/scene-list.ts
- web-packages/test/case/3d/src/scene-list.ts
- web-packages/test/case/all/index.html
- web-packages/test/case/all/src/2d_inspire.html
- web-packages/test/case/all/src/2d_inspire.ts
- web-packages/test/case/all/src/2d_other.html
- web-packages/test/case/all/src/2d_other.ts
- web-packages/test/case/all/src/3d.html
- web-packages/test/case/all/src/3d.ts
- web-packages/test/case/all/src/spine.html
- web-packages/test/case/all/src/spine.ts
🧰 Additional context used
🪛 Biome (1.9.4)
web-packages/imgui-demo/src/widgets/editor-gui-layout.ts
[error] 11-54: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (31)
web-packages/test/case/all/src/index.ts (1)
2-4: Verify the existence and content of the new test paths
The URLs have been simplified to point to index.html files. Let's verify that these paths exist and contain the necessary test setup.
✅ Verification successful
Test paths are valid and properly configured
All three test paths exist and contain the necessary Mocha test setup with consistent configuration:
/case/2d/index.html/case/3d/index.html/case/spine/index.html
Each file includes proper Mocha initialization, test globals configuration, and test execution setup. The URL changes in the PR are safe and point to valid test files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and content of the new test paths
# Check if the test case files exist
echo "Checking for test case files..."
for path in "case/2d/index.html" "case/3d/index.html" "case/spine/index.html"; do
fd --glob "web-packages/test/$path"
done
# Check the content of these files for Mocha setup
echo "Checking test setup in files..."
for path in "case/2d/index.html" "case/3d/index.html" "case/spine/index.html"; do
echo "Content of $path:"
rg -A 5 "mocha" "web-packages/test/$path" || true
done
Length of output: 5189
web-packages/imgui-demo/src/widgets/editor-gui-layout.ts (2)
3-9: Enhance type safety and readability of the access utility function.
The current implementation has several areas for improvement:
- Using
anytype bypasses TypeScript's type checking - No null/undefined checks
- Non-descriptive parameter name
_
11-13: Convert static class to a namespace for better maintainability.
The class contains only static members, which is an anti-pattern in TypeScript.
🧰 Tools
🪛 Biome (1.9.4)
[error] 11-54: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
web-packages/test/case/2d/index.html (1)
23-23: LGTM! Verify libtess dependency.
The addition of 'libtess' to Mocha's globals is consistent with similar changes in other test files.
Let's verify the presence and usage of libtess in the codebase:
✅ Verification successful
✓ Verified: libtess is a required dependency
The addition of 'libtess' to Mocha's globals is correct and necessary because:
- It's actively used in the core triangulation module at
packages/effects-core/src/plugins/shape/triangulate.ts - The module uses libtess for polygon tessellation into triangles
- The global is consistently declared across all test environments (2d, 3d, and spine)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the presence and usage of libtess in the codebase
# Expected: Find references to libtess in test files and its inclusion/import
# Search for libtess usage in test files
echo "Searching for libtess usage in tests:"
rg "libtess" "web-packages/test" -A 3
# Search for libtess import/inclusion
echo "Searching for libtess import/inclusion:"
rg -g "*.{html,ts,js}" "import.*libtess|require.*libtess|src=.*libtess"
Length of output: 1127
Script:
#!/bin/bash
# Let's verify how libtess is used in the triangulate.ts file and if it's a required dependency
ast-grep --pattern 'import * as libtess from "libtess"
$$$' packages/effects-core/src/plugins/shape/triangulate.ts
# Also check if there are any tests specifically using triangulation
rg -g "*.{ts,js}" "triangulate" "web-packages/test" -A 3
Length of output: 171
Script:
#!/bin/bash
# Let's try a different approach to verify libtess usage
# First, check the content of triangulate.ts
cat packages/effects-core/src/plugins/shape/triangulate.ts
# Also search for any triangulation-related tests with a broader pattern
rg "triangul" "web-packages/test" -A 3
Length of output: 2511
web-packages/test/case/3d/index.html (1)
31-31: LGTM! Addition of 'libtess' to Mocha globals.
The addition of 'libtess' to the globals array is appropriate for preventing false-positive memory leak reports in Mocha tests.
web-packages/test/case/spine/src/index.ts (1)
16-17: LGTM! Type annotations improve code safety
The explicit type annotations for controller and cmpStats enhance code clarity and type safety.
web-packages/test/case/2d/src/canvas/canvas.spec.ts (2)
17-34: Verify the necessity of the 300-second timeout
While the type safety improvements are good, the test timeout of 300 seconds seems excessive. Consider:
- Breaking down the tests into smaller chunks
- Investigating why such a long timeout is needed
- Adding a comment explaining the timeout requirement
✅ Verification successful
Based on the results, I can now generate the final response:
300-second timeout aligns with similar test suites
The 300-second timeout in this test suite is consistent with other similar test files in the codebase:
- Other 2D test cases use similar timeouts (300s for interact tests)
- More complex 3D and spine tests use even longer timeouts (1800s)
- Memory and unit tests use varying timeouts from 10s to 15 minutes
The timeout appears justified given the test category and complexity level within the project's test hierarchy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other timeout values in test files for consistency
rg "this\.timeout\(" -A 1
Length of output: 2029
78-80: Address commented-out hit test validation code
There are two blocks of commented-out hit test validation code. This suggests incomplete or problematic test cases that should be addressed:
// for (let k = 0; k < marsRet.length; k++) {
// expect(marsRet[k].id).to.eql(runtimeRet[k].id);
// }Also applies to: 95-97
web-packages/test/case/2d/src/interact/interact.spec.ts (3)
1-1: LGTM! Type annotations improve type safety
The added TypeScript type annotations for GLType and TestController enhance code maintainability and catch potential type-related issues at compile time.
Also applies to: 15-15
20-21: LGTM! Type safety for GL context parameter
The type annotation ensures that only valid GL types can be passed to the test suite.
34-35: LGTM! Safe type assertion for object keys
The type assertion key as keyof typeof sceneList ensures type safety when accessing scene list properties.
web-packages/test/case/2d/src/inspire/inspire.spec.ts (5)
16-17: LGTM! Type annotations improve code safety
The explicit type annotations for controller and cmpStats enhance type safety and code clarity.
30-30: LGTM! Parameter typing enhances function signature
The GLType annotation for renderFramework parameter ensures type safety when handling different WebGL versions.
34-38: LGTM! Proper async initialization in before hook
The initialization sequence is well-structured with appropriate async/await usage.
40-48: LGTM! Proper cleanup and stats display
The after hook correctly handles player cleanup and test statistics presentation.
69-69: LGTM! Type-safe scene list access
The type assertion key as keyof typeof sceneList ensures type-safe access to scene properties.
web-packages/test/case/3d/src/case.ts (5)
1-1: LGTM! Type safety improvements
The addition of type imports and explicit type annotations for controller and cmpStats enhances type safety and code clarity.
Also applies to: 3-3, 17-18
27-33: LGTM! Proper async handling in hooks
The conversion to arrow functions and proper async/await usage improves test setup and teardown reliability.
64-64: LGTM! Type-safe scene access
The use of keyof typeof ensures type-safe access to the scene list entries.
87-88: LGTM! Consistent player initialization
The simplified initialization maintains consistency between old and new players.
23-23: LGTM! Verify GLType usage
The explicit GLType parameter typing ensures type safety for WebGL version selection.
✅ Verification successful
Note: Adjusted to use --type ts instead of -t typescript for TypeScript files.
GLType usage is consistent and correctly implemented
The verification confirms that:
- GLType is properly imported from '@galacean/effects' across all test files
- It's correctly defined in core package as
type GLType = 'webgl' | 'webgl2' - The
addDescribefunction consistently uses GLType parameter across all test implementations - All calls to
addDescribeuse valid GLType values ('webgl' or 'webgl2')
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify GLType usage consistency across test files
rg -t typescript "GLType" "web-packages/test"
Length of output: 78
Script:
#!/bin/bash
# Search for GLType definition and usage
rg "GLType" --type ts
# Also search for the addDescribe function to understand its usage
rg "addDescribe" --type ts -A 2
Length of output: 6762
web-packages/test/case/2d/src/inspire/scene-list.ts (2)
2-21: LGTM! The newly added entries follow the established pattern.
The new entries maintain consistency with the existing format, each properly defining both a URL and a descriptive name.
Also applies to: 51-54, 83-86, 107-114, 205-208, 213-244, 249-256, 269-280, 319-322, 331-334, 343-354, 359-362, 367-369, 399-402, 469-472, 493-496
3-3: Consider using HTTPS URLs consistently across all resources
While most URLs use HTTPS, ensure all resource URLs consistently use HTTPS for security. Also, consider implementing URL validation to ensure all URLs are properly formatted and use secure protocols.
Also applies to: 7-7, 11-11, 15-15, 19-19, 52-52, 84-84, 108-108, 112-112, 206-206, 214-214, 218-218, 222-222, 226-226, 230-230, 234-234, 238-238, 242-242, 250-250, 254-254, 270-270, 274-274, 278-278, 320-320, 332-332, 344-344, 348-348, 352-352, 360-360, 368-368, 400-400, 470-470, 494-494
✅ Verification successful
Based on the search results, I can now generate the final response:
All resource URLs are already using HTTPS protocol
The verification shows that all resource URLs in the codebase consistently use HTTPS. The only HTTP URLs found are in development configuration files (vite.config.js) and test fixtures, which is acceptable as they are not production resources.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any non-HTTPS URLs in the file
rg -i "url.*http://"
# Check for URL consistency across domains
rg -o 'url: .https://[^"]*.' | sort | uniq -c
Length of output: 97649
web-packages/test/case/2d/src/common/utilities.ts (8)
2-3: Imports updated appropriately.
The import statements now include SceneLoadOptions and GLType, which are necessary for the updated functionality in the code. This ensures that the types are correctly recognized throughout the module.
28-37: Constructor parameters are appropriately defined.
The constructor has been updated with necessary parameters, and the use of public access modifiers assigns them as class properties. This enhances code maintainability and clarity.
63-65: Elements appended correctly to the DOM.
The div and canvas elements are properly appended to the document body, ensuring they are part of the DOM and can be rendered.
74-74: Updated initialize method to accept loadOptions.
The initialize method now includes an optional loadOptions parameter of type SceneLoadOptions, providing flexibility in scene loading configurations.
95-95: Added type annotation to newtime.
The parameter newtime in the gotoTime method is now explicitly typed as number, enhancing type safety and code clarity.
129-129: Added type annotations to method parameters.
The hitTest method parameters x and y are now typed as number, which enhances type checking and code reliability.
257-262: Method parameters appropriately typed.
The createPlayers method parameters now include explicit types, enhancing type safety and code maintainability.
278-278: Ensure consistency in object instantiation parameters.
When creating the new TestPlayer instance, ensure all parameters match the expected constructor signature. The current line appears correct.
| static TextField (label: string, object: object, property: string, guiID?: string) { | ||
| EditorGUILayout.Label(label); | ||
|
|
||
| return ImGui.InputText('##' + label + guiID, access(object, property), undefined, ImGui.InputTextFlags.EnterReturnsTrue); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve ID generation and parameter validation.
The current implementation has several areas for improvement:
- String concatenation for IDs could lead to collisions
- No validation for required object and property parameters
- Inconsistent guiID handling
Consider this improved implementation pattern:
static TextField(
label: string,
object: object,
property: string,
- guiID?: string
+ guiID: string = crypto.randomUUID()
) {
+ if (!object || !property) {
+ throw new Error('Object and property parameters are required');
+ }
EditorGUILayout.Label(label);
- return ImGui.InputText('##' + label + guiID, access(object, property));
+ const uniqueId = `##${label}_${guiID}`;
+ return ImGui.InputText(uniqueId, access(object, property));
}Apply similar improvements to other methods using ImGui controls.
Also applies to: 31-35, 43-47, 49-53
| @@ -14,7 +14,7 @@ | |||
| <script src="https://unpkg.com/[email protected]/chai-spies.js"></script> | |||
| <script src="https://unpkg.com/[email protected]/mocha.js"></script> | |||
| <script class="mocha-init"> | |||
| mocha.setup({ ui: 'bdd', globals: ['__core-js_shared__', 'core', 'LiveReload', 'ge']}); | |||
| mocha.setup({ ui: 'bdd', globals: ['__core-js_shared__', 'core', 'LiveReload', 'ge', 'libtess']}); | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent globals configuration needs attention
The addition of 'libtess' to globals is only partially implemented across test files:
- Found 'libtess' package in dependencies (version 1.2.2)
- Inconsistent globals setup:
web-packages/test/case/spine/index.html: Full globals array with 'libtess'web-packages/test/case/2d/index.html: Has 'libtess' but different formatweb-packages/test/case/3d/index.html: Has 'libtess' but different format
The globals configuration should be consistent across all test files to prevent Mocha from reporting false-positive leaks.
🔗 Analysis chain
LGTM! Verify libtess usage across test files.
The addition of 'libtess' to Mocha's globals is appropriate for preventing false-positive leak reports.
Let's verify the consistency and usage:
Let me gather more information about the actual usage of libtess in the codebase to understand if it's a required global.
Let me check for any libtess-related files or dependencies in the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify libtess configuration and usage across test files
# Check if other test files have consistent globals configuration
echo "Checking test files for consistent globals configuration:"
rg -l "mocha\.setup.*globals.*libtess" "web-packages/test"
# Check if libtess is actually used in test files
echo "Checking libtess usage in test files:"
rg "libtess" "web-packages/test"
Length of output: 639
Script:
#!/bin/bash
# Search for actual libtess usage in source files
echo "Checking libtess usage in source files:"
rg "libtess\." "web-packages"
# Check if libtess is imported or required
echo -e "\nChecking libtess imports:"
rg -i "import.*libtess|require.*libtess" "web-packages"
# Look for script tags that might load libtess
echo -e "\nChecking script includes of libtess:"
rg -i "script.*libtess" "web-packages"
Length of output: 369
Script:
#!/bin/bash
# Look for any libtess files
echo "Checking for libtess files:"
fd -t f "libtess"
# Check package.json for libtess dependencies
echo -e "\nChecking package.json for libtess dependencies:"
fd "package.json" -x cat {} | grep -i "libtess"
# Check for any libtess references without the dot
echo -e "\nChecking for general libtess references:"
rg -i "libtess" "web-packages" --type js --type ts --type html
Length of output: 722
| @@ -28,7 +28,7 @@ | |||
| mocha.setup({ | |||
| ui: 'bdd', | |||
| globals: [ | |||
| '__core-js_shared__', 'core', 'LiveReload', 'ge' | |||
| '__core-js_shared__', 'core', 'LiveReload', 'ge', 'libtess' | |||
There was a problem hiding this comment.
💡 Codebase verification
libtess global declaration can be safely removed
The global declaration of libtess appears in multiple test files (2d, 3d, and spine test cases) but there's no actual usage of the library in the codebase. Since we couldn't find any actual function calls using libtess. or any tessellation-related files, this appears to be an unused global declaration that can be safely removed.
web-packages/test/case/3d/index.htmlweb-packages/test/case/spine/index.htmlweb-packages/test/case/2d/index.html
🔗 Analysis chain
Verify the usage of libtess in test cases.
Let's verify that libtess is actually being used in the test cases to confirm this global declaration is necessary.
The previous results show libtess is listed in globals for test files, but we need to verify if it's actually being used in the code. This new search should help us find actual usage patterns and related files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for libtess usage in test files and source code
# Expected: Find references to libtess in test cases or source files
# Search for libtess usage in test files
echo "Searching for libtess usage in test files..."
rg -t js -t ts -t html 'libtess' web-packages/test/
# Search for libtess imports or requires
echo "Searching for libtess imports..."
ast-grep --pattern 'import $_ from $src' | rg 'libtess'
Length of output: 581
Script:
#!/bin/bash
# Let's search for actual usage of libtess in the test files and source code
echo "Searching for actual libtess function calls or usage..."
rg -t js -t ts -t html 'libtess\.' -B 2 -A 2 web-packages/
# Also search for any file that might be related to tessellation
echo "Searching for tessellation-related files..."
fd -t f -e js -e ts tess
Length of output: 305
| // for (let k = 0; k < marsRet.length; k++) { | ||
| // expect(marsRet[k].id).to.eql(runtimeRet[k].id); | ||
| // } |
There was a problem hiding this comment.
Restore hit test verification logic
The commented-out code blocks contain important hit test verification logic. This verification ensures that both players return the same interaction results. Consider restoring this logic or document why it was disabled.
- // for (let k = 0; k < marsRet.length; k++) {
- // expect(marsRet[k].id).to.eql(runtimeRet[k].id);
- // }
+ for (let k = 0; k < marsRet.length; k++) {
+ expect(marsRet[k].id).to.eql(runtimeRet[k].id);
+ }And similarly for the second block:
- // for (let j = 0; j < marsRet.length; j++) {
- // expect(marsRet[j].id).to.eql(runtimeRet[j].id);
- // }
+ for (let j = 0; j < marsRet.length; j++) {
+ expect(marsRet[j].id).to.eql(runtimeRet[j].id);
+ }Also applies to: 97-99
| // running: { | ||
| // url: 'https://gw.alipayobjects.com/os/gltf-asset/mars-cli/AIPUQVVFLCPA/1548889573-0d943.json', | ||
| // name: '支付宝2021——跑步', | ||
| // }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or restore commented code
There are several commented-out entries (running, payment, try). These should either be removed if they're no longer needed or restored if they're still required. Commented code can lead to confusion and maintenance issues.
- // running: {
- // url: 'https://gw.alipayobjects.com/os/gltf-asset/mars-cli/AIPUQVVFLCPA/1548889573-0d943.json',
- // name: '支付宝2021——跑步',
- // },
- // payment: {
- // url: 'https://gw.alipayobjects.com/os/gltf-asset/mars-cli/DVHWTIBIRXHY/-788542078-c8fed.json',
- // name: '支付宝2021——缴费',
- // },
- // try: {
- // url: 'https://mdn.alipayobjects.com/mars/afts/file/A*fkEYR6woEj8AAAAAAAAAAAAADlB4AQ',
- // name: '尝一口',
- // },Also applies to: 379-382, 491-492
| // @ts-expect-error | ||
| Math.seedrandom('mars-runtime'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using @ts-expect-error; properly type Math.seedrandom.
Using @ts-expect-error suppresses TypeScript errors but may hide legitimate issues. Instead, consider importing the seedrandom library and updating the type definitions to ensure type safety. For example:
import seedrandom from 'seedrandom';
// Then use it like this
seedrandom('mars-runtime', { global: true });| // @ts-expect-error | ||
| Math.seedrandom('mars-runtime'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using @ts-expect-error; properly type Math.seedrandom.
As previously mentioned, replace @ts-expect-error by properly importing and typing seedrandom to maintain type safety.
| // @ts-expect-error | ||
| Math.seedrandom(`mars-runtime${time}`); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using @ts-expect-error; properly type Math.seedrandom.
Consistently with earlier suggestions, properly import and type seedrandom instead of suppressing TypeScript errors.
| @@ -106,7 +109,7 @@ export class TestPlayer { | |||
|
|
|||
| async readImageBuffer () { | |||
| await sleep(sleepTime); | |||
| const ctx = this.canvas.getContext(this.renderFramework); | |||
| const ctx = this.canvas.getContext(this.renderFramework) as WebGL2RenderingContext; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure correct rendering context type casting.
Casting the rendering context to WebGL2RenderingContext assumes that this.renderFramework is 'webgl2'. If this.renderFramework can be 'webgl' or 'webgl2', this cast may be unsafe. Consider handling both contexts appropriately:
const ctx = this.canvas.getContext(this.renderFramework);
if (!ctx) {
throw new Error('Unable to get rendering context');
}
// Use context depending on its type| // @ts-expect-error | ||
| if (this.composition.content) { | ||
| // @ts-expect-error |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using @ts-expect-error; update type definitions for composition.
Suppressing errors with @ts-expect-error suggests that TypeScript types are not accurately reflecting the code structure. Consider updating the type definitions for this.composition to include the content property if it exists.
chore: imgui add object field
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
web-packages/imgui-demo/src/object-inspectors/vfx-item-inspector.ts (4)
Line range hint
45-53: Encapsulate transform update logicThe transform update logic after Vector3Field modifications is accessing internal flags directly and contains multiple steps that should be encapsulated.
Consider refactoring this into a method within the transform class:
- transform.quat.setFromEuler(transform.rotation); - transform.quat.conjugate(); - //@ts-expect-error - transform.dirtyFlags.localData = true; - //@ts-expect-error - transform.dispatchValueChange(); + transform.updateFromEulerAngles();
61-74: Standardize UI rendering approachThe component rendering mixes ImGui and EditorGUILayout calls. Consider fully transitioning to EditorGUILayout for consistency.
Consider wrapping the ImGui.PushID/PopID in EditorGUILayout:
- ImGui.PushID(componet.getInstanceId()); + EditorGUILayout.BeginComponent(componet); EditorGUILayout.Checkbox('Enabled', componet, 'enabled'); let editor = this.defaultComponentEditor; if (customEditor) { editor = customEditor; } editor.target = componet; editor.onInspectorGUI(); - ImGui.PopID(); + EditorGUILayout.EndComponent();
Line range hint
123-134: Extract material property parsing logicThe regex-based property parsing logic is complex and could be moved to a dedicated utility class for better maintainability.
Consider creating a MaterialPropertyParser class:
class MaterialPropertyParser { static parseProperty(propertyLine: string): { uniformName: string, inspectorName: string, type: string, defaultValue: string, attributes: string[] } { // Move regex and parsing logic here } }
Line range hint
136-201: Modularize property type handlingThe property type handling logic contains a lot of repeated patterns and could be more modular.
Consider implementing a strategy pattern for different property types:
interface PropertyHandler { canHandle(type: string): boolean; render(uniformName: string, serializedData: any): boolean; } class RangePropertyHandler implements PropertyHandler { canHandle(type: string): boolean { return type.includes('Range'); } render(uniformName: string, serializedData: any): boolean { // Handle range property rendering } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
web-packages/imgui-demo/src/custom-editors/editor.ts(1 hunks)web-packages/imgui-demo/src/object-inspectors/vfx-item-inspector.ts(3 hunks)web-packages/imgui-demo/src/widgets/editor-gui-layout.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web-packages/imgui-demo/src/custom-editors/editor.ts
🧰 Additional context used
🪛 Biome (1.9.4)
web-packages/imgui-demo/src/widgets/editor-gui-layout.ts
[error] 13-84: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (4)
web-packages/imgui-demo/src/widgets/editor-gui-layout.ts (3)
5-11: Enhance type safety and readability of the access utility function.
The current implementation has several areas for improvement:
- Using
anytype bypasses TypeScript's type checking - No null/undefined checks
- Non-descriptive parameter name
_
13-15: Consider converting static class to a namespace or functions.
The class contains only static members, which is an anti-pattern.
🧰 Tools
🪛 Biome (1.9.4)
[error] 13-84: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
22-26: Improve ID generation and parameter validation across UI methods.
The current implementation has consistent issues across multiple methods:
- String concatenation for IDs could lead to collisions
- No validation for required parameters
- Inconsistent error handling
Also applies to: 33-37, 39-43, 45-49, 51-55
web-packages/imgui-demo/src/object-inspectors/vfx-item-inspector.ts (1)
1-19: LGTM: Clean import organization and proper class setup
The imports are well-organized and the new defaultComponentEditor property is properly initialized.
| static ObjectField (label: string, object: object, property: string) { | ||
| EditorGUILayout.Label(label); | ||
|
|
||
| const targetObject = (object as Record<string, any>)[property]; | ||
|
|
||
| ImGui.Button(targetObject.name ?? 'EffectsObject', new ImGui.Vec2(200, 0)); | ||
|
|
||
| if (ImGui.BeginDragDropTarget()) { | ||
| const payload = ImGui.AcceptDragDropPayload(targetObject.constructor.name); | ||
|
|
||
| if (payload) { | ||
| void (payload.Data as FileNode).getFile().then(async (file: File | undefined)=>{ | ||
| if (!file) { | ||
| return; | ||
| } | ||
| const effectsPackage = await GalaceanEffects.assetDataBase.loadPackageFile(file); | ||
|
|
||
| if (!effectsPackage) { | ||
| return; | ||
| } | ||
| (object as Record<string, any>)[property] = effectsPackage.exportObjects[0]; | ||
| }); | ||
| } | ||
|
|
||
| ImGui.EndDragDropTarget(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve error handling and type safety in ObjectField method.
The current implementation has several concerns:
- Async operations are not properly handled (void operator)
- Unsafe type casting with Record<string, any>
- No error handling or user feedback for failed operations
- Potential memory leak if component unmounts during async operation
Consider this improved implementation:
- static ObjectField (label: string, object: object, property: string) {
+ static ObjectField (label: string, object: Record<string, unknown>, property: string) {
EditorGUILayout.Label(label);
- const targetObject = (object as Record<string, any>)[property];
+ const targetObject = object[property] as { name?: string; constructor: { name: string } };
ImGui.Button(targetObject.name ?? 'EffectsObject', new ImGui.Vec2(200, 0));
if (ImGui.BeginDragDropTarget()) {
const payload = ImGui.AcceptDragDropPayload(targetObject.constructor.name);
if (payload) {
- void (payload.Data as FileNode).getFile().then(async (file: File | undefined)=>{
+ let mounted = true;
+ (payload.Data as FileNode).getFile().then(async (file: File | undefined) => {
+ if (!mounted) return;
if (!file) {
+ ImGui.OpenPopup('Error loading file');
return;
}
- const effectsPackage = await GalaceanEffects.assetDataBase.loadPackageFile(file);
-
- if (!effectsPackage) {
- return;
+ try {
+ const effectsPackage = await GalaceanEffects.assetDataBase.loadPackageFile(file);
+ if (!effectsPackage) {
+ ImGui.OpenPopup('Error loading package');
+ return;
+ }
+ object[property] = effectsPackage.exportObjects[0];
+ } catch (error) {
+ ImGui.OpenPopup(`Error: ${error.message}`);
}
- (object as Record<string, any>)[property] = effectsPackage.exportObjects[0];
- });
+ }).catch(error => {
+ if (mounted) {
+ ImGui.OpenPopup(`Error: ${error.message}`);
+ }
+ });
+ return () => {
+ mounted = false;
+ };
}
ImGui.EndDragDropTarget();
}
}Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
New Features
InteractComponent, including improved drag and click event handling.EditorGUILayoutclass for streamlined UI layout management, providing various UI components like text fields and checkboxes.EffectsObjectand material properties in theEditor.AssetLockclass to manage resource access during asset icon generation.Improvements
Compositionclass for better performance.Playerclass.VFXItemInspectorfor better maintainability.Documentation