p5.js icon indicating copy to clipboard operation
p5.js copied to clipboard

[p5.js 2.0 Bug Report]: Transforms in `clip()` stopped working in 2.x

Open davepagurek opened this issue 6 months ago • 1 comments

Most appropriate sub-area of p5.js?

  • [ ] Accessibility
  • [ ] Color
  • [x] Core/Environment/Rendering
  • [ ] Data
  • [ ] DOM
  • [ ] Events
  • [ ] Image
  • [ ] IO
  • [ ] Math
  • [ ] Typography
  • [ ] Utilities
  • [ ] WebGL
  • [ ] Build process
  • [ ] Unit testing
  • [ ] Internationalization
  • [ ] Friendly errors
  • [ ] Other (specify if possible)

p5.js version

2.0.3

Web browser and version

Firefox

Operating system

MacOS

Steps to reproduce this

The following code clips to a circle at the center of the screen:

function setup() {
  createCanvas(400, 400);
}

function draw() {
  background(220);
  noStroke()
  
  clip(() => {
    push()
    translate(width/2, height/2)
    circle(0, 0, 60)
    pop()
  })
  fill(0)
  rect(0, 0, 400, 400)
}

In 1.11.8 it looks like this:

Image

In 2.0.3 it looks like this, which is unexpected:

Image

Live: https://editor.p5js.org/davepagurek/sketches/IVloB6s76W

davepagurek avatar Jun 12 '25 13:06 davepagurek

Between 1.x and 2.x, we changed how we generate the clip path. It used to be done by switching the fill color to transparent and drawing as usual after that, which also builds up the drawing context's clip path: https://github.com/processing/p5.js/blob/5c17bde8f193c7a9843faa29a8fcc42056ff63df/src/core/p5.Renderer2D.js#L174-L179

Now, to avoid the transparent drawing, we just build up a Path2D object with the clip path: https://github.com/processing/p5.js/blob/3eae27613378af782910034211bc328cdaa8f125/src/core/p5.Renderer2D.js#L264-L269

The problem, I think, is that addPath does not take in the active transformation by default. However, it can do that if you pass in a DOMMatrix with addPath(path, transformMatrix): https://developer.mozilla.org/en-US/docs/Web/API/Path2D/addPath

So maybe we need to update the addPath call with something like drawingContext.getTransform()? ("something like" because maybe we need to remove the initial transform from when beginClip is called? That might not be an issue though, have to try it and see.)

davepagurek avatar Jun 12 '25 19:06 davepagurek

Hi @davepagurek Thanks for the detailed breakdown of the issue I tested this locally and wanted to share some findings: I confirmed that on Linux (tested with Firefox and Chromium), the clip() behavior seems to work even without passing a transform matrix to addPath(). However, I understand this might be platform- or browser-specific, since the spec says addPath() doesn't apply transforms unless explicitly provided. To test this, I tried updating drawShape() in Renderer2D.js with:

if (this._clipping) {
  const transformMatrix = this.drawingContext.getTransform();
  this.clipPath.addPath(visitor.path, transformMatrix);
  this.clipPath.closePath();
}

With this change, clipping respects translate() and rotate() consistently and it aligns with your suggestion in the issue. Just wanted to discuss whether this approach looks solid to you, and if there are any side effects we should be aware of before using getTransform() in all cases.

VANSH3104 avatar Jul 13 '25 07:07 VANSH3104

Sorry for the delay, that approach looks good to me. I mentioned maybe needing to remove the initial transform -- does this break if, for example, you call scale(2) before clipping? would that result in the scale getting double-applied? If so we might have to do something like this.drawingContext.getTransform().multiply(inverseInitialTransform)

davepagurek avatar Jul 27 '25 18:07 davepagurek

@davepagurek Thanks for the insight! since we’re not modifying the Path2D directly and only passing it via addPath(visitor.path, transformMatrix), the transform should apply just once, correct? Want to confirm this before proceeding, in case there's any risk of double-applying transforms like scale()

VANSH3104 avatar Jul 30 '25 11:07 VANSH3104

The double-application could come from the fact that when the path is drawn to the screen, it applies the current transformation. So if you are additionally applying the transformation when constructing the path, that could be a second transformation. Here's an example:

scale(2)
clip(() => {
  circle(0, 0, 10)
})
rect(0, 0, 20, 20)

What might happen here is that when we construct the clipping path, we apply the scale(2) as it's part of drawingContext.getTransform(), so the path contains a 20px circle instead of 10. Then, when we draw the rectangle, we'd be masking the 20x20 rectangle with the 20px circle, but then when drawing it to the canvas, apply the 2x scale again, which is sort of equivalent to a 40x40 rectangle being clipped with a 40px circle.

That wouldn't happen in this scenario:

clip(() => {
  scale(2)
  circle(0, 0, 10)
})
rect(0, 0, 20, 20)

If clip has an internal push/pop (I can't remember for sure, need to double check that!) then the clip path still gets a 20px circle, but because the scale(2) doesn't apply to the rect call, we're correctly masking a 20x20 rect with a 20px circle at 1x scale, so the 2x multiplier is only added once to the circle.

If that is indeed what happens (we need to test to confirm), then my suggestion would be, when adding to the clip path, to apply only the transformations that have changed since the initial clip state. So, looking at this example again:

scale(2)
clip(() => { // Initial state is 2x
  circle(0, 0, 10) // The circle has the same transformation as the initial state
})
rect(0, 0, 20, 20)

For this example, since the scale(2) is outside of the clip, we should add the circle without any extra transformation, as it is in the same coordinate space as the content being drawn. And in this scenario:

clip(() => { // Initial state has no transform
  scale(2)
  circle(0, 0, 10) // The circle should be added with a 2x scale now
})
rect(0, 0, 20, 20)

...because the circle is drawn with 2x scale relative to the initial state, then we would need to add it to the clip path with a 2x transformation.

A way to implement that could be to store this.drawingContext.getTransform().inverse() at the start of clip() and then multiply that with this.drawingContext.getTransform() when we add new paths to the clip path rather than using this.drawingContext.getTransform() unmodified.

davepagurek avatar Jul 30 '25 12:07 davepagurek

@davepagurek We discussed this issue earlier I’d like to start working on it again. Could you please assign it to me?

VANSH3104 avatar Nov 03 '25 10:11 VANSH3104

Thanks @VANSH3104!

davepagurek avatar Nov 03 '25 17:11 davepagurek

@davepagurek I work into the clip() transform issue and found some new details that clarify what’s happening internally. First, while testing, I added logs inside both drawShape() and beginClip(). I noticed that drawShape() never runs at all during clipping meaning none of the shape-building logic inside it is actually executed. I also confirmed that options.shape is always undefined inside beginClip(), which suggests that no shape data is being passed into the clip stage. After tracing the calls, it looks like the primitive shape methods (ellipse, rect, triangle, etc.) completely bypass the vertex/shape system that would normally go through drawShape(). Instead, when clipping is active, they directly use:

const ctx = this.clipPath || this.drawingContext;
ctx.ellipse(centerX, centerY, radiusX, radiusY, 0, 0, 2 * Math.PI);

Here’s the problem: when this._clipping is true, ctx is a Path2D object, and methods like Path2D.ellipse() don’t automatically apply the current canvas transform. That’s why transforms like translate() and scale() have no visible effect inside clip(). I add it manually but it just swap the position from top left to top right its dont work Does that match your understanding of the current clipping flow? If it sounds right ?

VANSH3104 avatar Nov 05 '25 13:11 VANSH3104

That's a really good catch! I had forgotten that those primitives don't go through the existing Shape class. I think the Shape stuff I was talking about will still be required to support beginShape/endShape to generate a clip mask, so that means we might need to manually apply the current transformation matrix times the inverse of the "base" transformation (including scaling) like I was mentioning at the end of https://github.com/processing/p5.js/issues/7903#issuecomment-3136039238 for all those manual cases too.

While that could work, we might also consider whether we should move those primitives into the shape drawing system. That's also a little complicated, but that would help us with other future tasks like being able to import an SVG into a Shape, so it could be worth the effort. That would mean transforming shapes like rectangles, quads, etc into existing straight line segments, but it also means making a subclass of ShapePrimitive for arcs and ellipses, and updating the PrimitiveToPath2DVisitor to be able to convert those new primitives back into path drawing commands.

davepagurek avatar Nov 05 '25 14:11 davepagurek