spatialillusions / milsymbol

Military Symbols in JavaScript
www.spatialillusions.com/milsymbol
MIT License
527 stars 130 forks source link

Support for offscreen canvas #292

Open jjwtay opened 3 months ago

jjwtay commented 3 months ago

Currently line 15 and 39 of ascanvas.js calls:

document.createElement('canvas')

While this works in the main thread this makes this library unusable in a webworker utilizing an offscreen canvas. Testing this locally within one of our projects I added:

const makeCanvas = (width = 100, height = width, useOffscreenCanvas = true) => {
  if (typeof OffscreenCanvas === undefined || !useOffscreenCanvas) {
    const canvas = document.createElement("canvas");
    canvas.width = width;
    canvas.height = height;
    return canvas;
  }

  return new OffscreenCanvas(width, height);
};

This allowed us to proceed and render within a webworker. As far as I know the main ramification against this would be any calls to canvas.toDataURL() would need to be wrapped in a way to check if it's an offscreen canvas first with an alternative if it is an offscreen canvas. I don't know if this is something you would want in your library or not so wanted to bring this to your attention for discussion.

spatialillusions commented 3 months ago

Since this might be a breaking change, if we can't solve the problem you mentioned, I think it might be better to do this as a custom version of the asCanvas.

The function is quite small, and you should be able to add it in the same was as the current asCanvas function, or over write it.

import asCanvas from "./symbol/ascanvas.js"; Symbol.prototype.asCanvas = asCanvas;

Or if you think that you are able to modify the current function so that it works both with a web worker and without.

jjwtay commented 3 months ago

Locally for our project I have no problem just overriding it ourselves I just wanted to bring this to you in all fairness because we use your project heavily and love it and I wanted to kickoff a discussion if you wanted to add this capability. I also was a bit worried that for some users that use your library differently than us it would be breaking change but if you are ok with me putting up a PR to add a new file asoffscreencanvas.js that essentially is just an offscreen canvas copy of the ascanvas.js file i can do that. or potentially add a flag that gets passed down to alternatively use offscreencavas instead of htmlcanvas. Or again i can just override it locally for ourselves entirely up to you I just wanted to make sure I brought this to you in case you were interested.

jjwtay commented 3 months ago

For what it's worth I just tested internally adding a asoffscreencavas.js file then updated Symbol.js to add to the prototype and index.d.ts and that all works 100% for us if that would work for you.