mozilla / lightbeam-we

Web Extension version of the Firefox Lightbeam add-on
https://addons.mozilla.org/en-GB/firefox/addon/lightbeam/
Mozilla Public License 2.0
179 stars 61 forks source link

WIP: Cache favicon output to prevent flickering. Fixes #147 #185

Closed jonathanKingston closed 6 years ago

jonathanKingston commented 7 years ago

For some reason the scale of this is super off, however it does prevent the flickering when dragging around.

princiya commented 7 years ago

@jonathanKingston From MDN, for putImageData it says, "This method is not affected by the canvas transformation matrix." Maybe this is the reason, for the points to be off?

I assume, even you get this sort of output?

screen shot 2017-09-04 at 13 58 34

jonathanKingston commented 7 years ago

@princiya that is the exact issue yeah

princiya commented 7 years ago

@jonathanKingston we need to multiply this.scale for putImageData this.context.putImageData(node.image, x * this.scale, y * this.scale);

This is the reason for the points to be off. So I shall take care of this PR? Need to ensure putImageData works for transforms etc.

jonathanKingston commented 7 years ago

Er if you get to it first yeah go for it, if not I will give that a try. I thought I accounted for scale when testing without any luck.

jonathanKingston commented 7 years ago

I tried this and it works certainly much closer anyway, however it's under the icons etc.

princiya commented 7 years ago

@jonathanKingston I have a blocker with this issue #200 . The issue isn't related to this PR, but I can't get things running. Can you please look into this?

princiya commented 6 years ago

Here is the main cause for favicon trailing:

this.drawFirstParty(node);

drawFirstParty(node) {
    this.context.arc(node.x, node.y, this.circleRadius, 0, 2 * Math.PI);
 }
this.drawFavicon(node, x, y);

In the drawNodes() function, we have the following values set for x & y:

const x = node.fx || node.x;
const y = node.fy || node.y;

The fx and fy values refer to the fixed - x,y values and are used to drag the graph and remember the last dragged position.

As seen from the above two code snippets for drawFirstParty and drawFavicon, we aren't passing this fx or fy value into drawFirstParty whereas drawFavicon has the updated values for fx & fy. This way, the first party circles get drawn on one position and the favicons get drawn on the updated positions during drag event and hence the reason for flicker.

princiya commented 6 years ago

This caching of favicons is indeed a good solution, we don't have to drawImage for each repaint and wait for image onloads etc. But right now I have no clue why they appear underneath the first party white circles after the drag event? The rendering order works fine only during first page load.

I tried the following code snippet:

drawFavicons() {
    for (const node of this.nodes) {
      if (node.favicon) {
        this.drawFavicon(node, node.fx || node.x, node.fy || node.y);
      }
    }
  }

and this works fine for each drag event. I don't want to do this, this is an additional overhead to iterate the whole nodes array. But just trying different methods for debugging.

Update

@jonathanKingston I have the fix! :) Creating a new PR. #201

drawNodes() {
    // console.log('calling drawNodes', this.nodes);
    for (const node of this.nodes) {
      const x = node.fx || node.x;
      const y = node.fy || node.y;
      this.context.beginPath();
      this.context.moveTo(x, y);

      if (node.shadow) {
        this.drawShadow(x, y);
      }

      if (node.firstParty) {
        this.drawFirstParty(x, y);
      } else {
        this.drawThirdParty(x, y);
      }

      this.context.fillStyle = 'white';
      this.context.closePath();
      this.context.fill();

      if (node.favicon) {
        this.drawFavicon(node, x, y);
      }
    }
  }

Earlier, we had drawFavicon before this.context.fillStyle = 'white';

jonathanKingston commented 6 years ago

@princiya mopped up my code and made it work... closing