100pah commented on code in PR #19941:
URL: https://github.com/apache/echarts/pull/19941#discussion_r1836902326
##########
src/util/jitter.ts:
##########
@@ -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;
+ const baseAxis = coordinateSystem.getBaseAxis();
+ const { type: scaleType } = baseAxis.scale;
+ const seriesValid = coordType === 'cartesian2d'
+ && (scaleType === 'category' || scaleType === 'ordinal')
+ || 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;
+}
+
+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 });
Review Comment:
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.
<del>
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.)
</del>
(edit at Nov 13, 2024)
---
<del>
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).
</del>
(edit at Nov 13, 2024)
---
<del>
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.
</del>
(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
```ts
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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
