Conversation
📝 WalkthroughWalkthroughFFDComponent adds a private Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
plugin-packages/ffd/src/ffd-component.ts (2)
22-22: Consider enhancing readability and observability.While the GPU capability check logic is correct, consider these optional improvements:
- Format the conditional for better readability
- Extract the magic number 256 as a named constant with documentation
- Add logging when FFD is disabled to aid debugging
🔎 Proposed refactor
+ // Minimum vertex uniforms required for FFD shader (rowNum * colNum control points + other uniforms) + private static readonly MIN_VERTEX_UNIFORMS = 256; + constructor (engine: Engine) { super(engine); - if (engine.gpuCapability.detail.maxVertexUniforms < 256) {this.enableFFD = false;} + if (engine.gpuCapability.detail.maxVertexUniforms < FFDComponent.MIN_VERTEX_UNIFORMS) { + this.enableFFD = false; + console.warn('FFD disabled: GPU supports only', engine.gpuCapability.detail.maxVertexUniforms, 'vertex uniforms, requires at least', FFDComponent.MIN_VERTEX_UNIFORMS); + } }
49-59: Good conditional shader creation, but consider guarding uniform updates.The conditional shader assignment correctly prevents FFD shader usage on low-capability devices. However, when
enableFFDis false,updateShaderUniform()(called fromonUpdateandinitData) will still attempt to set FFD-specific uniforms on the material, wasting CPU cycles on uniforms that don't exist in the default shader.🔎 Suggested optimization
Add an early return in
updateShaderUniform()method:private updateShaderUniform (): void { + // Skip uniform updates when FFD is disabled + if (!this.enableFFD) { + return; + } + // 确保当前的 SpriteComponent 存在 if (!this.currentSpriteComponent) { return; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
plugin-packages/ffd/src/shader/vertex.glslis excluded by!**/*.glsl
📒 Files selected for processing (1)
plugin-packages/ffd/src/ffd-component.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ChengYi996
Repo: galacean/effects-runtime PR: 1118
File: packages/effects-core/src/components/ffd-component.ts:135-219
Timestamp: 2025-08-05T07:50:26.317Z
Learning: In packages/effects-core/src/components/ffd-component.ts, the mesh subdivision logic (lines 147-219) within the collectSpriteComponents method is temporary debug code that will be properly implemented later, as confirmed by ChengYi996.
📚 Learning: 2025-08-05T07:50:26.317Z
Learnt from: ChengYi996
Repo: galacean/effects-runtime PR: 1118
File: packages/effects-core/src/components/ffd-component.ts:135-219
Timestamp: 2025-08-05T07:50:26.317Z
Learning: In packages/effects-core/src/components/ffd-component.ts, the mesh subdivision logic (lines 147-219) within the collectSpriteComponents method is temporary debug code that will be properly implemented later, as confirmed by ChengYi996.
Applied to files:
plugin-packages/ffd/src/ffd-component.ts
📚 Learning: 2025-08-05T07:51:18.728Z
Learnt from: ChengYi996
Repo: galacean/effects-runtime PR: 1118
File: packages/effects-core/src/components/ffd-component.ts:162-164
Timestamp: 2025-08-05T07:51:18.728Z
Learning: In packages/effects-core/src/components/ffd-component.ts, the //ts-expect-error usage for accessing siblingComponent.splits (around lines 162-164) is temporary debugging code that will be properly implemented later, as confirmed by ChengYi996.
Applied to files:
plugin-packages/ffd/src/ffd-component.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
plugin-packages/ffd/src/ffd-component.ts (1)
10-10: LGTM!The
enableFFDflag provides a clean mechanism to conditionally disable FFD functionality based on GPU capabilities.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugin-packages/ffd/src/ffd-component.ts (1)
23-25: Add logging when FFD is disabled.The threshold of 256 vertex uniforms is appropriate for the shader's actual requirements (the FFD control points array uses ~169 uniform vec3 slots plus ~25 additional uniforms, totaling ~195 vectors). However, consider adding a warning log when FFD is disabled due to GPU limitations to help with debugging and render discrepancies.
💡 Suggested improvement: Add logging
constructor (engine: Engine) { super(engine); if (engine.gpuCapability.detail.maxVertexUniforms < 256) { this.enableFFD = false; + console.warn('FFD disabled: GPU maxVertexUniforms (%d) is below required threshold (256)', + engine.gpuCapability.detail.maxVertexUniforms); } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugin-packages/ffd/src/ffd-component.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ChengYi996
Repo: galacean/effects-runtime PR: 1118
File: packages/effects-core/src/components/ffd-component.ts:135-219
Timestamp: 2025-08-05T07:50:26.317Z
Learning: In packages/effects-core/src/components/ffd-component.ts, the mesh subdivision logic (lines 147-219) within the collectSpriteComponents method is temporary debug code that will be properly implemented later, as confirmed by ChengYi996.
Learnt from: ChengYi996
Repo: galacean/effects-runtime PR: 1118
File: packages/effects-core/src/components/ffd-component.ts:162-164
Timestamp: 2025-08-05T07:51:18.728Z
Learning: In packages/effects-core/src/components/ffd-component.ts, the //ts-expect-error usage for accessing siblingComponent.splits (around lines 162-164) is temporary debugging code that will be properly implemented later, as confirmed by ChengYi996.
📚 Learning: 2025-08-05T07:50:26.317Z
Learnt from: ChengYi996
Repo: galacean/effects-runtime PR: 1118
File: packages/effects-core/src/components/ffd-component.ts:135-219
Timestamp: 2025-08-05T07:50:26.317Z
Learning: In packages/effects-core/src/components/ffd-component.ts, the mesh subdivision logic (lines 147-219) within the collectSpriteComponents method is temporary debug code that will be properly implemented later, as confirmed by ChengYi996.
Applied to files:
plugin-packages/ffd/src/ffd-component.ts
📚 Learning: 2025-08-05T07:51:18.728Z
Learnt from: ChengYi996
Repo: galacean/effects-runtime PR: 1118
File: packages/effects-core/src/components/ffd-component.ts:162-164
Timestamp: 2025-08-05T07:51:18.728Z
Learning: In packages/effects-core/src/components/ffd-component.ts, the //ts-expect-error usage for accessing siblingComponent.splits (around lines 162-164) is temporary debugging code that will be properly implemented later, as confirmed by ChengYi996.
Applied to files:
plugin-packages/ffd/src/ffd-component.ts
🧬 Code graph analysis (1)
plugin-packages/ffd/src/ffd-component.ts (1)
packages/effects-core/src/utils/index.ts (1)
generateGUID(145-147)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
plugin-packages/ffd/src/ffd-component.ts (1)
10-10: LGTM: Feature flag added for GPU capability handling.The
enableFFDflag provides a clean way to disable FFD on devices with limited GPU capabilities while maintaining backward compatibility.
| if (this.enableFFD) { | ||
| const shader = new Shader(this.engine); | ||
|
|
||
| shader.fromData({ | ||
| vertex: vs, | ||
| fragment: fs, | ||
| id: generateGUID(), | ||
| dataType: spec.DataType.Shader, | ||
| }); | ||
| currentComponent.material.shader = shader; | ||
| } |
There was a problem hiding this comment.
Critical: updateShaderUniform doesn't respect the enableFFD flag.
While shader creation is correctly guarded by enableFFD, the updateShaderUniform method (lines 105-130) will still attempt to set uniforms even when FFD is disabled. This means it will try to set FFD-specific uniforms (_BoundMin, _BoundMax, _ControlPoints[], etc.) on the default shader, which likely doesn't have these uniforms.
🔎 Proposed fix
Add an early return in updateShaderUniform when FFD is disabled:
private updateShaderUniform (): void {
+ // Skip uniform updates when FFD is disabled
+ if (!this.enableFFD) {
+ return;
+ }
+
// 确保当前的 SpriteComponent 存在
if (!this.currentSpriteComponent) {
return;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In plugin-packages/ffd/src/ffd-component.ts (around lines 52-62 and 105-130),
updateShaderUniform currently runs even when this.enableFFD is false; add an
early return at the top of updateShaderUniform that checks if (!this.enableFFD)
return so the function exits before accessing/setting any FFD-specific uniforms
or touching the material.shader on non-FFD builds, preventing attempts to set
_BoundMin/_BoundMax/_ControlPoints[] on the default shader.
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.