Rewrite filler

[kurkle]

After implementing fastPath, I realized most of the drawing logic is duplicated in filler.
There I thought, exposing the path drawing part from Line would allow using it in filler and make it much simpler. It turned out not to be as simple as I thought.

Current implementation in master, fills based point on index in the data array. With most of the index bound stuff removed elsewhere, I think its good to do it here too. So I needed to add interpolate to Line to allow fill targeting between points. I imagine that functionality will be welcomed by the community (crosshairs, tooltips, getting the value under mouse etc)

Some fixtures needed to be updates, because master's fill and border don't agree on all aspects. For example, a radar with tension and missing points, border is drawn as straight line for the missing part, while fill follows the spline. Some filling bugs are also resolved.

So examples, config:

            datasets: [{
                backgroundColor: rgba(0, 0, 192, 0.25),
                borderColor: rgba(0, 0, 192, 0.75),
                data: [{x: 1, y: 5},{x:5, y: 2},{x:8, y:-3}],
                fill: false
            }, {
                backgroundColor: rgba(128, 0, 128, 0.25),
                borderColor: rgba(128, 0, 128, 0.75),
                data: [{x: NaN, y: NaN}, {x: 2, y: 3}, {x:6.5, y: 7}],
                fill: '-1'
            }]

Master:

PR:

[benmccann]

Ooh. This is a big one. And I've never used the filler or interpolation, so not sure how much I'll really be able to help reviewing, but I'll see if I have some time later

most of the drawing logic is duplicated in filler

I'm surprised there's more code after this change since the original motivation was duplicated code :-)

[benmccann]

It might be helpful to add some comments explaining the difference between r, radius, and hitRadius and why each is needed

[kurkle]

now that the r is renamed to angle, do you still need clarification between radius and hitRadius?

[etimberg]

Didn't get fully through this yet, but a few comments

[kurkle]

Another example

type: 'radar',
data: {
    datasets: [{
        backgroundColor: 'rgba(255, 0, 0, 0.25)',
        borderColor: 'rgba(255, 0, 0, 0.75)',
        data: [1,1,1,1,1,1,1,1,1],
        fill: true
    }, {
        backgroundColor: 'rgba(0, 255, 0, 0.25)',
        borderColor: 'rgba(0, 255, 0, 0.75)',
        data: [2,2,null,2,2,2,2,null,2],
        fill: '-1'
    }, {
        backgroundColor: 'rgba(0, 0, 255, 0.25)',
        borderColor: 'rgba(0, 0, 255, 0.75)',
        data: [3,3,3,3,3,3,3,3,3],
        fill: '-1'
    }, {
        backgroundColor: 'rgba(255, 0, 255, 0.25)',
        borderColor: 'rgba(255, 0, 255, 0.75)',
        data: [4,4,4,4,4,4],
        fill: '-1'
    }, {
        backgroundColor: 'rgba(0, 255, 255, 0.25)',
        borderColor: 'rgba(0, 255, 255, 0.75)',
        data: [5,5,null,null,5,5,5,5,5],
        fill: '-1'

    }],
    labels: ['label1', 'label2', 'label3', 'label4', 'label5', 'label6', 'label7', 'label8', 'label9']
},
options: {
    spanGaps: false,
    responsive: false,
    scale: {min: 0, display: false},
    elements: {
        line: {
            tension: 0.6
        }
    }
}

Master:

PR:

Turn spanGaps: true

Master:

PR:

[etimberg]

I had a read. I like the fixes and I think I understand how this all works. @benmccann what do you think?

[kurkle]

Just few comments on the update: line is now split in segments (split on line brreaks)
When fill is applied between two lines, segments are compared each matching segments on both lines is a new 'fill segment' those ard then pathed around an filled.
This is somewhat harder on radial lines, because of the looping. Radial segments are matched using angles (radians). I used r to match the axis radial scale uses. That could be changed ofc, but I did not want to grow this any more.

[kurkle]

@benmccann couple of places, where the angle stored in r is used.

[benmccann]

just a heads up that this needs to be rebased

[kurkle]

Rebased and made some adjustments based on reviews. Ready for next round.

[kurkle]

Last one fixes the issue described in #6838 comments.
Added a test to catch that too.
Demo

[benmccann]

move can't be specified in the options, can it? If that's the case, I think it'd make that clearer to declare it on its own line

[kurkle]

It can and is, from filler.

[benmccann]

Is it documented anywhere? I didn't see it on https://www.chartjs.org/docs/latest/charts/area.html. Could we add it?

[kurkle]

Oh, there is a misunderstanding here. Maybe I should rename options to args here, these are just options passed to the function, not user provided.

[benmccann]

Thanks for clarifying. Yeah, args or params or something else would be good

[benmccann]

How does this impact the uPlot benchmark? It looks like we now have to loop over all data points an additional time and I'm worried that might be expensive

[kurkle]

Inconclusive. uPlot bench uses fastPath that skips this segment stuff.

Heres a plunker with some preconfigured uPlot based benchmarks to check it out:
https://plnkr.co/edit/QJ9IMuiybyXWHtgdTfEK?p=preview

[kurkle]

Fixed a bug in interpolated filling and added a test to catch it. Same test in plunker

[benmccann]

should loop just be a parameter to useFastPath? if we make it an instance method then you wouldn't have to pass in loop or options

[kurkle]

Yet another update to this.

• fastPath (or fastPathSegment as its named now) is enabled for fill.
• Segment stuff moved to helpers.segment to make things a bit cleaner.
• Bezier stuff moved to helpers.curve.
• segments are now computed for fastPath too. snapGaps: true disables the segmentation. Added that to performance.md

Plunker

Quick average seems like its about 25% faster when fill is enabled. Other modes are roughly the same (GC causes a lot of variation, so its hard to compare).