Skip to content

feat: allow closing a curve on polar line series #17720

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.

Sign up for GitHub

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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

feat: allow closing a curve on polar line series #17720

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9b306ba
feat: allow closing a curve on polar line series
yassilah Oct 1, 2022
ba21d32
chore: remove pnpm lockfile
yassilah Oct 1, 2022
2bb3988
Merge branch 'apache:master' into feat-closed-line-series-polar
yassilah Nov 10, 2022
f21cadf
fix: change to
yassilah Nov 23, 2022
ef5f29e
test(line): add tests for loop option with various cases
yassilah Nov 23, 2022
8bf567d
Merge branch 'apache:master' into feat-closed-line-series-polar
yassilah Jan 22, 2023
0c1fb9e
Merge branch 'apache:master' into feat-closed-line-series-polar
yassilah Oct 10, 2023
0653a08
fix: clean code and skip process if no loop required
yassilah Oct 10, 2023
175d8cd
fix: remove unnecessary if statement
yassilah Oct 10, 2023
9cf4c8c
refactor(poly): use ECPolygon when Polyline is looped
yassilah Nov 16, 2023
60c8e93
refactor(poly): getLoopedPoints
yassilah Nov 16, 2023
8c2a932
refactor(poly): getSmoothableLoopedPoints
yassilah Nov 16, 2023
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
4 changes: 4 additions & 0 deletions src/chart/line/LineSeries.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,8 @@ export interface LineSeriesOption extends SeriesOption<LineStateOption<CallbackD
data?: (LineDataValue | LineDataItemOption)[]

triggerLineEvent?: boolean

loop?: boolean
}

class LineSeriesModel extends SeriesModel<LineSeriesOption> {
Expand Down Expand Up @@ -152,6 +154,8 @@ class LineSeriesModel extends SeriesModel<LineSeriesOption> {

clip: true,

loop: false,

label: {
position: 'top'
},
Expand Down
16 changes: 10 additions & 6 deletions src/chart/line/LineView.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,8 @@ class LineView extends ChartView {

const connectNulls = seriesModel.get('connectNulls');

const loop = isCoordSysPolar && seriesModel.get('loop');

const isIgnoreFunc = showSymbol && !isCoordSysPolar
&& getIsIgnoreFunc(seriesModel, data, coordSys as Cartesian2D);

Expand Down Expand Up @@ -725,10 +727,10 @@ class LineView extends ChartView {
}
}

polyline = this._newPolyline(points);
polyline = this._newPolyline(points, loop);
if (isAreaChart) {
polygon = this._newPolygon(
points, stackedOnPoints
points, stackedOnPoints, loop
);
}// If areaStyle is removed
else if (polygon) {
Expand All @@ -749,7 +751,7 @@ class LineView extends ChartView {
if (isAreaChart && !polygon) {
// If areaStyle is added
polygon = this._newPolygon(
points, stackedOnPoints
points, stackedOnPoints, loop
);
}
else if (polygon && !isAreaChart) {
Expand Down Expand Up @@ -1019,7 +1021,7 @@ class LineView extends ChartView {
polygon && setStatesFlag(polygon, toState);
}

_newPolyline(points: ArrayLike<number>) {
_newPolyline(points: ArrayLike<number>, loop: boolean) {
let polyline = this._polyline;
// Remove previous created polyline
if (polyline) {
Expand All @@ -1028,7 +1030,8 @@ class LineView extends ChartView {

polyline = new ECPolyline({
shape: {
points
points,
loop
Copy link
Contributor

@Ovilia Ovilia Feb 1, 2023

Choose a reason for hiding this comment

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

Can ECPolygon be used if loop is true? If so, you don't have to implement the loop logic.

Copy link
Contributor Author

@yassilah yassilah Feb 1, 2023

Choose a reason for hiding this comment

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

Yes it can, what do you mean I don't have to implement it?

Copy link
Contributor

@Ovilia Ovilia Feb 2, 2023

Choose a reason for hiding this comment

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

Yes. I think if it passes the test, you can use ECPolygon and remove getPoints.

},
segmentIgnoreThreshold: 2,
z2: 10
Expand All @@ -1041,7 +1044,7 @@ class LineView extends ChartView {
return polyline;
}

_newPolygon(points: ArrayLike<number>, stackedOnPoints: ArrayLike<number>) {
_newPolygon(points: ArrayLike<number>, stackedOnPoints: ArrayLike<number>, loop: boolean) {
let polygon = this._polygon;
// Remove previous created polygon
if (polygon) {
Expand All @@ -1051,6 +1054,7 @@ class LineView extends ChartView {
polygon = new ECPolygon({
shape: {
points,
loop,
stackedOnPoints: stackedOnPoints
},
segmentIgnoreThreshold: 2
Expand Down
34 changes: 28 additions & 6 deletions src/chart/line/poly.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,12 +211,34 @@ function drawSegment(
return k;
}

function getPoints(shape: ECPolygonShape|ECPolylineShape) {
let points = Array.from(shape.points);
Copy link
Contributor

@Ovilia Ovilia Oct 10, 2023

Choose a reason for hiding this comment

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

Please use shape.points.slice instead of Array.from. BTW, if shape.loop is false, you can just return shape.points to improve efficiency.

Copy link
Contributor Author

@yassilah yassilah Oct 10, 2023

Choose a reason for hiding this comment

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

One small thing: the type of ECPolygonShape|ECPolylineShape is ArrayLike<number> so the slice method doesn't exist as it's not a regular array (that's why i used Array.from in the first place). Is it still safe to just force it to be inferred as an array?


if (shape.loop) {
if (shape.connectNulls) {
const nonNull: number[] = [];
for (let i = 0; i < points.length; i += 2) {
if (!isPointNull(points[i], points[i + 1])) {
nonNull.push(points[i], points[i + 1]);
}
}
points = [nonNull[nonNull.length - 2], nonNull[nonNull.length - 1], ...points, nonNull[0], nonNull[1]];
}
else {
points = [points[points.length - 2], points[points.length - 1], ...points, points[0], points[1]];
}
}

return points;
}

class ECPolylineShape {
points: ArrayLike<number>;
smooth = 0;
smoothConstraint = true;
smoothMonotone: 'x' | 'y' | 'none';
connectNulls: boolean;
loop = false;
}

interface ECPolylineProps extends PathProps {
Expand Down Expand Up @@ -245,8 +267,7 @@ export class ECPolyline extends Path<ECPolylineProps> {
}

buildPath(ctx: PathProxy, shape: ECPolylineShape) {
const points = shape.points;

const points = getPoints(shape);
let i = 0;
let len = points.length / 2;

Expand All @@ -267,7 +288,7 @@ export class ECPolyline extends Path<ECPolylineProps> {
}
while (i < len) {
i += drawSegment(
ctx, points, i, len, len,
ctx, points, shape.loop ? i - 1 : i, len, shape.loop ? len - 1 : len,
1,
shape.smooth,
shape.smoothMonotone, shape.connectNulls
Expand Down Expand Up @@ -369,7 +390,7 @@ export class ECPolygon extends Path {
}

buildPath(ctx: PathProxy, shape: ECPolygonShape) {
const points = shape.points;
const points = getPoints(shape);
const stackedOnPoints = shape.stackedOnPoints;

let i = 0;
Expand All @@ -391,13 +412,14 @@ export class ECPolygon extends Path {
}
while (i < len) {
const k = drawSegment(
ctx, points, i, len, len,
ctx, points,
shape.loop ? i - 1 : i, len, shape.loop ? len - 1 : len,
1,
shape.smooth,
smoothMonotone, shape.connectNulls
);
drawSegment(
ctx, stackedOnPoints, i + k - 1, k, len,
ctx, stackedOnPoints, i + k - 1 - (shape.loop ? 1 : 0), k, shape.loop ? len - 1 : len,
-1,
shape.stackedOnSmooth,
smoothMonotone, shape.connectNulls
Expand Down
Loading