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

fix(xy): compute per series and global minPointsDistance #2571

Merged
merged 6 commits into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
12 changes: 12 additions & 0 deletions e2e/tests/line_stories.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,4 +58,16 @@ test.describe('Line series stories', () => {
);
});
});
test.describe('Points auto visibility', () => {
test('show points when space between point is enough', async ({ page }) => {
await common.expectChartAtUrlToMatchScreenshot(page)(
'http://localhost:9001/?path=/story/line-chart--isolated-data-points&globals=toggles.showHeader:true;toggles.showChartTitle:false;toggles.showChartDescription:false;toggles.showChartBoundary:false;theme:light&knob-default point radius=3&knob-enable fit function=true&knob-max data points=25&knob-point shape=circle&knob-point visibility=auto&knob-point visibility min distance=20&knob-series type=line&knob-visible series[0]=A&knob-visible series[1]=B&knob-visible series[2]=C&knob-visible series[3]=D',
);
});
test('hide points when space between point is small', async ({ page }) => {
await common.expectChartAtUrlToMatchScreenshot(page)(
'http://localhost:9001/?path=/story/line-chart--isolated-data-points&globals=toggles.showHeader:true;toggles.showChartTitle:false;toggles.showChartDescription:false;toggles.showChartBoundary:false;theme:light&knob-default point radius=3&knob-enable fit function=true&knob-max data points=60&knob-point shape=circle&knob-point visibility=auto&knob-point visibility min distance=20&knob-series type=line&knob-visible series[0]=A&knob-visible series[1]=B&knob-visible series[2]=C&knob-visible series[3]=D',
);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,13 @@ export function renderPoints(
{ opacity }: GeometryStateStyle,
pointStyle: PointStyle,
lineStrokeWidth: number,
minDistanceBetweenPoints: number,
minDistanceToShowPoints: number,
seriesMinPointDistance: number,
pointsDistanceVisibilityThreshold: number,
hasConnectingLine: boolean,
) {
const isHiddenOnAuto = pointStyle.visible === 'auto' && minDistanceBetweenPoints < minDistanceToShowPoints;
// seriesMinPointDistance could be Infinity if we don't have points, or we just have a single point per series.
// In this case the point should be visible if the visibility style is set to `auto`
const isHiddenOnAuto = pointStyle.visible === 'auto' && seriesMinPointDistance < pointsDistanceVisibilityThreshold;
const hideDataPoints = pointStyle.visible === 'never' || isHiddenOnAuto;
const hideIsolatedDataPoints = hasConnectingLine && hideDataPoints;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,6 @@ const DEFAULT_PROPS: ReactiveChartStateProps = {
areas: [],
bars: [],
lines: [],
points: [],
bubbles: [],
},
geometriesIndex: new IndexedGeometryMap(),
Expand Down
5 changes: 2 additions & 3 deletions packages/charts/src/chart_types/xy_chart/rendering/area.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import {
} from './utils';
import { Color } from '../../../common/colors';
import { ScaleBand, ScaleContinuous } from '../../../scales';
import { isBandScale } from '../../../scales/types';
import { CurveType, getCurveFactory } from '../../../utils/curves';
import { Dimensions } from '../../../utils/dimensions';
import { AreaGeometry } from '../../../utils/geometry';
Expand Down Expand Up @@ -71,7 +70,7 @@ export function renderArea(
if (y1Line) lines.push(y1Line);
if (y0Line) lines.push(y0Line);

const { pointGeometries, indexedGeometryMap } = renderPoints(
const { pointGeometries, indexedGeometryMap, minDistanceBetweenPoints } = renderPoints(
shift - xScaleOffset,
dataSeries,
xScale,
Expand Down Expand Up @@ -103,7 +102,7 @@ export function renderArea(
clippedRanges,
shouldClip: hasFit,
hasFit,
minPointDistance: isBandScale(xScale) ? xScale.bandwidth : xScale.scale(xScale.domain[0] + xScale.minInterval),
minPointDistance: minDistanceBetweenPoints,
};
return {
areaGeometry,
Expand Down
12 changes: 4 additions & 8 deletions packages/charts/src/chart_types/xy_chart/rendering/line.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import { renderPoints } from './points';
import { getClippedRanges, getY1ScaledValueFn, getYDatumValueFn, isYValueDefinedFn, MarkSizeOptions } from './utils';
import { Color } from '../../../common/colors';
import { ScaleBand, ScaleContinuous } from '../../../scales';
import { isBandScale } from '../../../scales/types';
import { CurveType, getCurveFactory } from '../../../utils/curves';
import { Dimensions } from '../../../utils/dimensions';
import { LineGeometry } from '../../../utils/geometry';
Expand Down Expand Up @@ -50,7 +49,7 @@ export function renderLine(
.defined((datum) => definedFn(datum, y1Accessor))
.curve(getCurveFactory(curve));

const { pointGeometries, indexedGeometryMap } = renderPoints(
const { pointGeometries, indexedGeometryMap, minDistanceBetweenPoints } = renderPoints(
shift - xScaleOffset,
dataSeries,
xScale,
Expand All @@ -70,7 +69,7 @@ export function renderLine(
// TODO we can probably avoid computing the clipped ranges if no fit function is applied.
const clippedRanges = getClippedRanges(dataSeries.data, xScale, xScaleOffset);

const lineGeometry = {
const lineGeometry: LineGeometry = {
line: pathGenerator(dataSeries.data) || '',
points: pointGeometries,
color,
Expand All @@ -83,10 +82,7 @@ export function renderLine(
clippedRanges,
shouldClip: hasFit,
hasFit,
minPointDistance: isBandScale(xScale) ? xScale.bandwidth : xScale.scale(xScale.domain[0] + xScale.minInterval),
};
return {
lineGeometry,
indexedGeometryMap,
minPointDistance: minDistanceBetweenPoints,
};
return { lineGeometry, indexedGeometryMap };
}
142 changes: 78 additions & 64 deletions packages/charts/src/chart_types/xy_chart/rendering/points.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export function renderPoints(
styleAccessor?: PointStyleAccessor,
): {
pointGeometries: PointGeometry[];
minDistanceBetweenPoints: number;
indexedGeometryMap: IndexedGeometryMap;
} {
const indexedGeometryMap = new IndexedGeometryMap();
Expand All @@ -74,78 +75,91 @@ export function renderPoints(
let isolatedPointStyle = buildPointGeometryStyles(color, isolatedPointThemeStyle);
let styleOverrides: Partial<PointStyle> | undefined = undefined;

const pointGeometries = dataSeries.data.reduce<SortedArray<PointGeometry>>((acc, datum, dataIndex) => {
const { x: xValue, mark } = datum;
const prev = dataSeries.data[dataIndex - 1];
const next = dataSeries.data[dataIndex + 1];
// don't create the point if not within the xScale domain
if (!xScale.isValueInDomain(xValue)) return acc;
const { pointGeometries, minDistanceBetweenPoints } = dataSeries.data.reduce<{
pointGeometries: SortedArray<PointGeometry>;
minDistanceBetweenPoints: number;
prevX: number | undefined;
}>(
(acc, datum, dataIndex) => {
const { x: xValue, mark } = datum;
const prev = dataSeries.data[dataIndex - 1];
const next = dataSeries.data[dataIndex + 1];
// don't create the point if not within the xScale domain
if (!xScale.isValueInDomain(xValue)) return acc;

// don't create the point if it that point was filled
const x = xScale.scale(xValue);
// don't create the point if it that point was filled
const x = xScale.scale(xValue);

if (Number.isNaN(x)) return acc;
if (!isFiniteNumber(x)) return acc;

const yDatumKeyNames: Array<keyof Omit<FilledValues, 'x'>> = isBandedSpec ? ['y0', 'y1'] : ['y1'];
const seriesIdentifier: XYChartSeriesIdentifier = getSeriesIdentifierFromDataSeries(dataSeries);
const isPointIsolated = allowIsolated && isIsolatedPoint(dataIndex, dataSeries.data.length, yDefined, prev, next);
if (styleAccessor) {
styleOverrides = getPointStyleOverrides(datum, seriesIdentifier, isPointIsolated, styleAccessor);
style = buildPointGeometryStyles(color, pointStyle, styleOverrides);
isolatedPointStyle = buildPointGeometryStyles(color, isolatedPointThemeStyle, styleOverrides);
}
yDatumKeyNames.forEach((yDatumKeyName, keyIndex) => {
const valueAccessor = getYDatumValueFn(yDatumKeyName);
const y = yDatumKeyName === 'y1' ? y1Fn(datum) : y0Fn(datum);
const originalY = getDatumYValue(datum, keyIndex === 0, isBandedSpec, dataSeries.stackMode);
if (isFiniteNumber(acc.prevX) && !isDatumFilled(datum)) {
acc.minDistanceBetweenPoints = Math.min(acc.minDistanceBetweenPoints, Math.abs(x - acc.prevX));
}
acc.prevX = x;

const yDatumKeyNames: Array<keyof Omit<FilledValues, 'x'>> = isBandedSpec ? ['y0', 'y1'] : ['y1'];
const seriesIdentifier: XYChartSeriesIdentifier = getSeriesIdentifierFromDataSeries(dataSeries);
const isPointIsolated = allowIsolated && isIsolatedPoint(dataIndex, dataSeries.data.length, yDefined, prev, next);
if (styleAccessor) {
styleOverrides = getPointStyleOverrides(datum, seriesIdentifier, isPointIsolated, styleAccessor);
style = buildPointGeometryStyles(color, pointStyle, styleOverrides);
isolatedPointStyle = buildPointGeometryStyles(color, isolatedPointThemeStyle, styleOverrides);
}
yDatumKeyNames.forEach((yDatumKeyName, keyIndex) => {
const valueAccessor = getYDatumValueFn(yDatumKeyName);
const y = yDatumKeyName === 'y1' ? y1Fn(datum) : y0Fn(datum);
const originalY = getDatumYValue(datum, keyIndex === 0, isBandedSpec, dataSeries.stackMode);

// if radius is defined with the mark, limit the minimum radius to the theme radius value
const radius = isPointIsolated
? isolatedPointRadius(lineStrokeWidth)
: markSizeOptions.enabled
? Math.max(getRadius(mark), pointStyle.radius)
: styleOverrides?.radius ?? pointStyle.radius;
// if radius is defined with the mark, limit the minimum radius to the theme radius value
const radius = isPointIsolated
? isolatedPointRadius(lineStrokeWidth)
: markSizeOptions.enabled
? Math.max(getRadius(mark), pointStyle.radius)
: styleOverrides?.radius ?? pointStyle.radius;

const pointGeometry: PointGeometry = {
x,
y: y === null ? NaN : y,
radius,
color,
style: isPointIsolated ? isolatedPointStyle : style,
value: {
x: xValue,
y: originalY,
mark,
accessor: isBandedSpec && keyIndex === 0 ? BandedAccessorType.Y0 : BandedAccessorType.Y1,
datum: datum.datum,
},
transform: {
x: shift,
y: 0,
},
seriesIdentifier,
panel,
isolated: isPointIsolated,
};
indexedGeometryMap.set(pointGeometry, geometryType);
// use the geometry only if the yDatum in contained in the current yScale domain
if (
isFiniteNumber(y) &&
yDefined(datum, valueAccessor) &&
yScale.isValueInDomain(valueAccessor(datum)) &&
!isDatumFilled(datum)
) {
if (needSorting) {
inplaceInsertInSortedArray(acc, pointGeometry, (p) => p?.radius ?? NaN);
} else {
acc.push(pointGeometry);
const pointGeometry: PointGeometry = {
x,
y: y === null ? NaN : y,
radius,
color,
style: isPointIsolated ? isolatedPointStyle : style,
value: {
x: xValue,
y: originalY,
mark,
accessor: isBandedSpec && keyIndex === 0 ? BandedAccessorType.Y0 : BandedAccessorType.Y1,
datum: datum.datum,
},
transform: {
x: shift,
y: 0,
},
seriesIdentifier,
panel,
isolated: isPointIsolated,
};
indexedGeometryMap.set(pointGeometry, geometryType);
// use the geometry only if the yDatum in contained in the current yScale domain
if (
isFiniteNumber(y) &&
yDefined(datum, valueAccessor) &&
yScale.isValueInDomain(valueAccessor(datum)) &&
!isDatumFilled(datum)
) {
if (needSorting) {
inplaceInsertInSortedArray(acc.pointGeometries, pointGeometry, (p) => p?.radius ?? NaN);
} else {
acc.pointGeometries.push(pointGeometry);
}
}
}
});
return acc;
}, []);
});
return acc;
},
{ pointGeometries: [], minDistanceBetweenPoints: Infinity, prevX: undefined },
);
return {
pointGeometries,
minDistanceBetweenPoints,
indexedGeometryMap,
};
}
Expand Down
10 changes: 1 addition & 9 deletions packages/charts/src/chart_types/xy_chart/state/utils/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,7 @@

import { SmallMultiplesSeriesDomains } from '../../../../common/panel_utils';
import { ScaleBand, ScaleContinuous } from '../../../../scales';
import {
PointGeometry,
BarGeometry,
AreaGeometry,
LineGeometry,
BubbleGeometry,
PerPanel,
} from '../../../../utils/geometry';
import { BarGeometry, AreaGeometry, LineGeometry, BubbleGeometry, PerPanel } from '../../../../utils/geometry';
import { GroupId } from '../../../../utils/ids';
import { XDomain, YDomain } from '../../domains/types';
import { IndexedGeometryMap } from '../../utils/indexed_geometry_map';
Expand Down Expand Up @@ -48,7 +41,6 @@ export interface ComputedScales {

/** @internal */
export interface Geometries {
points: PointGeometry[];
bars: Array<PerPanel<BarGeometry[]>>;
areas: Array<PerPanel<AreaGeometry>>;
lines: Array<PerPanel<LineGeometry>>;
Expand Down
35 changes: 18 additions & 17 deletions packages/charts/src/chart_types/xy_chart/state/utils/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,7 @@ import { TextMeasure } from '../../../../utils/bbox/canvas_text_bbox_calculator'
import { isFiniteNumber, isNil, isUniqueArray, mergePartial, Rotation } from '../../../../utils/common';
import { CurveType } from '../../../../utils/curves';
import { Dimensions, Size } from '../../../../utils/dimensions';
import {
AreaGeometry,
BarGeometry,
BubbleGeometry,
LineGeometry,
PerPanel,
PointGeometry,
} from '../../../../utils/geometry';
import { AreaGeometry, BarGeometry, BubbleGeometry, LineGeometry, PerPanel } from '../../../../utils/geometry';
import { GroupId, SpecId } from '../../../../utils/ids';
import { SeriesCompareFn } from '../../../../utils/series_sort';
import { ColorConfig, Theme } from '../../../../utils/themes/theme';
Expand Down Expand Up @@ -307,7 +300,6 @@ function renderGeometries(
fallBackTickFormatter: TickFormatter,
measureText: TextMeasure,
): Omit<ComputedGeometries, 'scales'> {
const points: PointGeometry[] = [];
const bars: Array<PerPanel<BarGeometry[]>> = [];
const areas: Array<PerPanel<AreaGeometry>> = [];
const lines: Array<PerPanel<LineGeometry>> = [];
Expand All @@ -325,7 +317,9 @@ function renderGeometries(
bubblePoints: 0,
};
const barsPadding = enableHistogramMode ? chartTheme.scales.histogramPadding : chartTheme.scales.barsPadding;

// This var remains Infinity if we don't have points, or we just have a single point per series.
// In this case the point should be visible if the visibility style is set to `auto`
let globalMinPointsDistance = Infinity;
dataSeries.forEach((ds) => {
const spec = getSpecsById<BasicSeriesSpec>(seriesSpecs, ds.specId);
if (spec === undefined) {
Expand Down Expand Up @@ -463,11 +457,12 @@ function renderGeometries(
});
geometriesCounts.linePoints += renderedLines.lineGeometry.points.length;
geometriesCounts.lines += 1;
globalMinPointsDistance = Math.min(globalMinPointsDistance, renderedLines.lineGeometry.minPointDistance);
} else if (isAreaSeriesSpec(spec)) {
const areaShift = barIndexOrder && barIndexOrder.length > 0 ? barIndexOrder.length : 1;
const areaSeriesStyle = getAreaSeriesStyles(chartTheme.areaSeriesStyle, spec.areaSeriesStyle);
const xScaleOffset = computeXScaleOffset(xScale, enableHistogramMode, spec.histogramModeAlignment);
const renderedAreas = renderArea(
const renderedArea = renderArea(
// move the point on half of the bandwidth if we have mixed bars/lines
(xScale.bandwidth * areaShift) / 2,
ds,
Expand All @@ -487,22 +482,28 @@ function renderGeometries(
hasFitFnConfigured(spec.fit),
spec.pointStyleAccessor,
);
geometriesIndex.merge(renderedAreas.indexedGeometryMap);
geometriesIndex.merge(renderedArea.indexedGeometryMap);
areas.push({
panel,
value: renderedAreas.areaGeometry,
value: renderedArea.areaGeometry,
});
geometriesCounts.areasPoints += renderedAreas.areaGeometry.points.length;
geometriesCounts.areasPoints += renderedArea.areaGeometry.points.length;
geometriesCounts.areas += 1;
globalMinPointsDistance = Math.min(globalMinPointsDistance, renderedArea.areaGeometry.minPointDistance);
}
});

return {
geometries: {
points,
bars,
areas,
lines,
areas: areas.map((a) => {
a.value.minPointDistance = globalMinPointsDistance;
return a;
}),
lines: lines.map((l) => {
l.value.minPointDistance = globalMinPointsDistance;
return l;
}),
bubbles,
},
geometriesIndex,
Expand Down
Loading