feat: support multiple masks#1482
Open
ChengYi996 wants to merge 4 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors runtime masking from a single MaskMode enum to a MaskProcessor multi-reference API; updates fallback migration, components, plugins, tests, and adds a multi-mask demo. ChangesMulti-Mask Reference System Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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/components/base-render-component.ts (2)
321-325:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset the mask manager when
data.maskis absent.
fromData()only updates mask state inside theif (maskOptions)branch. On re-initialization from masked data to unmasked data, the previous references/flags stay live, so this component can keep stale stencil and alpha-mask behavior. Push an explicit empty mask config here, or call the clear/reset path before rebuildingthis.renderer.Proposed fix
const maskOptions = data.mask; - if (maskOptions) { - this.maskManager.setMaskOptions(this.engine, maskOptions); - } + if (maskOptions) { + this.maskManager.setMaskOptions(this.engine, maskOptions); + } else { + this.maskManager.setMaskOptions(this.engine, { references: [] }); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/effects-core/src/components/base-render-component.ts` around lines 321 - 325, fromData() only sets mask state when maskOptions exists, leaving previous mask live when data.mask is absent; update the branch so that when maskOptions is falsy you explicitly clear or reset the mask before rebuilding the renderer—e.g. call a clear/reset API (maskManager.clearMask or similar) or invoke this.maskManager.setMaskOptions(this.engine, {}) prior to recreating this.renderer so stale stencil/alpha-mask flags and references are removed; modify the block around maskOptions and this.maskManager.setMaskOptions to handle the absent-mask case.
327-335:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't hardcode the deprecated
renderer.maskto a truthy value.Lines 22-25 say this field is still kept for compatibility reads, but Line 334 makes every
MaskableGraphiclook masked even whendata.maskis missing. Any legacy consumer that still branches onrenderer.maskwill now misclassify unmasked components after upgrade. Derive it from the actual mask state instead of a constant.Proposed fix
+ const hasMaskState = + this.maskManager.isMask || this.maskManager.getMaskReferences().length > 0; + this.renderer = { renderMode: renderer.renderMode ?? spec.RenderMode.MESH, blending: renderer.blending ?? spec.BlendingMode.ALPHA, texture: renderer.texture ? this.engine.findObject<Texture>(renderer.texture) : this.engine.whiteTexture, occlusion: !!renderer.occlusion, transparentOcclusion: !!renderer.transparentOcclusion || this.maskManager.isMask, side: renderer.side ?? spec.SideMode.DOUBLE, - mask: 1, + mask: hasMaskState ? 1 : 0, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/effects-core/src/components/base-render-component.ts` around lines 327 - 335, The deprecated renderer.mask should not be hardcoded to 1; instead derive it from the real mask state so legacy consumers are preserved. In the this.renderer initialization (the object built inside base-render-component), set mask to the existing renderer.mask when provided, otherwise compute it from the actual mask state (e.g., use renderer.mask ?? (this.maskManager.isMask ? 1 : 0) or an equivalent truthy/falsey numeric value) so MaskableGraphic and any consumers that check renderer.mask behave correctly.
🧹 Nitpick comments (1)
web-packages/test/unit/src/effects-core/mask-processor.spec.ts (1)
373-390: ⚡ Quick winAlign the test name with asserted behavior.
Line 373 says this case verifies both warning and capping, but the assertions only validate capping. Please either assert the warning path (e.g., spy on warn) or rename the case to avoid overclaiming coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-packages/test/unit/src/effects-core/mask-processor.spec.ts` around lines 373 - 390, The test title overclaims by mentioning a warning but only asserts capping; update the test description to match the asserted behavior (e.g., change the it(...) title from "should warn and cap when mask references exceed the stencil limit" to "should cap mask references to the stencil limit when exceeding it") or alternatively add an assertion that verifies the warning path by spying on the logger/warn before calling MaskProcessor.setMaskOptions; references to help locate code: MaskProcessor, mp (the instance), setMaskOptions, and getMaskReferences.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/effects-core/src/material/mask-ref-manager.ts`:
- Around line 256-257: copyStencilArrayValue currently no-ops when the source
stencil arrays are undefined, which leaves previous stencil state in
prevStencilOpFail/prevStencilOpZFail (and the other prevStencil* arrays) and
causes state leakage across materials; change copyStencilArrayValue (the
function defined around lines 323-326) so that if the source is undefined it
explicitly clears the destination (e.g., set dest.length = 0 or set dest to
undefined/empty) rather than returning early, and keep the existing call sites
(the lines that copy into prevStencilOpFail/prevStencilOpZFail and the other
prevStencil* variables) so they will now correctly clear stale values when
material.stencilOpFail or material.stencilOpZFail (and the other stencil fields)
are undefined.
---
Outside diff comments:
In `@packages/effects-core/src/components/base-render-component.ts`:
- Around line 321-325: fromData() only sets mask state when maskOptions exists,
leaving previous mask live when data.mask is absent; update the branch so that
when maskOptions is falsy you explicitly clear or reset the mask before
rebuilding the renderer—e.g. call a clear/reset API (maskManager.clearMask or
similar) or invoke this.maskManager.setMaskOptions(this.engine, {}) prior to
recreating this.renderer so stale stencil/alpha-mask flags and references are
removed; modify the block around maskOptions and this.maskManager.setMaskOptions
to handle the absent-mask case.
- Around line 327-335: The deprecated renderer.mask should not be hardcoded to
1; instead derive it from the real mask state so legacy consumers are preserved.
In the this.renderer initialization (the object built inside
base-render-component), set mask to the existing renderer.mask when provided,
otherwise compute it from the actual mask state (e.g., use renderer.mask ??
(this.maskManager.isMask ? 1 : 0) or an equivalent truthy/falsey numeric value)
so MaskableGraphic and any consumers that check renderer.mask behave correctly.
---
Nitpick comments:
In `@web-packages/test/unit/src/effects-core/mask-processor.spec.ts`:
- Around line 373-390: The test title overclaims by mentioning a warning but
only asserts capping; update the test description to match the asserted behavior
(e.g., change the it(...) title from "should warn and cap when mask references
exceed the stencil limit" to "should cap mask references to the stencil limit
when exceeding it") or alternatively add an assertion that verifies the warning
path by spying on the logger/warn before calling MaskProcessor.setMaskOptions;
references to help locate code: MaskProcessor, mp (the instance),
setMaskOptions, and getMaskReferences.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e0f1b8a4-dd5c-4174-9ebd-c7693594bbb2
📒 Files selected for processing (17)
packages/effects-core/src/components/base-render-component.tspackages/effects-core/src/fallback/migration.tspackages/effects-core/src/material/mask-ref-manager.tspackages/effects-core/src/material/types.tspackages/effects-core/src/material/utils.tspackages/effects-core/src/plugins/particle/particle-mesh.tspackages/effects-core/src/plugins/particle/particle-system.tspackages/effects-core/src/plugins/particle/trail-mesh.tspackages/effects-core/src/plugins/shape/shape-component.tsplugin-packages/spine/src/slot-group.tsplugin-packages/spine/src/spine-component.tsplugin-packages/spine/src/spine-mesh.tsweb-packages/demo/html/multi-mask.htmlweb-packages/demo/index.htmlweb-packages/demo/src/multi-mask.tsweb-packages/demo/src/single.tsweb-packages/test/unit/src/effects-core/mask-processor.spec.ts
💤 Files with no reviewable changes (4)
- packages/effects-core/src/plugins/particle/particle-mesh.ts
- plugin-packages/spine/src/slot-group.ts
- packages/effects-core/src/plugins/particle/trail-mesh.ts
- packages/effects-core/src/plugins/particle/particle-system.ts
| export function processContent (composition: spec.CompositionData) { | ||
| //@ts-expect-error | ||
| for (const item of composition.items) { | ||
| const items = composition.items; |
Contributor
There was a problem hiding this comment.
这个数据转换要在新的 migration 函数上改。不然老版本数据会和编辑器发的对不上
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add support for multiple mask references per component, replacing the previous single-mask model. A component can now be clipped by multiple masks simultaneously (intersection of forward masks, with optional inverted masks to punch holes).
Core Changes (effects-core)
Spine Plugin (plugin-packages/spine)
Demo & Tests
Summary by CodeRabbit
New Features
Deprecations
Bug Fixes