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.

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

Fix #7049: P5.Graphics.remove() doesn't release all related resources #7060

Merged
merged 3 commits into from
May 22, 2024

Conversation

iambiancafonseca
Copy link
Contributor

Resolves #7049

Changes:

  • Modified the remove() method in src/core/p5.Graphics.js to set this._renderer, this.canvas, and this.elt to undefined.
  • Updated documentation in src/core/p5.Graphics.js accordingly.
  • Added a boolean variable doubleClickedBool to track whether the graphics object has been removed due to a double-click event.
  • Added unit test in test/unit/core/p5.Graphics.js to cover scenario described in the issue.

Now when we call remove() all associated resources should be released.

Screenshots of the change:
(no screenshots)

PR Checklist

  • npm run lint passes
  • [Inline documentation] is included / updated
  • [Unit tests] are included / updated

… resources

The proposed fix involves modifying the p5.Graphics.remove() method by
setting this._renderer, this.canvas, and this.elt to undefined.
Additionally, a boolean flag doubleClickedBool is introduced to prevent
the execution of the draw() function after a double-click event
ensuring that the graphics object is not displayed again after removal
This provides a smoother user experience by preventing the graphics
object from reappearing unexpectedly.
Copy link

welcome bot commented May 19, 2024

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page!

@@ -359,6 +360,7 @@ p5.Graphics = class extends p5.Element {
* // the user double-clicks.
* function doubleClicked() {
* pg.remove();
* doubleClickedBool = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than introducing a new variable here, could we instead do something like this?

Suggested change
* doubleClickedBool = true;
* pg = undefined;

That way, we can continue to use the previous if (pg) condition. Your changes make it so setting this to undefined isn't necessary for the bulk of the content to get GCed, but it's probably a good habit to get users into doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! I'll implement that change right away.

@davepagurek
Copy link
Contributor

Thanks for making this change and adding tests! Just one minor note about the example.

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@davepagurek davepagurek merged commit 61f050b into processing:main May 22, 2024
2 checks passed
@davepagurek
Copy link
Contributor

@all-contributors please add @iambiancafonseca for docs

Copy link
Contributor

@davepagurek

I've put up a pull request to add @iambiancafonseca! 🎉

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.

p5.Graphics.remove() doesn't remove some resources
2 participants