[UEPR-527] Investigate membership sprites rendering differently in paint editor vs. stage#492
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes inconsistent rendering between the paint editor and stage by ensuring transforms are correctly applied when clip-path is involved during SVG normalization in scratch-svg-renderer.
Changes:
- Adds logic to detect
clip-pathreferences on container elements and apply the accumulated transform to the referenced<clipPath>. - Refactors transform composition in the container traversal to compute
currentMatrixonce and reuse it for children.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const clipPathId = _parseUrl(clipPathAttr.value, null); | ||
| if (!clipPathId) return; | ||
| const clipPathEl = svgTag.getElementById(clipPathId); | ||
| if (!clipPathEl) return; | ||
| clipPathEl.setAttribute('transform', Matrix.toString(matrix)); |
There was a problem hiding this comment.
_parseUrl is called with null for windowRef, which disables the absolute-URL stripping logic used for Firefox (see _parseUrl implementation). If a clip-path reference is serialized as an absolute URL, getElementById will fail and the transform won't be applied. Pass through the windowRef argument from transformStrokeWidths instead of null.
| const clipPathEl = svgTag.getElementById(clipPathId); | ||
| if (!clipPathEl) return; | ||
| clipPathEl.setAttribute('transform', Matrix.toString(matrix)); | ||
| }; |
There was a problem hiding this comment.
Setting transform directly on the referenced <clipPath> overwrites any existing transform on that clipPath and also mutates a shared definition by ID. This will break valid SVGs where the same clipPath is referenced from multiple elements with different transforms. Consider cloning the clipPath (similar to how gradients are cloned in _createGradient) and updating the element’s clip-path URL to point at the per-transform clone, or at minimum compose with any existing clipPath transform instead of replacing it.
| @@ -523,12 +534,15 @@ const transformStrokeWidths = function (svgTag, windowRef, bboxForTesting) { | |||
| if (element.attributes.stroke) stroke = element.attributes.stroke.value; | |||
| } | |||
|
|
|||
| const currentMatrix = Matrix.compose(matrix, _parseTransform(element)); | |||
| _applyTransformToClipPath(element, currentMatrix); | |||
|
|
|||
There was a problem hiding this comment.
This change introduces new clip-path transform propagation behavior, but there are no existing tests covering clip-path handling in packages/scratch-svg-renderer/test/transform-applier.js. Please add a regression test that includes a transformed group with a clip-path reference (and ideally a case where a clipPath is reused) to ensure this stays correct across browsers.
There was a problem hiding this comment.
I think we already have similar tests.
| const inherited = Matrix.identity(); | ||
|
|
||
| // pass down transforms to leaf level | ||
| const _applyTransformToClipPath = function (element, matrix) { |
There was a problem hiding this comment.
Why did the SVG (e.g. Mia's eyes) render correctly in the canvas, but not in the stage?
There was a problem hiding this comment.
It probably has something to do with the transformation recalculating the gradient relative to a new bounding box after transforms are applied. The reason it goes wrong is because the code for the eye on the right
<linearGradient id="av" x1="-1309.38" y1="83.32" x2="-1294.744" y2="73.781" gradientTransform="translate(-1196.214) rotate(-180) scale(1 -1)" xlink:href="#as"/>
is missing
gradientUnits="userSpaceOnUse", which we do need to set here as well in order to correctly apply the colors at the right coordinates.
Resolves
https://scratchfoundation.atlassian.net/browse/UEPR-527
Proposed Changes
Explicitly handles logic for "clip-path" elements which otherwise don't pass down transform to its children, thus their disappearance. Big credit to Claude for this one.