feat(2d): add FilledSpriteAssembler and sprite filled mode support#2989
feat(2d): add FilledSpriteAssembler and sprite filled mode support#2989cptbtptpbcptdtptp wants to merge 8 commits into
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughThis PR adds filled-mode sprite rendering to the Galacean 2D engine. It introduces new enums ( ChangesFilled Sprite Rendering
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2989 +/- ##
===========================================
+ Coverage 79.31% 79.38% +0.07%
===========================================
Files 919 923 +4
Lines 102450 103283 +833
Branches 11365 11479 +114
===========================================
+ Hits 81255 81988 +733
- Misses 21007 21106 +99
- Partials 188 189 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
packages/core/src/2d/assembler/FilledSpriteAssembler.ts (3)
113-115: 💤 Low value
@ts-ignorehides an interface gap onrenderer._bounds.The
_boundsfield isn't declared onISpriteRenderer, so this access requires@ts-ignore. Other assemblers presumably need the same hatch. Consider wideningISpriteRendererto include_bounds: BoundingBox(or expose a small_setBounds(box)method) so the suppression can be removed and the interface accurately reflects what the assemblers depend on.🤖 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/core/src/2d/assembler/FilledSpriteAssembler.ts` around lines 113 - 115, The `@ts-ignore` exists because assemblers access renderer._bounds which isn't declared on ISpriteRenderer; update the renderer interface (ISpriteRenderer) to include a private/internal _bounds: BoundingBox (or add a small method like _setBounds(box: BoundingBox) and use that) so FilledSpriteAssembler (where BoundingBox.transform(renderer._getBounds(), modelMatrix, renderer._bounds) is called) no longer needs `@ts-ignore`; change the interface accordingly and remove the suppression from FilledSpriteAssembler (and any other assemblers using _bounds).
137-140: 💤 Low valueExtract repeated
amount <= 0.001short-circuit and the magic threshold.The same
if (amount <= 0.001) { renderer._subChunk.indices.length = 0; return; }guard is repeated in 4_filled*methods, and the threshold itself is a magic number. Consider hoisting both into a single named constant + helper (or moving the early-out up intoupdatePositionsso the per-mode functions don't have to duplicate it).Also applies to: 222-225, 288-291, 383-386
🤖 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/core/src/2d/assembler/FilledSpriteAssembler.ts` around lines 137 - 140, Extract the repeated small-amount guard into a named constant and single helper (or move the early-out into updatePositions) to remove duplication: define a descriptive constant (e.g. MIN_FILL_AMOUNT = 0.001) in FilledSpriteAssembler and replace the four occurrences of the literal check (the lines in the _filled* methods that do `if (amount <= 0.001) { renderer._subChunk.indices.length = 0; return; }`) with a call to a new helper like shouldSkipFill(amount, renderer) or performSkipFillIfNeeded(amount, renderer) so the threshold is centralized and the index-clear/return logic is not duplicated across _filled* methods. Ensure you update all four locations mentioned (the guards at the shown snippet and the ones around lines 222-225, 288-291, 383-386) and keep behavior identical.
244-271: ⚡ Quick winRadial handlers silently no-op on unexpected origins.
_filledRadial90only handles the 4 corner origins,_filledRadial180and_filledRadial360only handle the 4 edge origins. Thedefault: break;branches leave_inPositions/_inUVspopulated from a previous invocation (radial 90/180) orstartAngle = 0(radial 360), producing visually wrong geometry without any indication of misuse.Since
SpriteRenderer.filledOriginsetter does not validate againstfilledMode, a user can put the renderer in a bad state. Consider either:
- Validating in
SpriteRenderer.filledOriginsetter (reject/coerce origins that don't match the current mode), or- Adding an explicit warning/
Logger.warnin the default branch here.Also applies to: 320-371, 389-404
🤖 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/core/src/2d/assembler/FilledSpriteAssembler.ts` around lines 244 - 271, The switch default branches in _filledRadial90, _filledRadial180 and _filledRadial360 silently leave inPositions/inUVs from prior calls causing wrong geometry; update each default case to log a warning (use Logger.warn or the project logger) mentioning the function name and the invalid origin, and then clear/reset the arrays (inPositions and inUVs) or set them to safe defaults so stale data isn't reused; alternatively, add validation/coercion in the SpriteRenderer.filledOrigin setter to reject or coerce origins incompatible with the current filledMode, but if you choose the logger approach make sure to reference the origin value and the method name in the message.packages/core/src/2d/sprite/SpriteRenderer.ts (1)
179-186: ⚡ Quick winConsider validating
filledOriginagainst the activefilledMode.The setter accepts any
SpriteFilledOrigin, but eachfilledModeonly renders correctly for a subset of origins (Horizontal: Left/Right; Vertical: Top/Bottom; Radial90: corners; Radial180/360: edge midpoints). Combined with the silentdefault: break;branches inFilledSpriteAssembler, an invalid combination produces broken geometry with no diagnostic.Validating here (e.g.,
Logger.warnand coercing to a valid origin, or clamping based on_filledMode) would help users catch misuse early. Same applies to thefilledModesetter when the previously-set origin is no longer valid for the new mode.🤖 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/core/src/2d/sprite/SpriteRenderer.ts` around lines 179 - 186, The filledOrigin setter currently accepts any SpriteFilledOrigin even when incompatible with the active _filledMode, which can produce broken geometry; update the set filledOrigin(value: SpriteFilledOrigin) to validate value against the current _filledMode (e.g., allowed origins for Horizontal = Left/Right, Vertical = Top/Bottom, Radial90 = corners, Radial180/360 = edge midpoints), and if invalid call Logger.warn with a clear message and coerce/clamp to a valid origin before assigning _filledOrigin and setting the dirty flag (_dirtyUpdateFlag |= SpriteRendererUpdateFlags.WorldVolumeAndUV) for SpriteDrawMode.Filled; also mirror this validation in the filledMode setter so when changing _filledMode you adjust or coerce the existing _filledOrigin (and warn) to a compatible value to avoid relying on the silent default branches in FilledSpriteAssembler.packages/core/src/2d/assembler/ISpriteRenderer.ts (1)
20-20: ⚡ Quick winConsider renaming
filledClockWise→filledClockwise."Clockwise" is a single English word; splitting the W creates an unusual public API name. Since this property is also exposed via
SpriteRendererand is part of a 2.0 release, normalizing it now avoids a breaking rename later. Same rename should be applied inSpriteRenderer.ts(private field, getter/setter) andFilledSpriteAssembler.ts(read site).♻️ Suggested rename
- filledClockWise?: boolean; + filledClockwise?: boolean;🤖 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/core/src/2d/assembler/ISpriteRenderer.ts` at line 20, Rename the public property and all usages from filledClockWise to filledClockwise: update the ISpriteRenderer interface property (filledClockWise → filledClockwise), rename the private field and its getter/setter in SpriteRenderer (ensure method names and internal references use filledClockwise), and update the read site in FilledSpriteAssembler to read the new property name; keep behavior unchanged and run tests to catch any missed references.
🤖 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/core/src/2d/assembler/FilledSpriteAssembler.ts`:
- Around line 117-119: The FilledSpriteAssembler.updateUVs is a no-op which
leaves UVs stale when atlasRegion changes because SpriteRenderer._onSpriteChange
sets only SpriteRendererUpdateFlags.UV for SpriteModifyFlags.atlasRegion and
_render will call updateUVs (skipped updatePositions); fix this by changing
SpriteRenderer._onSpriteChange to treat atlasRegion the same as other
UV-affecting changes for filled mode: when atlasRegion is detected, set the
update flags to WorldVolumeAndUV (or include WorldVolume) instead of only UV so
FilledSpriteAssembler.updatePositions runs and recomputes baked UVs; update
references to SpriteModifyFlags.atlasRegion and SpriteRendererUpdateFlags.*
accordingly.
In `@packages/core/src/2d/sprite/SpriteRenderer.ts`:
- Around line 160-170: The filledMode setter currently sets _filledOrigin to
Bottom for all non-Radial90 modes which makes Horizontal default to an invalid
Bottom origin; update the logic in the set filledMode(value: SpriteFilledMode)
to choose a mode-appropriate default: when value === SpriteFilledMode.Horizontal
set _filledOrigin to SpriteFilledOrigin.Left (so
FilledSpriteAssembler._filledLinear sees originIsStart true), when value ===
SpriteFilledMode.Radial90 set it to SpriteFilledOrigin.BottomLeft, otherwise
keep SpriteFilledOrigin.Bottom; keep the existing branch that sets
_dirtyUpdateFlag |= SpriteRendererUpdateFlags.WorldVolumeAndUV when
this._drawMode === SpriteDrawMode.Filled.
---
Nitpick comments:
In `@packages/core/src/2d/assembler/FilledSpriteAssembler.ts`:
- Around line 113-115: The `@ts-ignore` exists because assemblers access
renderer._bounds which isn't declared on ISpriteRenderer; update the renderer
interface (ISpriteRenderer) to include a private/internal _bounds: BoundingBox
(or add a small method like _setBounds(box: BoundingBox) and use that) so
FilledSpriteAssembler (where BoundingBox.transform(renderer._getBounds(),
modelMatrix, renderer._bounds) is called) no longer needs `@ts-ignore`; change the
interface accordingly and remove the suppression from FilledSpriteAssembler (and
any other assemblers using _bounds).
- Around line 137-140: Extract the repeated small-amount guard into a named
constant and single helper (or move the early-out into updatePositions) to
remove duplication: define a descriptive constant (e.g. MIN_FILL_AMOUNT = 0.001)
in FilledSpriteAssembler and replace the four occurrences of the literal check
(the lines in the _filled* methods that do `if (amount <= 0.001) {
renderer._subChunk.indices.length = 0; return; }`) with a call to a new helper
like shouldSkipFill(amount, renderer) or performSkipFillIfNeeded(amount,
renderer) so the threshold is centralized and the index-clear/return logic is
not duplicated across _filled* methods. Ensure you update all four locations
mentioned (the guards at the shown snippet and the ones around lines 222-225,
288-291, 383-386) and keep behavior identical.
- Around line 244-271: The switch default branches in _filledRadial90,
_filledRadial180 and _filledRadial360 silently leave inPositions/inUVs from
prior calls causing wrong geometry; update each default case to log a warning
(use Logger.warn or the project logger) mentioning the function name and the
invalid origin, and then clear/reset the arrays (inPositions and inUVs) or set
them to safe defaults so stale data isn't reused; alternatively, add
validation/coercion in the SpriteRenderer.filledOrigin setter to reject or
coerce origins incompatible with the current filledMode, but if you choose the
logger approach make sure to reference the origin value and the method name in
the message.
In `@packages/core/src/2d/assembler/ISpriteRenderer.ts`:
- Line 20: Rename the public property and all usages from filledClockWise to
filledClockwise: update the ISpriteRenderer interface property (filledClockWise
→ filledClockwise), rename the private field and its getter/setter in
SpriteRenderer (ensure method names and internal references use
filledClockwise), and update the read site in FilledSpriteAssembler to read the
new property name; keep behavior unchanged and run tests to catch any missed
references.
In `@packages/core/src/2d/sprite/SpriteRenderer.ts`:
- Around line 179-186: The filledOrigin setter currently accepts any
SpriteFilledOrigin even when incompatible with the active _filledMode, which can
produce broken geometry; update the set filledOrigin(value: SpriteFilledOrigin)
to validate value against the current _filledMode (e.g., allowed origins for
Horizontal = Left/Right, Vertical = Top/Bottom, Radial90 = corners,
Radial180/360 = edge midpoints), and if invalid call Logger.warn with a clear
message and coerce/clamp to a valid origin before assigning _filledOrigin and
setting the dirty flag (_dirtyUpdateFlag |=
SpriteRendererUpdateFlags.WorldVolumeAndUV) for SpriteDrawMode.Filled; also
mirror this validation in the filledMode setter so when changing _filledMode you
adjust or coerce the existing _filledOrigin (and warn) to a compatible value to
avoid relying on the silent default branches in FilledSpriteAssembler.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 24f0fb05-7587-4d34-9703-6bb15d3ffab5
📒 Files selected for processing (7)
packages/core/src/2d/assembler/FilledSpriteAssembler.tspackages/core/src/2d/assembler/ISpriteRenderer.tspackages/core/src/2d/enums/SpriteDrawMode.tspackages/core/src/2d/enums/SpriteFilledMode.tspackages/core/src/2d/enums/SpriteFilledOrigin.tspackages/core/src/2d/index.tspackages/core/src/2d/sprite/SpriteRenderer.ts
| static updateUVs(renderer: ISpriteRenderer): void { | ||
| // UVs are computed in updatePositions. | ||
| } |
There was a problem hiding this comment.
Empty updateUVs leaves UVs stale on atlas-region changes.
updateUVs is a no-op because UVs are baked inside updatePositions. However, SpriteRenderer._onSpriteChange handles SpriteModifyFlags.atlasRegion by setting only SpriteRendererUpdateFlags.UV (no WorldVolume). In _render, this triggers updateUVs while updatePositions is skipped — so for filled mode the UVs are never refreshed when the sprite's atlas region changes (e.g., dynamic atlas repacking). Other assemblers don't have this regression because they have a real updateUVs.
Two reasonable fixes:
- In
SpriteRenderer._onSpriteChange, treatatlasRegionlike other UV-affecting events for filled mode (setWorldVolumeAndUV), or - Implement a real UV-only path here (extract the UV computation from each
_filled*function so it can be invoked independently).
Option 1 is the minimal fix.
🛠 Minimal fix in SpriteRenderer._onSpriteChange
case SpriteModifyFlags.atlasRegion:
- this._dirtyUpdateFlag |= SpriteRendererUpdateFlags.UV;
+ this._dirtyUpdateFlag |=
+ this._drawMode === SpriteDrawMode.Filled
+ ? SpriteRendererUpdateFlags.WorldVolumeAndUV
+ : SpriteRendererUpdateFlags.UV;
break;🤖 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/core/src/2d/assembler/FilledSpriteAssembler.ts` around lines 117 -
119, The FilledSpriteAssembler.updateUVs is a no-op which leaves UVs stale when
atlasRegion changes because SpriteRenderer._onSpriteChange sets only
SpriteRendererUpdateFlags.UV for SpriteModifyFlags.atlasRegion and _render will
call updateUVs (skipped updatePositions); fix this by changing
SpriteRenderer._onSpriteChange to treat atlasRegion the same as other
UV-affecting changes for filled mode: when atlasRegion is detected, set the
update flags to WorldVolumeAndUV (or include WorldVolume) instead of only UV so
FilledSpriteAssembler.updatePositions runs and recomputes baked UVs; update
references to SpriteModifyFlags.atlasRegion and SpriteRendererUpdateFlags.*
accordingly.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- spriteUVs[15] -> spriteUVs[3]: Sprite._uvs only has 4 entries, so the out-of-bounds right-top UV crashed filled rendering on real sprites - filledMode setter now picks a valid default origin per mode (Horizontal -> Left) - atlasRegion change refreshes UV in filled mode (UV is baked in updatePositions) - radial fills fall back to the default origin for unsupported origins - add tests covering filled linear/radial rendering and the above fixes Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add the render-level coverage that was missing: Radial180 (incl. unsupported-origin fallback), counter-clockwise radial, flipX mirroring, linear non-default origins (Right/Top), linear index values, Vertical UVs, and an indices-count topology table across all 5 modes and amounts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a single e2e case rendering all five fill modes across fill amounts 0/0.25/0.5/0.75/1 in one baseline image, covering the topology boundaries (45-degree quad/triangle, 90-degree quadrant edges). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cptbtptpbcptdtptp
force-pushed
the
feat/sprite-filled-mode
branch
from
1fa9f7d to
17ac677
Compare
filledOrigin is a shared 8-member enum but each mode only accepts a subset (linear: Left/Right or Top/Bottom; Radial90: corners; Radial180/360: edges). A mismatched origin previously stuck and the assembler silently fell back to the default, so the getter disagreed with what was rendered. Snap the origin to a mode-valid value in both setters via a shared helper, keeping the current origin on mode change when it stays valid. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@GuoLei1990 感谢逐条复核,已推送修复( P1(已修)
额外修复(顺带发现)
P3(已修)
P2
|
Extract the repeated 0.001 fill-amount threshold into a named constant, and add comments for the fixed 16-vertex allocation and the 45-degree quadrant diagonal in the radial cut. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cptbtptpbcptdtptp
force-pushed
the
feat/sprite-filled-mode
branch
from
71222d1 to
768c767
Compare
…ed-mode # Conflicts: # e2e/config.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@e2e/case/sprite-filled.ts`:
- Around line 30-63: Replace the external CDN URL in the resourceManager.load()
call with a path to a repo-hosted texture fixture to ensure test stability.
Additionally, add a .catch() rejection handler to the promise chain (after the
existing .then() block) to explicitly handle and log load failures, so that
asset loading errors are properly diagnosed in CI.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fdb391c4-0ddb-461b-89f6-e5deef3d9ec6
⛔ Files ignored due to path filters (1)
e2e/fixtures/originImage/Sprite_sprite-filled.jpgis excluded by!**/*.jpg
📒 Files selected for processing (5)
e2e/case/sprite-filled.tse2e/config.tspackages/core/src/2d/assembler/FilledSpriteAssembler.tspackages/core/src/2d/sprite/SpriteRenderer.tstests/src/core/SpriteRenderer.test.ts
✅ Files skipped from review due to trivial changes (1)
- e2e/config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/2d/sprite/SpriteRenderer.ts
- packages/core/src/2d/assembler/FilledSpriteAssembler.ts
| engine.resourceManager | ||
| .load<Texture2D>({ | ||
| url: "https://gw.alipayobjects.com/mdn/rms_7c464e/afts/img/A*rgNGR4Vb7lQAAAAAAAAAAAAAARQnAQ", | ||
| type: AssetType.Texture | ||
| }) | ||
| .then((texture) => { | ||
| const sprite = new Sprite(engine, texture); | ||
| const modes = [ | ||
| SpriteFilledMode.Horizontal, | ||
| SpriteFilledMode.Vertical, | ||
| SpriteFilledMode.Radial90, | ||
| SpriteFilledMode.Radial180, | ||
| SpriteFilledMode.Radial360 | ||
| ]; | ||
| const amounts = [0, 0.25, 0.5, 0.75, 1]; | ||
|
|
||
| modes.forEach((mode, row) => { | ||
| amounts.forEach((amount, col) => { | ||
| const entity = rootEntity.createChild(`filled-${row}-${col}`); | ||
| entity.transform.setPosition((col - 2) * 3, (2 - row) * 3, 0); | ||
| const renderer = entity.addComponent(SpriteRenderer); | ||
| renderer.sprite = sprite; | ||
| renderer.width = 2.4; | ||
| renderer.height = 2.4; | ||
| renderer.drawMode = SpriteDrawMode.Filled; | ||
| renderer.filledMode = mode; | ||
| renderer.filledAmount = amount; | ||
| }); | ||
| }); | ||
|
|
||
| updateForE2E(engine); | ||
| initScreenshot(engine, camera); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Use a repo-hosted texture fixture and handle load failures explicitly.
Line 32 fetches from an external CDN, which makes this baseline test vulnerable to network/service/asset drift. Also, the load chain has no rejection handler, so failures are harder to diagnose in CI.
Suggested hardening
- engine.resourceManager
- .load<Texture2D>({
- url: "https://gw.alipayobjects.com/mdn/rms_7c464e/afts/img/A*rgNGR4Vb7lQAAAAAAAAAAAAAARQnAQ",
- type: AssetType.Texture
- })
- .then((texture) => {
+ const textureUrl = "e2e/assets/sprite-filled.png"; // checked-in deterministic fixture
+ engine.resourceManager
+ .load<Texture2D>({
+ url: textureUrl,
+ type: AssetType.Texture
+ })
+ .then((texture) => {
const sprite = new Sprite(engine, texture);
// ...
updateForE2E(engine);
initScreenshot(engine, camera);
- });
+ })
+ .catch((err) => {
+ throw new Error(`sprite-filled e2e setup failed: ${String(err)}`);
+ });📝 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.
| engine.resourceManager | |
| .load<Texture2D>({ | |
| url: "https://gw.alipayobjects.com/mdn/rms_7c464e/afts/img/A*rgNGR4Vb7lQAAAAAAAAAAAAAARQnAQ", | |
| type: AssetType.Texture | |
| }) | |
| .then((texture) => { | |
| const sprite = new Sprite(engine, texture); | |
| const modes = [ | |
| SpriteFilledMode.Horizontal, | |
| SpriteFilledMode.Vertical, | |
| SpriteFilledMode.Radial90, | |
| SpriteFilledMode.Radial180, | |
| SpriteFilledMode.Radial360 | |
| ]; | |
| const amounts = [0, 0.25, 0.5, 0.75, 1]; | |
| modes.forEach((mode, row) => { | |
| amounts.forEach((amount, col) => { | |
| const entity = rootEntity.createChild(`filled-${row}-${col}`); | |
| entity.transform.setPosition((col - 2) * 3, (2 - row) * 3, 0); | |
| const renderer = entity.addComponent(SpriteRenderer); | |
| renderer.sprite = sprite; | |
| renderer.width = 2.4; | |
| renderer.height = 2.4; | |
| renderer.drawMode = SpriteDrawMode.Filled; | |
| renderer.filledMode = mode; | |
| renderer.filledAmount = amount; | |
| }); | |
| }); | |
| updateForE2E(engine); | |
| initScreenshot(engine, camera); | |
| }); | |
| }); | |
| const textureUrl = "e2e/assets/sprite-filled.png"; // checked-in deterministic fixture | |
| engine.resourceManager | |
| .load<Texture2D>({ | |
| url: textureUrl, | |
| type: AssetType.Texture | |
| }) | |
| .then((texture) => { | |
| const sprite = new Sprite(engine, texture); | |
| const modes = [ | |
| SpriteFilledMode.Horizontal, | |
| SpriteFilledMode.Vertical, | |
| SpriteFilledMode.Radial90, | |
| SpriteFilledMode.Radial180, | |
| SpriteFilledMode.Radial360 | |
| ]; | |
| const amounts = [0, 0.25, 0.5, 0.75, 1]; | |
| modes.forEach((mode, row) => { | |
| amounts.forEach((amount, col) => { | |
| const entity = rootEntity.createChild(`filled-${row}-${col}`); | |
| entity.transform.setPosition((col - 2) * 3, (2 - row) * 3, 0); | |
| const renderer = entity.addComponent(SpriteRenderer); | |
| renderer.sprite = sprite; | |
| renderer.width = 2.4; | |
| renderer.height = 2.4; | |
| renderer.drawMode = SpriteDrawMode.Filled; | |
| renderer.filledMode = mode; | |
| renderer.filledAmount = amount; | |
| }); | |
| }); | |
| updateForE2E(engine); | |
| initScreenshot(engine, camera); | |
| }) | |
| .catch((err) => { | |
| throw new Error(`sprite-filled e2e setup failed: ${String(err)}`); | |
| }); | |
| }); |
🤖 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 `@e2e/case/sprite-filled.ts` around lines 30 - 63, Replace the external CDN URL
in the resourceManager.load() call with a path to a repo-hosted texture fixture
to ensure test stability. Additionally, add a .catch() rejection handler to the
promise chain (after the existing .then() block) to explicitly handle and log
load failures, so that asset loading errors are properly diagnosed in CI.
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
增量审查（HEAD 812638eb9）
自上轮 CHANGES_REQUESTED（44702de）以来，作者推了 4 个修复 commit（67d06add6/a70ffa40e/17ac677d9/477a770f4，后续 merge dev/2.0 到 812638eb9）。两条阻塞 P1 + 一个潜伏崩溃已全部修复，并补齐了单测与 e2e。我已本地 build + 反向证伪逐条核验，解除上轮门控。
已关闭问题清单（逐条已对照实际代码 + 反向测试验证，不再重提）
| 问题 | 关闭依据 |
|---|---|
[P1] atlasRegion 在 Filled 模式不刷新 UV |
✅ 已修。_onSpriteChange 的 atlasRegion 分支按建议在 Filled 下置 WorldVolumeAndUV。反向验证：把它改回 UV-only，且令 sprite 设了 custom size（使 atlasRegion setter 跳过 size 派发）后，rightU 保持 1（UV stale）；修复在场时 1→0.5。fix 真实有效。 |
[P1] filledMode setter 默认 origin 错（Horizontal→Bottom 致反向填充） |
✅ 已修，且优于我的建议：抽出 _correctOrigin(mode, origin) 收口，filledMode/filledOrigin 两个 setter 共用——既给 mode 合理默认（H→Left/V→Bottom/R90→BottomLeft/R180·360→Bottom，对齐 Unity），又把不匹配 origin 收敛到合法子集，消除了"getter 与实际渲染不一致"。反向验证：把 Horizontal 默认改回 Bottom，3 个测试 fail。 |
spriteUVs[0]/[15] 依赖 #2990 16-point UV |
✅ 被作者的额外修复推翻并修正。当前 base Sprite._uvs 仍是 4 元素（[0]=LB/[3]=RT），[15] 取到 undefined，filled 在任意真实 sprite 上一渲染即 Cannot destructure undefined 崩溃。已全部改 [3]。反向验证：改回 [15]，9 个 filled 测试崩溃。我上轮"已关闭（依赖 #2990）"的结论对当前 base 是错的，作者抓得对。 |
| atlasRotated UV 翻转 | ✅ moot。Sprite._updateUVs 本身不 bake 旋转，filled 与 Simple/Tiled 走同一 _getUVs()，rotation 行为一致、非本 PR 引入。 |
静态 _vertexOffset/_indicesOffset 跨 renderer 污染、fillAmount=0 残留脏顶点 |
✅ 前几轮已自查撤回（用前 reset / <=epsilon 时 indices.length=0 = no-draw）。 |
updateColor 遍历全 16 slot |
✅ P3 闭环（未用顶点无 indices）。 |
resetData 每帧 realloc |
✅ 闭环。已确认仅在 drawMode 切换时调用一次，且补了说明注释。 |
@ts-ignore on _bounds |
✅ 解释成立。Renderer._bounds 为 protected，public 的 ISpriteRenderer 无法声明；4 个 assembler 一致模式，宜独立小重构统一处理，不在本 feature PR 夹带。 |
| 测试覆盖缺失（patch ~20%） | ✅ 已修。五种 mode × origin × amount 边界 + flip/CCW 顶点·索引断言，patch ~93%；exact-value 断言（revert 即红），粒度正确（subChunk 输出即 assembler 契约）。 |
总结
FilledSpriteAssembler 实现 Horizontal/Vertical/Radial90/180/360 五种填充，对标 Unity Image.Type.Filled。_correctOrigin 收口设计干净，radial _radialCut 的 tan 插值 + 象限分解正确。方向正确，质量达标，无 P0/P1，可合入。 本地 vitest 全绿（28/28），三条 fix 均通过反向证伪。
问题（均不阻塞）
[P2] tests/src/core/SpriteRenderer.test.ts:1763 — "filled mode refreshes UV when the atlas region changes" 是 false guard，并不守住它声称的 fix
该测试只设了 spriteRenderer.width/height（renderer 的 custom size），没设 sprite.width/height。于是 Sprite.atlasRegion setter（Sprite.ts:130）因 sprite 自身 _customWidth/_customHeight === undefined，在派发 atlasRegion 之外又派发了 size；size 在 Filled 下置 WorldVolumeUVAndColor → updatePositions 照样重跑 → UV 刷新。
实测：把 atlasRegion 分支改回 UV-only（重引入 bug），此测试仍 pass（28/28 全绿）。即它无法守住 atlasRegion fix——断言走的是 size 派发这条旁路，绕过了被测修复路径。
修法：让 sprite 也设 custom size，使 atlasRegion 单独派发：
const sprite = new Sprite(engine, texture2D);
sprite.width = 1;
sprite.height = 1; // 否则 atlasRegion 会连带派发 size，掩盖被测 bug
// ...
sprite.atlasRegion = new Rect(0, 0, 0.5, 0.5);加上后，revert fix 即可让该测试转红，才真正守住回归。
[P3] e2e/case/sprite-filled.ts:31 — e2e 用外部 CDN 纹理 URL，CI 稳定性依赖外网
CodeRabbit 也提了。其余 sprite e2e 用 https://gw.alipayobjects.com/... 是既有惯例，非本 PR 引入，但新 case 可考虑改用 repo 内 fixture 纹理（如已有的内置贴图），并对 .load() 加 .catch 记录失败，便于 CI 诊断。不阻塞。
[P3] 注释末尾句号
FilledSpriteAssembler.ts 多条单行 // 注释带句号（:43 "...degenerate geometry."、:51 "...at runtime."、:267/:361 "...stale data."、:561 "...a triangle."）；SpriteRenderer.ts 新增 _correctOrigin 区域同样（"...subset of origins."、"...a no-op."、"...corner origins."、"...edge origins."、"...getter === rendered)."）。项目约定单行 // 不加句号（多行 JSDoc 才加）。不过 SlicedSpriteAssembler 既有注释也带句号，惯例本身不统一，纯锦上添花。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
设计探讨（forward-looking，非阻塞，不改变此前 APPROVE）
本 PR 功能正确、测试守得住，几何 filled 与 Unity
Image.Type.Filled一致、是合格实现，APPROVE 不变。以下是一条 roadmap 级的设计建议，核心只有一个值得在合入前权衡的真问题（radial 进 public API），其余是未来演进方向，不要求本 PR 改动。
经多引擎对标 + 第一性原理分析，把 filled 拆成三个正交轴看，结论是 linear 与 radial 应区别对待：
轴1 — 实现：GPU 不是“完全占优”，要分 linear/radial
把 fill 做成 shader（恒定 4 顶点 + analytic mask）听起来更现代，但在 Galacean 的批处理架构上只对 radial 成立：
PrimitiveChunk是 non-instanced 顶点拼接器，共享布局固定 9 floats / stride 36（POSITIONvec3 +TEXCOORD_0vec2 +COLOR_0vec4，见packages/core/src/RenderPipeline/PrimitiveChunk.ts:42-48）。要让 GPU filled 保合批，fill 参数只能：① 加进共享顶点布局 → 全场 99% 的 Simple sprite 每帧每顶点白扛字节（落在最热的逐帧上传路径，全员税）；或 ② 走 per-instance → 但 2D 主路径没有 instancing 管线，等于新开一条。- 而几何在 batching 这轴是真·零成本（同布局同材质，自然 coalesce），且输出普通顶点 → SpriteMask / 自定义材质 / atlas 全部 just work；fill 一旦封进 shader 分支，用户每个自定义材质都要重写 fill 才支持 filled。
逐轴记分：batching、composition → 几何赢；fill-rate → 打平；radial 边缘 AA → GPU 决定性赢（几何硬边只有 framebuffer MSAA，救不了慢旋转的对角切边；analytic atan2+smoothstep 分辨率无关）。
→ linear：几何成本本就极低（quad UV 上 step），GPU 化要付税/开管线/丢 composition，得不偿失。
→ radial：硬边是它唯一真打不过的地方，GPU 的 analytic AA 值这个钱。
轴2 — 需求：linear 都要（几何够），radial 都有真需求但不宜做成几何 drawMode
- linear（进度/血条）：UI 控件刚需 + 世界头顶血条，几何已完美（Unity/UE 的线性进度也都是几何）。
- radial（冷却环 + 世界范围/扇形指示）：是 ARPG/MOBA/塔防刚需，且 world-space 大尺寸下硬边最刺眼（UI 通常小且正交对齐反而不明显）。用几何交付 radial = 交付一个永远锯齿的控件。
轴3 — 封装：行为进 shader + 参数走 renderer ShaderData（正好绕开轴1 全员税）
若做 GPU radial，最小封装面 = 一个官方 GPU Filled 材质（mask/AA 封 fragment shader），fillAmount/origin/clockwise 走 renderer 级 ShaderData（per-renderer uniform，不进共享顶点流 → 不向 Simple sprite 征税）。代价：同材质 filled sprite 彼此不合批——但 radial 指示器数量少，可接受，且圈在真正用它的少量 renderer 上。不要给主 sprite shader 加 fill dynamic branch（污染 Simple 路径 + 逼自定义材质 opt-in）。
唯一需要本 PR 现在权衡的点（你拍板）
drawMode 是 public API。radial 用几何进 public SpriteDrawMode.Filled，等于把“radial = 锯齿”锁进 1.0 契约，将来 GPU 化要么 breaking、要么背一个永久劣质 drawMode。两个选择：
- 现在就把 radial 从 drawMode 拆出来，走可选 GPU 材质（linear 仍作几何 drawMode，是终态）——避免 public API 锁死硬边契约。
- 先合几何 radial 当 1.0 baseline，roadmap 标记“radial AA 走 GPU 材质重构”，接受后续可能的 breaking。
我的倾向是 1（radial 进 public API 前三思），但这是产品节奏取舍，最终由你定。linear 无此问题，保持现状即可。
总纲：你的方案没有“不够现代”的问题——集中式 filled 与 Unity 一致、是现行标准；做在 world-space SpriteRenderer 上反而比 Unity 的 Canvas-only 更适配 2D。唯一值得动的是 radial 的 AA + 是否让硬边 radial 进 public drawMode，而非推翻几何方案。
2 participants
Summary
Adds support for Sprite Filled draw mode — render only a portion of a sprite based on a fill ratio along a configurable axis/origin. Useful for progress bars, cooldown indicators, etc.
Changes
FilledSpriteAssembler(~613 lines) handles vertex generation for filled modeSpriteFilledMode— direction (Horizontal / Vertical / Radial90 / Radial180 / Radial360)SpriteFilledOrigin— anchor point for the fill directionSpriteDrawModewithFilledISpriteRendererinterfaceSpriteRendererintegrates filled assemblerTest Plan
SpriteFilledMode×SpriteFilledOrigincombinationsSummary by CodeRabbit
Release Notes
New Features
Tests