fix(rendering): make SpriteEcsComponent.pivot Y-up to match the rest of the engine - #590
Open
stormmuller wants to merge 1 commit into
Open
fix(rendering): make SpriteEcsComponent.pivot Y-up to match the rest of the engine#590stormmuller wants to merge 1 commit into
stormmuller wants to merge 1 commit into
Conversation
…of the engine pivot used a Y-down convention ((0, 0) was the sprite's top-left) while world position, rotation, and every other Y-facing value are Y-up. The mismatch was an internal projection detail (the shader flips Y) leaking through pivot's public API, invisible at the default centered pivot. Flips pivot's Y in sprite.vert.glsl before it's combined with the quad's local position, and updates computeNineSliceRegions' pivot handling to match so nine-slice region placement agrees with the corrected shader. Breaking change for content using a non-centered pivot. Fixes #585
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Member
Author
|
In the ECS demo. The position looks correct, but the rotation looks inverted? |
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
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
SpriteEcsComponent.pivotused a Y-down convention ((0, 0)was the sprite's top-left) while world position, rotation, and every other Y-facing value in the engine are Y-up. The mismatch was an internal projection detail (the vertex shader flips Y before the projection matrix flips it back forposition/rotation) leaking through pivot's public API - invisible at the default centered pivot, which is why it survived.Traced the full pipeline per the issue and confirmed the analysis:
bindSpriteInstanceDatapre-negatesposition.world.yandrotation.worldto compensate for the projection's Y flip, butsprite.vert.glslcomputes the pivot-based local offset after that compensation, so it never gets flipped -pivot.y = 0landed at the sprite's top instead of its bottom.Changes
sprite.vert.glsl: negatea_instancePivot.yalongside the existing[0,1] -> [-1,1]doubling, so pivot's Y now gets the same flipposition/rotationalready get.(0, 0)is now bottom-left,(1, 1)is top-right.computeNineSliceRegions(compute-nine-slice-regions.ts): updated the region-offset math ((1 - pivot.y) * height - ...instead ofpivot.y * height - ...) so nine-slice region placement agrees with the corrected shader convention. The centered pivot(0.5, 0.5)produces identical output before and after (1 - 0.5 === 0.5), so this is a no-op for every sprite in the repo (no demo or test uses a non-centered pivot).sprite-component.ts: updatedpivot's JSDoc to describe the new Y-up convention.compute-nine-slice-regions.test.ts: replaced the single top-left-pivot test with a parameterized case pinning all four corners (bottom-left, bottom-right, top-left, top-right), per the issue's test-coverage callout.CHANGELOG.md: added aFixedentry under[Unreleased], flagged as a breaking change for non-centered pivots.Related issue(s)
Closes #585
Out of scope
The issue's design doc reference (
design/ui-system.md's pivot bridge, from #580) isn't indevyet, so there's nothing to remove there in this PR.Verification checklist
npm run check-typespasses with 0 errorsnpm testpasses (1036 tests)npm run lintpasses with 0 errors (pre-existing, unrelated TODO warnings only)npm run cspellpasses with 0 errorsnpm run check-exportspassespivot's documented semantics - no export changes needed/documentation-site/docs/docs- no page documents the pivot convention, so nothing to updategrep), so no demo code changes; still built/dist, randocumentation-site'stypecheck/build, and visually checked thenine-sliceandbrick-breakerdemos in a browser to confirm no regression at the default centered pivotChangelog
## [Unreleased]→#### FixedinCHANGELOG.mdGenerated by Claude Code