Conversation
WalkthroughThis PR removes multiple public API surfaces across the rendering pipeline, including custom viewport handling (isCustomViewport), render frame resource management (createResource, createCopyMesh, getRPAttachments), attachment reset methods (resetColorAttachments, resetAttachments), texture copy infrastructure (ExtWrap, CopyTexturePass), and FBOOptions parameters. Corresponding test coverage for these features is also removed or simplified. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/effects-core/src/render/render-frame.ts (1)
266-339: Viewport manipulation is confirmed to be dead code and should be removed.The verification confirms your review comment is correct. The
viewportvariable is created and modified throughout the gaussian pass setup loops (lines 267, 316–317, 320–321, 337–338), but it is never passed to any pass constructor or assigned as a property. Each gaussian pass independently calculates its render size via theonResize()method:
HQGaussianDownSamplePass.onResize()computes size asMath.floor(getWidth() / Math.pow(2, level + 1))HQGaussianUpSamplePass.onResize()computes size asMath.floor(getWidth() / Math.pow(2, level - 1))These methods are called at pass initialization (lines 152 and 236) and bound to the resize event, making the viewport manipulations entirely redundant.
- const viewport: vec4 = [0, 0, this.renderer.getWidth() / 2, this.renderer.getHeight() / 2]; const gaussianDownResults = new Array<RenderTargetHandle>(gaussianStep); ... for (let i = 0; i < gaussianStep; i++) { ... - viewport[2] /= 2; - viewport[3] /= 2; } - viewport[2] *= 4; - viewport[3] *= 4; for (let i = 0; i < gaussianStep - 1; i++) { ... - viewport[2] *= 2; - viewport[3] *= 2; }packages/effects-core/src/render/render-pass.ts (1)
331-332: Dead code -isCustomViewportandcustomViewportare genuinely unused.Verification confirms these private fields are never assigned or meaningfully used:
isCustomViewportis never read or written anywhere in the codebasecustomViewportis referenced only ingetViewport()(line 478) but never assigned, making it permanently undefinedRemove both fields and simplify
getViewport():- private isCustomViewport: boolean; - private customViewport?: [x: number, y: number, width: number, height: number];getViewport (): vec4 { - const ret = this.framebuffer?.viewport || this.customViewport; + const ret = this.framebuffer?.viewport; if (ret) { return ret; }
🧹 Nitpick comments (2)
packages/effects-threejs/src/three-render-pass.ts (1)
45-51: Emptydisposeoverride - verify intentional.The
disposemethod is empty, which may be intentional for this THREE.js adapter if cleanup is handled elsewhere. Consider adding a comment explaining why no cleanup is needed, or callsuper.dispose(options)if the base class disposal logic should apply.override dispose (options?: RenderPassDestroyOptions) { - + // THREE.js resources are managed externally; no disposal needed here. }packages/effects-core/src/render/render-pass.ts (1)
439-439: Naming inconsistency in viewport tuple type.The tuple uses
z: number, w: numberbut viewport dimensions are typically namedwidthandheight, notzandw. Consider using consistent naming:- const viewport: [x: number, y: number, z:number, w:number] = [0, 0, renderer.getWidth(), renderer.getHeight()]; + const viewport: [x: number, y: number, width: number, height: number] = [0, 0, renderer.getWidth(), renderer.getHeight()];
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/effects-core/src/render/framebuffer.ts(0 hunks)packages/effects-core/src/render/post-process-pass.ts(2 hunks)packages/effects-core/src/render/render-frame.ts(1 hunks)packages/effects-core/src/render/render-pass.ts(1 hunks)packages/effects-threejs/src/three-render-pass.ts(1 hunks)packages/effects-webgl/src/ext-wrap.ts(0 hunks)packages/effects-webgl/src/gl-framebuffer.ts(1 hunks)packages/effects-webgl/src/gl-renderer.ts(0 hunks)plugin-packages/model/src/runtime/cache.ts(0 hunks)plugin-packages/model/src/utility/plugin-helper.ts(1 hunks)web-packages/test/unit/src/effects-webgl/gl-dispose.spec.ts(0 hunks)web-packages/test/unit/src/effects-webgl/gl-frame-buffer.spec.ts(0 hunks)web-packages/test/unit/src/effects-webgl/gl-render-pass.spec.ts(0 hunks)web-packages/test/unit/src/effects-webgl/renderer.spec.ts(2 hunks)
💤 Files with no reviewable changes (7)
- packages/effects-core/src/render/framebuffer.ts
- plugin-packages/model/src/runtime/cache.ts
- web-packages/test/unit/src/effects-webgl/gl-dispose.spec.ts
- web-packages/test/unit/src/effects-webgl/gl-render-pass.spec.ts
- packages/effects-webgl/src/ext-wrap.ts
- web-packages/test/unit/src/effects-webgl/gl-frame-buffer.spec.ts
- packages/effects-webgl/src/gl-renderer.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-10-18T09:15:08.038Z
Learnt from: wumaolinmaoan
Repo: galacean/effects-runtime PR: 691
File: packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts:39-42
Timestamp: 2024-10-18T09:15:08.038Z
Learning: In `packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts`, when accumulating color components, avoid using `clone()` methods because it can cause additional garbage collection overhead.
Applied to files:
web-packages/test/unit/src/effects-webgl/renderer.spec.tspackages/effects-threejs/src/three-render-pass.tspackages/effects-core/src/render/render-frame.ts
📚 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:
packages/effects-core/src/render/render-frame.ts
📚 Learning: 2024-10-17T07:15:16.523Z
Learnt from: liuxi150
Repo: galacean/effects-runtime PR: 0
File: :0-0
Timestamp: 2024-10-17T07:15:16.523Z
Learning: Imports in `plugin-packages/model/src/utility/plugin-helper.ts` do not need to be merged as per project preference or coding standards.
Applied to files:
plugin-packages/model/src/utility/plugin-helper.ts
🧬 Code graph analysis (2)
web-packages/test/unit/src/effects-webgl/renderer.spec.ts (1)
packages/effects-webgl/src/gl-renderer.ts (1)
gl(36-38)
packages/effects-core/src/render/render-pass.ts (3)
packages/effects-webgl/src/gl-engine.ts (1)
viewport(509-511)packages/effects/src/player.ts (1)
renderer(66-68)packages/effects-threejs/src/three-display-object.ts (1)
renderer(39-41)
⏰ 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 (8)
plugin-packages/model/src/utility/plugin-helper.ts (1)
1-10: Inconsistency:RenderPassimport retained despite summary claiming removal.The AI summary states that
RenderPasswas removed from the@galacean/effectsimport group (Line 5), but the import still appears in the final code. Furthermore,RenderPassremains actively used on Line 335 in thedeleteRenderPass()method signature and Line 340-341 inRenderPassDestroyAttachmentTypereferences.Either this represents an incomplete refactor (the import should have been removed but wasn't), or the summary is inaccurate (the import should remain). Please clarify the intended state and ensure consistency between the imports and their actual usage.
packages/effects-core/src/render/post-process-pass.ts (1)
150-155: LGTM - Initialize overrides ensure proper initial sizing.The
initializemethods correctly callsuper.initialize(renderer)followed bythis.onResize()to set framebuffer dimensions based on the level during initialization.One observation:
BloomThresholdPasshas a similaronResizehandler bound in its constructor (line 57-58) but lacks aninitializeoverride. If the framebuffer needs to be sized correctly before the first frame, consider adding a similar override there for consistency. However, since it uses the full renderer dimensions (not level-based), it may rely on the base class initialization being sufficient.Also applies to: 234-239
packages/effects-webgl/src/gl-framebuffer.ts (1)
34-36: LGTM - Clean removal ofisCustomViewportflag.The removal of
isCustomViewportfrom the constructor destructuring aligns with the broader API simplification. Viewport handling continues to work through the remainingviewportproperty.web-packages/test/unit/src/effects-webgl/renderer.spec.ts (2)
1-1: LGTM - Import cleanup aligns with API simplification.The simplified imports reflect the removal of texture copy and custom viewport test coverage, keeping only what's needed for the remaining test cases.
34-34: Good -constis appropriate here.The change from
lettoconstis correct sincetexis not reassigned.packages/effects-threejs/src/three-render-pass.ts (1)
1-6: LGTM - Import cleanup consistent with API removal.The removal of
TextureandRenderPassAttachmentOptionsimports aligns with the removal ofresetColorAttachmentsandresetAttachmentsmethods from the base class.packages/effects-core/src/render/render-frame.ts (2)
3-23: LGTM - Import reorganization is clean.The imports are properly organized with type-only imports where appropriate, aligning with the API simplification across the render pipeline.
76-97: I'll help you verify whether theRenderFrameResourceinterface is still used in the codebase. Let me search for all occurrences of this interface.
<function_calls>
#!/bin/bashSearch for all occurrences of RenderFrameResource in the codebase
echo "=== Searching for RenderFrameResource usage ==="
rg -n 'RenderFrameResource' --type=ts
</function_calls>
<function_calls>
#!/bin/bashGet more context around usages - check imports and exports
echo "=== Checking for imports and exports ==="
rg -n 'RenderFrameResource' --type=ts -B2 -A2
</function_calls>
<function_calls>
#!/bin/bashCheck if it's exported from index files or barrel exports
echo "=== Checking for exports in barrel/index files ==="
fd -e ts -e js | xargs grep -l 'export.*RenderFrameResource' 2>/dev/null || echo "No explicit exports found in index files"Check the render-frame.ts file itself for exports
echo ""
echo "=== Checking render-frame.ts exports ==="
grep -n 'export' packages/effects-core/src/render/render-frame.ts | head -20
</function_calls>
2 participants
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.