jonobr1 / two.js

A renderer agnostic two-dimensional drawing api for the web.
https://two.js.org
MIT License
8.27k stars 454 forks source link

[Linear Gradient Internal SVG Defs Are Not Removed] #688

Closed Jamzy01 closed 1 year ago

Jamzy01 commented 1 year ago

Describe the bug Linear Gradient definitions aren't being garbage collected, and then removed from svg defs tree after all references to the object are destroyed, and all my objects using the linear gradient as a fill are either removed, or their fill property is changed. As an extra measure, I have even tried parenting the linear gradients to a group, and even when I clear that group's children and remove all references to the object, the linear gradient stays in the defs tree. Admittedly, this may not be a bug in the Two.js repo, as I do not fully understand Javascript's garbage collection system, I simply want the reassurance that this will not cause memory leaks, and any problems for me in the future. To my knowledge, this is only a problem in svg renderer in Two.js.

To Reproduce

  1. Create a linear gradient (using the new Two.LinearGradient(...) constructor instead of two.makeLinearGradient())
  2. Remove all references to the linear gradient object

Expected behavior I expect the linear gradient to be removed from the svg defs tree as soon as I remove all references to the gradient (garbage collecting the gradient)

Screenshots

image

Environment (please select one):

Desktop (please complete the following information):

Additional context I am using Tauri, React 18, Two.js version 0.8.10.

jonobr1 commented 1 year ago

Thanks for posting. This sounds like an issue with Two.js. I'll take a deeper look.

jonobr1 commented 1 year ago

This commit should fix your issue: https://github.com/jonobr1/two.js/commit/413d7d3316eff620f4828b1a8f59b8bf133e0434

If you try using the github latest instead of npm, it should resolve your issue.

Jamzy01 commented 1 year ago

Hi there, I'm glad you fixed this bug, because it may have ended up slowing down my app considering how many gradients I might end up using. I was going through the source code myself, and I actually believe that all of the two js effects that add things to the defs tree don't remove what they added when they are no longer needed. I am very happy you fixed this, but it might be worth looking into the other two js effects and seeing if they're working properly as well.

jonobr1 commented 1 year ago

Which other effects are you using? Images? If you have an example I can test against that would be great.

Jamzy01 commented 1 year ago

It was also an issue for the radial gradient, and when I did a search for "defs.removeChild" before, I found nothing. So I assume before this fix, all object types that added to the defs tree did not remove anything when they weren't needed anymore. I was just checking other types like radial gradient, textures and clipping masks which also take advantage of defs had the same fixes applied to them.

jonobr1 commented 1 year ago

Oh yes, this did not exist for any effects before. But, should work for all effects now.