Brush animation

[kum-deepak]

Looks like I have some understanding of transitions now.

Only changes in coordinate-grid-mixin.js so far. Test cases fail for derived classes with brush. For your initial feedback.

[kum-deepak]

Completed for now. I have currently enabled transition only for resizes. During brushing we definitely should not enable transitions.

Let me know if it makes sense to enable it for any other case as well.

[gordonwoodhull]

Thanks @kum-deepak! This works very well, and the implementation is nice and clean. Remaining issues:

• the brush handles don't transition in Y - they just go to the final position. For example, if you make the chart much taller, the handles leave the chart entirely, and then the chart catches up. This is probably why setHandlePath takes gBrush, so that it can be either a selection or transition.
• resizing a line chart causes the brush to go away (viz resizing-right-axis.html)

Please also add

• bubble chart resizing example
• scatter plot resizing example
  and
• enable the brush in all resizing examples

These might expose other issues, but I think it would be good to get brush resizing working across all charts, while it is fresh in your memory.

One older issue, having nothing to do with your changes, while you're here:

• pie resizing example is broken, because pieChart doesn't implement rescale. (I wish rescale didn't exist but that's another story.) (fixed in D3v5 compatibility #1394)

[gordonwoodhull]

The only other case I can think of where an animated brush might be nice is when setting the brush programmatically. But I don't know if we can detect that, and I agree it's definitely annoying to animate the brush when dragging it. (E.g. you've fixed #1071)

Thanks for dc.utils.arraysEqual, good idea!

[kum-deepak]

I will rebase it again before working further.

[gordonwoodhull]

Have you considered defaulting this the other way? I'm not clear which circumstances lead to infinite recursion, or which way is more backward-compatible

[kum-deepak]

If we set it to false, this will invoke the zoom handler in the focus chart, which in turn will invoke brush handler in the range chart. Whenever we are using focus/range chart pairs we need it true.

Examples where that is not the case like http://localhost:8888/web/examples/multi-focus.html, it can be false.

I chose the default to false as that will be the case if someone did not pass the parameter at all.

While at this example - what stops a series chart to be used as a range chart? The reason I am asking is that I have cleaned up some code in composite chart over iterations and it might have resolved the issue.

[kum-deepak]

I will actually love to name the 2nd parameter in _chart.focus to raiseEvents instead of noRaiseEvents. To use default value false, I had to settle for that name.

[kum-deepak]

I have rebased to 3.0 again.

Brush transition in Y is now working, line chart (composite chart) brushing and transition works.

Need to add resizing examples for Bubble and Scatter, once added I can check the functionality.

Signing off for today.

[kum-deepak]

Cleaned up brushing in composite chart. The strategy I am taking is to enable and apply brushing only in the container. While applying a filter, it is applied to all children.

Visual examples work. My next course of action:

• Fix Strange interaction between child.brushOn and Titles #1404 before I can move on this one.
• Add example for bubble chart brushing and resizing.
• Reverse the default for second argument for chart.focus.

[gordonwoodhull]

Cleaning up composite brushing is a noble effort. I can guarantee that you will fix some issues, but also break some other things.

I agree it's the right approach, but just be prepared for blowback. At the very least, everyone who has worked around this one way or another is likely to break now.

Some of the earlier issues: #878 #1216 #390 #479

[kum-deepak]

Wow! Ignorance can be bliss sometimes. Now I realize that I build upon earlier work of many in this regard. The current work is likely to fix most, if not all, of the issues that you have referred.

The approach I have taken is similar to the one earlier discussed.

I am already quite way ahead in this. I will fix the title issue and then actually try to use series chart as a range chart. I have mixed feelings that it might work now :)

Also, I have now started realizing that X axis brush and 2D brush (only for scatter plots) is currently determined by the chart type. Interesting case happens when scatter plot is used inside a composite chart - where it is supposed to work with X axis brush. It was interesting to see that brushing test cases do put a scatter chart inside composite chart.

So, it appears that brush type should not depend on chart type, but coordinate mixin should offer it as a setting. As of now I am not sure about best API approach. I will create a marker issue for it, so that I can come back to it.

[kum-deepak]

I tracked the reason for #1404, There is following code in line-chart.js:

    function drawDots (chartBody, layers) {
        if (_chart.xyTipsOn() === 'always' || (!_chart.brushOn() && _chart.xyTipsOn())) {

Since I am now setting brushOn(false) for integrated charts, the condition is now true which is making the test case to fail.

I need to know here if this chart is part of a composite chart and if the parent has brush switched on. I see following approaches:

• Add a field parentBrushOn
• If there is a way to find out this chart has a parent, rewiring some functions to call out to parent. This definitely has more work. But it may be worth to try it out (or some other generic approach) post 3.0.

Currently going ahead with parentBrushOn

[gordonwoodhull]

I like PRs to have one distinct purpose if possible.

I'm glad that you are tackling all of these other issues related to composite brushing, and I mostly agree with your approach. However, these fixes have a lot more risk than the original intent of this PR, which was to fix brush resizing.

I'm going to merge the commits for brush resizing now. Please create separate PRs for the other features.

In particular, I'd like to merge the composite brushing stuff after we go beta, so that when it breaks something (and it will, and it's nothing to fear), people can say "gee, everything was working fine with 3.0 beta 1 but 3.0 beta 2 causes the following problems..."

[kum-deepak]

Yes, I agree. Once it has been merged I will create a new PR with remaining changes.

Thanks!

[gordonwoodhull]

Great, thanks for understanding. It looks like the two purposes got entwined in c001c0e - so I'll check if the line chart brush still goes away after resizing, and open a separate issue for that if necessary.

[gordonwoodhull]

Awesome, I took just the related commits and brush resizing works for all the examples.

Merging for 3.0 alpha 7. When you rebase your branch I think you should find the commits I didn't merge - lmk if you have any questions.