Skip to content

fix(🐛): pop shader children from the top of the declaration stack - #4048

Open
giaBaoJS wants to merge 1 commit into
Shopify:mainfrom
giaBaoJS:fix-shader-children-stack-order
Open

fix(🐛): pop shader children from the top of the declaration stack#4048
giaBaoJS wants to merge 1 commit into
Shopify:mainfrom
giaBaoJS:fix-shader-children-stack-order

Conversation

@giaBaoJS

@giaBaoJS giaBaoJS commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Context

declareShader in packages/skia/src/sksg/Recorder/commands/Shaders.ts consumed its
child shaders with ctx.shaders.splice(0, children), taking them from the bottom of
the declaration stack. Every other producer pushes onto the end (ctx.shaders.push(...)),
and the native recorder pops from the end too:

// packages/skia/cpp/api/recorder/DrawingCtx.h
std::vector<sk_sp<SkShader>> popShaders(int count) {
  ...
  // Get the last 'actualCount' shaders
  auto start = shaders.end() - actualCount;
  ...
}

The two agree while the stack holds exactly children entries, so simple trees are fine.
They diverge as soon as a multi-child <Shader> is not the first declaration under its
parent, because entries belonging to earlier siblings are still sitting below it.

Reproduction

<Fill>
  <Shader source={pickFirstChild}>
    <ColorShader color="red" />
    <Shader source={pickFirstChild}>
      <ColorShader color="green" />
      <ColorShader color="blue" />
    </Shader>
  </Shader>
</Fill>

Declarations are emitted depth first, so the stack reads [red, green, blue] when the
inner shader is declared. It takes [red, green] instead of [green, blue], leaving
[blue, inner] for the outer shader, whose first child is then blue. The fill paints
blue where it should paint red. The native recorder paints red.

Fix

Pop the last children entries, mirroring popShaders including its clamp to the stack
size.

Test

packages/skia/src/sksg/__tests__/ShaderChildren.spec.tsx renders the tree above and
reads the resulting pixel. Reverting the one-line fix turns it red:

● Shader children › resolves the children of a nested multi-child shader

  expect(received).toEqual(expected) // deep equality

  - Expected  - 1
  + Received  + 1

    Array [
  -   255,
      0,
      0,
  +   255,
    ]

The runtime effect uses mix(c0.eval(xy), c1.eval(xy), w) with w supplied as a uniform
so the SkSL optimizer cannot drop the second child, which would change the child count the
test depends on.

The rest of yarn test in packages/skia still passes, along with yarn tsc and yarn lint.

declareShader consumed its children with splice(0, children), which takes
them from the bottom of the shader stack. The native recorder pops the last
`children` entries instead (DrawingCtx::popShaders), so any tree where a
multi-child shader is not the first declaration under its parent resolved
different children on JS than on native.

A runtime effect whose second child is itself a multi-child runtime effect
is enough to hit it: the inner shader stole the shaders declared before it
and the outer one received a leftover leaf in their place.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant