cytoscape / cytoscape.js

Graph theory (network) library for visualisation and analysis
https://js.cytoscape.org
MIT License
10.12k stars 1.64k forks source link

Node label flickering when animating font-size #3270

Open likeamike opened 2 months ago

likeamike commented 2 months ago

Environment info

Current (buggy) behaviour

What does the bug do?

When I try to add a hover effect to a node to enlarge the font size, the node's label blinks. Specifically, when I hover over one node, the label of another node or both nodes start blinking.

https://github.com/user-attachments/assets/a9eb0de2-eff2-4a3b-a634-2a0bcf9a4bb4

Desired behaviour

What do you expect Cytoscape.js to do instead?

Node labels should remain stable and not blink when hovering over another node.

Minimum steps to reproduce

What do you need to do to reproduce the issue?

  1. Hover over the nodes.
  2. If everything appears fine initially, refresh the page and hover over the nodes again.

The issue is reproduced here: https://jsbin.com/narayik/edit?html,css,js,output

Fork/clone this JSBin demo and reproduce your issue so that your issue can be addressed quickly and effectively: http://jsbin.com/fiqugiq

For reviewers

Reviewers should ensure that the following tasks are carried out for incorporated issues:

mikekucera commented 1 month ago

By "blinking" you are referring to the non-smooth resizing animation that sometimes happens?

mikekucera commented 1 month ago

Removed the workaround in this comment because there is a better one below...

mikekucera commented 1 month ago

I’ve managed to diagnose the problem.

The bug can be triggered just by animating the font size, and you only need one node. It has nothing to do with mouse inputs or hovering.

I created a looping animation with a single node like this…

var node = cy.getElementById('a');
var expand = true;
var duration = 1000;
var timeout = duration + 100;

function testAnimate() {
  if(expand) {
    node.animate({
      style: {
          'font-size': '12px',
      }, 
      duration: duration,
      queue: false,
    });
    expand = false;
  } else {
    node.animate({
      style: {
          'font-size': '10px',
      }, 
      duration: duration,
      queue: false,
    });
    expand = true;
  }
  setTimeout(testAnimate, timeout);
}
setTimeout(testAnimate, timeout);

Eventually the weird flickering behaviour will start to show up.

The problem is that animating the font size like this causes the renderer’s labelDimCache to fill up as it calculates and saves label dimensions for every frame of the animation. In my tests the cache can grow to about 400-500 entries for this 1 second looping animation.

The reason the flicker happens is because of a cache collision. It calculates a cache key which just happens to be shared with a different label size, and pulls the wrong dimensions from the cache. That frame of the animation gets the wrong label size and that causes the flicker.

I’m going to hand this one off to @maxkfranz for now, as he is way more familiar with the style hashing code. I’m not sure why the hashes are colliding and I would need some time to dig through the code to understand it.

In the meantime there is a workaround. Add a complete function to the animation options like this…

node.animate({
    style: {
        'font-size': '10px',
    }, 
    duration: duration,
    queue: false,
    complete: () => { cy.renderer().labelDimCache = []; }  // clear the label dimensions cache
});

That will clear out the cache after each animation. Warning, the labelDimCache field is not public API. This might harm performance on a large graph, but for a small one it should be fine. If it still doesn’t fix it then use step instead of complete.

likeamike commented 1 month ago

Thank you, @mikekucera, for your diagnostic. I just want to mention that the workaround doesn't really work for me. I've tried both "complete" and "step," and neither of them fixes the flickering. Additionally, I still have two nodes because the problem with animated font resizing might affect not only the target node that is resizing but also other nodes that should not have any changes.

https://github.com/user-attachments/assets/4e66df32-fd76-4413-ba11-86ddad197702

maxkfranz commented 2 days ago

@likeamike I put together a WIP PR with an idea for a fix based on @mikekucera's diagnosis, but I didn't have time to test it. Try the PR and let us know if it makes a difference for you

https://github.com/cytoscape/cytoscape.js/pull/3298