fix(svg): DPR-aware rasterization, source-region crop, zero-copy upload#41
Merged
Conversation
- Rasterize SVGs at stage.pixelRatio so they stay sharp on HiDPI/4K displays instead of being upscaled from a logical-pixel raster. - Use the 9-arg drawImage form for sx/sy/sw/sh, matching the source-region crop semantics documented on ImageTextureProps. The previous code drew the SVG at the destination size then cropped via getImageData, which produced a destination crop (top-left slice) rather than sampling the intended source region. - Return an ImageBitmap when createImageBitmap is available so the GL uploader (which already accepts ImageBitmap/HTMLImageElement) skips the forced getImageData CPU readback and the w*h*4 byte allocation. Falls back to ImageData on Chrome <50. - Set img.crossOrigin = 'anonymous' for non-base64 SVG sources so cross- origin SVGs don't taint the canvas and trigger SecurityError on read. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
5 tasks
3b70317 to
8ce29f0
Compare
4 tasks
chiefcll
added a commit
that referenced
this pull request
May 26, 2026
…xelRatio < 1 (#44) 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
Three related fixes to SVG loading in
src/core/lib/textureSvg.ts:stage.pixelRatioinstead of logical (CSS) pixels. Previously the backing canvas was sized to the requested logicalw × h, then the renderer upscaled it on a 2×-DPR display, giving a half-resolution result.sx/sy/sw/shnow use the 9-argdrawImage(img, sx, sy, sw, sh, 0, 0, w, h)form, matching the source-rect semantics documented onImageTextureProps. The old path drew at full destination size then calledgetImageData(sx, sy, sw, sh), which was a destination-region crop (top-left slice), not a sample of the intended source region.createImageBitmapis available, the rasterized canvas is converted to anImageBitmapand returned. The existing WebGL uploader inWebGlCtxTexture.ts:182-196already acceptsImageBitmap, so the previousgetImageData()CPU readback and thew·h·4-byteUint8ClampedArrayallocation are eliminated.ImageDatais retained as the Chrome <50 fallback (preserves the Chrome 38 floor perBROWSERS.md).Also sets
img.crossOrigin = 'anonymous'for non-base64 SVG sources so cross-origin SVGs don't taint the canvas and triggerSecurityErroron the readback path.Why
Uint8ClampedArrayallocation per load (4 MB at 2× DPR). Larger SVGs scale linearly.getImageData()readback in the common path.sw/sh < w/h. No callers currently exercise that path in-tree, but the prop docs advertise source-crop semantics.Touched files
src/core/lib/textureSvg.ts—loadSvgrewritten (now async, acceptspixelRatio, returnsImageBitmapwhen available).src/core/CoreTextureManager.ts— added a publicpixelRatiogetter that forwards tostage.pixelRatio(soImageTexturedoesn't need to reach through privatestage).src/core/textures/ImageTexture.ts— passesthis.txManager.pixelRatiotoloadSvg; collapsed the duplicatetype === 'svg'/isSvgImage(src)branches.Backwards compatibility
loadSvgis module-local (re-exported only inside the engine), and its newpixelRatioparameter has a single caller in this repo.w: 200, h: 200on a 2× DPR display now gets a 400×400 GPU texture (still drawn at 200×200 logical). This matches how the engine treats other texture sources at HiDPI and is the whole point of the fix — flagging it here in case any consumer was relying on the prior behavior to keep memory low.premultiplyAlpha: falsepreserved (canvas-2D output is straight alpha).Test plan
pnpm build— cleanpnpm test— 193/193 passpnpm linton touched files — no new warningsSecurityErrorsx/sy/sw/shon an SVG texture, confirm the cropped output matches the documented source-region semanticsexamples/tests/to lock in the new behavior🤖 Generated with Claude Code