dc-js / dc.js

Multi-Dimensional charting built to work natively with crossfilter rendered with d3.js
Apache License 2.0
7.43k stars 1.8k forks source link

"brushing on ordinal bar chart" example row chart animation is very slow #1860

Open gordonwoodhull opened 2 years ago

gordonwoodhull commented 2 years ago

After brushing on the ordinal bar chart in the example, the row chart takes more than two minutes to stop moving.

There is nothing obvious in the code that would cause this. The row chart initialization is as simple as can be:

        var row = new dc.RowChart('#first-letters');
        row
            .width(1000)
            .height(350)
            .gap(1)
            .dimension(letterDimension)
            .group(letterGroup);

There must have been a change to D3 that caused this.

kum-deepak commented 2 years ago

The same chart when done with the alternate approach does not show the lag. I checked

https://dc-js.github.io/dc.js/examples/brush-ordinal.html - does not show lag https://dc-js.github.io/dc.js/examples/brush-ordinal-dynamic.html - shows lag

gordonwoodhull commented 2 years ago

Wild!

Spacarar commented 2 years ago

I thought I would take a look out of curiosity and I did notice that this line seems to severely affect performance, and when taking it out, I'm not immediately seeing any errors or issues.

https://github.com/dc-js/dc.js/blob/develop/web-src/examples/brush-ordinal-dynamic.html#L120

bar.on('preRedraw', chart =>  chart.rescale());

What is this line for and can it be removed?

gordonwoodhull commented 2 years ago

Thanks @Spacarar! Hmm, this line is indeed the culprit, but it was put in to solve #1770.

The bar chart needs to be rescaled when it is redrawn in order to get the tick labels right when the row chart is filtered. But this somehow causes a large number of brush events which in turn cause more redraws when the bar chart is filtered. Unintended recursion of events, perhaps.

An extremely ugly workaround is to use

        row.on('filtered', () => bar.rescale());

instead. But having to add this to every other chart is a drag.