Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Canvas based scatterPlot implementation #1361

Closed

Conversation

@HamsterHuey
Copy link

HamsterHuey commented Jan 28, 2018

@gordonwoodhull I've implemented a canvas based dc.js scatterPlot that is considerably more performant than the current SVG based chart when plotting more than a few thousand data points.

Some things I wanted to note:

  1. Currently, this only supports circle symbol types for the Canvas backend. Since dc.js relies on d3 version 3, there is no easy way to create shape generators that can render out d3.symbol elements to a canvas backend. This is trivial in D3v4 but I didn't implement it as I doubt you would want dependencies for both versions of D3 creeping into dc.js.

  2. Canvas backend does not support any custom transition animations when hovering over legends or applying filters as is the case with the SVG backend. Transitions are more tricky and tedious when using Canvas, and since the primary use case for the canvas backend is when performance is critical, it seems reasonable to remove these aspects to the chart in order to make it feel more responsive and performant.

  3. I've utilized a hybrid SVG + Canvas approach to rendering the canvas charts. An SVG element is used to draw all axes and legends, etc. A canvas element is perfectly aligned and overlayed over the SVG element in order to plot the scatter points. In order to achieve this alignment of the SVG and Canvas elements, the following CSS properties are applied to the following elements only when using the canvas backend.

    a. The anchor div (i.e., the parent element supplied to dc.scatterPlot) is modified to have a CSS style of position: relative

    b. The SVG element is styled with position: relative

    c. The Canvas element is styled with position: absolute; z-index: -1; pointer-events: none. If the SVG element has any top/left properties, these are applied to the Canvas element in order to try and align it perfectly with the SVG element.

    These CSS stylings may cause conflicts in more complex UIs though they are fairly non-intrusive. Not sure if there is anything much that can be done to mitigate these side-effects. Technically, you can embed a canvas element within an SVG as a foreign object, but this is not well supported across browsers and even chrome has a longstanding bug report centered around their incorrect implementation of the spec which they haven't gotten around to fixing. The CSS approach above seems like the better way to go from a compatibility standpoint.

  4. By default, all scatter plots will use the SVG backend for rendering the plots unless useCanvas(true) is passed in to the scatterPlot chart object during initialization. If using SVG backend, the scatterPlot object should behave as it currently does in dc.js with no changes.

  5. I ran all existing tests and everything but 1 passes. This solitary failure is not due to my edits. I had the failure even prior to making any commits.

  6. I've added a couple of example html files to demo this and contrast against SVG backend. I also have these hosted on my site. You can check out the comparison here:

  1. I'm not too familiar with Jasmine so I haven't gotten around to writing any tests yet. Testing with canvas is also going to be harder. I noted that a lot of the current tests compare against the DOM state of SVG elements in the chart but that is not possible with Canvas since it is just a bitmap with no state information. Any suggestions/thoughts?

  2. I tried to add documentation as best I could for the couple of extra methods introduced. Things look pretty good in the jsDoc compiled html files. But this is my first attempt at writing documentation in jsdoc format so I might have gotten some things wrong =)

Cheers

@HamsterHuey

This comment has been minimized.

Copy link
Author

HamsterHuey commented Feb 14, 2018

Any thoughts/feedback? There's a better chance that I can revise/improve this if needed while it is still fresh in my mind.

// Calculates element radius for canvas plot to be comparable to D3 area based symbol sizes
function canvasElementSize (d, isFiltered) {
if (!_existenceAccessor(d)) {
return _emptySize / Math.sqrt(Math.PI);

This comment has been minimized.

Copy link
@gordonwoodhull

gordonwoodhull Feb 14, 2018

Contributor

I was surprised by Math.sqrt(Math.PI) here. How did you ever figure this out?

I checked in the debugger and yes that's the factor by which the d3.svg.symbol-generated arc differs from the radius.

I always thought they were coming out too small!

I bet the assumption in elementSize that we should square the radius in order to get d3.svg.symbol.size is wrong. So it's easy to see being off by a factor of π but the sqrt is really confusing.

This comment has been minimized.

Copy link
@HamsterHuey

HamsterHuey Feb 15, 2018

Author

Haha, yeah, I had the same issue initially. I believe it is because the D3 docs mention that the supplied symbol size should specify the area of the symbol, not the radius.

For circles, since the area is Math.PI * (r^2) when we define the radius of the symbol in dc.js, the method elementSize() should actually return Math.pow(_emptySize, 2) * Math.PI. But since it doesn't do this, the actual radius of the D3 symbol that gets plotted in the dc.js chart is off by a factor of sqrt(Math.PI). Hence the correction by that factor.

I also believe that for all the other symbol types, D3 still relies on a circle based area calculation with the symbol extents guaranteed to lie within the circle defined by the provided area parameter. This block seems to support this guess - https://bl.ocks.org/mbostock/6d9d75ee13abbcfea6e0

So if other symbol types get implemented for canvas plots, the method for drawing the canvas symbol would need to make sure that each symbol type respects the perimeter defined by the circle.

This comment has been minimized.

Copy link
@gordonwoodhull

gordonwoodhull Feb 15, 2018

Contributor

That example shows that the radius varies a lot for the same area depending on the shape.

So dc.js is definitely doing this wrong. The API names are correct but it should be using the size directly for d3.svg.symbol and not squaring it as if it's a radius (and getting the calculation wrong).

Would you be willing to fix the original problem and document the API change, so that the new code is not compounding the problem by introducing an obscure calculation in order to be compatible with an incorrect calculation? 😄

API changes are fine in the 2.1 branch.

This comment has been minimized.

Copy link
@HamsterHuey

HamsterHuey Feb 15, 2018

Author

Doh! Didn't notice that the example had identical symbol sizes. I just assumed that the way I had it in my head was how it would be implemented since that makes the most sense as the different symbols would then look to have a similar perceptual size for the same area parameter.

I don't mind implementing a fix that switches to size being defined as the area parameter as per the d3 docs for d3.symbol. Did you envision that being part of this PR or as a separate PR?

@gordonwoodhull

This comment has been minimized.

Copy link
Contributor

gordonwoodhull commented Feb 15, 2018

Hi @HamsterHuey, sorry for my slow response and thanks for the ping!

This is great! Add

          dc.constants.EVENT_DELAY = 0;

to the beginning of the demo and it is even better!

I feel this would be better organized as a dc.scatterMixin and dc.canvasScatterPlot and dc.svgScatterPlot - because almost none of the implementation is shared, except for the brushing. But we could fix that later.

@HamsterHuey

This comment has been minimized.

Copy link
Author

HamsterHuey commented Feb 15, 2018

Thanks @gordonwoodhull . Yup, dropping EVENT_DELAY to 0 does make it nicer though the brush motion can get a little stuttery. I do hope to get a chance to take a look at improving how throttling is implemented at some point to see if that helps with the responsiveness of dc plots.

As for refactoring to a separate dc.canvasScatterPlot and dc.svgScatterPlot - That's certainly easily do-able. Though given the identical API for both plot types, I thought that having them remain within dc.scatterPlot and have the backend being used be a single parameter might be a better way to go. Kind of like how d3v4 has the same API for shapes and easily allows switching the rendering backend between Svg and Canvas.

That being said, if you'd prefer to have them as two separate entities, I can take a stab at switching this out to dc.canvasScatterPlot.

Any thoughts on how writing tests for the canvas plot would work?

@gordonwoodhull

This comment has been minimized.

Copy link
Contributor

gordonwoodhull commented Feb 15, 2018

Yeah definitely a better approach for throttling (instead of debounce #630) or brushend (e.g. #627 (comment)) would be very welcome.

Let's deal with the radius/size problem with a deprecation. My suggestion: add a flag sizeIsRadius, default it to undefined for now. On first render, check if it's still undefined and if so, warn that sizeIsRadius will default to false in the future.

I definitely want the bad calculation to go away, and I don't want to merge this without the calculation that makes sense (calculating the radius based on size like d3 does). But we can be kind to users who are accustomed to their radius being divided by √π, and provide the old behavior with a warning and clear way forward.

It actually would not be hard (and very small) to include just d3v4's symbol class. In dc.graph.js I build just d3-force into a module named d3v4 using rollup. Given that it still may take a while for dc.js to upgrade to d3v4, maybe that's worthwhile. But let's not deal with that now.

I also don't think we need to split the class now, but it's worth thinking about for later. It would be similar to the way coordinateGridMixin is a base class for lineChart and barChart - a good part of the interface and any shared logic resides there, but anything that is specific is implemented in an override. (The use of the term "mixin" is incorrect for most of dc.js because you can't mix and match. They were originally called base classes and that's what they really are.)

As for tests, it's not a lot of fun, but in the spirit of the other tests, you could write a test that tests half a dozen pixels that you know to be certain colors or white, using context.getImageData().

@HamsterHuey

This comment has been minimized.

Copy link
Author

HamsterHuey commented Feb 16, 2018

Thanks @gordonwoodhull . I'll try to spend some time on this in the next couple of weeks. Btw, I was just going through the docs and noticed viewBox based resizing that I guess got introduced a little while back. Seeing as that is part of baseMixin, I'm a bit concerned that the scatter chart will likely break if someone tries to use viewBox with the canvas scatter chart. Any thoughts on the best way to handle that?

@gordonwoodhull

This comment has been minimized.

Copy link
Contributor

gordonwoodhull commented Feb 16, 2018

I think it is fine to document (or even warn in the browser console) that the canvas mode is not compatible with viewBox resizing.

I know a lot of people like that feature because it's an easy way to get the chart to resize, but it distorts text and circles and I don't think it's the right way forward. It's much better to keep width and height updated with the actual pixel size. This is currently not as automatic as it should be - once resize listener is available on on all browsers this will get a lot better.

@HamsterHuey

This comment has been minimized.

Copy link
Author

HamsterHuey commented Feb 16, 2018

Sounds good. Yup, I use dc.js in a react application and have access to resize callbacks when resizing divs which makes it a lot easier to implement responsive resizing without the viewBox resizing tradeoffs.

@kum-deepak

This comment has been minimized.

Copy link
Collaborator

kum-deepak commented Apr 7, 2018

Hi @HamsterHuey, I have been assisting @gordonwoodhull in porting dc to D3v4, the work is moving along quite well. I realize that this feature will work much better with D3v4 which has better built in support for Canvas and also enhancements to d3.shapes. Will you be able to rebase your work onto the 3.0 branch?

That branch has updated dc code, specs and examples.

Many thanks for your great work 👍

@HamsterHuey

This comment has been minimized.

Copy link
Author

HamsterHuey commented Apr 9, 2018

@gordonwoodhull

This comment has been minimized.

Copy link
Contributor

gordonwoodhull commented Apr 9, 2018

I think I was too verbose above. Really my only issue is the faulty area calculation, and we could deal with that before or after moving to d3v4.

I don't want to divide effort between d3v3 and d3v4 implementations, so I'd like to merge this for dc.js 3.0 on d3v4 and solve the shapes problem that way. I doubt anyone is stuck with d3v3.

@HamsterHuey

This comment has been minimized.

Copy link
Author

HamsterHuey commented Apr 9, 2018

@elChapoSing

This comment has been minimized.

Copy link

elChapoSing commented May 14, 2018

Used it when digging for a faster implementation of the scatterplot and it works as advertised.

@icoert icoert referenced this pull request Sep 11, 2019
gordonwoodhull added a commit that referenced this pull request Sep 11, 2019
Bugfix in canvasElementSize calculations and add documentation

Update examples

squashed from 3 commits in PR #1361

lint
@gordonwoodhull

This comment has been minimized.

Copy link
Contributor

gordonwoodhull commented Sep 11, 2019

Since I got multiple requests for this e.g. #1571, I decided to go ahead and port and release it even without tests (#1572).

Here are the new examples:

scatter canvas brushing

scatter canvas large

scatter svg large (for comparison)

Thanks @HamsterHuey!

@HamsterHuey

This comment has been minimized.

Copy link
Author

HamsterHuey commented Sep 18, 2019

@gordonwoodhull, You may want to close this issue since this MR takes care of it: #1571

@gordonwoodhull

This comment has been minimized.

Copy link
Contributor

gordonwoodhull commented Sep 18, 2019

Thanks @HamsterHuey. I think I closed the relevant issues, but thanks for the heads up.

gordonwoodhull added a commit that referenced this pull request Sep 19, 2019
Bugfix in canvasElementSize calculations and add documentation

Update examples

squashed from 3 commits in PR #1361

lint

converted to ES6 according to the plan in #1561

convert scatter canvas examples to D3v5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.