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

feat(scatter): jittering for category data #19941

Open
wants to merge 8 commits into
base: next
Choose a base branch
from
Open
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
41 changes: 39 additions & 2 deletions src/chart/helper/SymbolDraw.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ import { ScatterSeriesOption } from '../scatter/ScatterSeries';
import { getLabelStatesModels } from '../../label/labelStyle';
import Element from 'zrender/src/Element';
import SeriesModel from '../../model/Series';
import { fixJitter, needFixJitter } from '../../util/jitter';
import type Axis2D from '../../coord/cartesian/Axis2D';
import type SingleAxis from '../../coord/single/SingleAxis';

interface UpdateOpt {
isIgnore?(idx: number): boolean
Expand Down Expand Up @@ -182,17 +185,51 @@ class SymbolDraw {
opt = normalizeUpdateOpt(opt);

const group = this.group;
const seriesModel = data.hostModel;
const seriesModel = data.hostModel as SeriesModel;
const oldData = this._data;
const SymbolCtor = this._SymbolCtor;
const disableAnimation = opt.disableAnimation;
const coord = seriesModel.coordinateSystem;
const baseAxis = coord.getBaseAxis ? coord.getBaseAxis() : null;
const hasJitter = baseAxis && needFixJitter(seriesModel, baseAxis);

const seriesScope = makeSeriesScope(data);

const symbolUpdateOpt = { disableAnimation };

const getSymbolPoint = opt.getSymbolPoint || function (idx: number) {
return data.getItemLayout(idx);
const layout = data.getItemLayout(idx);
const rawSize = data.getItemVisual(idx, 'symbolSize');
const size = rawSize instanceof Array ? (rawSize[1] + rawSize[0]) / 2 : rawSize;

// return layout
if (hasJitter) {
const dim = baseAxis.dim;
const orient = (baseAxis as SingleAxis).orient;
const isSingleY = orient === 'horizontal' && baseAxis.type !== 'category'
|| orient === 'vertical' && baseAxis.type === 'category';
if (dim === 'y' || dim === 'single' && isSingleY) {
// x is fixed, and y is floating
const jittered = fixJitter(
baseAxis as Axis2D | SingleAxis,
layout[0],
layout[1],
size / 2
);
return [layout[0], jittered];
}
else if (dim === 'x' || dim === 'single' && !isSingleY) {
// y is fixed, and x is floating
const jittered = fixJitter(
baseAxis as Axis2D | SingleAxis,
layout[1],
layout[0],
size / 2
);
return [jittered, layout[1]];
}
}
return layout;
};


Expand Down
3 changes: 3 additions & 0 deletions src/coord/axisCommonTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ export interface AxisBaseOptionCommon extends ComponentOption,
*/
max?: ScaleDataValue | 'dataMax' | ((extent: {min: number, max: number}) => ScaleDataValue);

jitter?: number;
Copy link
Member

@100pah 100pah Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • (readability issue): The meaning of this option jitter can't be presumed by the naming and no comment here.

  • I think we should better to support automatic calculation of the jitter boundary (radius) based on tick span of the axis? Otherwise, if the chart is resizable, or if their are multiple values of the category axis, users may be not able to find an appropriate pixel value of boundary radius. If using a wrong jitter value, some of the outcome coords might be inappropriate (e.g. a negative value or out of the cartesian and not drawn). If auto calculation is supported, users can simply configured it as

    • xAxis: { jitter: true } to get a perfect result, where jitter radius is auto calculated by tick span.
    • xAxis: { jitter: { radius: 400, overlap: false }} to set as 400px.
    • xAxis: { jitter: { radius: '80%', overlap: false }} to get 80% of the tick span.

jitterOverlap?: boolean;
jitterMargin?: number;
}

export interface NumericAxisBaseOptionCommon extends AxisBaseOptionCommon {
Expand Down
3 changes: 3 additions & 0 deletions src/coord/axisDefault.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,9 @@ const categoryAxis: AxisBaseOption = zrUtil.merge({
boundaryGap: true,
// Set false to faster category collection.
deduplication: null,
jitter: 0,
jitterOverlap: true,
jitterMargin: 2,
// splitArea: {
// show: false
// },
Expand Down
6 changes: 5 additions & 1 deletion src/coord/single/AxisModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,11 @@ class SingleAxisModel extends ComponentModel<SingleAxisOption>
type: 'dashed',
opacity: 0.2
}
}
},

jitter: 0,
jitterOverlap: true,
jitterMargin: 2,
};
}

Expand Down
129 changes: 129 additions & 0 deletions src/util/jitter.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
import type Axis from '../coord/Axis';
import type { AxisBaseModel } from '../coord/AxisBaseModel';
import Axis2D from '../coord/cartesian/Axis2D';
import type SingleAxis from '../coord/single/SingleAxis';
import type SeriesModel from '../model/Series';
import { makeInner } from './model';

export function needFixJitter(seriesModel: SeriesModel, axis: Axis): boolean {
const { coordinateSystem } = seriesModel;
const { type: coordType } = coordinateSystem;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A series instance is not necessarily has a coordinateSystem instance.
It would be better if adding a null checking here.

const baseAxis = coordinateSystem.getBaseAxis();
const { type: scaleType } = baseAxis.scale;
const seriesValid = coordType === 'cartesian2d'
&& (scaleType === 'category' || scaleType === 'ordinal')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scaleType can never be 'category'.
only ordinal, interval, log, time are possible.

|| coordType === 'single';

const axisValid = (axis.model as AxisBaseModel).get('jitter') > 0;
return seriesValid && axisValid;
}

export type JitterData = {
fixedCoord: number,
floatCoord: number,
r: number
};

const inner = makeInner<{ items: JitterData[] }, Axis2D | SingleAxis>();

/**
* Fix jitter for overlapping data points.
*
* @param fixedAxis The axis whose coord doesn't change with jitter.
* @param fixedCoord The coord of fixedAxis.
* @param floatCoord The coord of the other axis, which should be changed with jittering.
* @param radius The radius of the data point, considering the symbol is a circle.
* @returns updated floatCoord.
*/
export function fixJitter(
fixedAxis: Axis2D | SingleAxis,
fixedCoord: number,
floatCoord: number,
radius: number
): number {
if (fixedAxis instanceof Axis2D) {
const scaleType = fixedAxis.scale.type;
if (scaleType !== 'category' && scaleType !== 'ordinal') {
return floatCoord;
}
}
const axisModel = fixedAxis.model as AxisBaseModel;
const jitter = axisModel.get('jitter');
const jitterOverlap = axisModel.get('jitterOverlap');
const jitterMargin = axisModel.get('jitterMargin') || 0;
if (jitter > 0) {
if (jitterOverlap) {
return fixJitterIgnoreOverlaps(floatCoord, jitter);
}
else {
return fixJitterAvoidOverlaps(fixedAxis, fixedCoord, floatCoord, radius, jitter, jitterMargin);
}
}
return floatCoord;
}

function fixJitterIgnoreOverlaps(floatCoord: number, jitter: number): number {
return floatCoord + (Math.random() - 0.5) * jitter;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think also need to clamp with the coordinate system boundary, otherwise the invalid outcome coords will cause that some points can not be draw and hard to be discovered by developers. The same goes for the overlap processing.

}

function fixJitterAvoidOverlaps(
fixedAxis: Axis2D | SingleAxis,
fixedCoord: number,
floatCoord: number,
radius: number,
jitter: number,
margin: number
): number {
const store = inner(fixedAxis);
if (!store.items) {
store.items = [];
}
const items = store.items;

const floatA = placeJitterOnDirection(items, fixedCoord, floatCoord, radius, jitter, margin, 1);
const floatB = placeJitterOnDirection(items, fixedCoord, floatCoord, radius, jitter, margin, -1);
let minFloat = Math.abs(floatA - floatCoord) < Math.abs(floatB - floatCoord) ? floatA : floatB;
if (Math.abs(minFloat - floatCoord) > jitter / 2) {
// If the new item is moved too far, then give up.
// Fall back to random jitter.
minFloat = fixJitterIgnoreOverlaps(floatCoord, jitter);
}

items.push({ fixedCoord, floatCoord: minFloat, r: radius });
Copy link
Member

@100pah 100pah Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that implicitly adding a big array and pushing data item to the input fixedAxis is probably not a good practice and error-prone.

The instance of fixedAxis happens to be recreated each time going through the rending pipeline. But it's not necessarily like this. If someone change that behavior to reuse the axis instance, the implement here will lead to mistakes and not easy to be found.

Theoretically it would be better that a util function is a pure function (not modify any input data), especially when the name of the function appears to have that connotation according to convention. (sometimes a cache can break the principle but must be very careful about the memory issue.) (edit at Nov 13, 2024)
In this scenario I think there is anther inappropriate factor due to this code structure. It makes the time complexity of the algorithm O(n^2) (caused by `placeJitterOnDirection`). But I think it's supposed to be O(n). (edit at Nov 13, 2024)
So I think the jitter fixing should be performed at the layout phase, rather then in the final render phase. That it, should not be called in `SymbolDraw`, should better be called in some place like `layout/points.ts`. And in that layout phase, we can perform the jitter in or around the loop logic, preparing some auxiliary array before the loop in `placeJitterOnDirection` performed, and do not need to make it persistent. (edit at Nov 13, 2024)

(edit at Nov 13, 2024)
I misunderstood the algorithm earlier.
Considering multiple series, an axis based store is needed, as it's implemented currently. And O(n) seems not possible.

But there are suggestions about the code structure (in my personal opinion):

  • Instead of using inner to mount a store to the axis instance, declare the store property on axis explicitly to make it noticeable and comment that the lifetime of the store is a concern if some modification is needed in future. For example create some ts interface like

    interface JitterStorable {
        // some comment about the lifetime.
        jitterStore: JitterData[]
    }
    class Axis2D extends Axis, JitterStorable { /* ... */ }
    class SingleAxis extends Axis, JitterStorable { /* ... */ }
  • Prefer to add an new processor with the priority of registers.PRIORITY.VISUAL.POST_CHART_LAYOUT to perform the jitter, instead of currently doing it in SymbolDraw, where is literally and conventionally not a place to do layout jobs. And if some other components need the layout data, it's not easy to get the final accurate data if there will be modified by SymbolDraw.

return minFloat;
}

function placeJitterOnDirection(
items: JitterData[],
fixedCoord: number,
floatCoord: number,
radius: number,
jitter: number,
margin: number,
direction: 1 | -1
) {
// Check for overlap with previous items.
let y = floatCoord;
for (let i = 0; i < items.length; i++) {
const item = items[i];
const dx = fixedCoord - item.fixedCoord;
const dy = y - item.floatCoord;
const d2 = dx * dx + dy * dy;
const r = radius + item.r + margin;
if (d2 < r * r) {
// Overlap. Try to move the new item along otherCoord direction.
const newY = item.floatCoord + Math.sqrt(r * r - dx * dx) * direction;
if (direction > 0 && newY > y || direction < 0 && newY < y) {
y = newY;
i = 0; // Back to check from the first item.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if the items is ordered by floatCoord, the backtracking (i = 0) here is not necessary. With this backtracking, the entire algorithm could degraded to O(n^3) in the worst case. But without it, it can keep O(n^2).

Just have a try, to modify it (with a linked list and sort in ascending order by floatCoord):

export type JitterData = {
    fixedCoord: number;
    floatCoord: number;
    r: number;
    next: JitterData | null;
    prev: JitterData | null;
};

// Items is a circular linked list, in the ascending order by floatCoord.
const inner = makeInner<{ items: JitterData }, Axis2D | SingleAxis>();

function fixJitterAvoidOverlaps(
    fixedAxis: Axis2D | SingleAxis,
    fixedCoord: number,
    floatCoord: number,
    radius: number,
    jitter: number,
    margin: number
): number {
    const store = inner(fixedAxis);
    if (!store.items) {
        store.items = {
            fixedCoord: -1,
            floatCoord: -1,
            r: -1,
            next: null, // head of a link list
            prev: null, // tail of a link list
        };
        store.items.next = store.items;
        store.items.prev = store.items;
    }
    const items = store.items;

    const overlapA = placeJitterOnDirection(items, fixedCoord, floatCoord, radius, jitter, margin, 1);
    const overlapB = placeJitterOnDirection(items, fixedCoord, floatCoord, radius, jitter, margin, -1);
    const overlapResult = Math.abs(overlapA.resultCoord - floatCoord) < Math.abs(overlapB.resultCoord - floatCoord)
        ? overlapA : overlapB;
    let minFloat = overlapResult.resultCoord;
    if (Math.abs(minFloat - floatCoord) > jitter / 2) {
        // If the new item is moved too far, then give up.
        // Fall back to random jitter.
        minFloat = fixJitterIgnoreOverlaps(floatCoord, jitter);
    }

    // Insert to store
    const insertBy = overlapResult.insertBy;
    const resultDirection = overlapResult.direction;
    const pointer1 = resultDirection > 0 ? 'next' : 'prev';
    const pointer2 = resultDirection > 0 ? 'prev' : 'next';
    const newItem: JitterData = {
        fixedCoord: fixedCoord,
        floatCoord: overlapResult.resultCoord,
        r: radius,
        next: null,
        prev: null,
    };
    newItem[pointer1] = insertBy[pointer1];
    newItem[pointer2] = insertBy;
    insertBy[pointer1][pointer2] = newItem;
    insertBy[pointer1] = newItem;

    return minFloat;
}

function placeJitterOnDirection(
    items: JitterData,
    fixedCoord: number,
    floatCoord: number,
    radius: number,
    jitter: number,
    margin: number,
    direction: 1 | -1
): {
    resultCoord: number;
    insertBy: JitterData;
    direction: 1 | -1;
} {
    // Check for overlap with previous items.
    let y = floatCoord;
    const pointer1 = direction > 0 ? 'next' : 'prev';
    let insertBy = items;
    let item = items[pointer1];

    while (item !== items) {
        const dx = fixedCoord - item.fixedCoord;
        const dy = y - item.floatCoord;
        const d2 = dx * dx + dy * dy;
        const r = radius + item.r + margin;
        if (d2 < r * r) {
            // Overlap. Try to move the new item along otherCoord direction.
            y = item.floatCoord + Math.sqrt(r * r - dx * dx) * direction;
            insertBy = item;

            if (Math.abs(y - floatCoord) > jitter / 2) {
                // If the new item is moved too far, then give up.
                // Fall back to random jitter.
                return {resultCoord: Number.MAX_VALUE, insertBy, direction};
            }
        }

        item = item[pointer1];
    }

    return {resultCoord: y, insertBy, direction};
}

}

if (Math.abs(newY - floatCoord) > jitter / 2) {
// If the new item is moved too far, then give up.
// Fall back to random jitter.
return Number.MAX_VALUE;
}
}
}
return y;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering the size issue, should we consider that introduce this kind of features on demand rather than including it by default?
That is, support to import it by users manually.
I'm not sure yet 🤔.

Loading