Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix endShape() to Properly Close Paths and Prevent Shape Merging #7583

Merged
merged 3 commits into from
Mar 7, 2025
Merged

Fix endShape() to Properly Close Paths and Prevent Shape Merging #7583

merged 3 commits into from
Mar 7, 2025
+1 −0

Conversation

Forchapeatl
Copy link
Contributor

@Forchapeatl Forchapeatl commented Feb 28, 2025 •
edited
Loading

This PR resolves an issue where endShape() could unintentionally merge with subsequent shapes (e.g., ellipse()), causing unintended dragging effects.

Resolves #7532

Changes:

Ensures this.drawingContext.closePath(); happens on endClip() before any new shape starts.

Screenshots of the change:

before

before

after

Screenshot from 2025-03-07 04-51-25

PR Checklist

Sorry, something went wrong.

@davepagurek
Copy link
Contributor

Thanks for taking this on @Forchapeatl! We're also a few weeks away from releasing p5 2.0, which lives on the dev-2.0 branch, and has a beta you can try out here: https://github.com/processing/p5.js/releases/tag/v2.0.0-beta.3

I think this is still worth merging into the main branch, but we should also test if the same issue is needed in 2.0, and if we need to make a second PR into dev-2.0 with a similar fix?

@Forchapeatl
Copy link
Contributor Author

Thank you @davepagurek , it the issue replicates in the beta 2.0 . Should I create a PR for the dev-2.0 branch ?

@Forchapeatl Forchapeatl changed the base branch from main to dev-2.0 March 4, 2025 12:29
@Forchapeatl Forchapeatl changed the base branch from dev-2.0 to main March 4, 2025 12:31
@davepagurek
Copy link
Contributor

That would be great, thanks!

@Forchapeatl
Copy link
Contributor Author

Please this is the new PR for dev2.0 branch. I have to close this one

@Forchapeatl Forchapeatl closed this Mar 4, 2025
@davepagurek
Copy link
Contributor

davepagurek commented Mar 5, 2025 •
edited
Loading

@Forchapeatl we can re-open this PR and merge it into main as well, since we will likely release another p5 1.x release with bug fixes in the future. Is everything else in this one good to go?

@davepagurek davepagurek reopened this Mar 5, 2025
@Forchapeatl
Copy link
Contributor Author

Yes it works as expected now. It can be tested here https://editor.p5js.org/forchapearl1/sketches/96C3TOR8y

@davepagurek
Copy link
Contributor

Thanks @Forchapeatl!

@davepagurek davepagurek merged commit 7982e2f into processing:main Mar 7, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug when using many beginShapes() inside beginClip()
2 participants
@Forchapeatl @davepagurek