fix(svg): preserve dimension contract under sw/sh and pixelRatio < 1#44
Merged
Conversation
…xelRatio < 1 PR #41 introduced two unintended dimension changes for SVG textures: 1. Multiplying targetW/H by stage.pixelRatio meant any app rendering at sub-1 logical pixel ratio (e.g. the examples app at 720p/1080p = 0.667) downscaled SVG rasters below the requested size — making textures *less* sharp than pre-fix, the opposite of the intent. 2. A node that set only srcWidth/srcHeight (no w/h) used to produce a texture sized to the source crop, because the old getImageData(sx, sy, sw, sh) returned an sw×sh ImageData regardless of canvas size. The new code keyed targetW/H off width || naturalWidth, ignoring sw, so the texture became natural-size with a stretched crop drawn into it. This broke texture-svg.ts test #4 (rocko2 partial crop, expects 81x218; was getting 181x218). Fix: - Clamp the DPR multiplier to >= 1 so a sub-1 stage pixelRatio never downscales. HiDPI upscaling (the original goal) still applies. - Fall through to sw/sh when w/h aren't set, restoring the "crop dims drive texture dims" contract. Refreshed the chromium-ci texture-svg-1.png snapshot — all 5 dimension + event assertions now pass (181x218, 200x268, 125x25, 81x218, failure). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
#41 introduced two unintended changes to SVG texture dimensions that broke the existing texture-svg VRT (
Loaded Event Test: Fail). Both narrow tweaks inloadSvg.Regressions
1. Sub-1
stage.pixelRatiodownscales the raster.The examples app sets
logicalPixelRatio = resolution / appHeight = 720 / 1080 = 0.667(examples/index.ts:78). #41'sMath.ceil(targetW * pixelRatio)multiplied by that, so a 181-wide SVG rasterized to 121 pixels — lower resolution than before, the opposite of the HiDPI sharpness goal. Tests 1, 2, 3 hit this.2.
sw/shno longer drive texture size whenw/hare unset.Pre-fix,
getImageData(sx, sy, sw, sh)returned answ × shImageDataregardless of canvas size, so a node that set onlysrcWidth/srcHeightgot a texture sized to the source crop. #41 keyedtargetW/Hoffwidth || naturalWidth, ignoringsw, so the texture became natural-size with the crop stretched into it. Test 4 (rocko2 partial: expects 81×218, was getting 181×218) hit this.Fix
Two small adjustments to
textureSvg.ts:57-59:sw/shfall through whenw/hare null, restoring the "crop dims drive texture dims" contract.pixelRatio > 1 ? pixelRatio : 1clamps so sub-1 stages never downscale. HiDPI upscaling (the original intent) still applies.The fallback to
naturalWidth || widthis intentional —naturalWidthis preferred (intrinsic SVG dims), with.widthas the back-stop for browsers that return 0 for SVGs without explicit width attributes.Verification
Captured a fresh chromium-ci
texture-svg-1.pngvia the Docker runner. All five existing assertions pass:loaded/failedevents were never broken — they fire and carry adimensionspayload as before. This PR just makes that payload match the pre-#41 contract.Test plan
pnpm buildcleanpnpm test— 193/193 passtexture-svg— all 5 assertions passvisual-regression/certified-snapshots/chromium-ci/texture-svg-1.png🤖 Generated with Claude Code