Conversation
26467af to
dd3a25e
Compare
estobbart
left a comment
There was a problem hiding this comment.
Couple of questions..
L2 had a force draw capability so that every raf would draw, is that possible here?
| const gl = | ||
| // TODO: Remove this assertion once this issue is fixed in TypeScript | ||
| // https://github.com/microsoft/TypeScript/issues/53614 | ||
| (canvas.getContext(forceWebGL2 ? 'webgl2' : 'webgl', config) || |
There was a problem hiding this comment.
this bit was confusing.. when should it ask canvas for a context vs the platform layer?
There was a problem hiding this comment.
this is just to force the context to webgl2 if that's supported based on config - that is added for when devices support both but we want to force it
There was a problem hiding this comment.
why not get the canvas from the platform?
| sh ?? 0, | ||
| options, | ||
| override fetch(url: string): Promise<Blob> { | ||
| return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
not sure I understand the intent w/ wrapping fetch here
There was a problem hiding this comment.
added that because not all devices support fetch API and require some specifics around using XMLHttpRequest - some older Smart TVs give us trouble with local URLs or other specifics, to allow Platform layers to modify the requests if needed,
| return new Promise((resolve, reject) => { | ||
| const xhr = new XMLHttpRequest(); | ||
| xhr.responseType = ''; | ||
| xhr.onreadystatechange = function () { |
There was a problem hiding this comment.
IMO loadend would be better
There was a problem hiding this comment.
can look into that - but as said some devices are really picky about this, especially older Samsung devices
| canvas.width = deviceLogicalWidth * devicePhysicalPixelRatio; | ||
| canvas.height = deviceLogicalHeight * devicePhysicalPixelRatio; | ||
| // set main canvas reference and size | ||
| platform.canvas = this.canvas; |
There was a problem hiding this comment.
maybe canvas should be one of the platform settings (along with force & num workers).. then the platform knows if it needs to create a canvas or not. Then platform createCanvas could be getCanvas.. making it more clear if it should be creating, and/or what the ownership is supposed to be of the canvas.
There was a problem hiding this comment.
yeah struggling with that a little bit, there are multiple Canvas being used in certain cases:
- Main drawing canvas
- Font generation canvas
- SVG canvas (if used)
So I initially just had platform set itself, but then got stuck with a 1x1px Canvas used for fonts with clipping calculations.
Its not just the platform, its also the font engine that determines whether it is 1 or more Canvas'es used. This was easier, but open to improve it!
There was a problem hiding this comment.
having the property and the method is the confusing piece to me.. overall be good to understand the canvas usage/lifecycle that you described above.
my recommendation would be:
const platform = new (settings.platform as any)({
numImageWorkers: settings.numImageWorkers,
forceWebGL2: settings.forceWebGL2,
canvas: settings.canvas
});
// platform layer responsible for providing the canvas from settings or making one if not provided.
this.canvas = platform.getCanvas();
Move Image creation and GL wrapper to Platform.
This PR changes:
createImageBitmapsupport toPlatformPlatform(by default usingXMLHttpRequest)ImageWorkerhandling toPlatformGLWrappertoPlatformPlatformRemoved:
createImageBitmapauto detection - this needs to be done by PlatformNOTE: BREAKING CHANGES
This requires a different Platform import / init to make it work.
The concept is you create a Platform that matches the functionality the platform exposes. I.e. if the Platform does not support
createImageBitmapor a less version ofcreateImageBitmapyou should extend the platform to match that.@estobbart for ION you can create or upstream an ION specific platform (or we can create one) that:
loadImageandcreateImagein Platform (possiblyFetchtoo?)Todo:
createImageBitmapplatformcreateImageBitmapplatformfetchsupported platform