feat: add frame component for clip content and draw background#1398
feat: add frame component for clip content and draw background#1398wumaolinmaoan wants to merge 9 commits intofeat/2.9from
Conversation
📝 WalkthroughWalkthroughIntroduces multi-mask MaskProcessor and numeric mask references, moves maskManager/frameClipMask into RendererComponent, adds FrameComponent and onPreRender hook, adds a SceneTicking pre-render phase, and renames Engine.render to mainLoop. Changes
Sequence Diagram(s)sequenceDiagram
participant Engine as Engine
participant SceneTick as SceneTicking
participant Item as RendererComponent
participant MaskMgr as MaskProcessor
participant GPU as GPU/RendererAPI
Engine->>SceneTick: preRender.tick()
SceneTick->>Item: onPreRender()
Engine->>Item: render(renderer)
Item->>MaskMgr: drawStencilMask(renderer, Item)
MaskMgr->>MaskMgr: determine/assign maskRef(s)
MaskMgr->>GPU: drawGeometryMask(geometry, worldMatrix, material, maskRef, subIndex)
GPU-->>MaskMgr: mask draw complete
Item->>GPU: draw geometry/material with stencil test
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 4
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/plugins/particle/trail-mesh.ts (1)
33-35:⚠️ Potential issue | 🟡 MinorRemove unused
maskandmaskModefields fromTrailMeshProps.These interface fields are declared but never consumed in the constructor. They remain only as dead code that creates confusion for callers.
🧹 Remove unused fields from TrailMeshProps
export type TrailMeshProps = { maxTrailCount: number, pointCountPerTrail: number, colorOverLifetime?: Array<GradientStop>, texture?: Texture, minimumVertexDistance: number, blending: number, widthOverTrail: ValueGetter<number>, colorOverTrail?: Array<GradientStop>, // order: number, matrix?: Matrix4, opacityOverLifetime: ValueGetter<number>, occlusion: boolean, transparentOcclusion: boolean, lifetime: ValueGetter<number>, - mask: number, shaderCachePrefix: string, - maskMode: number, textureMap: vec4, name: string, };packages/effects-core/src/plugins/particle/particle-mesh.ts (1)
89-90:⚠️ Potential issue | 🟡 MinorRemove unused
maskandmaskModefromParticleMeshProps— constructor never uses them.These interface fields are declared but the constructor doesn't destructure them (lines 150–157), making them dead code passed from particle-system.ts but ignored.
🧹 Remove unused fields from ParticleMeshProps
export interface ParticleMeshProps extends ParticleMeshData { renderMode?: number, blending?: number, - mask: number, - maskMode: number, side: number, transparentOcclusion?: boolean,
🤖 Fix all issues with AI agents
In `@packages/effects-core/src/components/frame-component.ts`:
- Around line 18-73: The clipGeometry allocated in onAwake via Geometry.create
is never released; add an override of onDestroy that checks this.clipGeometry
and calls its release/dispose method (the same API used elsewhere to free
Geometry instances) and then nulls the field; ensure the onDestroy lives on the
same class as onAwake so clipGeometry is cleaned up when the component is
destroyed (leave material handling to the base class if already covered).
In `@packages/effects-core/src/material/mask-ref-manager.ts`:
- Around line 210-224: setupMaskedMaterial currently enables stencilTest and
configures stencilRef/stencilMask/stencilFunc but doesn't reset stencilOpZPass,
which can leave a previous REPLACE from drawGeometryMask active and cause
unintended writes; update setupMaskedMaterial (method setupMaskedMaterial in
MaskRefManager/class) to explicitly set material.stencilOpZPass to
[glContext.KEEP, glContext.KEEP] (or equivalent KEEP for both entries) whenever
stencilTest is enabled so masked materials do not perform REPLACE on z-pass;
ensure the change complements the REPLACE set in drawGeometryMask to avoid stale
stencil operations.
In `@packages/effects-threejs/src/three-composition.ts`:
- Line 69: The call this.rootItem.getComponent(RendererComponent).render(render)
can NPE if getComponent returns null; modify the code in the three-composition
rendering path to first retrieve the component into a local (e.g., const
renderer = this.rootItem.getComponent(RendererComponent)) and guard it (if
(!renderer) return or skip) before calling renderer.render(render); ensure you
reference the rootItem, getComponent and RendererComponent symbols and avoid
invoking render on a possibly-null value.
In `@plugin-packages/spine/src/spine-component.ts`:
- Around line 176-177: this.materials is only initialized in onAwake from
content.meshGroups so newly created SpineMesh instances (added to
content.meshGroups during onUpdate) are never included when
maskManager.drawStencilMask() configures masks; update this.materials just
before maskManager.drawStencilMask() is called (e.g. at start of render()) or
whenever content.meshGroups changes by reassigning this.materials =
this.content?.meshGroups.map(mg => mg.mesh.material) ?? []; reference the
methods/fields this.materials, onAwake, render, maskManager.drawStencilMask,
content.meshGroups and the SpineMesh creation site to locate where to add the
refresh.
🧹 Nitpick comments (8)
packages/effects-core/src/components/composition-component.ts (2)
71-78: Per-frame array reallocation and sorting creates GC pressure.
collectRendererComponents()re-creates the array and re-traverses the item tree on everyrender()call. For compositions with many items, this incurs allocation overhead and sorting cost each frame.Consider caching the collected components and invalidating via a dirty flag when the item tree changes (children added/removed/reordered).
♻️ Sketch of a caching approach
+ private rendererComponentsDirty = true; override render (renderer: Renderer): void { - this.collectRendererComponents(); - this.rendererComponents.sort((a, b) => a.priority - b.priority); + if (this.rendererComponentsDirty) { + this.collectRendererComponents(); + this.rendererComponents.sort((a, b) => a.priority - b.priority); + this.rendererComponentsDirty = false; + } for (const rendererComponent of this.rendererComponents) { rendererComponent.render(renderer); } }You'd set
this.rendererComponentsDirty = truewhenever items are added, removed, or have their active state / priority changed.
285-291: Array is re-created every frame — reuse the array instead.Even without full dirty-flag caching, you can avoid the allocation by clearing the existing array:
♻️ Reuse the array
private collectRendererComponents (): void { - this.rendererComponents = []; + this.rendererComponents.length = 0; for (const item of this.item.children) { this.collectRendererComponentRecursive(item, this.rendererComponents); } }web-packages/imgui-demo/src/ge.ts (2)
2-2: Unused imports:ComponentandTextComponent.Neither
ComponentnorTextComponentis referenced anywhere in this file. Remove them to keep the import list clean.Suggested fix
-import { Component, FrameComponent, GLSLVersion, Geometry, Material, Player, RenderPass, RenderPassPriorityPostprocess, TextComponent, VFXItem, glContext, math } from '@galacean/effects'; +import { FrameComponent, GLSLVersion, Geometry, Material, Player, RenderPass, RenderPassPriorityPostprocess, VFXItem, glContext, math } from '@galacean/effects';
25-386: Large inline JSON payload with@ts-expect-errortype bypass.The
playURLmethod signature expects astring(line 389), but a raw object is passed here. This silences a real type mismatch. Consider either:
- Updating
playURLto acceptstring | object, or- Extracting the scene data to a separate
.jsonfile (similar to the existinganimationSceneimport at line 11) and passing the URL.This is a demo file, so this is a lower priority concern, but the
@ts-expect-errorhides potential issues ifplayURLinternals ever enforce thestringtype.packages/effects-core/src/components/frame-component.ts (2)
75-97: Per-frame recursive tree traversal may be expensive for deep hierarchies.
setClipRectangleRecursivewalks the entire descendant tree every frame inonUpdate. If the item tree is large or deeply nested, this adds non-trivial overhead. Consider setting clip masks once (e.g., inonStartor when the tree structure changes) and using a dirty flag or event-driven approach to re-traverse only when children are added/removed.
99-119: Duplicate world-matrix + scale logic betweenrender()anddrawStencilMask().Lines 100-103 and 113-116 are identical. Extract a shared helper to reduce duplication and keep the two paths in sync.
Additionally,
drawStencilMaskreaches forthis.engine.renderer(line 118) whilerenderreceives the renderer as a parameter. If these ever diverge (e.g. off-screen rendering), this could produce incorrect results. Consider whetherdrawStencilMaskshould also accept arendererparameter, or document whythis.engine.rendereris correct here.Proposed DRY refactor
+ private computeScaledWorldMatrix (): Matrix4 { + this.worldMatrix.copyFrom(this.transform.getWorldMatrix()); + multiplyMatrixByScale(this.worldMatrix, this.transform.size.x, this.transform.size.y); + + return this.worldMatrix; + } + override render (renderer: Renderer): void { - this.worldMatrix.copyFrom(this.transform.getWorldMatrix()); - multiplyMatrixByScale(this.worldMatrix, this.transform.size.x, this.transform.size.y); - renderer.drawGeometry(this.clipGeometry, this.worldMatrix, this.material); + renderer.drawGeometry(this.clipGeometry, this.computeScaledWorldMatrix(), this.material); } drawStencilMask (maskRef: number): void { if (!this.isActiveAndEnabled) { return; } - this.worldMatrix.copyFrom(this.transform.getWorldMatrix()); - multiplyMatrixByScale(this.worldMatrix, this.transform.size.x, this.transform.size.y); - this.maskManager.drawGeometryMask(this.engine.renderer, this.clipGeometry, this.worldMatrix, this.material, maskRef); + this.maskManager.drawGeometryMask(this.engine.renderer, this.clipGeometry, this.computeScaledWorldMatrix(), this.material, maskRef); }plugin-packages/spine/src/spine-component.ts (1)
199-203: Avoid allocatingVector2in the render loop.
new Vector2(...)is created every frame for every mesh group, causing GC pressure. Consider caching a temporaryVector2at the class or module level.♻️ Suggested fix
Add a module-level temp vector (near line 17 or as a private static):
+const tempSizeVec = new Vector2();Then in
render:- material.setVector2('_Size', new Vector2(this.startSize * this.scaleFactor, this.startSize * this.scaleFactor)); + const s = this.startSize * this.scaleFactor; + material.setVector2('_Size', tempSizeVec.set(s, s));Based on learnings: "avoid using
clone()methods because it can cause additional garbage collection overhead" — the same principle applies to unnecessarynewallocations in hot paths.packages/effects-core/src/material/mask-ref-manager.ts (1)
29-42:currentMaskIndexis never updated —getRefValue()always returns 1.
currentMaskIndexis initialized to 0 and never modified anywhere in the class. This meansgetRefValue()always returns1 << 0 = 1. In the new multi-mask system, bit allocation is handled dynamically indrawStencilMask(line 151:1 << i), makingcurrentMaskIndexvestigial.If
getRefValue()is still needed for backward compatibility (e.g., mesh props), consider either documenting that it always returns 1, or removingcurrentMaskIndexand simplifyinggetRefValue()toreturn 1.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/effects-core/src/composition/scene-ticking.ts (1)
50-53:⚠️ Potential issue | 🔴 Critical
clear()does not clear thepreRendertick data — will leak component references.
updateandlateUpdateare cleared, butpreRenderis not. When a composition is cleared/disposed, stale component references will remain inpreRender.components, leading to use-after-dispose errors whenpreRender.tick(0)is called.Proposed fix
clear (): void { this.update.clear(); this.lateUpdate.clear(); + this.preRender.clear(); }
🤖 Fix all issues with AI agents
In `@packages/effects-core/src/components/frame-component.ts`:
- Around line 85-97: Replace the single-component lookup in
setClipRectangleRecursive so every RendererComponent on a child gets the
frameClipMask: use child.getComponents(RendererComponent) to iterate all
renderer components, set rendererComponent.frameClipMask = this for each, and
detect whether any of those components is an instance of FrameComponent to
decide whether to recurse (i.e., only call this.setClipRectangleRecursive(child)
when no renderer component is a FrameComponent); update references to
RendererComponent, FrameComponent, setClipRectangleRecursive and VFXItem
accordingly.
In `@packages/effects-core/src/engine.ts`:
- Around line 279-281: The pre-render loop is calling
composition.sceneTicking.preRender.tick(0) for every composition without
checking whether the composition is disposed; add the same guard used in the
render loop (check composition.renderFrame.isDisposed or
comps[i].renderFrame.isDisposed) before invoking
composition.sceneTicking.preRender.tick(0) so you skip disposed compositions and
avoid calling preRender on ended/cleaned-up compositions.
🧹 Nitpick comments (2)
packages/effects-core/src/components/frame-component.ts (2)
75-97: Per-frame full subtree traversal inonPreRendermay be costly.
setClipRectangleRecursivewalks the entire item subtree every frame to reassignframeClipMaskon each descendant'sRendererComponent. If the item tree is large, this adds unnecessary overhead since the tree structure typically doesn't change frame-to-frame. Consider caching the list of affected components and only re-traversing when children are added/removed.
99-119: Duplicated world-matrix setup betweenrenderanddrawStencilMask.Both methods perform the same
copyFrom+multiplyMatrixByScalesequence. A small private helper would reduce duplication.Proposed refactor
+ private computeScaledWorldMatrix (): Matrix4 { + this.worldMatrix.copyFrom(this.transform.getWorldMatrix()); + multiplyMatrixByScale(this.worldMatrix, this.transform.size.x, this.transform.size.y); + + return this.worldMatrix; + } + override render (renderer: Renderer): void { - this.worldMatrix.copyFrom(this.transform.getWorldMatrix()); - multiplyMatrixByScale(this.worldMatrix, this.transform.size.x, this.transform.size.y); - renderer.drawGeometry(this.clipGeometry, this.worldMatrix, this.material); + renderer.drawGeometry(this.clipGeometry, this.computeScaledWorldMatrix(), this.material); } drawStencilMask (maskRef: number): void { if (!this.isActiveAndEnabled) { return; } - this.worldMatrix.copyFrom(this.transform.getWorldMatrix()); - multiplyMatrixByScale(this.worldMatrix, this.transform.size.x, this.transform.size.y); - this.maskManager.drawGeometryMask(this.engine.renderer, this.clipGeometry, this.worldMatrix, this.material, maskRef); + this.maskManager.drawGeometryMask(this.engine.renderer, this.clipGeometry, this.computeScaledWorldMatrix(), this.material, maskRef); }
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/effects-core/src/material/mask-ref-manager.ts`:
- Around line 142-165: The loop in mask-ref-manager allocating maskBit = 1 << i
for each entry can exceed the stencil bit depth (typical 8 bits) causing
collisions; add a guard before assigning maskBit that checks i against the
renderer/platform stencil bit depth (or a constant maxStencilBits) and
early-exits or skips additional maskReferences, and emit a dev-mode
warning/error referencing maskReferences, activeMaskBits, expectedMaskBits and
the reference.maskable.drawStencilMask call; ensure when skipping masks you do
not modify activeMaskBits/expectedMaskBits for those entries and document the
limit so callers can handle or flatten masks.
- Around line 181-188: drawGeometryMask in mask-ref-manager.ts only
saves/restores material.colorMask, so stencil state set by setupMaskMaterial
(stencilTest, stencilFunc, stencilOpZPass, stencilRef, stencilMask) leaks into
subsequent normal rendering; fix by capturing the current stencil properties
before calling setupMaskMaterial (e.g., previousStencilTest,
previousStencilFunc, previousStencilOpZPass, previousStencilRef,
previousStencilMask), call setupMaskMaterial(material, maskRef), perform the
draw, then restore material.colorMask and all saved stencil properties to their
previous values so the material metadata is unchanged after drawGeometryMask
returns.
- Around line 30-72: currentMaskIndex is never updated so getRefValue() always
returns 1; update currentMaskIndex inside the stencil mask drawing loop so it
reflects the mask currently being processed (e.g., set this.currentMaskIndex = i
before drawing each mask in the drawStencilMask loop that currently uses 1 <<
i), then restore or reset it after the loop if needed; ensure getRefValue(),
maskReferences and any callers (shape-component.ts, base-render-component.ts,
particle-system.ts) rely on that updated index and guard against exceeding
available bit capacity.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/effects-core/src/material/mask-ref-manager.ts`:
- Around line 138-178: drawStencilMask can remove an existing permanent mask
when maskedComponent.frameClipMask refers to the same Maskable already in
maskReferences because addMaskReference silently skips duplicates but
removeMaskReference unconditionally removes; change the logic so
addMaskReference returns a boolean (e.g., true when it actually inserted) and
capture that result in drawStencilMask, then only call removeMaskReference (or
clear maskedComponent.frameClipMask) if addMaskReference returned true;
alternatively, store a local flag (e.g., frameClipWasAdded) when calling
addMaskReference and check that flag before calling removeMaskReference—update
references to addMaskReference, removeMaskReference and the frameClipMask
handling in drawStencilMask accordingly.
| drawStencilMask (renderer: Renderer, maskedComponent: RendererComponent): void { | ||
| const frameClipMask = maskedComponent.frameClipMask; | ||
|
|
||
| if (frameClipMask) { | ||
| this.addMaskReference(frameClipMask, false); | ||
| } | ||
|
|
||
| if (this.maskReferences.length > 0) { | ||
| renderer.clear(this.stencilClearAction); | ||
| this.maskable.drawStencilMask(renderer); | ||
|
|
||
| // 重置 bit 组合 | ||
| this.activeMaskBits = 0; | ||
| this.expectedMaskBits = 0; | ||
|
|
||
| // 为每个蒙版分配一个 bit 并绘制 | ||
| for (let i = 0; i < this.maskReferences.length; i++) { | ||
| const maskBit = 1 << i; | ||
|
|
||
| this.activeMaskBits |= maskBit; | ||
|
|
||
| const reference = this.maskReferences[i]; | ||
|
|
||
| // 如果是正向蒙版（不是 inverted），期望该 bit 为 1 | ||
| if (!reference.inverted) { | ||
| this.expectedMaskBits |= maskBit; | ||
| } | ||
| // 如果是反向蒙版（inverted），期望该 bit 为 0（不设置） | ||
|
|
||
| // 传入 maskBit 作为 maskref 值 | ||
| reference.maskable.drawStencilMask(maskBit); | ||
| } | ||
| } | ||
|
|
||
| for (const material of maskedComponent.materials) { | ||
| this.setupMaskedMaterial(material); | ||
| } | ||
|
|
||
| if (frameClipMask) { | ||
| this.removeMaskReference(frameClipMask); | ||
| maskedComponent.frameClipMask = null; | ||
| } |
There was a problem hiding this comment.
drawStencilMask can inadvertently remove a permanent mask reference if frameClipMask is the same object.
If maskedComponent.frameClipMask happens to be the same Maskable instance already present in maskReferences, addMaskReference (line 142) skips the duplicate, but removeMaskReference (line 176) still removes the original entry. This silently drops a permanent mask reference after the first frame.
Consider guarding the removal:
Proposed fix
if (frameClipMask) {
- this.removeMaskReference(frameClipMask);
+ // Only remove if it was actually added (not a pre-existing reference)
+ const wasAdded = !this.maskReferences.some(ref => ref.maskable === frameClipMask) === false;
+ // simpler: track whether addMaskReference actually inserted
maskedComponent.frameClipMask = null;
}A cleaner approach — let addMaskReference return a boolean indicating whether the entry was inserted, then only remove if it was:
- addMaskReference (maskable: Maskable, inverted = false): void {
+ addMaskReference (maskable: Maskable, inverted = false): boolean {
const exists = this.maskReferences.some(ref => ref.maskable === maskable);
if (!exists) {
if (this.maskReferences.length >= 8) {
console.warn('Maximum of 8 mask references exceeded. Additional masks will be ignored.');
- return;
+ return false;
}
this.maskReferences.push({ maskable, inverted });
+
+ return true;
}
+
+ return false;
}Then in drawStencilMask:
+ let addedFrameClip = false;
+
if (frameClipMask) {
- this.addMaskReference(frameClipMask, false);
+ addedFrameClip = this.addMaskReference(frameClipMask, false);
}
// ... drawing logic ...
- if (frameClipMask) {
- this.removeMaskReference(frameClipMask);
+ if (frameClipMask && addedFrameClip) {
+ this.removeMaskReference(frameClipMask);
maskedComponent.frameClipMask = null;
}🤖 Prompt for AI Agents
In `@packages/effects-core/src/material/mask-ref-manager.ts` around lines 138 -
178, drawStencilMask can remove an existing permanent mask when
maskedComponent.frameClipMask refers to the same Maskable already in
maskReferences because addMaskReference silently skips duplicates but
removeMaskReference unconditionally removes; change the logic so
addMaskReference returns a boolean (e.g., true when it actually inserted) and
capture that result in drawStencilMask, then only call removeMaskReference (or
clear maskedComponent.frameClipMask) if addMaskReference returned true;
alternatively, store a local flag (e.g., frameClipWasAdded) when calling
addMaskReference and check that flag before calling removeMaskReference—update
references to addMaskReference, removeMaskReference and the frameClipMask
handling in drawStencilMask accordingly.
1 participant
Summary by CodeRabbit
New Features
Refactor
API