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

Draw inner border for arc elements #5841

Merged
merged 2 commits into from
Dec 18, 2018
Merged

Conversation

nagix
Copy link
Contributor

@nagix nagix commented Nov 15, 2018

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

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/

screen shot 2018-12-01 at 5 01 35 pm

screen shot 2018-12-01 at 5 02 52 pm

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

screen shot 2018-12-01 at 4 59 46 pm

screen shot 2018-12-01 at 5 00 28 pm

Fixes #2381
Fixes #5645

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

image

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

image

The miter limit seems problematic with tiny slices:

5841

image

src/elements/element.arc.js Outdated Show resolved Hide resolved
src/controllers/controller.doughnut.js Outdated Show resolved Hide resolved
@nagix
Copy link
Contributor Author

nagix commented Nov 17, 2018

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.
slide4

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
Copy link
Member

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

image

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
Copy link
Contributor

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

mw-gm758_market_20180718204202_ns

@simonbrunel
Copy link
Member

@benmccann tiny slices happen on animation

@nagix nagix force-pushed the issue-5841 branch 2 times, most recently from 33c9822 to 946ffa0 Compare December 1, 2018 12:20
@nagix
Copy link
Contributor Author

nagix commented Dec 1, 2018

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
Copy link
Contributor Author

nagix commented Dec 1, 2018

Updated the jsfiddle and images above

@nagix nagix force-pushed the issue-5841 branch 2 times, most recently from 56e5c62 to 125626f Compare December 1, 2018 15:27
max = hoverWidth > max ? hoverWidth : max;
for (i = 0, ilen = arcs.length; i < ilen; ++i) {
custom = arcs[i].custom || {};
borderAlign = custom.borderAlign ? custom.borderAlign : valueAtIndexOrDefault(dataset.borderAlign, i, elementOpts.borderAlign);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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.

image

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@nagix
Copy link
Contributor Author

nagix commented Dec 12, 2018

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.

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@simonbrunel simonbrunel added this to the Version 2.8 milestone Dec 18, 2018
@simonbrunel simonbrunel merged commit db8f6c3 into chartjs:master Dec 18, 2018
@nagix nagix deleted the issue-5841 branch January 5, 2019 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FEATURE: Piechart: draw border lines around each part Incorrect border highlight in Pie Charts
4 participants