fix(🐛): pop shader children from the top of the declaration stack - #4048
Open
giaBaoJS wants to merge 1 commit into
Open
fix(🐛): pop shader children from the top of the declaration stack#4048giaBaoJS wants to merge 1 commit into
giaBaoJS wants to merge 1 commit into
Conversation
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.
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.
Context
declareShaderinpackages/skia/src/sksg/Recorder/commands/Shaders.tsconsumed itschild shaders with
ctx.shaders.splice(0, children), taking them from the bottom ofthe declaration stack. Every other producer pushes onto the end (
ctx.shaders.push(...)),and the native recorder pops from the end too:
The two agree while the stack holds exactly
childrenentries, so simple trees are fine.They diverge as soon as a multi-child
<Shader>is not the first declaration under itsparent, because entries belonging to earlier siblings are still sitting below it.
Reproduction
Declarations are emitted depth first, so the stack reads
[red, green, blue]when theinner shader is declared. It takes
[red, green]instead of[green, blue], leaving[blue, inner]for the outer shader, whose first child is thenblue. The fill paintsblue where it should paint red. The native recorder paints red.
Fix
Pop the last
childrenentries, mirroringpopShadersincluding its clamp to the stacksize.
Test
packages/skia/src/sksg/__tests__/ShaderChildren.spec.tsxrenders the tree above andreads the resulting pixel. Reverting the one-line fix turns it red:
The runtime effect uses
mix(c0.eval(xy), c1.eval(xy), w)withwsupplied as a uniformso the SkSL optimizer cannot drop the second child, which would change the child count the
test depends on.
The rest of
yarn testinpackages/skiastill passes, along withyarn tscandyarn lint.