Feat/batched feather rendering#1438
Open
ZJUSaltFish wants to merge 11 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 with no reviewable changes (1)
📝 WalkthroughWalkthroughMajor refactoring of the feather mesh rendering system replacing stroke-based contour construction with union-normalized contour processing, introducing scatter-edge simplification utility, adding adaptive gather-based rendering path, and extending render target pool with anisotropic filtering support. Changes
Sequence Diagram(s)sequenceDiagram
participant SC as ShapeComponent
participant VFR as VectorFeatherRenderer
participant GP as GatherPass
participant IP as IndicatorPass
participant SP as ScatterPass
rect rgb(100, 150, 200, 0.5)
Note over SC,SP: Old Rendering Path
SC->>VFR: render()
VFR->>IP: drawIndicatorPass()
IP-->>VFR: rendered
VFR->>SP: drawScatterPass()
SP-->>VFR: rendered
VFR-->>SC: complete
end
rect rgb(100, 200, 150, 0.5)
Note over SC,SP: New Rendering Path (High Coverage)
SC->>VFR: render() with params.kernelCoverage
alt kernelCoverage >= threshold
VFR->>VFR: updateUpsampleQuad()
VFR->>GP: drawGatherPass(featherRadiusScreen)
GP-->>VFR: rendered via upsampleGeometry
else kernelCoverage < threshold
VFR->>IP: drawIndicatorPass()
IP-->>VFR: rendered
VFR->>SP: drawScatterPass()
SP-->>VFR: rendered
end
VFR-->>SC: complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/effects-core/src/math/shape/vector-feather-renderer.ts (3)
14-14: Remove unused importremoveShortEdgesFloat.ESLint reports this import is never used in the file.
♻️ Proposed fix
-import { simplifyScatterEdges, removeShortEdges, removeShortEdgesFloat } from './scatter-edge-simplifier'; +import { simplifyScatterEdges, removeShortEdges } from './scatter-edge-simplifier';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects-core/src/math/shape/vector-feather-renderer.ts` at line 14, Remove the unused import removeShortEdgesFloat from the import statement in vector-feather-renderer (currently importing simplifyScatterEdges, removeShortEdges, removeShortEdgesFloat from './scatter-edge-simplifier'); update the import to only include the actually used symbols (simplifyScatterEdges and removeShortEdges) so ESLint no longer reports an unused import.
320-333: Useconstinstead ofvarand prefer strict equality.Line 324 uses
varwhich is flagged by ESLint. Line 327 uses loose equality (!=) withundefined.♻️ Proposed fix
- drawGatherPass(renderer: Renderer, orthoProjection: Matrix4, featherRadius: number): void{ + drawGatherPass (renderer: Renderer, orthoProjection: Matrix4, featherRadius: number): void { this.gatherMaterial.setMatrix('uProjection', orthoProjection); this.gatherMaterial.setFloat('uRadius', featherRadius); - this.gatherMaterial.setInt('uMeshStart', 0); - var numEdges : number = this.scatterEdgeTexture?.getWidth() || 0; - this.gatherMaterial.setInt('uMeshEnd', numEdges); + this.gatherMaterial.setInt('uMeshStart', 0); + const numEdges: number = this.scatterEdgeTexture?.getWidth() || 0; + this.gatherMaterial.setInt('uMeshEnd', numEdges); this.gatherMaterial.setInt('uEdgeTextureWidth', numEdges); - if (this.scatterEdgeTexture != undefined){ + if (this.scatterEdgeTexture !== undefined) { this.gatherMaterial.setTexture('uEdgeTexture', this.scatterEdgeTexture); } - renderer.drawGeometry( + renderer.drawGeometry( this.upsampleGeometry, Matrix4.IDENTITY, this.gatherMaterial ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects-core/src/math/shape/vector-feather-renderer.ts` around lines 320 - 333, In drawGatherPass, replace the use of "var numEdges" with "const numEdges" (or let if reassignment is needed) when computing numEdges from this.scatterEdgeTexture?.getWidth(), and change the scatterEdgeTexture check from "!= undefined" to a strict comparison "!== undefined" (or simply truthiness check) before calling this.gatherMaterial.setTexture; update references to numEdges and the conditional to use these strict/const changes (symbols: drawGatherPass, numEdges, scatterEdgeTexture, gatherMaterial).
399-411: Acknowledged:updateUpsampleQuadis called twice in gather path.The TODO at line 403 notes this redundancy. In the gather branch (lines 402-405),
updateUpsampleQuadis called beforedrawGatherPass, and then again unconditionally at line 411 beforedrawUpsamplePass. This is functionally correct but could be optimized by moving the unconditional call (line 411) into the scatter branch only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects-core/src/math/shape/vector-feather-renderer.ts` around lines 399 - 411, The updateUpsampleQuad call is duplicated in the gather path (called inside the else before drawGatherPass and again unconditionally before the upsample pass); remove the updateUpsampleQuad invocation from the else branch (the one before drawGatherPass) and instead call updateUpsampleQuad from the scatter branch (i.e., after drawScatterPass) so updateUpsampleQuad runs exactly once before drawUpsamplePass; update the logic around drawIndicatorPass, drawScatterPass, drawGatherPass, and drawUpsamplePass to reflect this single-call change.packages/effects-core/src/math/shape/feather-mesh-builder.ts (1)
1-1: Remove unused importssplitandunionMax.ESLint reports these imports are never used in this file.
♻️ Proposed fix
-import { split, union, unionMax } from "./split-union"; +import { union } from "./split-union";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects-core/src/math/shape/feather-mesh-builder.ts` at line 1, The import line currently brings in split and unionMax which are unused; update the import from "./split-union" in feather-mesh-builder.ts to only import the symbol that is actually used (keep union, remove split and unionMax) so ESLint no longer reports unused imports.packages/effects-core/src/math/shape/scatter-edge-simplifier.ts (1)
168-180: Edge case:simplifyClosedContourreturns closed edges forn < 3.When
n < 3, the function returns edges connecting each point to the next (modulon). Forn=1, this creates a degenerate edge from point 0 to point 0. Forn=2, it creates two edges forming a line segment (point 0→1 and 1→0).This may not be the intended behavior for degenerate inputs. Consider returning an empty array for
n < 3.♻️ Proposed fix
function simplifyClosedContour (points: Point[], targetEdges: number): number[] { const n = points.length; - if (n <= targetEdges || n < 3) { + if (n < 3) { + return []; + } + + if (n <= targetEdges) { const result: number[] = []; for (let i = 0; i < n; i++) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects-core/src/math/shape/scatter-edge-simplifier.ts` around lines 168 - 180, simplifyClosedContour currently emits closed edges when n < 3 (producing degenerate self-edges for n=1 and duplicated opposite edges for n=2); update simplifyClosedContour to explicitly return an empty array when n < 3 (before the loop that builds edges) while preserving the existing behavior for n >= 3 and the targetEdges check so callers receive no edges for degenerate contours.packages/effects-core/src/math/shape/split-union.ts (1)
18-20: Error callback only logs without tessellator state recovery.Similar to the existing
triangulate.tspattern (context snippet 2), if tessellation fails the singleton instance may be left in an inconsistent state. Subsequent calls could produce corrupted output. Consider adding a brief comment documenting this limitation, or implement tessellator recreation on error if robustness is critical.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects-core/src/math/shape/split-union.ts` around lines 18 - 20, The errorCallback in split-union.ts currently only logs the error (function errorCallback) which can leave the shared tessellator singleton in an inconsistent state; follow the triangulate.ts pattern by either adding a short comment warning about potential stale tessellator state or, for robustness, recreate/reset the tessellator instance inside errorCallback (or invoke the same tessellator-recreation helper used by triangulate.ts) so subsequent calls start from a clean state and avoid corrupted output. Ensure you reference and reuse the existing tessellator reset/creation routine rather than duplicating logic if one exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/effects-core/feather-report.md`:
- Around line 9-15: The table row label "Scatter（最大降采样=4）" appears twice; change
the second occurrence to "Gather（最大降采样=4）" so the modes read "Scatter（最大降采样=4）"
and "Gather（最大降采样=4）", ensuring the performance numbers remain aligned with
their correct mode in the feather-report.md table.
In `@packages/effects-core/src/math/shape/split-union.ts`:
- Around line 125-142: splitMax and unionMax call resolveContour(...) then
immediately reduce the result, which throws when resolveContour returns an empty
array; update both functions (splitMax and unionMax) to handle empty output from
resolveContour by checking if output.length === 0 and returning an empty array
(or a sensible default) before calling reduce, or alternatively provide a safe
initial value to reduce; ensure you reference resolveContour and signedArea when
locating the change.
---
Nitpick comments:
In `@packages/effects-core/src/math/shape/feather-mesh-builder.ts`:
- Line 1: The import line currently brings in split and unionMax which are
unused; update the import from "./split-union" in feather-mesh-builder.ts to
only import the symbol that is actually used (keep union, remove split and
unionMax) so ESLint no longer reports unused imports.
In `@packages/effects-core/src/math/shape/scatter-edge-simplifier.ts`:
- Around line 168-180: simplifyClosedContour currently emits closed edges when n
< 3 (producing degenerate self-edges for n=1 and duplicated opposite edges for
n=2); update simplifyClosedContour to explicitly return an empty array when n <
3 (before the loop that builds edges) while preserving the existing behavior for
n >= 3 and the targetEdges check so callers receive no edges for degenerate
contours.
In `@packages/effects-core/src/math/shape/split-union.ts`:
- Around line 18-20: The errorCallback in split-union.ts currently only logs the
error (function errorCallback) which can leave the shared tessellator singleton
in an inconsistent state; follow the triangulate.ts pattern by either adding a
short comment warning about potential stale tessellator state or, for
robustness, recreate/reset the tessellator instance inside errorCallback (or
invoke the same tessellator-recreation helper used by triangulate.ts) so
subsequent calls start from a clean state and avoid corrupted output. Ensure you
reference and reuse the existing tessellator reset/creation routine rather than
duplicating logic if one exists.
In `@packages/effects-core/src/math/shape/vector-feather-renderer.ts`:
- Line 14: Remove the unused import removeShortEdgesFloat from the import
statement in vector-feather-renderer (currently importing simplifyScatterEdges,
removeShortEdges, removeShortEdgesFloat from './scatter-edge-simplifier');
update the import to only include the actually used symbols
(simplifyScatterEdges and removeShortEdges) so ESLint no longer reports an
unused import.
- Around line 320-333: In drawGatherPass, replace the use of "var numEdges" with
"const numEdges" (or let if reassignment is needed) when computing numEdges from
this.scatterEdgeTexture?.getWidth(), and change the scatterEdgeTexture check
from "!= undefined" to a strict comparison "!== undefined" (or simply truthiness
check) before calling this.gatherMaterial.setTexture; update references to
numEdges and the conditional to use these strict/const changes (symbols:
drawGatherPass, numEdges, scatterEdgeTexture, gatherMaterial).
- Around line 399-411: The updateUpsampleQuad call is duplicated in the gather
path (called inside the else before drawGatherPass and again unconditionally
before the upsample pass); remove the updateUpsampleQuad invocation from the
else branch (the one before drawGatherPass) and instead call updateUpsampleQuad
from the scatter branch (i.e., after drawScatterPass) so updateUpsampleQuad runs
exactly once before drawUpsamplePass; update the logic around drawIndicatorPass,
drawScatterPass, drawGatherPass, and drawUpsamplePass to reflect this
single-call change.
🪄 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: 4bb61766-0504-414d-900b-1315d482a371
⛔ Files ignored due to path filters (4)
packages/effects-core/src/math/shape/shaders/feather-gather.frag.glslis excluded by!**/*.glslpackages/effects-core/src/math/shape/shaders/feather-gather.vert.glslis excluded by!**/*.glslpackages/effects-core/src/math/shape/shaders/feather-scatter.frag.glslis excluded by!**/*.glslpackages/effects-core/src/math/shape/shaders/feather-upsample.frag.glslis excluded by!**/*.glsl
📒 Files selected for processing (8)
.gitignorepackages/effects-core/feather-report.mdpackages/effects-core/src/components/shape-component.tspackages/effects-core/src/math/shape/feather-mesh-builder.tspackages/effects-core/src/math/shape/scatter-edge-simplifier.tspackages/effects-core/src/math/shape/split-union.tspackages/effects-core/src/math/shape/vector-feather-renderer.tspackages/effects-core/src/render/feather-offscreen-pass.ts
Comment on lines
+9
to
+15
| | Mode /FPS | Radius=10 | Radius=20 | Radius=30 | Radius=40 | | ||
| | ----------------------- | --------- | --------- | --------- | --------- | | ||
| | Scatter（不限制降采样） | 60 | 60 | 60 | 60 | | ||
| | Gather（不限制降采样） | 60 | 60 | 60 | 60 | | ||
| | Scatter（最大降采样=4） | 60 | 56 | 33 | 20 | | ||
| | Scatter（最大降采样=4） | 60 | 34 | 20 | 13 | | ||
|
|
Contributor
There was a problem hiding this comment.
Possible typo: Lines 13 and 14 both show "Scatter（最大降采样=4）".
The table appears to have a duplicate mode label. Should line 14 be "Gather（最大降采样=4）" instead?
📝 Proposed fix
| Scatter（不限制降采样） | 60 | 60 | 60 | 60 |
| Gather（不限制降采样） | 60 | 60 | 60 | 60 |
| Scatter（最大降采样=4） | 60 | 56 | 33 | 20 |
-| Scatter（最大降采样=4） | 60 | 34 | 20 | 13 |
+| Gather（最大降采样=4） | 60 | 34 | 20 | 13 |📝 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.
Suggested change
| | Mode /FPS | Radius=10 | Radius=20 | Radius=30 | Radius=40 | | |
| | ----------------------- | --------- | --------- | --------- | --------- | | |
| | Scatter（不限制降采样） | 60 | 60 | 60 | 60 | | |
| | Gather（不限制降采样） | 60 | 60 | 60 | 60 | | |
| | Scatter（最大降采样=4） | 60 | 56 | 33 | 20 | | |
| | Scatter（最大降采样=4） | 60 | 34 | 20 | 13 | | |
| | Mode /FPS | Radius=10 | Radius=20 | Radius=30 | Radius=40 | | |
| | ----------------------- | --------- | --------- | --------- | --------- | | |
| | Scatter（不限制降采样） | 60 | 60 | 60 | 60 | | |
| | Gather（不限制降采样） | 60 | 60 | 60 | 60 | | |
| | Scatter（最大降采样=4） | 60 | 56 | 33 | 20 | | |
| | Gather（最大降采样=4） | 60 | 34 | 20 | 13 | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/effects-core/feather-report.md` around lines 9 - 15, The table row
label "Scatter（最大降采样=4）" appears twice; change the second occurrence to
"Gather（最大降采样=4）" so the modes read "Scatter（最大降采样=4）" and "Gather（最大降采样=4）",
ensuring the performance numbers remain aligned with their correct mode in the
feather-report.md table.
| @@ -0,0 +1,57 @@ | |||
| ## 测试 | |||
| api | ||
| www | ||
| **/__definations__/*.ts | ||
| AGENTS.md |
… 1 px for padding; set min feathering to 1 px
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
packages/effects-core/src/math/shape/build-line.ts (2)
706-717: Square cap extension vector simplification.Lines 708-709 compute
exx = perpY / width * widthwhich simplifies to justperpY. The division and multiplication cancel out.♻️ Simplify redundant computation
} else if (style.cap === spec.LineCap.Square) { // square cap 直接在 inner/outer 各加一个延伸点 - const exx = perpY / width * width; // 方向向量 - const eyy = -perpX / width * width; + const exx = perpY; // 方向向量 (magnitude = width) + const eyy = -perpX; innerVerts.push(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects-core/src/math/shape/build-line.ts` around lines 706 - 717, The computation for the square cap extension vector in the block checking style.cap === spec.LineCap.Square is redundant: replace exx = perpY / width * width with exx = perpY (and similarly eyy = -perpX / width * width with eyy = -perpX) inside the build-line logic so the extension vector uses the simpler perp components (variables exx and eyy) when pushing to innerVerts and outerVerts.
819-847: FixMe comments indicate uncertainty about duplicate vertices.Multiple
// FixMe: 重复顶点在这里有必要吗？comments suggest the author is uncertain whether duplicate vertex pushes are necessary. Consider:
- Documenting why duplicates are needed (if they are), or
- Removing them if they're unnecessary artifacts from the original algorithm
Would you like me to analyze the vertex deduplication impact on the resulting contours?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects-core/src/math/shape/build-line.ts` around lines 819 - 847, The FixMe comments in build-line.ts indicate uncertain duplicate pushes to innerVerts/outerVerts around the round-join and other join branches; inspect the logic in the function handling line joins (look for the style.join === spec.LineJoin.Round branch, the clockwise flag, and the pushRoundFan calls) and either remove redundant innerVerts.push/outerVerts.push calls that push the same coordinates twice (ensuring mesh winding/triangle generation still works), or replace each questionable duplicate with a clear comment explaining why the duplicate vertex is required for correct triangle fan continuity; verify changes by checking the sequences passed to pushRoundFan and any downstream triangulation that expects the duplicated boundary vertex.packages/effects-core/src/render/feather-offscreen-pass.ts (1)
10-10: Use type-only imports for types.
FeatherRenderParamsandVectorFeatherRendererare only used as types here. Usingimport typeimproves tree-shaking.♻️ Proposed fix
-import { FeatherRenderParams, VectorFeatherRenderer, getExpandedRadius } from '../math/shape/vector-feather-renderer'; +import type { FeatherRenderParams, VectorFeatherRenderer } from '../math/shape/vector-feather-renderer'; +import { getExpandedRadius } from '../math/shape/vector-feather-renderer';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects-core/src/render/feather-offscreen-pass.ts` at line 10, The import line brings in FeatherRenderParams and VectorFeatherRenderer but they are only used as types; change their imports to type-only imports so the line uses "import type" for FeatherRenderParams and VectorFeatherRenderer while keeping getExpandedRadius as a normal import; update the import statement in packages/effects-core/src/render/feather-offscreen-pass.ts that references FeatherRenderParams and VectorFeatherRenderer accordingly so only getExpandedRadius remains a runtime import.packages/effects-core/src/math/shape/split-union.ts (2)
3-47: Singleton tessellator may not be safe for reentrant/concurrent calls.
tessySplitandtessyUnionare module-level singletons. IfresolveContourorresolveContoursis called reentrantly (e.g., from within a callback during tessellation) or concurrently (in a worker context), the shared state could be corrupted.In the current single-threaded synchronous usage, this is likely fine. However, consider adding a comment documenting this limitation.
📝 Suggested documentation
+/** + * Singleton tessellator for split (ODD winding rule). + * Note: Not safe for reentrant or concurrent calls. + */ const tessySplit = (function initSplitTesselator() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects-core/src/math/shape/split-union.ts` around lines 3 - 47, tessySplit (and its sibling tessyUnion) is a module-level singleton and can be corrupted if tessellation is reentrant or called concurrently (e.g., from resolveContour/resolveContours during a callback or across workers); either create a fresh libtess.GluTesselator per call or document this limitation clearly: add a top-of-file comment near the tessySplit/tessyUnion definitions stating they are singletons not safe for reentrant or concurrent use, and recommend using a per-call instance or external synchronization (mutex/queue) if reentrancy/concurrency is required.
117-120: Inconsistent indentation.Lines 118-119 use 6-space indentation instead of the 2-space convention used elsewhere in this file.
♻️ Fix indentation
if (needsClose) { - const closeCoords = [firstX, firstY, 0]; - tessy.gluTessVertex(closeCoords, closeCoords); + const closeCoords = [firstX, firstY, 0]; + tessy.gluTessVertex(closeCoords, closeCoords); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects-core/src/math/shape/split-union.ts` around lines 117 - 120, The block that adds the closing vertex is indented with 6 spaces instead of the project's 2-space convention; reformat the if (needsClose) { ... } block so the lines declaring closeCoords and calling tessy.gluTessVertex(closeCoords, closeCoords); use the same 2-space indentation as surrounding code (adjust spacing for the closeCoords declaration and the tessy.gluTessVertex call to align with the file's convention).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/effects-core/src/math/shape/build-line.ts`:
- Around line 788-793: Fix the swapped weight usage when pushing vertices after
the 180° corner handling: in the block that pushes to innerVerts and outerVerts
(using symbols innerVerts, outerVerts, perp1x, perp1y, innerWeight, outerWeight,
x1, y1) replace the inverted values so innerVerts receives perp1*innerWeight and
outerVerts receives perp1*outerWeight, matching the correct pattern used later
(the same logic around buildLine/build-line functions).
In `@packages/effects-core/src/math/shape/feather-mesh-builder.ts`:
- Line 1: The import statement in feather-mesh-builder.ts brings in unused
symbols `split` and `splitContours`; remove those from the import to avoid
unused imports and lint errors by changing the import from `import { split,
union, unionContours, splitContours } from "./split-union";` to only include the
used symbols `union` and `unionContours` so the file imports `import { union,
unionContours } from "./split-union";`.
In `@packages/effects-core/src/math/shape/vector-feather-renderer.ts`:
- Around line 262-269: computeRenderParams currently mutates this.featherRadius
when computing screen-space adjustments, which can cause drift; change the
implementation in computeRenderParams (or the method containing
computeScreenRadius, refinedScreenRadius, and the current this.featherRadius
assignment) to stop mutating the instance property: compute refinedScreenRadius
as a local variable (e.g., refinedFeatherRadius) and use/return that local value
to downstream code that needs the adjusted radius instead of writing back to
this.featherRadius; if mutation was intentional, add an explicit comment and a
clearly named method (e.g., applyFeatherRadiusAdjustment) so callers opt into
the side effect.
- Line 14: The import list includes an unused symbol removeShortEdgesFloat;
remove it from the import statement so only used exports (simplifyScatterEdges,
removeShortEdges) are imported. Locate the import line that references
simplifyScatterEdges, removeShortEdges, removeShortEdgesFloat and delete
removeShortEdgesFloat from that list to eliminate the unused import.
- Around line 346-351: Replace the mutable/loose declarations and comparisons:
change the `var numEdges` declaration to `const numEdges` (computed from
`this.scatterEdgeTexture?.getWidth() || 0`) and use strict equality when
checking the texture by replacing `if (this.scatterEdgeTexture != undefined)`
with `if (this.scatterEdgeTexture !== undefined)`, keeping the rest of the logic
that calls `this.gatherMaterial.setInt('uMeshEnd', numEdges)`,
`setInt('uEdgeTextureWidth', numEdges)`, and `setTexture('uEdgeTexture',
this.scatterEdgeTexture)` unchanged.
- Around line 594-598: getExpandedRadius currently divides by
featherRadiusScreen which can be zero; update getExpandedRadius to defensively
guard against division-by-zero by clamping featherRadiusScreen to a safe minimum
(e.g., 1 or a small epsilon) or returning a sensible fallback (like
featherRadius) when featherRadiusScreen is 0 or extremely small. Locate the
function getExpandedRadius in vector-feather-renderer.ts and modify the
calculation to use the clamped value (or conditional fallback) so the return
never becomes Infinity or NaN.
In `@packages/effects-core/src/render/render-target-pool.ts`:
- Around line 51-54: The cache key generation for the Framebuffer pool omits the
anisotropic parameter, so the computed `hash` variable can cause textures with
different anisotropic settings to be reused incorrectly; update the `hash`
construction in the function that returns a Framebuffer to include the
`anisotropic` value (e.g., append `${anisotropic}` into the template used to
build `hash`) so each unique anisotropic setting yields a distinct cache key.
---
Nitpick comments:
In `@packages/effects-core/src/math/shape/build-line.ts`:
- Around line 706-717: The computation for the square cap extension vector in
the block checking style.cap === spec.LineCap.Square is redundant: replace exx =
perpY / width * width with exx = perpY (and similarly eyy = -perpX / width *
width with eyy = -perpX) inside the build-line logic so the extension vector
uses the simpler perp components (variables exx and eyy) when pushing to
innerVerts and outerVerts.
- Around line 819-847: The FixMe comments in build-line.ts indicate uncertain
duplicate pushes to innerVerts/outerVerts around the round-join and other join
branches; inspect the logic in the function handling line joins (look for the
style.join === spec.LineJoin.Round branch, the clockwise flag, and the
pushRoundFan calls) and either remove redundant innerVerts.push/outerVerts.push
calls that push the same coordinates twice (ensuring mesh winding/triangle
generation still works), or replace each questionable duplicate with a clear
comment explaining why the duplicate vertex is required for correct triangle fan
continuity; verify changes by checking the sequences passed to pushRoundFan and
any downstream triangulation that expects the duplicated boundary vertex.
In `@packages/effects-core/src/math/shape/split-union.ts`:
- Around line 3-47: tessySplit (and its sibling tessyUnion) is a module-level
singleton and can be corrupted if tessellation is reentrant or called
concurrently (e.g., from resolveContour/resolveContours during a callback or
across workers); either create a fresh libtess.GluTesselator per call or
document this limitation clearly: add a top-of-file comment near the
tessySplit/tessyUnion definitions stating they are singletons not safe for
reentrant or concurrent use, and recommend using a per-call instance or external
synchronization (mutex/queue) if reentrancy/concurrency is required.
- Around line 117-120: The block that adds the closing vertex is indented with 6
spaces instead of the project's 2-space convention; reformat the if (needsClose)
{ ... } block so the lines declaring closeCoords and calling
tessy.gluTessVertex(closeCoords, closeCoords); use the same 2-space indentation
as surrounding code (adjust spacing for the closeCoords declaration and the
tessy.gluTessVertex call to align with the file's convention).
In `@packages/effects-core/src/render/feather-offscreen-pass.ts`:
- Line 10: The import line brings in FeatherRenderParams and
VectorFeatherRenderer but they are only used as types; change their imports to
type-only imports so the line uses "import type" for FeatherRenderParams and
VectorFeatherRenderer while keeping getExpandedRadius as a normal import; update
the import statement in
packages/effects-core/src/render/feather-offscreen-pass.ts that references
FeatherRenderParams and VectorFeatherRenderer accordingly so only
getExpandedRadius remains a runtime import.
🪄 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: ee4b0e82-5151-4320-bb43-185ae33fed86
⛔ Files ignored due to path filters (1)
packages/effects-core/src/math/shape/shaders/feather-upsample.frag.glslis excluded by!**/*.glsl
📒 Files selected for processing (9)
packages/effects-core/feather-report.mdpackages/effects-core/src/components/shape-component.tspackages/effects-core/src/math/shape/build-line.tspackages/effects-core/src/math/shape/feather-mesh-builder.tspackages/effects-core/src/math/shape/split-union.tspackages/effects-core/src/math/shape/vector-feather-renderer.tspackages/effects-core/src/render/feather-offscreen-pass.tspackages/effects-core/src/render/render-target-pool.tspackages/effects-core/src/render/renderer.ts
✅ Files skipped from review due to trivial changes (1)
- packages/effects-core/feather-report.md
Comment on lines
+788
to
+793
| innerVerts.push( | ||
| x1 - perp1x * outerWeight, | ||
| y1 - perp1y * outerWeight); | ||
| outerVerts.push( | ||
| x1 + perp1x * innerWeight, | ||
| y1 + perp1y * innerWeight); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the original buildLine has the same pattern and any comments explaining it
rg -n -B5 -A5 "perp1x \* outerWeight" packages/effects-core/src/math/shape/build-line.tsRepository: galacean/effects-runtime
Length of output: 5898
Fix swapped inner/outer weights at lines 788–793.
The weights are inverted in the pushes after the 180° corner handling:
innerVertsreceivesperp1x * outerWeight(should beinnerWeight)outerVertsreceivesperp1x * innerWeight(should beouterWeight)
This matches the consistent pattern at lines 878–879, where the same logic correctly uses:
innerVerts.push(x1 - perp1x * innerWeight, ...)outerVerts.push(x1 + perp1x * outerWeight, ...)
The swap is a copy-paste artifact from buildLine (lines 381–386), where both values were pushed to the same array. Correct to match the pattern at 878–879.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/effects-core/src/math/shape/build-line.ts` around lines 788 - 793,
Fix the swapped weight usage when pushing vertices after the 180° corner
handling: in the block that pushes to innerVerts and outerVerts (using symbols
innerVerts, outerVerts, perp1x, perp1y, innerWeight, outerWeight, x1, y1)
replace the inverted values so innerVerts receives perp1*innerWeight and
outerVerts receives perp1*outerWeight, matching the correct pattern used later
(the same logic around buildLine/build-line functions).
| @@ -1,3 +1,5 @@ | |||
| import { split, union, unionContours, splitContours } from "./split-union"; | |||
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused imports.
split and splitContours are imported but never used in this file.
♻️ Proposed fix
-import { split, union, unionContours, splitContours } from "./split-union";
+import { union, unionContours } from "./split-union";📝 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.
Suggested change
| import { split, union, unionContours, splitContours } from "./split-union"; | |
| import { union, unionContours } from "./split-union"; |
🧰 Tools
🪛 ESLint
[error] 1-1: 'split' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 1-1: 'splitContours' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/effects-core/src/math/shape/feather-mesh-builder.ts` at line 1, The
import statement in feather-mesh-builder.ts brings in unused symbols `split` and
`splitContours`; remove those from the import to avoid unused imports and lint
errors by changing the import from `import { split, union, unionContours,
splitContours } from "./split-union";` to only include the used symbols `union`
and `unionContours` so the file imports `import { union, unionContours } from
"./split-union";`.
| import { Texture, TextureLoadAction } from '../../texture'; | ||
| import { glContext } from '../../gl'; | ||
| import { Float16ArrayWrapper } from '../float16array-wrapper'; | ||
| import { simplifyScatterEdges, removeShortEdges, removeShortEdgesFloat } from './scatter-edge-simplifier'; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused import.
removeShortEdgesFloat is imported but never used.
♻️ Proposed fix
-import { simplifyScatterEdges, removeShortEdges, removeShortEdgesFloat } from './scatter-edge-simplifier';
+import { simplifyScatterEdges, removeShortEdges } from './scatter-edge-simplifier';📝 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.
Suggested change
| import { simplifyScatterEdges, removeShortEdges, removeShortEdgesFloat } from './scatter-edge-simplifier'; | |
| import { simplifyScatterEdges, removeShortEdges } from './scatter-edge-simplifier'; |
🧰 Tools
🪛 ESLint
[error] 14-14: 'removeShortEdgesFloat' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/effects-core/src/math/shape/vector-feather-renderer.ts` at line 14,
The import list includes an unused symbol removeShortEdgesFloat; remove it from
the import statement so only used exports (simplifyScatterEdges,
removeShortEdges) are imported. Locate the import line that references
simplifyScatterEdges, removeShortEdges, removeShortEdgesFloat and delete
removeShortEdgesFloat from that list to eliminate the unused import.
Comment on lines
+262
to
+269
| // 这里计算屏幕radius。 | ||
| const featherRadiusScreen = this.computeScreenRadius( | ||
| renderer, worldMatrix, setFeatherRadius | ||
| ); | ||
| // 羽化半径至少要有1px，否则只会导致一些奇奇怪怪的锯齿（当使用1px羽化时，表现为抗锯齿效果）。 | ||
| const refinedScreenRadius = Math.max(featherRadiusScreen, 1.0); | ||
| // 这里传递回去修改原始羽化 | ||
| this.featherRadius = setFeatherRadius * refinedScreenRadius / Math.max(featherRadiusScreen, 0.0001); |
Contributor
There was a problem hiding this comment.
Mutating this.featherRadius inside computeRenderParams may cause side effects.
The method mutates this.featherRadius (line 269) based on the computed screen radius. If computeRenderParams is called multiple times per frame or with different world matrices, this could lead to unexpected accumulated drift in the feather radius value.
Consider using a local variable for the refined radius or documenting this intentional mutation clearly.
🧰 Tools
🪛 ESLint
[error] 269-269: Expected blank line before this statement.
(padding-line-between-statements)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/effects-core/src/math/shape/vector-feather-renderer.ts` around lines
262 - 269, computeRenderParams currently mutates this.featherRadius when
computing screen-space adjustments, which can cause drift; change the
implementation in computeRenderParams (or the method containing
computeScreenRadius, refinedScreenRadius, and the current this.featherRadius
assignment) to stop mutating the instance property: compute refinedScreenRadius
as a local variable (e.g., refinedFeatherRadius) and use/return that local value
to downstream code that needs the adjusted radius instead of writing back to
this.featherRadius; if mutation was intentional, add an explicit comment and a
clearly named method (e.g., applyFeatherRadiusAdjustment) so callers opt into
the side effect.
Comment on lines
+346
to
+351
| var numEdges : number = this.scatterEdgeTexture?.getWidth() || 0; | ||
| this.gatherMaterial.setInt('uMeshEnd', numEdges); | ||
| this.gatherMaterial.setInt('uEdgeTextureWidth', numEdges); | ||
| if (this.scatterEdgeTexture != undefined){ | ||
| this.gatherMaterial.setTexture('uEdgeTexture', this.scatterEdgeTexture); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use const and strict equality.
Line 346 uses var which should be const. Line 349 uses != instead of !==.
♻️ Proposed fix
- var numEdges : number = this.scatterEdgeTexture?.getWidth() || 0;
+ const numEdges: number = this.scatterEdgeTexture?.getWidth() ?? 0;
this.gatherMaterial.setInt('uMeshEnd', numEdges);
this.gatherMaterial.setInt('uEdgeTextureWidth', numEdges);
- if (this.scatterEdgeTexture != undefined){
+ if (this.scatterEdgeTexture !== undefined) {
this.gatherMaterial.setTexture('uEdgeTexture', this.scatterEdgeTexture);
}📝 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.
Suggested change
| var numEdges : number = this.scatterEdgeTexture?.getWidth() || 0; | |
| this.gatherMaterial.setInt('uMeshEnd', numEdges); | |
| this.gatherMaterial.setInt('uEdgeTextureWidth', numEdges); | |
| if (this.scatterEdgeTexture != undefined){ | |
| this.gatherMaterial.setTexture('uEdgeTexture', this.scatterEdgeTexture); | |
| } | |
| const numEdges: number = this.scatterEdgeTexture?.getWidth() ?? 0; | |
| this.gatherMaterial.setInt('uMeshEnd', numEdges); | |
| this.gatherMaterial.setInt('uEdgeTextureWidth', numEdges); | |
| if (this.scatterEdgeTexture !== undefined) { | |
| this.gatherMaterial.setTexture('uEdgeTexture', this.scatterEdgeTexture); | |
| } |
🧰 Tools
🪛 ESLint
[error] 346-346: Unexpected space before the ':'.
(@typescript-eslint/type-annotation-spacing)
[error] 347-347: Expected blank line before this statement.
(padding-line-between-statements)
[error] 349-351: Missing space before opening brace.
(@typescript-eslint/space-before-blocks)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/effects-core/src/math/shape/vector-feather-renderer.ts` around lines
346 - 351, Replace the mutable/loose declarations and comparisons: change the
`var numEdges` declaration to `const numEdges` (computed from
`this.scatterEdgeTexture?.getWidth() || 0`) and use strict equality when
checking the texture by replacing `if (this.scatterEdgeTexture != undefined)`
with `if (this.scatterEdgeTexture !== undefined)`, keeping the rest of the logic
that calls `this.gatherMaterial.setInt('uMeshEnd', numEdges)`,
`setInt('uEdgeTextureWidth', numEdges)`, and `setTexture('uEdgeTexture',
this.scatterEdgeTexture)` unchanged.
Comment on lines
+594
to
+598
| export function getExpandedRadius( | ||
| featherRadius: number, | ||
| featherRadiusScreen: number | ||
| ):number { | ||
| return featherRadius + featherRadius / featherRadiusScreen; |
Contributor
There was a problem hiding this comment.
Potential division by zero when featherRadiusScreen is 0.
getExpandedRadius divides by featherRadiusScreen. While the caller typically clamps this to ≥1, defensively handle zero to avoid Infinity results.
🛡️ Proposed defensive fix
export function getExpandedRadius(
featherRadius: number,
featherRadiusScreen: number
):number {
+ if (featherRadiusScreen <= 0) {
+ return featherRadius;
+ }
return featherRadius + featherRadius / featherRadiusScreen;
}📝 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.
Suggested change
| export function getExpandedRadius( | |
| featherRadius: number, | |
| featherRadiusScreen: number | |
| ):number { | |
| return featherRadius + featherRadius / featherRadiusScreen; | |
| export function getExpandedRadius( | |
| featherRadius: number, | |
| featherRadiusScreen: number | |
| ):number { | |
| if (featherRadiusScreen <= 0) { | |
| return featherRadius; | |
| } | |
| return featherRadius + featherRadius / featherRadiusScreen; | |
| } |
🧰 Tools
🪛 ESLint
[error] 594-594: Missing space before function parentheses.
(@typescript-eslint/space-before-function-paren)
[error] 597-597: Expected a space after the ':'.
(@typescript-eslint/type-annotation-spacing)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/effects-core/src/math/shape/vector-feather-renderer.ts` around lines
594 - 598, getExpandedRadius currently divides by featherRadiusScreen which can
be zero; update getExpandedRadius to defensively guard against division-by-zero
by clamping featherRadiusScreen to a safe minimum (e.g., 1 or a small epsilon)
or returning a sensible fallback (like featherRadius) when featherRadiusScreen
is 0 or extremely small. Locate the function getExpandedRadius in
vector-feather-renderer.ts and modify the calculation to use the clamped value
(or conditional fallback) so the return never becomes Infinity or NaN.
Comment on lines
+51
to
54
| anisotropic?: number, | ||
| ): Framebuffer { | ||
| // 使用参数计算 hash 值作为缓存 key | ||
| const hash = `${width}_${height}_${depthBuffer}_${filter}_${format}`; |
Contributor
There was a problem hiding this comment.
Cache key does not include anisotropic value, risking incorrect texture reuse.
The hash on line 54 doesn't incorporate the anisotropic parameter. If a caller requests a render target with anisotropic=1 and later another caller requests one with the same dimensions but anisotropic=4, the pool will return the anisotropic=1 texture, ignoring the different setting.
🐛 Proposed fix
get (
name: string,
width: number,
height: number,
depthBuffer = 0,
filter = FilterMode.Linear,
format = RenderTextureFormat.RGBA32,
anisotropic?: number,
): Framebuffer {
// 使用参数计算 hash 值作为缓存 key
- const hash = `${width}_${height}_${depthBuffer}_${filter}_${format}`;
+ const hash = `${width}_${height}_${depthBuffer}_${filter}_${format}_${anisotropic ?? 4}`;📝 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.
Suggested change
| anisotropic?: number, | |
| ): Framebuffer { | |
| // 使用参数计算 hash 值作为缓存 key | |
| const hash = `${width}_${height}_${depthBuffer}_${filter}_${format}`; | |
| anisotropic?: number, | |
| ): Framebuffer { | |
| // 使用参数计算 hash 值作为缓存 key | |
| const hash = `${width}_${height}_${depthBuffer}_${filter}_${format}_${anisotropic ?? 4}`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/effects-core/src/render/render-target-pool.ts` around lines 51 - 54,
The cache key generation for the Framebuffer pool omits the anisotropic
parameter, so the computed `hash` variable can cause textures with different
anisotropic settings to be reused incorrectly; update the `hash` construction in
the function that returns a Framebuffer to include the `anisotropic` value
(e.g., append `${anisotropic}` into the template used to build `hash`) so each
unique anisotropic setting yields a distinct cache key.
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 by CodeRabbit
New Features
Bug Fixes
Chores