Draw inner border for arc elements

[nagix]

Currently, the border of arc elements is aligned to the center, but this causes a few problems:

• Only a part of border lines are visible in a pie/doughnut/polarArea chart (FEATURE: Piechart: draw border lines around each part #5645)
• When hovering, only a part of border lines are highlighted (Incorrect border highlight in Pie Charts #2381)
• If the border is thick and the first element has a very small value, it becomes invisible

With this PR, the border is drawn inside arc elements in the same way as rectangle elements. This fixes the issues above and the following issues as well.

• The border looks ugly because lineJoin is set to 'bevel'. 'miter' should be better
• Custom borderWith was not correctly reflected in the outerRadius

Chart.js 2.7.3: https://jsfiddle.net/nagix/tj1na4vm/

This PR: https://jsfiddle.net/nagix/87wvucjy/

Fixes #2381
Fixes #5645

[simonbrunel]

It looks really good @nagix, though it breaks one (common?) use case: same border color for all slices with same width on all edges. With your changes, external edges will be half the size of the internal ones and I think it can cause trouble to some of our users, what do you think?

Also, I noticed a few glitches on your fiddle when changing the border color:

The miter limit seems problematic with tiny slices:

[nagix]

How about drawing the half-width border for radial edges keeping the original width for the outer and inner edges using a clip? The code would be much simpler and generate better results.

If we enlarge a clipping area a little bit, we can eliminate glitches. The problem of a sharp spike with tiny slices is caused by too small arc angle. I put a margin of 0.0001 to prevent this, but it seems to generate different results depending environments. However, this can be also eliminated by clipping.

[simonbrunel]

Drawing half border doesn't fix the issue in case of multiple datasets:

Putting aside glitches (manually removed on the screenshot), I agree that the result looks better when border colors / widths are different but in this specific case, which may be common, I don't find the result very aesthetic (but that's subjective).

I don't know what the best way to handle that case, neither if we need to handle it (I think we should). An idea would be to preserve the old behavior (with your new calculations) and implement alignment behind a new option? Something like borderAlign: 'center' | 'inner' | 'outer' (default 'center'). Not sure that's easy, neither the best approach, other suggestions are welcome :)

[benmccann]

It's quite rare to see super tiny slices in pie / doughnut charts. Usually these are all grouped together under a category like "Other". There's probably some size under which we would need to decide that we're not going to render an individual slice

The pie chart with the smallest slices I've ever seen is the one below. Even on it you get to a point where the individual slices are no longer being rendered and you're just seeing the white border

[simonbrunel]

@benmccann tiny slices happen on animation

[nagix]

I added a new borderAlign option to support both the proposed inner border and the existing center-aligned border. I removed complicated calculation, and use a clip and draw a double-width line to express an inner border. It's not very difficult to support an outer border, but I have no idea about use cases with it, so I just added 'inner' and 'center' (default).

To eliminate glitches between borders, the clipping area is enlarged by 0.33 pixels.

In addition, getMaxBorderWidth for pie/doughnut charts didn't pick the borderWidth from the outermost dataset or support custom/indexable hoverBorderWidth, so I fixed it as well.

[nagix]

Updated the jsfiddle and images above

[simonbrunel]

I think we should avoid to resolve element options twice (also done in updateElement). We could store resolved options in element._options (as done in the bubble controller) before computing max border width and updating element, something like:

update: function(reset) {
    // ...

    for (i = 0, ilen = arcs.length; i < ilen; ++i) {
        arcs[i]._options = me._resolveElementOptions(arcs[i], i);
    }

    // compute chart.borderWidth, innerRadius, ...

    for (i = 0, ilen = arcs.length; i < ilen; ++i) {
        me.updateElement(arc, index, reset);
    }
}

updateElement: function(arc, index, reset) {
    var options = arc._options;
    // ...
}

Note: I would not implement scriptable options in this PR.

[nagix]

_resolveElementOptions has been added. As the visible outmost dataset can be different from the current one, the cached values is used only when they are the for the current dataset in getMaxBorderWidth.

[simonbrunel]

I didn't realize that getMaxBorderWidth(), when called without parameters, was computing the width based on the outmost visible dataset, which is most of the time not the same as the current one when there is more than 1 dataset.

It's not really optimal because _resolveElementOptions get called 2 * datasets.length - 1 times per chart.update() while ideally getMaxBorderWidth() should be called only one time, so _resolveElementOptions only datasets.length times. This can be a performance issue when we will introduce scriptable options but not sure how to make that better right now since there is no way to centralize these computations for all datasets.

[simonbrunel]

I'm wondering if 'bevel' is still the best lineJoin method in case of 'center', which is now inconsistent with the inner behavior. I can't find any ticket asking for, it seems to come from the initial code. Since nobody reported this as an issue, I guess we should not touch it.

[etimberg]

Looks good! I like the idea of the option for the alignment. I agree with @simonbrunel's comment regarding caching the options

[nagix]

I didn't touch 'bevel' join style because the acute angle at the center of the circle becomes too sharp when 'miter' is used, and it's hard to maintain consistent appearance. I guess this is why the initial code was implemented like this.

In the 'inner' mode, this is not an issue because the border is clipped.

[simonbrunel]

Just a little bit worry about this comment but it should be fine in most cases. I think we can merge this implementation and later investigate solutions that prevent to compute getMaxBorderWidth for every dataset.

Thanks @nagix