Skip to content

Add textured Crayon and paper-fixed Magic brushes to the native drawing candidate - #2740

Draft
KyleMit wants to merge 22 commits into
codex/native-drawing-magic-dependenciesfrom
codex/native-crayon-accepted-n1
Draft

KyleMit wants to merge 22 commits into
codex/native-drawing-magic-dependenciesfrom
codex/native-crayon-accepted-n1

Conversation

@KyleMit

@KyleMit KyleMit commented Oct 9, 2026 •

Copy link
Copy Markdown
Owner

Crayon retains visible paper tooth, builds coverage on repeated passes, and mixes crossing pigments. Magic reveals the selected coloring page’s fills at fixed paper coordinates while preserving earlier ink. Screen rendering and PNG capture use the same page and appearance; changing a page creates one Undo entry. Undo also clears the stale opened-picture/page notice.

The repair is composed with its drawing dependencies on base bdb826d. Head 71ed474 preserves the original PR ancestry and 28 historical evidence files. The current diff includes 29 product/test paths; five obsolete probe files are retired.

Validation: strict candidate TypeScript, repository check, lint and formatting passed; the full tools tier passed 8,926 tests across 402 files. The focused mounted checks passed 33 tests. Removing the Undo notice fix caused the three intended assertions to fail; restoring it passed.

Keep this PR in draft. Exact current browser/native output, both-OS qualification, accessibility, physical performance, and release checks remain open. Earlier native and CI captures retain their original source attribution. No framework selection or migration acceptance is claimed.

The original R1–R3 reviews remain on this PR; the last completed review covers the earlier f8539fc head.

These existing screenshots show the earlier 4f077 renderer; they do not qualify this repair. Fresh screenshots for the changed coloring-page behavior are pending.

Prior first Crayon pass Prior repeated pass Prior crossing pigments
Prior first Crayon pass Prior Crayon buildup Prior crossing pigments

@KyleMit KyleMit left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review by the claude rival agent, round 1, of pull request 2740.

The range matches the requested commits (head 4f077fa, tree e7120297, base 9f64c4c). I read the full diff: the model, parser, brushes, crayon, Ink, both CrayonGlaze variants, the DrawingSurface and screen wiring, the probe and comparator, the six new test files and the evidence README. I also read the surrounding save/open code (drawingFiles*.ts runs parseDrawing on both save and open) and the gating of the Clear, Undo and Pictures buttons while a stroke is in progress.

What I ran

  • The seven targeted test files with 2 workers: 62/62 passed.
  • A scratch script against crayon.ts that measured texture size, pass counts and phase periodicity:
    • Each band's texture paths are [0, 109223, 29883] and [0, 110002, 29883] characters long, about 279 KB per Crayon stroke. Shade 0 is always empty.
    • A 20-sweep scribble produces 20 passes.
    • Phases repeat every 256 stroke seeds.
    • 100k points split into 249 passes in about 96 ms on Node.
  • I traced react-native-svg 15.15.4's native filter, toDataURL and BackgroundImage code on Android and iOS.

Model and parser: no defects found

  • Seed derivation, the rainbow advancing on Clear and restoring on Undo, and v1 Pencil/Marker compatibility all behave as intended.
  • The parser is strict: it rejects v1 Crayon/Magic, a missing or out-of-range rainbow, a mismatched Magic rainbow, and bad seeds or colors.
  • Saving re-runs parseDrawing, so a Magic stroke whose rainbow differed from the drawing's would make the picture unsaveable. I checked whether the UI can produce one: Clear, Undo and Pictures are disabled while a stroke is in progress, and the paper refuses to hand its touch to another control, so I don't consider it reachable.
  • The useId-based IDs (_R_0_ form) parse correctly in react-native-svg's url(#…) regexes.
  • The web glaze and the export path are covered by the actual raster tests and the published browser evidence.

Main finding (from reading the library source, not reproduced on a device)
The native glaze reads BackgroundImage. On Android, react-native-svg's export function toDataURL(width, height) draws into a fresh 1024×768 bitmap but never updates the bitmap that BackgroundImage comes from. That input stays as the export view's last on-screen frame, which is already finished and is at device-pixel size. FeBlendView then draws it unscaled at 0,0, so Android PNG exports would darken Crayon against misaligned, already-composited content. This is a concrete mechanism that the pending native PNG gate is likely to hit; the fix is in the source, not the device rig.

Other findings

  • One memory/scaling suggestion: the per-stroke texture definitions and per-pass full-paper filters, with measured sizes. The README already names both causes, so this adds numbers and a concrete fix rather than new information.
  • One nit: shade 0 is dead code.

Pending obligations I am not waiving
Native fidelity, the first 245-pixel native mismatch, the 298.875 MiB SMALL RSS result, the observers, and both-OS rich-brush output remain open. They are pending, not defects demonstrated by this diff.

Verdict: the web-scoped integration claim holds up. Do not treat the native Crayon glaze/export path as on track until the Android BackgroundImage problem below is addressed.

Comment on lines +11 to +19
<Filter
id={id}
filterUnits="userSpaceOnUse"
x={0}
y={0}
width={PAPER_WIDTH}
height={PAPER_HEIGHT}
>
<FeBlend in="SourceGraphic" in2="BackgroundImage" mode="darken" result="mixed" />

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking: Android PNG export would mix Crayon against the wrong backdrop. I traced this in react-native-svg 15.15.4's source; I did not reproduce it on a device.

The native glaze uses FeBlend in2="BackgroundImage". Here is how Android supplies that input:

  1. RenderableView.render takes the background from getSvgView().getCurrentBitmap(). It also allocates the element bitmap at canvas.getWidth() × canvas.getHeight().
  2. mCurrentBitmap is only set in SvgView.drawOutput(), which is the on-screen render. That bitmap is getWidth() × getHeight() in device pixels.
  3. SvgView.toDataURL(int width, int height) is the function DrawingSurface.capturePng reaches through toDataURL(..., { width: PAPER_WIDTH, height: PAPER_HEIGHT }). It draws into a new 1024×768 bitmap and never updates mCurrentBitmap.
  4. During export, every Crayon pass's BackgroundImage is therefore the export view's last on-screen frame. That frame:
    • already contains every stroke, including the pass itself and later Pencil/Marker ink;
    • is 1024·density × 768·density pixels.
  5. FeBlendView.applyFilter does canvas.drawBitmap(in2, 0, 0, paint) with no scaling. So on any device whose density is not 1, the darken step reads a zoomed top-left crop of the finished picture.

Result: Crayon colors in Android PNG exports would be darkened by unrelated ink and would not match the screen. This is exactly the "real PNG composition" this unit owns.

A crash is also possible, though it depends on timing. SvgView.invalidate() recycles mBitmap but leaves mCurrentBitmap pointing at it. If the export view is invalidated after its last draw and toDataURL runs immediately (notRendered() is false), FilterUtils.applySourceAlphaFilter(background) draws a recycled bitmap.

The web path (CrayonGlaze.web.tsx, CSS mix-blend-mode) and iOS (CGBitmapContextCreateImage(context) on the live context) don't take this route. That is why the browser evidence can't catch it.

Fix: don't depend on BackgroundImage for export on Android. Options:

  • render the darken against an explicit FeImage or other input you control;
  • composite the glaze outside SVG filters;
  • at minimum, add an Android-specific export check (draw yellow Crayon, cross it with blue, export, and compare the crossing pixels to the screen) before declaring the native glaze viable.

Comment on lines +52 to +63
<Defs>
{CRAYON_TEXTURES.flatMap((paths, bandIndex) =>
paths.map((path, shade) => (
<Path
key={`${bandIndex}-${shade}`}
id={`${id}-wax-${bandIndex}-${shade}`}
d={path}
fill={waxColor(color, shade)}
/>
))
)}
</Defs>

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Texture geometry is still duplicated per stroke, and filters scale with the number of passes.

The "reuse geometry" change removes the per-pass copies, but each CrayonInk instance still emits its own six <Path> definitions:

  • Measured d lengths are [0, 109223, 29883] and [0, 110002, 29883] characters, about 279 KB per Crayon stroke.
  • The export Artwork mounts a second copy for every stroke, so 1,000 Crayon strokes (the MAX_STROKES limit) is about 279 MB of path props sent to native while live, and about twice that during export.

Each pass also gets its own full-paper Filter inside CrayonGlaze. crayonPasses splits on every reversal; a 20-sweep scribble gave 20 passes. react-native-svg on Android creates several full-canvas bitmaps per filter on every redraw (element, SourceAlpha, BackgroundAlpha, a result per primitive, plus copies), and every draft sample invalidates the whole SvgView. Back-and-forth coloring, the typical way kids use a crayon, therefore multiplies full-screen allocations per frame.

The README already lists both effects as unproved contributors to the 298.875 MiB SMALL result. This finding adds the numbers and a concrete fix rather than a new defect.

Fix:

  • Hoist the texture <Path>s into one Defs per Artwork/Svg, with no fill, and set the wax color on each <Use fill=…>. Alternatively, key definitions by palette color and shade; there are at most 10 × 3 × 2.
  • Consider batching consecutive passes of one stroke into a single glaze where the order of darkening permits.
  • Measure before and after with the unchanged SMALL fixture.

for (let x = 0; x <= CRAYON_TILE_PX; x++) {
const height = tooth(x, y);
const next =
x < CRAYON_TILE_PX && waxAt(x, y, 0, coverage) ? Math.min(2, Math.floor(height * 3)) : -1;

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Shade 0 is unreachable. waxAt(x, y, 0, coverage) only keeps the top 45%/63% of PAPER_HEIGHTS, and in practice those pixels always have tooth ≥ 1/3. As a result, Math.min(2, Math.floor(height * 3)) never returns 0.

Measured crayonTexture output is '' for shade 0 in both bands. Consequences:

  • the lighter waxColor(…, 0) (+4%) never appears, so the three-shade texture is really two shades;
  • every stroke still emits an empty <Path> definition;
  • every pass emits 8 <Use> references to it, which the native side still has to resolve.

Fix: either rescale the shade mapping over the wax-covered height range so all three shades occur, or drop shade 0 from the paths and waxColor. Update native-crayon-references.test.mjs to match (it currently counts the empty shade).

@KyleMit KyleMit left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review by the claude rival agent, round 2, of pull request 2740.

The range matches the requested head (41aff66, base 9f64c4c). I checked my three round-1 findings against commit 41aff66 and reviewed everything that commit changed:

  • the react-native-svg@15.15.4 patch and its pnpm lock/workspace registration;
  • the patch qualifier, check-native-topology.mjs and the migration test edits;
  • InkArtwork/Ink/DrawingSurface;
  • the updated reference, phase and output tests.

Round-1 findings

  1. Android export backdrop (blocking): fixed in source. The patch adds SvgView.drawBitmap, which:

    • sets mCurrentBitmap to the bitmap being drawn;
    • calls drawChildren;
    • restores the previous binding in finally.

    drawOutput and both toDataURL overloads now use it. I traced what this means in the library:

    • RenderableView.render reads getCurrentBitmap() only during drawChildren. The background is now the export bitmap itself. It is 1024×768, the same size as the element bitmap from canvas.getWidth()/getHeight(), so FeBlendView's unscaled drawBitmap(in2, 0, 0) lines up.
    • It holds only the content painted so far: earlier passes are composited before the next filter's saveLayer.
    • The glaze's G opacity sibling draws into its own layer bitmap but is composited before the next pass reads the backdrop.
    • The stale/recycled-bitmap path is gone because the binding never outlives a draw.
    • The only other drawChildren callers are nested Svg and ForeignObject, which the app doesn't use.

    The installed node_modules/react-native-svg/.../SvgView.java hashes to 6e59ea9f…bc62, identical to the committed patched fixture. npm run check:migration:native-topology exits 0, and its svgBackdrop qualification covers 1,762 installed files and the patch hash 894b760c….

  2. Memory/scaling (suggestion): the texture-geometry half is fixed. InkArtwork now defines each band/shade Path once per used color per Svg root, so live and export roots get distinct useId scopes. Strokes and passes refer to those definitions through <Use>. The draft's color is included, and paint order is unchanged. Labels are all single-word palette names, so the IDs are well formed. The per-pass full-paper filter cost is unchanged, and the PR says so. I'm not re-reporting it.

  3. Shade-zero nit: fixed as suggested. Empty paths are dropped and the original shade indices and wax colors are kept. The lighter shade still never appears, which is acceptable as a design choice.

New code

I found no new defects. Tests I ran in my sandbox with 2 workers:

  • the 7 brush test files plus the 5 migration suites: 85 + 113 tests passed;
  • the new byte-for-byte comparisons against the hash-bound earlier Ink renderer, covering repeated colors, backtracking, Magic, later solid ink and a live draft;
  • the missing-scope refusal test.

One process note: my first run of three migration suites failed with Unreviewed SVG directory: …/horcrux/svg/.claude. My own session had created that empty directory inside the package while my working directory was there. After I deleted it, the qualifier passed. This shows the qualifier rejects unknown directories as intended; it is not a code defect.

Not independently verified

I did not compile or run anything on Android or iOS. The following rest only on the handler's private receipts:

  • the claimed Android Release build and workflow of joined candidate c81312b;
  • any Android Crayon crossing-pixel output at a display density other than 1;
  • iOS backdrop behaviour;
  • the private manifests at /private/tmp/....

The PR records these honestly as still to be done, along with the 245-pixel mismatch, the RSS results over the 256 MiB ceiling, full depth, and the release gates. I did not treat any of them as passed.

Verdict: all three round-1 findings are addressed in source, and the new code adds no defects I can demonstrate. Native runtime proof is still pending, as the PR states.

@KyleMit KyleMit left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review by the claude rival agent, round 3, of pull request 2740.

The range matches the requested head (f8539fc, base 9f64c4c). As claimed, the only commit since round 2 changes exactly four paths: .github/actions/setup-pnpm/action.yml, tools/migration/lib/web-host-source.mjs, tools/migration/tests/native-svg-backdrop-patch.test.mjs and tools/tests/workflow-hygiene.test.mjs. No renderer, patch or lock bytes changed.

Earlier findings: nothing reopened

  • Round 1's Android export backdrop: fixed by the SvgView.drawBitmap patch, as I traced in round 2.
  • Texture-geometry duplication: fixed by the per-Artwork, per-color definitions.
  • Shade-zero nit: fixed by omitting empty paths.
  • The per-pass full-paper filter cost is unchanged and still acknowledged.

The CI repair

setup-pnpm action. PNPM_CONFIG_PACKAGE_IMPORT_METHOD: copy is set as step env on pnpm/setup. That same step runs the install (install: ${{ inputs.install }}), so the setting applies to the install. No workflow runs a separate pnpm install afterwards. Its purpose is to make installed files independent copies, which the qualifier's nlink === 1 check requires. The cost is slower installs across all CI jobs; that is a trade-off, not a defect.

web-host-source.mjs (freezeSource). Before, any lock different from the reviewed topology lock was refused. Now exactly one alternative is admitted, and only when all of these hold:

  • qualifySvgBackdropPatch passes. That function authenticates:
    • the hash-pinned input (5d594882…);
    • the actual lock (47c8d8a7…) and the workspace;
    • the patch bytes;
    • the baseline lock and workspace fixtures.
  • Projecting the lock and workspace back removes the SVG change and leaves exactly the baseline.
  • The full installed react-native-svg tree matches, with independent files and authenticated bin links.
  • Two cross-checks pass: baselineLockSha256 === topologyLockSha256 and actualLockSha256 === lockSha256.

I confirmed the input's originalLockSha256 equals the authenticated baseline fixture hash, and that both equal the base commit's pnpm-lock.yaml (9fed398b…). A different composition can't pass under the reviewed baseline. When the lock is unchanged, the earlier path is unchanged (nativeQualification: null). Any other mismatch is still refused, and the cause is preserved. The policy is not weakened beyond this single pinned change.

Tests. The new retained-source tests call the real freezeSource against real git fixtures. They are not source-string checks. They include:

  • a positive admission;
  • a reviewed ancestor with a different baseline lock;
  • changed lock, workspace, patch or installed Java, each with restoration;
  • a hardlinked generated bin target, with restoration.

The workflow-hygiene test checks the YAML text. It includes removed and hardlink negative cases.

What I ran in my sandbox

  • I first generated web/.svelte-kit/tsconfig.json via svelte-kit sync. Without it, Vitest's transform fails; that is an environment issue.
  • With 2 workers: native-svg-backdrop-patch, workflow-hygiene and the web-host-* suites. 258 of 262 tests passed.
  • The 4 failures are in web-host-processes and web-host-preview. Both suites are unchanged by this delta. They fail at processIsRunning, which shells out to ps, and my sandbox refuses ps ("operation not permitted").
  • The handler's rerun of the same two files failed the same way at the same ps-based assertions, so I cannot confirm they pass at this head. I recorded this below.

Not independently verified
I did not read any of the handler's receipts:

  • the official run, the merge-tree identity and the artifact hashes;
  • the ZIP/CRC and source-row verification;
  • the build and browser controls;
  • the /private/tmp readback manifests;
  • the Android c813 observations.

All native, RSS, observer, fidelity, performance, release and full-product gates remain open as the handler listed them. This round adds no evidence for them.

Verdict: no new source defects. All three earlier findings remain addressed.

Unverified

  • The unchanged process-group suites web-host-processes.test.mjs (3 SIGINT/SIGTERM tests) and web-host-preview.test.mjs (which imports the changed web-host-source.mjs) pass at this head. — wanted node_modules/.bin/vitest run --maxWorkers=2 tools/migration/tests/web-host-processes.test.mjs tools/migration/tests/web-host-preview.test.mjs; The handler declined my first request because it used a tail pipeline. It then ran the command without one: exit 1, 4 failures, all at the ps-based processIsRunning assertion. My sandbox refuses ps outright ('operation not permitted'), and the handler's run failed in the same pattern, so this looks like an environment limit rather than a code problem. Not confirmed either way.

@KyleMit
KyleMit changed the base branch from codex/native-migration to codex/native-drawing-magic-dependencies October 10, 2026 23:48

This branch has not been deployed

No deployments
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