[pull] main from Shopify:main - #221
Merged
Merged
Conversation
#4015 fixed processTransform3d but missed the C++ mirror of it in the native recorder, which builds the same matrix for transform props with the same two axes swapped. The SkM44 constructor reads its arguments row-major, so skewX has to put tan(angle) in row 0 - exactly like the TypeScript version it shadows. Since the snapshots were regenerated for the corrected behaviour, native now disagrees with them and Transforms.spec fails on main for both test-android and build-test-ios-graphite. Co-authored-by: William Candillon <wcandillon@gmail.com>
…ty (#4023) * fix(🐛): derive the inner shadow from the shape outline, not its opacity An inner shadow is generated outside the shape, offset, blurred, and clipped back into it. "Outside" is produced with `SrcOut` against the source graphic, which yields `shadowColor x (1 - dst.alpha)` - the complement of the source's *alpha*, not of its silhouette. Inside a translucent shape `1 - alpha` is non-zero, so the shadow is generated across the whole interior and then clipped onto it, tinting the shape towards the shadow colour. The extent of the effect is decoupled from the blur radius and the offset: a shadow with `blur={0} dx={0} dy={0}`, which cannot move anything from outside the shape to inside it, still tints a translucent shape uniformly. Saturating alpha before the `SrcOut` makes the complement geometric again: the shadow is generated only outside the outline, and its reach is bounded by the blur and the offset, for any fill opacity. Opaque shapes are unaffected, which is why the existing inner shadow baselines do not move. The same graph is built twice - once in the TypeScript player used on web and in the JS recorder, once in `cpp/api/recorder/ImageFilters.h` for iOS, Android and macOS - so both are updated. --------- Co-authored-by: William Candillon <wcandillon@gmail.com>
* fix(🐛): scale SkMatrix uniformly when y is omitted SkMatrix.scale() and postScale() take an optional y. The CanvasKit backend falls back to x for a uniform scale, but the JSI bindings fell back to 1, so matrix.scale(0.5) halved only the x axis on iOS and Android while halving both on Web. The renderer's own "Scale with origin using a matrix" test pins the uniform result ([0.5, 0, 192, 0, 0.5, 192, 0, 0, 1]), but it runs on CanvasKit only, so the native path was never covered. * test(🧪): run the SkMatrix scale spec against native too Move it to renderer/__tests__/e2e and go through surface.eval, so the JSI bindings this fixes are actually exercised. As a plain unit test it only ever ran against CanvasKit, which was already correct. --------- Co-authored-by: William Candillon <wcandillon@gmail.com>
* fix(🐛): don't emit a CTM restore for a CTM that never saved
processCTM() records a SaveCTM command whenever any CTM prop is defined, and
visitNode() then pairs it with an unconditional restoreCTM(). saveCTM() is
narrower: it only saves the canvas when there is a transform/matrix, a clip or
a layer. When a group carries a CTM prop that falls outside that set, the
restore has no matching save and pops a save belonging to an ancestor, so every
later sibling of that group is drawn without the ancestor's transform or clip.
Three ways to hit it, all of them ordinary code:
- `origin` with no `transform` or `matrix` (an identity transform, so
processTransformProps2 returns null and nothing is saved);
- `clip={cond && path}` where `cond` is false, which passes `clip={false}`;
- `invertClip` on a group that has no clip.
Emit the CTM command only when it will actually save. This is the shared
visitor, so the fix covers the native recorder, the CanvasKit player and the
static container at once, and it leaves the two save conditions expressed in
one place instead of two that can drift.
Adds a regression test covering each of the three props plus a control and a
CTM that does save; the three inert cases fail without this change.
* test(🧪): assert CTM balance with checkImage
Move the spec to renderer/__tests__/e2e so it runs against native as
well as CanvasKit, and compare rendered images instead of reading back
individual pixels. Every inert CTM case now has to match the reference
scene, where the group under test has no props at all.
---------
Co-authored-by: William Candillon <wcandillon@gmail.com>
…4022) * fix(🐛): apply the mask srcIn composite once per group, not per draw `<Mask>` finishes by drawing its children onto the mask with `srcIn`, using `<Group blendMode="srcIn">`. `blendMode` on a `<Group>` is a paint property, so it is applied once per draw call inside the group rather than once to the group as a whole - `Group.tsx` only requests a layer when a `layer` prop is given. `srcIn` replaces the destination inside the coverage of the draw it is attached to. With a single child that coincides with compositing the children once, which is why the existing mask baselines are unaffected. With more than one child, every child after the first is composited against the previous child's result instead of against the mask, so overlapping translucent children lose their mutual blending and their alpha is multiplied a second time. Requesting a layer instead composites the children together first and applies `srcIn` once, to the result. --------- Co-authored-by: William Candillon <wcandillon@gmail.com>
Every SavePaint command must push exactly one frame onto the paint and
opacity stacks, because RestorePaint and RestorePaintDeclaration each pop
exactly one. Both backends break that invariant, in opposite directions
and on opposite branches.
The native recorder pushes twice for a standalone paint:
ctx->savePaint();
if (standalone) {
SkPaint freshPaint;
ctx->pushPaint(freshPaint); // second push
}
while the TypeScript player resets the frame savePaint() already pushed.
A <Paint> declaration is always closed by RestorePaintDeclaration, which
pops one frame, so each declaration leaves one stale frame behind. The
enclosing group's RestorePaint then pops that stale frame instead of its
own, and the group's opacity stays on the stack and is applied to every
sibling drawn afterwards — including siblings that are not in the group
at all. That is issue #3355: it only shows up with a <Mask> (or any
<Paint> child) inside the group, because that is what emits a standalone
paint.
The TypeScript player has the mirror defect on the other branch: the
`paint` prop pushes onto `paints` without pushing an opacity, so
restorePaint() underflows the opacity stack. getOpacity() then returns
undefined and setAlphaf(alpha * undefined) yields NaN, so everything
drawn after the group disappears entirely.
Fix both so a SavePaint always moves each stack by exactly one frame.
Fixes #3355
Co-authored-by: William Candillon <wcandillon@gmail.com>
* fix(🐛): trim a path before stroking it
drawPath applied the stroke prop before start/end, so trimming walked
the perimeter of the generated outline rather than the path the caller
drew. <Path path="M 20 128 L 236 128" stroke={{ width: 20 }} end={0.5} />
painted the right half of the line instead of the left.
The native recorder already trims first (PathCmd in
cpp/api/recorder/Drawings.h), so this only affected the JS player - Web
and the static container.
* test(🧪): compare rendered images for the path trim spec
Move it to renderer/__tests__/e2e so it also runs against native, draw
through surface.draw and compare with checkImage instead of sampling
alpha by hand. An untrimmed baseline sits next to the trimmed one so
the two halves are easy to tell apart.
---------
Co-authored-by: William Candillon <wcandillon@gmail.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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )