search

catdad / canvas-confetti

🎉 performant confetti animation in the browser
https://catdad.github.io/canvas-confetti/
ISC License
10.18k stars 358 forks source link

cannot initialize multiple canvases when using workers #107

Open catdad opened 4 years ago

catdad commented 4 years ago

Currently, workers only accept a single canvas, so multiple instances cannot be used along with useWorker.

Steps:

  1. update the "custom canvas" demo to use workers
  2. start the "school pride" demo
  3. start the "custom canvas" demo

Expected: both demos work in their own canvases Actual: once the second demo is started, the first demo starts populating confetti in the custom canvas

image

Tidwell commented 3 years ago

Just curious if there is a branch with this, I'm going to need to fix this bug for a project I'm currently working on, and wondering if theres a starting point I could use and submit back a PR for.

catdad commented 3 years ago

There's no branch for this yet. I would probably try to just create a new worker for each instance that is created, and see how that works out. Let me know if you run into any trouble

Tidwell commented 3 years ago

Yea, that was my first instinct instead of trying to get the worker to have multiple canvas instances inside it. I wasn't sure if it's better to attach the worker to the canvas element or to use a Map to store them.

https://github.com/Tidwell/canvas-confetti/pull/2 https://github.com/Tidwell/canvas-confetti/pull/3

They both work fine, and because the workers require transferControlToOffscreen, it has the same support matrix as Map, so I don't think that really matters. (see: https://caniuse.com/?search=transferControlToOffscreen & https://caniuse.com/?search=javascript%20Map )

Theres something to be said for not wanting to store the worker on the canvas element (which is already stated to be out of the users control once workers are enabled), though it would expose it on the canvas so the user could terminate it if they remove the canvas element.

By storing in the Map, its not polluting the DOM element, but I'm not sure if there are any hooks exposed that would allow for the user to destroy the worker.

Either one seems sufficient for my purposes, so if you have a preference, I can add tests to whichever version and get a PR put up.

catdad commented 3 years ago

Hmm... I have been meaning to move over a few things into a WeakMap, but that's too much scope creep for now and can happen later.

I guess currently, I have a slight preference for storing it on the canvas object, due to memory concerns. And speaking of memory concerns (I know this is not a review yet), we'll now want to make sure to call worker.terminate() whenever a confetti instance is destroyed, so we don't leak a bunch of workers (I know some users have React cases where they'll create a new instance every time their component mounts).

yornaath commented 5 months ago

Yeah this is an issue when working with react atm. Any fix for it?