Fix transforms in clip() by using shape system for primitives using manual method #8236

VANSH3104 · 2025-11-06T07:41:32Z

Resolves #7903

Changes:

Modified ellipse() method in p5.Renderer2D to apply transforms during clipping
Used existing this.initialClipTransform (inverse of initial transform) stored in beginClip()
Applied correct matrix multiplication order: initialClipTransform.multiply(currentTransform)

Screenshots of the change:

PR Checklist

npm run lint passes
Inline reference is included / updated
Unit tests are included / updated

…anual method

VANSH3104 · 2025-11-06T07:46:50Z

@davepagurek I've implemented the manual fix for the ellipse() method and it's working correctly now!
I think moving to the class-based approach you suggested would be better long-term. Should I extend this manual fix to other primitives (rect, arc, etc.) first, or start working on migrating them to the shape system? and also drawshape function is useless i think its never call while clipping on or off

perminder-17

function setup() {
  createCanvas(400, 200);
  pixelDensity(1);
  noLoop();
}

function draw() {
  background(240);
  translate(200, 0); 
  clip(() => {
    scale(-1,1);                 
    ellipse(50, 100, 100, 100);  
  });
  fill(0, 0, 255, 120);
  ellipse(100, 100, 300, 300); 
}

what you get when you use this sketch? I think transformation is not correctly applied?

perminder-17

Also, your current solution applies to ellipse only? Do you think we need to add this solution in other shapes as well? arc/quad/line etc?

btw, thanks for working on this one.

VANSH3104 · 2025-11-11T19:55:49Z

@perminder-17 yes it is only applicable for ellipse now I need to change for all too

VANSH3104 · 2025-11-11T19:57:32Z

@perminder-17 I also have another idea class based approach also just want to discuss which one is better. I think This approach is temporary we need to find a perfect solution for that

VANSH3104 · 2025-11-11T20:03:25Z

it shows this @perminder-17

perminder-17 · 2025-11-12T08:44:10Z

@perminder-17 I also have another idea class based approach also just want to discuss which one is better. I think This approach is temporary we need to find a perfect solution for that

I saw @davepagurek's comment about the shape-system approach. It would be great long-term and would also unlock future tasks, but it feels like a fairly large refactor right now with higher regression risk. Since we already have the alternative approach working, how about we stick with that for now and plan to migrate the primitives into the shape drawing system later?

perminder-17 · 2025-11-12T09:02:37Z

it shows this @perminder-17

I think this is not the expected behavior.

I think you're storing inverse of initial clip transform into beginClip(), and multiplying with currentTransform ,

inside endClip() clip() is running with the current transform, and it's applying again i think. I am totally not sure with this but

Since, in the 1.x versions, it works something like: https://editor.p5js.org/aman12345/sketches/Csnrbz9BX , so would be great to test that too.

CC: @davepagurek for the confirmation on the approach as well as the bug if it's exist?

VANSH3104 · 2025-11-12T12:48:11Z

@perminder-17 i think When we using scale(-1,1) inside clip(), the negative scale breaks the matrix calculations and makes the clipping region invisible. Basic clipping are working fine, but it give issue with complex transformations.

… components

VANSH3104 · 2025-11-12T13:04:00Z

@perminder-17 Now it work as expected it can handle -ve values also and i add condition and use absolute values to find the original position

perminder-17 · 2025-11-13T10:40:11Z

src/core/p5.Renderer2D.js

+      if (currentTransform.a < 0 || currentTransform.d < 0) {
+        const fixedTransform = new DOMMatrix([
+          Math.abs(currentTransform.a), currentTransform.b,
+          currentTransform.c, Math.abs(currentTransform.d),
+          currentTransform.e, currentTransform.f


Looks like you're now forcing the geometry with negative transformations to make it positive. If the user actually wanted to mirror the orientation, it will actually cause problem to the geometry. I still think the problem is that our transformation is applied twice.

I just tested code, and it looks like:

When I apply scale(3) , the expected result is:

The current behavior in the changes of this PR:

function setup() { createCanvas(500, 500); background(240); clip(()=>{ scale(3); ellipse(20, 40, 40, 40); }) fill(255, 80, 80); rect(0, 0, 500, 500); }

this is the code I am using, Please ignore the calculations, I was just testing scale() and other transformations behaviour.

scale(2) is equivalent to scale(4), scale(3) is equivalent to scale(9) and I think this is the problem with the negetive stuff as well.

As I was mentioning on previous comments,

I think you're storing inverse of initial clip transform into beginClip(), and multiplying with currentTransform , inside endClip() clip() is running with the current transform, and it's applying again i think. I am totally not sure with this but

@perminder-17 Yes you re right I test it I am working on the solution

@perminder-17 Yes you re right I test it I am working on the solution

Feel free to reach out to me or @davepagurek if you face problems on the solution.

VANSH3104 · 2025-11-19T08:48:05Z

@perminder-17 I implemented the clip transform fix we discussed. Following our conversation about the double transform issue where scale(2) was behaving like scale(4), I used the manual coordinate transformation approach. Instead of complex matrix math, I transform all coordinates to absolute screen space upfront, then reset the transform in endClip() before applying the clip. This ensures transforms are applied only once during path creation, not again during clipping.

VANSH3104 · 2025-11-19T08:48:31Z

@perminder-17 am I on the right track ?

VANSH3104 · 2025-11-21T12:25:23Z

@perminder-17 is this need any changes?

VANSH3104 · 2025-11-25T09:01:04Z

@perminder-17 should i apply this on other shapes also ?

perminder-17 · 2025-11-26T10:53:25Z

src/core/p5.Renderer2D.js

+    if (this._clipping) {
+      const tempPath = new Path2D();
+      const current = this.drawingContext.getTransform();
+      // Transform coordinates manually but preserve scale signs
+      const transformPoint = (x, y) => {
+        return {
+          x: current.a * x + current.c * y + current.e,
+          y: current.b * x + current.d * y + current.f
+        };
+      };
+      const transformedCenter = transformPoint(centerX, centerY);
+      // Calculate transformed radii WITHOUT Math.abs() to preserve negative scaling
+      const scaleX = Math.sqrt(current.a * current.a + current.c * current.c);
+      const scaleY = Math.sqrt(current.b * current.b + current.d * current.d);
+
+      // Preserve the sign of the scaling for mirroring effects
+      const signX = current.a < 0 ? -1 : 1;
+      const signY = current.d < 0 ? -1 : 1;
+
+      const transformedRadiusX = scaleX * radiusX * signX;
+      const transformedRadiusY = scaleY * radiusY * signY;
+      tempPath.ellipse(
+        transformedCenter.x,
+        transformedCenter.y,
+        Math.abs(transformedRadiusX),
+        Math.abs(transformedRadiusY),
+        0, 0, 2 * Math.PI
+      );
+      this.clipPath.addPath(tempPath);
+    } else {
+      ctx.beginPath();
+      ctx.ellipse(centerX, centerY, radiusX, radiusY, 0, 0, 2 * Math.PI);
+      ctx.closePath();
+      if (doFill) {
+        ctx.fill();
+      }
+      if (doStroke) {
+        ctx.stroke();
+      }


Hi @VANSH3104 I am sorry for the delay in the response, I didn't got time to review this one. I just did some testing with this, the behaviour looks fine to me.

But, you should implement it not in a manual way but something like storing this.drawingContext.getTransform().inverse() at the start of clip() and then multiply that with this.drawingContext.getTransform(). The exact way which you do in your first commit: a5e1523

So, that commit will apply extra transforms internally, like you would be getting scale(2) ---> as scale(4).

Fix for that would be:

in the beginClip() we should fetching the transformations.

beginClip(){ //.. this._clipBaseTransform = this.drawingContext.getTransform(); //....

And in the endClip() we must set that.

const saved = this.drawingContext.getTransform(); // here we will save the current transform to a variable. this.drawingContext.setTransform(this._clipBaseTransform); // While applying the clip, the canvas was given the same transform as _clipBaseTransform. this.drawingContext.clip(this.clipPath); // Now the clipPath will apply on the correct space. this.drawingContext.setTransform(saved); // In this puts your original/current transform back. Restoring that.

This could be the flow, no extra transform/rotations will be applied. Can you please check? Don't go with the manula logic, use the multiply logic which you used in your first commit. Thanks for the hlep.

@perminder-17 thanks for the review i have implement yor suggestion of transform storing. I also test this and it work on all cases that we discussed previously

…handling

VANSH3104 · 2025-11-27T08:08:22Z

@perminder-17 should i apply it to all shapes it look good from my side looking for your suggestion.

Fix transforms in clip() by using shape system for primitives using m…

a5e1523

…anual method

Merge branch 'dev-2.0' into transform/Clip

c4b5ca5

perminder-17 requested changes Nov 11, 2025

View reviewed changes

Handle negative scales in clipping by using absolute values for scale…

cc98f19

… components

Merge branch 'dev-2.0' into transform/Clip

34c4706

perminder-17 reviewed Nov 13, 2025

View reviewed changes

Prevent double transform application in clip() with absolute coordinates

e5f5a41

perminder-17 self-requested a review November 25, 2025 09:34

Merge branch 'dev-2.0' into transform/Clip

22f8093

perminder-17 reviewed Nov 26, 2025

View reviewed changes

Reset transforms during clip application to ensure proper coordinate …

fc826d4

…handling

Uh oh!

Fix transforms in clip() by using shape system for primitives using manual method #8236

Are you sure you want to change the base?

Fix transforms in clip() by using shape system for primitives using manual method #8236

Uh oh!

Conversation

VANSH3104 commented Nov 6, 2025

PR Checklist

Uh oh!

VANSH3104 commented Nov 6, 2025

Uh oh!

perminder-17 left a comment

Choose a reason for hiding this comment

Uh oh!

perminder-17 left a comment

Choose a reason for hiding this comment

Uh oh!

VANSH3104 commented Nov 11, 2025

Uh oh!

VANSH3104 commented Nov 11, 2025

Uh oh!

VANSH3104 commented Nov 11, 2025

Uh oh!

perminder-17 commented Nov 12, 2025

Uh oh!

perminder-17 commented Nov 12, 2025

Uh oh!

VANSH3104 commented Nov 12, 2025

Uh oh!

VANSH3104 commented Nov 12, 2025

Uh oh!

perminder-17 Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

When I apply scale(3) , the expected result is:

The current behavior in the changes of this PR:

Uh oh!

VANSH3104 Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

perminder-17 Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VANSH3104 commented Nov 19, 2025

Uh oh!

VANSH3104 commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VANSH3104 commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VANSH3104 commented Nov 25, 2025

Uh oh!

perminder-17 Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VANSH3104 Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

VANSH3104 commented Nov 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

perminder-17 Nov 13, 2025 •

edited

Loading

perminder-17 Nov 17, 2025 •

edited

Loading

VANSH3104 commented Nov 19, 2025 •

edited

Loading

VANSH3104 commented Nov 21, 2025 •

edited

Loading

perminder-17 Nov 26, 2025 •

edited

Loading