samizdatco / skia-canvas

A GPU-accelerated 2D graphics environment for Node.js
MIT License
1.7k stars 66 forks source link

Nothing renders if any of the lineTo's coordinates contain NaN, inconsistent against html canvas #84

Closed MarosPistej closed 2 years ago

MarosPistej commented 2 years ago

About

If any of the lineTo(x,y) of path receive NaN as coordinate value, whole context is't rendered, on the other hand html canvas just skip incorrect coordinates and try to draw path if possible, it's possible that same issue might be in 'arcTo,bezierCurveTo,quadraticCurveTo, etc.' but i didn't tested that

Example

// canvas width="300" height="150"
ctx.beginPath();
ctx.lineTo(NaN, 100);
ctx.lineTo(50, 50);
ctx.lineTo(100, NaN);
ctx.lineTo(250, 100);
ctx.stroke();

html canvas image

skia-canvas image

samizdatco commented 2 years ago

It's not so much that it silently renders nothing, right? It should be throwing TypeErrors in those cases.

You're right though that it should probably conform to the spec and silently ignore calls that have the correct number of arguments but include NaN or Infinity.

MarosPistej commented 2 years ago

Sure we shouldn't be able to provide NaN or similar variable as coordinate, but "crashing" context isn't good, silent ignore in case function somehow receive such variable is sure good, i guess browser implementation have it for a reason

for now i fixed it by monkey-patching (didn't checking Infinity cuz in my case i cannot receive one but can receive NaN)

  const moveTo = canvasContext.moveTo
  canvasContext.moveTo = function (...args) {
    if (Number.isNaN(args[0]) || Number.isNaN(args[1])) return
    moveTo.apply(this, args)
  }

  const lineTo = canvasContext.lineTo
  canvasContext.lineTo = function (...args) {
    if (Number.isNaN(args[0]) || Number.isNaN(args[1])) return
    lineTo.apply(this, args)
  }

but check like this could be done in rust layer

samizdatco commented 2 years ago

Thanks for catching this. I've updated the drawing and transform methods to be more lenient and browser-like. Now the only thing it will throw a TypeError for is an insufficient number of arguments—if they're not the correct types it will silently ignore the call.

I suspect there are other cases where I'm currently being stricter than the standard when it comes to arg validation. Let me know if you find any...