Skip to content

Commit 7e87f9e

Browse files
Brooooooklynclaude
andauthored
fix: clip with nested transforms intersects in device space (#1202)
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 60a34e8 commit 7e87f9e

File tree

6 files changed

+126
-40
lines changed

6 files changed

+126
-40
lines changed

__test__/draw.spec.ts

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -451,6 +451,98 @@ test('clip-no-divergence-after-promotion', async (t) => {
451451
await snapshotImage(t)
452452
})
453453

454+
// Regression test for https://github.com/Brooooooklyn/canvas/issues/1198
455+
// Nested clips at different transforms should intersect in device space, not raw path coordinates
456+
test('clip-nested-different-transforms', async (t) => {
457+
const canvas = createCanvas(200, 200)
458+
const ctx = canvas.getContext('2d')!
459+
ctx.fillStyle = 'white'
460+
ctx.fillRect(0, 0, 200, 200)
461+
462+
// Clip 1 at 2x scale: rect(0,0,80,80) → device space 0,0 to 160,160
463+
ctx.setTransform(2, 0, 0, 2, 0, 0)
464+
const clip1 = new Path2D()
465+
clip1.rect(0, 0, 80, 80)
466+
ctx.clip(clip1)
467+
468+
// Clip 2 at identity: rect(0,0,160,160) → device space 0,0 to 160,160
469+
ctx.setTransform(1, 0, 0, 1, 0, 0)
470+
const clip2 = new Path2D()
471+
clip2.rect(0, 0, 160, 160)
472+
ctx.clip(clip2)
473+
474+
// Fill the intersection — should be 160x160 (both clips map to same device-space region)
475+
ctx.setTransform(1, 0, 0, 1, 0, 0)
476+
ctx.fillStyle = 'green'
477+
ctx.fillRect(0, 0, 200, 200)
478+
479+
// Expected: 160x160 green area (both clips cover 0,0 to 160,160 in device space)
480+
// Bug: only 80x80 green area (intersection computed in raw coords without transforms)
481+
await snapshotImage(t, { canvas, ctx })
482+
})
483+
484+
// Regression test for https://github.com/Brooooooklyn/canvas/issues/1198
485+
// Y-flip transform with nested clip (PDF.js use case)
486+
test('clip-nested-y-flip-transform', async (t) => {
487+
const canvas = createCanvas(200, 200)
488+
const ctx = canvas.getContext('2d')!
489+
ctx.fillStyle = 'white'
490+
ctx.fillRect(0, 0, 200, 200)
491+
492+
// Y-flip transform: maps (0,0)-(200,200) in path coords to full canvas
493+
ctx.setTransform(1, 0, 0, -1, 0, 200)
494+
495+
// First clip covering full canvas in flipped coords
496+
const clip1 = new Path2D()
497+
clip1.rect(0, 0, 200, 200)
498+
ctx.clip(clip1)
499+
500+
// Second clip at different scale
501+
ctx.setTransform(2, 0, 0, 2, 0, 0)
502+
const clip2 = new Path2D()
503+
clip2.rect(0, 0, 50, 50)
504+
ctx.clip(clip2)
505+
506+
// Reset transform and fill
507+
ctx.setTransform(1, 0, 0, 1, 0, 0)
508+
ctx.fillStyle = 'blue'
509+
ctx.fillRect(0, 0, 200, 200)
510+
511+
// Expected: 100x100 blue area (clip2 at 2x scale maps to 0,0-100,100 in device space,
512+
// intersected with clip1 which covers full canvas)
513+
await snapshotImage(t, { canvas, ctx })
514+
})
515+
516+
// Regression test for https://github.com/Brooooooklyn/canvas/issues/1198
517+
// Current path clip (beginPath/rect/clip) also affected, not just Path2D
518+
test('clip-nested-different-transforms-current-path', async (t) => {
519+
const canvas = createCanvas(200, 200)
520+
const ctx = canvas.getContext('2d')!
521+
ctx.fillStyle = 'white'
522+
ctx.fillRect(0, 0, 200, 200)
523+
524+
// Clip 1 at 2x scale: rect(0,0,80,80) → device space 0,0 to 160,160
525+
ctx.setTransform(2, 0, 0, 2, 0, 0)
526+
ctx.beginPath()
527+
ctx.rect(0, 0, 80, 80)
528+
ctx.clip()
529+
530+
// Clip 2 at identity: rect(0,0,160,160) → device space 0,0 to 160,160
531+
ctx.setTransform(1, 0, 0, 1, 0, 0)
532+
ctx.beginPath()
533+
ctx.rect(0, 0, 160, 160)
534+
ctx.clip()
535+
536+
// Fill the intersection — should be 160x160
537+
ctx.setTransform(1, 0, 0, 1, 0, 0)
538+
ctx.fillStyle = 'red'
539+
ctx.fillRect(0, 0, 200, 200)
540+
541+
// Expected: 160x160 red area
542+
// Bug: only 80x80 red area
543+
await snapshotImage(t, { canvas, ctx })
544+
})
545+
454546
test('closePath', async (t) => {
455547
const { ctx } = t.context
456548
ctx.beginPath()
628 Bytes
Loading
630 Bytes
Loading
628 Bytes
Loading

src/ctx.rs

Lines changed: 29 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -252,8 +252,7 @@ impl Context {
252252
}
253253

254254
pub fn clip(&mut self, path: Option<&mut SkPath>, fill_rule: FillType) {
255-
// Clone the clip path to avoid borrow conflicts
256-
let mut clip_path = match path {
255+
let clip_path = match path {
257256
Some(p) => {
258257
p.set_fill_type(fill_rule);
259258
p.clone()
@@ -264,38 +263,30 @@ impl Context {
264263
}
265264
};
266265

267-
// Per Canvas2D spec, multiple clip() calls should intersect with each other.
268-
// If there's an existing clip, compute the intersection.
269-
let should_update_state = if let Some(ref existing_clip) = self.state.clip_path {
270-
// op() returns false if intersection fails (degenerate paths, numerical instability)
271-
// If op() fails, clip_path remains unchanged (the new path) while the canvas will
272-
// still intersect with its existing clip. To avoid state divergence, we keep the
273-
// existing state.clip_path when op() fails, ensuring layer promotion restores correctly.
274-
if !clip_path.op(existing_clip, PathOp::Intersect) {
275-
#[cfg(debug_assertions)]
276-
eprintln!("Warning: Path intersection operation failed in clip()");
277-
false // Don't update state - keep existing clip to match canvas behavior
278-
} else {
279-
true
280-
}
281-
} else {
282-
true // No existing clip, always update state
283-
};
266+
// For state tracking (used by save/restore and layer promotion), compute the
267+
// cumulative clip in device space. Transform the new path by the current CTM
268+
// and intersect with the existing device-space clip.
269+
let mut device_clip = clip_path.clone();
270+
device_clip.transform_self(&self.state.transform);
284271

285-
// Store the cumulative clip path in state for restoration after layer promotion
286-
// Only update if intersection succeeded (or there was no existing clip)
287-
// IMPORTANT: We must also skip calling set_clip_path when op() fails to avoid
288-
// state divergence. If op() fails, the tracked state keeps OLD while Skia's
289-
// clipPath() would compute OLD ∩ NEW. After layer promotion, we'd restore to
290-
// OLD instead of OLD ∩ NEW. By skipping both state update AND canvas clip,
291-
// we make clip() a no-op when intersection fails, keeping everything consistent.
292-
if should_update_state {
293-
self.state.clip_path = Some(clip_path.clone());
294-
self.sync_clip_to_recorder();
295-
self.with_canvas_state(|canvas| {
296-
canvas.set_clip_path(&clip_path);
297-
});
272+
if let Some(ref existing_clip) = self.state.clip_path
273+
&& !device_clip.op(existing_clip, PathOp::Intersect)
274+
{
275+
#[cfg(debug_assertions)]
276+
eprintln!("Warning: Path intersection operation failed in clip()");
277+
// op() failed (degenerate paths). Skip both Skia and state update
278+
// to avoid divergence between tracked state and actual canvas clip.
279+
return;
298280
}
281+
282+
// Pass the raw path to Skia. Skia's clipPath() is cumulative and applies the
283+
// current canvas CTM, so it correctly handles nested clips at different transforms.
284+
self.with_canvas_state(|canvas| {
285+
canvas.set_clip_path(&clip_path);
286+
});
287+
288+
self.state.clip_path = Some(device_clip);
289+
self.sync_clip_to_recorder();
299290
}
300291

301292
pub fn clear_rect(
@@ -388,16 +379,18 @@ impl Context {
388379
if self.page_recorder.is_some() {
389380
let transform = self.state.transform.clone();
390381
let clip = self.state.clip_path.clone();
391-
self.with_canvas_state(|canvas| {
392-
canvas.set_transform(&transform);
393-
});
394382
// Re-apply clip if the restored state has one.
395-
// If restored state has no clip, canvas.restore() already removed it.
383+
// The clip is stored in device space, so apply at identity transform first.
396384
if let Some(ref clip_path) = clip {
397385
self.with_canvas_state(|canvas| {
386+
canvas.reset_transform();
398387
canvas.set_clip_path(clip_path);
399388
});
400389
}
390+
// Then restore the actual transform
391+
self.with_canvas_state(|canvas| {
392+
canvas.set_transform(&transform);
393+
});
401394
}
402395

403396
self.sync_transform_to_recorder();

src/page_recorder.rs

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -146,14 +146,15 @@ impl PageRecorder {
146146
for _ in 0..self.save_count {
147147
canvas.save();
148148
}
149+
// Restore clip state first (clip is stored in device space, apply at identity)
150+
if let Some(ref clip_path) = self.current_clip {
151+
canvas.reset_transform();
152+
canvas.set_clip_path(clip_path);
153+
}
149154
// Then restore transform state
150155
if let Some(ref transform) = self.current_transform {
151156
canvas.set_transform(transform);
152157
}
153-
// Then restore clip state
154-
if let Some(ref clip_path) = self.current_clip {
155-
canvas.set_clip_path(clip_path);
156-
}
157158
}
158159
self.changed = false;
159160
}

0 commit comments

Comments
 (0)