-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[charts] Add batch renderer for scatter chart #19075
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
[charts] Add batch renderer for scatter chart #19075
Conversation
Deploy preview: https://deploy-preview-19075--material-ui-x.netlify.app/ Updated pages: Bundle size report
|
c75733b
to
2c3e0b0
Compare
CodSpeed Performance ReportMerging #19075 will not alter performanceComparing Summary
Benchmarks breakdown
|
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.
Might be interesting to explore building the string as an array of bytes rather than arrays of string
s. It might have an edge in large datasets.
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.
Yeah, I can try that.
Do you mean using something like a Uint16Array
and adding the char codes directly?
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.
Probably a Uint8Array
, because the bytes need to be converted to a string and the TextDecoder
API is probably the best candidate for that.
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.
Added the buffer alternative and it seems to be about 2x slower. Here's the commit.
Do you think there's something I could have done better?
Here are the traces:
I'll revert that commit for now as it's slower
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.
@bernardobelchior buffer is not meant to be "fast string", at least not in JS, since strings are built-in the VM there is no conversion 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.
That commit looks good, seems like it doesn't pay off to do it that way then.
511d9d7
to
03e35a4
Compare
Unstable_FastScatter
componentuseFastRenderer
prop for scatter chart
1c832be
to
2d3dbe0
Compare
2d3dbe0
to
000af6b
Compare
docs/data/charts/scatter/scatter.md
Outdated
|
||
Scatter charts can have a lot of data points, which can impact performance. The default rendering of scatter points uses SVG `circle` elements, which can be slow for a large number of points. | ||
|
||
To improve performance, you can use the `useFastRenderer` prop, which renders the circles more efficiently. However, this comes with the following limitations: |
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.
Maybe fastRenderer
instead, as the use*
has a use in react.
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.
What about preferFastRenderer
or renderer: "batch" | "single"
?
Maybe this last one is better. We can replace "single" with "individual".
The idea is:
- "batch" renderer: renders circles in batch
- "single": renders circles individually
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.
Yeah, actually I prefer this "renderer" prop. In the future, if we add canvas, we can add a new value rendered="canvas"
. Otherwise we'd have a weird API where renderer="canvas"
and preferFastRenderer
would be incompatible but possible to specify.
So it might make sense to call them:
renderer="svg-single"
renderer="svg-batch"
So in the future we can add:
renderer="canvas"
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 simpler to leave single/batch
without the svg- prefix. We can still add canvas
later. If it causes confusion we can add the svg- prefix option, though as long as it is documented it should be ok
} | ||
|
||
const MAX_POINTS_PER_PATH = 1000; | ||
const ALMOST_ZERO = 0.01; |
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.
Why is this necessary? Probably good to leave a comment.
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.
Added 👍
21b5ebe
to
4d49a8d
Compare
useFastRenderer
prop for scatter chartThere 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.
small detail
f381d25
to
8c0c72c
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.
Looks good:
- deprecating the
disableVoronoi
could be done in a follow up PR - I'm less confident with the
onItemClick
. I think it can be simplified
docs/data/charts/scatter/scatter.md
Outdated
On top of that, there's also some differences in behavior: | ||
|
||
- The rendering order might be different, which might cause overlapping circles to render at different depths when compared to the default rendering; | ||
- When `disableVoronoi` is true, the `onItemClick` event target will be the SVG root, instead of the circle the click targeted; |
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.
What about if disableVoronoi
is true
the onItemClick
is never called?
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.
No, if disableVoronoi
is true, onItemClick
is still called. Here's the logic.
> & { | ||
/** | ||
* If true, a point is only returned if the pointer is within the radius of the point. | ||
*/ | ||
disableClosestPoint: boolean; |
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.
If you go that way, you could also add this parameter in the Params, and add a deprecation warning on the disableVoronoi
encouraging to use this new name
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.
Yeah, that's my idea, but I'm planning on doing that in a follow-up.
...ages/x-charts/src/internals/plugins/featurePlugins/useChartHighlight/highlightStates.test.ts
Outdated
Show resolved
Hide resolved
const disableOnItemClick = onItemClick == null; | ||
const disableVoronoi = disableClosestPoint && disableOnItemClick; |
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.
In don't get the interest of this part.
You already have in the useScatterProps
hook a logic that moves the onItemClick
on the scatter mark or the chart container according to the value of disableVoronoi
onItemClick: useVoronoiOnItemClick
? (onItemClick as UseChartVoronoiSignature['params']['onItemClick'])
: undefined,
FOr me the hook should be straightforward. If use set disableClosestPoint
to true, this hook should do nothing. Even if onItemClick
is not nullable.
If I get it correctly, you currently have a weird spot where
- If I set
disableVoronoi=true
I don't get voronoid - If I set
disableVoronoi=true
but I provide an onItemClick with a batch render, then voronoid is activated but with the markSIze as a radius value
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.
The voronoi is an implementation detail.
When batch rendering, if disableClosestPoint
is false, but onItemClick
is set, we'll use the voronoi to find element that's being clicked on.
Why do you think this is weird?
I agree it's confusing at the moment, but I plan to fix it in a follow-up when renaming the voronoi plugin.
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.
That's not at all about voronoi/closest point. I'm in favor of that renaming. In this comment I will use only "closest point" assuming the renaming is already done
Current behavior
The idea of disableClosestPoint
is to switch between two ways of managing interaction:
- disableClosestPoint=true : We rely on the item event handlers: Straightforward computation, but interactions are limited
- disableClosestPoint=false : We rely on some computation for each pointer move event: More complex, more JS computation, but better user interaction
The initial reason I introduced this props was to save computation. If you don't need closest point for highlight or click, then it's useless to search for the closest point at each pointer move, so you can disable it.
For example it's value is true
if their are no scatter series in the chart container.
Why I find it weird
I prefer when props have an order of priority. Currently, we have disableClosestPoint
that impacts how onItemClick
onHighlightChange
are done.
With this PR we will have an edge case where onItemClick
impacts the value of disableClosestPoint
reversing the order of impacts.
The order revers is weird, and not necessary, because the only usecase I can see is when a dev at the same time
disableClosestPoint=true
- use batch rendering
- provide an
onItemClick
In such case we should warn them that onItemClick
+ barch rendering needs to enable the "closest point" computation. Not ignore the props they provide
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.
In such case we should warn them that onItemClick + barch rendering needs to enable the "closest point" computation. Not ignore the props they provide
The fact that we use the closest point logic for the item click when using batch rendering is a detail that doesn't need to be exposed to the user.
To me, disableClosestPoint
means that we should no longer calculate the closest point, and instead only show the tooltip and call onItemClick
when over a point. If we use the closest point calculations to compute the onItemClick
click is something that should not concern the user.
This is obviously a trade-off. Your focus on disableClosestPoint
not disabling the closest point algorithm seems to come from a performance concern because the closest point algorithm have a perf impact, is that right? My proposal is more focused on behavior: onItemClick
should work with disableClosestPoint: true
regardless of batch rendering or not.
I think the biggest difference is our definition of disableClosestPoint
. To me, it means "do not find the closest point, only handle clicks/show tooltips when over the point". IMO, that's clearer to users. Do you see any disadvantages in this definition?
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.
I think the biggest difference is our definition of disableClosestPoint.
Partially. The other distinction is how much should we hide internals from users. For me we should favorise warning over saving user bad combination
Do you see any disadvantages in this definition?
- If you set
disableClosestPoint=true
+onItemClick=()=>{}
: highlight and tooltip work - If you set
disableClosestPoint=true
+onItemClick=undefined
: highlight and tooltip do not work.
Capture.video.du.2025-09-10.16-01-23.mp4
I think it's easier to tell the user
:::
With batch rendering, items are not a single SVG item, so the chart rely on closest point for all its item interaction.
If you disable it with disableClosestPoint
none of the tooltip, highlght, or click will work
:::
That's for the pedagogical aspect.
On the feature point of view, I agree we told user to do
set the
disableVoronoi
prop totrue
to trigger interactions only when hovering exactly over an element instead of Voronoi cells.
It's a mistake. because before batch rendering disabling the internal computation and only interaction on hover were the same.
What about having voronoiMaxRadius: "item"
which would have exactly the same behavior as your current implementation: a maxRadius equal to the item size.
From that we could
- Remove
disableVoronoi
/disableClosestPoint
or keep it just as an hidden performance trick - Only configure the scatter interaction with
voronoiMaxRadius
that could be renamedinteractionRadius
,interactionDistance
, ...
That would provide
- A clear distinction between props impacting the implementation strategy
- A simpler interaction API: one props instead of a combination of two props
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.
we should favorise warning over saving user bad combination
"Bad combination" depends on the semantics of disableClosestPoint
😄
If you set disableClosestPoint=true + onItemClick=undefined: highlight and tooltip do not work.
Yeah, that's a mistake. The tooltip should keep working in this case. I suppose we need to change the logic to enable the voronoi when using the batch renderer or when disableClosestPoint
is false. Do you think that would work?
I think it's easier to tell the user
:::
With batch rendering, items are not a single SVG item, so the chart rely on closest point for all its item interaction.
If you disable it with disableClosestPoint none of the tooltip, highlght, or click will work
:::
This is an internal implementation detail, and we should avoid exposing this to users. In the future we might change this implementation, so I think our API should depend on the desired behavior rather than the current implementation.
If we focus on the behavior, we don't need to inform the user how we implement batch rendering.
What about having voronoiMaxRadius: "item" which would have exactly the same behavior as your current implementation: a maxRadius equal to the item size.
IMO, we should try to keep the API between the single and batch renderer as similar as possible. With that in mind, we can either:
- Apply the
voronoiMaxRadius: 'item'
to both renderers:
a. In the batch renderer, it would use the voronoi;
b. In the single renderer, it would use the SVG elements'onClick
. - Use the
disableVoronoi
to interact only on hover:
a. In the batch renderer, it would use the voronoi;
b. In the single renderer, it would use the SVG elements' pointer events.
If we go for 1, I think we should remove disableVoronoi
to avoid disabling the voronoi plugin which would break the batch renderer's onItemClick
(and tooltip interaction). Since this would be a breaking change, I think the second option is better and in v9 we rename disableVoronoi
to disableClosestPoint
. It wouldn't disable the plugin, but rather disable the closest point logic (which onItemClick
isn't a part of, so it would keep working).
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 an extra thaugh: We only discussed about the <ScatterChart />
but the composition, users can not ignore the internal details.
To use item interaction they need to pass onItemClick
to the ScatterPlot. But for closetes point implementation they need to pass it to the ChartDataProvider. So they need to know that for batch rendering the click has to be on the data provider even if they don't want the closest point feature
What about going with 1. deprecate the disableVoronoi
with a warning message encouraging to use voronoiMaxRadius: 'item'
instead, and to open an issue if this solution does not correspond to their needs. It's a simpler migration, and encourage users to provide feedback about use-cases we could forget before the beginning of a next major
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.
That's a good point. I'll think more about how composition will work with this new batch renderer.
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.
As discussed offline, we came to this conclusion:
- Add
voronoiMaxRadius: 'item'
; - Deprecate
disableVoronoi
, and remove in v9:- In the batch renderer, when
disableVoronoi
istrue
,onItemClick
won’t be called; - In the single renderer, when
disableVoronoi
istrue
,onItemClick
will be called (to prevent a breaking change).
- In the batch renderer, when
- In v9, refactor
onItemClick
for the single renderer to use the same code path as the batch renderer (requires a breaking change because we can no longer provide the svgcircle
in theonItemClick
callback).
4ff36d9
to
7fc3799
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.
🚀
docs/data/charts/scatter/scatter.md
Outdated
- Highlighted style transparency: for performance reasons, the highlighted state creates another circle on top of the original circle. This allows us to skip re-rendering all paths when a data point is highlighted, but this also means that applying transparency to the highlighted circle will cause the original, not highlighted circle to become partially visible. | ||
- Transparent highlight style: the highlighted state creates a circle on top of the original circle. Applying transparency to the highlighted circle can cause the original circle to be partially visible. |
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.
They look like the same. The second one being more concise
- Highlighted style transparency: for performance reasons, the highlighted state creates another circle on top of the original circle. This allows us to skip re-rendering all paths when a data point is highlighted, but this also means that applying transparency to the highlighted circle will cause the original, not highlighted circle to become partially visible. | |
- Transparent highlight style: the highlighted state creates a circle on top of the original circle. Applying transparency to the highlighted circle can cause the original circle to be partially visible. | |
- Transparent highlight style: the highlighted state creates a circle on top of the original circle. Applying transparency to the highlighted circle can cause the original circle to be partially visible. |
docs/data/charts/scatter/scatter.md
Outdated
On top of that, there's also some differences in behavior: | ||
|
||
- The rendering order might be different, which might cause overlapping circles to render at different depths when compared to the default rendering; | ||
- When `disableVoronoi` is true, the `onItemClick` event target will be the SVG root, instead of the circle the click targeted; |
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.
- When `disableVoronoi` is true, the `onItemClick` event target will be the SVG root, instead of the circle the click targeted; | |
- When `disableVoronoi` is `true`, the `onItemClick` does not work; |
This reverts commit 20ce535.
7d2939d
to
9b017ec
Compare
Part of #12960.
Cherry-picked from #18941.
This PR adds a new
BatchScatter
component that can replace the currentScatter
component in a scatter chart.Though much faster, this new component has some limitations. Here's a list of the features from
Scatter
that don't work or whose behavior differs inBatchScatter
:circle
with CSS is no longer possible;marker
slot:BatchScatter
only renders circles at the moment and there is no way to customize the shape to render;Scatter
, so overlapping circles may render at different depths (i.e., z-index);onItemClick
event: whendisableVoronoi
is true, theonItemClick
event target will be the SVG root, instead of the circle the click targeted.