[charts] Save full-circle flag on rotation axis#22162
[charts] Save full-circle flag on rotation axis#22162alexfauquette merged 5 commits intomui:masterfrom
Conversation
…ion axis Record the decision made in getRange() as `isPointScaleFullCircle` on the computed polar axis so ChartsRadialGrid reads a flag instead of recomputing the full-circle check with floating-point math. Closes mui#22135
|
@alexfauquette @JCQuintas I'd appreciate a review on this PR whenever you get a chance. Thanks! |
Bundle size
Deploy previewhttps://deploy-preview-22162--material-ui-x.netlify.app/ Check out the code infra dashboard for more information about this PR. |
|
What about renaming it |
Good call - will do. I'll rename to |
… axes Address review feedback: rename the flag to `isFullCircle` and compute it for every rotation axis (point/band/continuous), not only the point-scale branch. Wire the flag through to `ChartsRadiusGrid` as a prop so it drops its local `EPSILON`-based derivation. The point-scale shortening keeps its existing `2π - 0.1` threshold as a local decision.
|
Done in bdd6fcc. I left |
| triggerTooltip?: boolean; | ||
| /** @ignore - internal. True when a rotation axis covers a full circle. */ | ||
| isFullCircle?: boolean; | ||
| }; |
There was a problem hiding this comment.
Is this one necessary?
The isFullCircle is only added in the computed axis
There was a problem hiding this comment.
I tried removing it and TypeScript breaks at the three write sites in computeAxisValue.ts (the completeAxis[axis.id] = { ..., isFullCircle } literals in the band, point, and continuous branches). completeAxis is typed DefaultizedAxisConfig<ChartsAxisProps> -> PolarAxisDefaultized, so the object literal hits the excess-property check if the field isn't declared there.
The reads go through ComputedAxis and the writes go through PolarAxisDefaultized, so both need it. Happy to remove it from PolarAxisDefaultized if you'd prefer to widen the write site with a cast though I'd lean toward keeping both since the @ignore - internal keeps it out of the public surface either way.
| ]; | ||
| const diff = angles[1] - angles[0]; | ||
| const isFullCircle = diff >= Math.PI * 2 - EPSILON; | ||
| if (axis.scaleType === 'point' && diff > Math.PI * 2 - 0.1) { |
There was a problem hiding this comment.
| if (axis.scaleType === 'point' && diff > Math.PI * 2 - 0.1) { | |
| if (axis.scaleType === 'point' && isFullCircle) { |
There was a problem hiding this comment.
Applied in 9f9196917. Thanks for catching it.
Merging this PR will improve performance by 9.02%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | ScatterChartPro with big data amount (single renderer) |
473.1 ms | 434 ms | +9.02% |
Comparing Anexus5919:fix/polar-axis-full-circle-flag (8a98f2d) with master (2e2d35a)1
Footnotes
…ircle Apply review suggestion: use `isFullCircle` (the `2π - EPSILON` check) as the condition for the point-scale range shortening instead of the looser `2π - 0.1`. Removes the remaining threshold discrepancy between the flag and the decision that triggers the shortening.
alexfauquette
left a comment
There was a problem hiding this comment.
Thanks for the contribution
Closes #22135
Follow-up to #22134
Summary
When a rotation axis uses a
pointscale and covers a full circle, the polar plugin'sgetRange()intentionally shortens the angular range by2π / Nto avoid overlapping the first and last data points. #22134 patchedChartsRadialGridto still draw closed circles in that case by re-deriving the decision with a floating-pointEPSILONcomparison on the already-shortened range.As @JCQuintas pointed out in #22134 (comment):
This PR records that decision at its source in
getRange()and exposes it as an internal flag on the computed polar axis, so consumers can read a boolean instead of reconstructing the verdict from shortened ranges.Changes
packages/x-charts/src/internals/plugins/featurePlugins/useChartPolarAxis/computeAxisValue.tsgetRange()now returns{ range, isPointScaleFullCircle }for all three branches (rotation+point, rotation+non-point, radius).isPointScaleFullCircleistrueonly in the existing rotation+point branch when the pre-shorteningdiff > 2π − 0.1, the exact condition that was already triggering the range shortening. It isfalse(and irrelevant) everywhere else.isPointScaleConfig(axis)branch, where it has meaning.packages/x-charts/src/models/axis.tsisPointScaleFullCircle?: booleantoPolarAxisDefaultized(where the polar plugin assigns it) andComputedAxis(whatuseRotationAxes()returns at the type level, since [charts-premium] Create a'radialLine'series type #22066 unified the return type). Marked@ignore - internalon both.packages/x-charts/src/ChartsRadialGrid/ChartsRadialGrid.tsxEPSILON-based reconstruction (dataRotationGap/angleGap/Math.abs(...) < EPSILON) with a direct flag read.EPSILONimport.rotationAxisConfig?.isPointScaleFullCircleuses optional chaining, so the code no longer has therotationAxisConfig.scaleType === 'point'non-null access that was introduced by [charts] Fix radius grid lines when axis uses point scale #22134.Behavior
For every canonical scenario the output is byte-identical to current master. Verified against #22134's two tests:
scaleType: 'point'→ closed circles rendered (circleelements). ✅startAngle: 0, endAngle: Math.PI+scaleType: 'point'→ open arcs rendered (pathelements). ✅One subtle behavior change (arguably an internal-consistency fix)
The threshold that triggers range shortening in
getRange()isdiff > 2π − 0.1(≈ 354.28°). The old EPSILON check inChartsRadialGrid, after algebra, only fires when the user-specifieddiffis withinEPSILON · N/(N−1)of2π(≈ 0.06°–0.08° in typical cases). That created a narrow window where a user specifying e.g.endAngle: 357°with a point scale and 5 data points would see:getRangeand the grid renderer disagreeing with each otherAfter this PR, the single source of truth (the shortening decision in
getRange()) drives both. In that narrow window, the grid now closes consistently with how the scale is already laid out.This is a defensible consistency fix, but worth calling out explicitly in case it surprises anyone.
Why not just cast in
ChartsRadialGrid?Considered. Because #22066 intentionally typed the polar selector's return via
ComputedAxis/ComputedAxisConfig(unified with cartesian to enable the radial line series), a cast would have been needed to surface the flag. An optional internal field onComputedAxisis the minimal, non-fragile way; the field is?and@ignore - internal, so cartesian consumers are unaffected.Testing
Everything run against this branch. Failures called out below are pre-existing on
upstream/master(verified independently by stashing and re-running).pnpm --filter "@mui/x-charts*" run typescript(x-charts, x-charts-pro, x-charts-premium)pnpm test:unit --project "x-charts" --runChartsRadialGridtests)pnpm test:unit --project "x-charts-pro" --runpnpm test:unit --project "x-charts-premium" --runpnpm test:browser --project "x-charts" --run src/ChartsRadialGridpnpm test:browser --project "x-charts" --run(full)src/utils/niceDomain.test.tsxare pre-existing timezone artifacts (also fail on cleanupstream/master— unrelated to this PR)pnpm eslint--max-warnings 0)pnpm proptypespnpm generate:exportsTest plan
pnpm proptypesproduces no diff (no public API change -ChartsRadialGridPropsis untouched)