drawing image fails if ctx.rotate() precision is too deep

[Flicksie]

Issue or Feature

The following function works perfectly in node-canvas 2.4 and below. in 2.5 it simply ignores the image being drawn inside the hexagon shape.

Steps to Reproduce

try the following in both 2.4 and 2.5

const Canvas = require('canvas');
async function Hex(size, picture) {
  let globalOffset = 0
  size = size / 2
  let x = size + 10
  let y = -size

  let hex = new Canvas.createCanvas(size * 2 + 20, size * 2 + 20)
  let c = hex.getContext("2d")
  c.rotate(1.5708)
  c.save();
  c.beginPath();
  c.moveTo(x + size * Math.cos(0), y + size * Math.sin(0));

  for (side = 0; side < 7; side++) {
    c.lineTo(x + size * Math.cos(side * 2 * Math.PI / 6), y + size * Math.sin(side * 2 * Math.PI / 6));
  }

  c.fillStyle = "#ffffff"
  c.fill();
  if (picture) {
    c.clip();
    let pict = await Canvas.loadImage(picture);
    c.rotate(-1.5708)
    c.drawImage(pict, 0, x - size, size * 2, size * 2);
    c.restore()

    c.globalCompositeOperation = 'xor';
    c.shadowOffsetX = 0;
    c.shadowOffsetY = 0;
    c.shadowBlur = 10;
    c.shadowColor = 'rgba(30,30,30,1)';

    c.beginPath();
    for (side = 0; side < 7; side++) {
      c.lineTo(x + size * Math.cos(side * 2 * Math.PI / 6), y + size * Math.sin(side * 2 * Math.PI / 6));
    }
    c.stroke();

    c.globalCompositeOperation = 'destination-atop';

  } else {
    c.shadowColor = "rgba(34, 31, 59, 0.57)"
    c.shadowBlur = 8
  }

  return hex

}

Your Environment

• Version of node-canvas (output of npm list canvas or yarn list canvas):
  2.5.0 (faulty)
  2.4.0 (working)

• Environment (e.g. node 4.2.0 on Mac OS X 10.8):
  node 11.14
  ubuntu 16.04

[LinusU]

ping @asturur, do you think this can be related to #1255? 🤔

[asturur]

argh. tomorrow morning i ll leave some time to verify

[asturur]

@Flicksie i want to help seeing in this. Can you provide an usage example too rahter than just the function? a full index.js i can run under node

[zbjornson]

@Flicksie running your code with:

Hex(250, "./test/fixtures/clock.png").then(canvas => {
	fs.writeFileSync("./1417.png", canvas.toBuffer());
})

I think I get the same result between master/2.5.0 (left) and 2.4.0 (right):

Can you provide a repro case and screen shot with the two versions please?

[asturur]

since most of the changes to drawImage are applicable only with the 9 arguments call, i think we should make this example slightly different in order to see what happens with that case.
Also we should compare with before any of my drawImage pr was ever added, since before we were using clip to implement it, i wonder how 2 stacked clip command were behaving.

[asturur]

this is the code put in the test suite, with 9 arguments call, with the 5 argument mode it was fine too.

[asturur]

this is the same example run with 5 or 9 arguments with the code of september 3, before my prs were introduced. Same results, is working.

[asturur]

this also show the shadow, that triggers additional codepaths inside the drawImage function
I think we could have this test in the suite, since we do not have anything similar

[asturur]

#1425

This those not exclude there may be some bug, but we need a reproducible case

[Flicksie]

Sorry for going radio silent guys. it was a busy month. so, this code is the minima l i could get to reproduce this. i did though a standalone app.js that when ran will output the blank hex image instead of the clipped image. just to be sure that nothing in my deps is causing it i tested it on a clean install of canvas with just it and nothing else.

the app.js is this one

(async()=>{
    
    const Canvas = require('canvas');
    async function Hex(size, picture) {
    
      size = size / 2
      let x = size + 10
      let y = -size
    
      let hex = new Canvas.createCanvas(size * 2 + 20, size * 2 + 20)
      let c = hex.getContext("2d")
      c.rotate(1.5708)
      c.save();
      c.beginPath();
      c.moveTo(x + size * Math.cos(0), y + size * Math.sin(0));
    
      for (side = 0; side < 7; side++) {
        c.lineTo(x + size * Math.cos(side * 2 * Math.PI / 6), y + size * Math.sin(side * 2 * Math.PI / 6));
      }
    
      c.fillStyle = "#ffffff"
      c.fill();
      if (picture) {
        c.clip();
        let pict = await Canvas.loadImage(picture);
        c.rotate(-1.5708)
        c.drawImage(pict, 0, x - size, size * 2, size * 2);
        c.restore()
    
        c.globalCompositeOperation = 'xor';
        c.shadowOffsetX = 0;
        c.shadowOffsetY = 0;
        c.shadowBlur = 10;
        c.shadowColor = 'rgba(30,30,30,1)';
    
        c.beginPath();
        for (side = 0; side < 7; side++) {
          c.lineTo(x + size * Math.cos(side * 2 * Math.PI / 6), y + size * Math.sin(side * 2 * Math.PI / 6));
        }
        c.stroke();
    
        c.globalCompositeOperation = 'destination-atop';
    
      } else {
        c.shadowColor = "rgba(34, 31, 59, 0.57)"
        c.shadowBlur = 8
      }
    
      return hex
    
    }
    
    let img = await Hex(256,"https://interactive-examples.mdn.mozilla.net/media/examples/grapefruit-slice-332-332.jpg");
    fs = require('fs')
    fs.writeFileSync("./orange.png", await img.toBuffer());
    
    
})()

put it alone in a folder with only canvas@latest installed and node app.js it will output it as a white hexagon

[Flicksie]

ok guys, i spent some time commenting and uncommenting lines back and forth and it seems that the entire problem (in my case at least) is because of <context>.rotate(1.5708)
by just removing the 4th decimal digit (8) it worked as intended.
google is to blame for my choice of number 😂

[Flicksie]

ping @asturur, do you think this can be related to #1255? 🤔

i've built canvas locally one commit before and at this very merge and the issue starts right there

[Flicksie]

turns out the depth is not exactly the issue, i had another function that rotated an image by 0.15rad and failed, then i set it to0.155 rad and it worked. 0.1555rad breaks it again.

[Flicksie]

i didnt see this since npm install canvas@latest points to 2.5

but this problem is gone in 2.6

[zbjornson]

I still can't repro the failure, and nothing between 2.5 and 2.6 should have had an effect on that, so along with #1431 I'm sort of nervous about this being closed. 😕

[asturur]

missing a screenshot in first place, would be nice to have one. No failire for me either

[Flicksie]

i've set up a test express app on my server. can I email you with the IP + port? i also did it on a separate user altogether so I grant you access if you need to look closer

[asturur]

for.me a screenshot, the exact code and eventually library used to build are enough. Cannot speak for others

[Flicksie]

exact code is above, nothing more than that and [email protected] were used, it is a clean install.
screenshot:

[zbjornson]

@Flicksie if that means I could ssh into it, sure. My email is in my GitHub bio. Thanks!

[Flicksie]

alright, I mailed you the address and credentials.

[zbjornson]

Thanks @Flicksie! This makes sense now -- it was fixed by 81cf69b, which makes sense.

I think we're up to three bugs from abs vs std::abs. 🙄

[asturur]

what is the result difference between the 2? what abs does vs std::abs?

[zbjornson]

It's an implementation-specific mess that can also be influenced by other (2nd+-order) includes, see https://stackoverflow.com/questions/21392627/abs-vs-stdabs-what-does-the-reference-say. On @Flicksie's system it was converting to an int.

[asturur]

ok so is system specific too.

Sorry for the bug anyway, really unpredictable from my side

[zbjornson]

No worries, didn't mean to sound upset with you. Was just half-annoyed about the namespace mess again :-)