-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Rewrite filler #6795
Rewrite filler #6795
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :-)
src/controllers/controller.radar.js
Outdated
y: y, | ||
x, | ||
y, | ||
r, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be helpful to add some comments explaining the difference between r
, radius
, and hitRadius
and why each is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that the r
is renamed to angle
, do you still need clarification between radius
and hitRadius
?
56f33aa
to
8658851
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't get fully through this yet, but a few comments
c97c694
to
4d9fded
Compare
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
}
}
} Turn |
I had a read. I like the fixes and I think I understand how this all works. @benmccann what do you think? |
Just few comments on the update: line is now split in segments (split on line brreaks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benmccann couple of places, where the angle stored in r
is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a heads up that this needs to be rebased
2797a92
to
41c8aaf
Compare
Rebased and made some adjustments based on reviews. Ready for next round. |
src/elements/element.line.js
Outdated
const {points, _view: vm} = line; | ||
const lineMethod = getLineMethod(vm); | ||
const count = points.length; | ||
let {move = true, reverse} = options || {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can and is, from filler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it documented anywhere? I didn't see it on https://www.chartjs.org/docs/latest/charts/area.html. Could we add it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying. Yeah, args
or params
or something else would be good
366ac10
to
12921c9
Compare
src/elements/element.line.js
Outdated
if (!prev.skip) { | ||
last = start; | ||
} | ||
for (end = start + 1; end < max; ++end) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Fixed a bug in interpolated filling and added a test to catch it. Same test in plunker |
e855635
to
2a9c325
Compare
src/elements/element.line.js
Outdated
} | ||
|
||
if (useFastPath(options)) { | ||
fastPath(ctx, points, spanGaps); | ||
if (!loop && useFastPath(options, params)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Yet another update to this.
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). |
2cba6ad
to
9673960
Compare
After implementing
fastPath
, I realized most of the drawing logic is duplicated infiller
.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
toLine
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:
Master:
PR: