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

* Compute per series and global minPointsDistance * Add auto visibility test * fix filled check * consider single point visibility on auto * test(vrt): update screenshots [skip ci] --------- Co-authored-by: Nick Partridge <[email protected]> Co-authored-by: elastic-datavis[bot] <98618603+elastic-datavis[bot]@users.noreply.github.com>
elastic · Dec 10, 2024 · 8dae2c1 · 8dae2c1
1 parent feacfd6
commit 8dae2c1
Show file tree

Hide file tree

Showing 14 changed files with 133 additions and 108 deletions.
diff --git a/...ll.test.ts-snapshots/baselines/line-chart/isolated-data-points-chrome-linux.png b/...ll.test.ts-snapshots/baselines/line-chart/isolated-data-points-chrome-linux.png
diff --git a/...s/area-with-isolated-data-points/render-correctly-fit-function-chrome-linux.png b/...s/area-with-isolated-data-points/render-correctly-fit-function-chrome-linux.png
diff --git a/...-auto-visibility/hide-points-when-space-between-point-is-small-chrome-linux.png b/...-auto-visibility/hide-points-when-space-between-point-is-small-chrome-linux.png
diff --git a/...auto-visibility/show-points-when-space-between-point-is-enough-chrome-linux.png b/...auto-visibility/show-points-when-space-between-point-is-enough-chrome-linux.png
diff --git a/e2e/tests/line_stories.test.ts b/e2e/tests/line_stories.test.ts
@@ -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',
+      );
+    });
+  });
 });
diff --git a/packages/charts/src/chart_types/xy_chart/renderer/canvas/points.ts b/packages/charts/src/chart_types/xy_chart/renderer/canvas/points.ts
@@ -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;
 

diff --git a/packages/charts/src/chart_types/xy_chart/renderer/canvas/xy_chart.tsx b/packages/charts/src/chart_types/xy_chart/renderer/canvas/xy_chart.tsx
@@ -207,7 +207,6 @@ const DEFAULT_PROPS: ReactiveChartStateProps = {
     areas: [],
     bars: [],
     lines: [],
-    points: [],
     bubbles: [],
   },
   geometriesIndex: new IndexedGeometryMap(),

diff --git a/packages/charts/src/chart_types/xy_chart/rendering/area.ts b/packages/charts/src/chart_types/xy_chart/rendering/area.ts
@@ -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';
@@ -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,
@@ -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,

diff --git a/packages/charts/src/chart_types/xy_chart/rendering/line.ts b/packages/charts/src/chart_types/xy_chart/rendering/line.ts
@@ -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';
@@ -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,
@@ -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,
@@ -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 };
 }
diff --git a/packages/charts/src/chart_types/xy_chart/rendering/points.ts b/packages/charts/src/chart_types/xy_chart/rendering/points.ts
@@ -56,6 +56,7 @@ export function renderPoints(
   styleAccessor?: PointStyleAccessor,
 ): {
   pointGeometries: PointGeometry[];
+  minDistanceBetweenPoints: number;
   indexedGeometryMap: IndexedGeometryMap;
 } {
   const indexedGeometryMap = new IndexedGeometryMap();
@@ -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,
   };
 }

diff --git a/packages/charts/src/chart_types/xy_chart/state/utils/types.ts b/packages/charts/src/chart_types/xy_chart/state/utils/types.ts
@@ -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';
@@ -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>>;

diff --git a/packages/charts/src/chart_types/xy_chart/state/utils/utils.ts b/packages/charts/src/chart_types/xy_chart/state/utils/utils.ts
@@ -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';
@@ -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>> = [];
@@ -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) {
@@ -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,
@@ -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,