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.

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

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

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 2 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

closed?: boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

I think loop (inspired by gl.LINE_LOOP of WebGL) may be a better name than closed because it may not always be closed due to the existance of null data.

}

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

clip: true,

closed: 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 closed = isCoordSysPolar && seriesModel.get('closed');

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, closed);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a better idea to define const closed = isCoordSysPolar && seriesModel.get('closed'); here. It will make the code more readable and postpone the execution until necessary.

if (isAreaChart) {
polygon = this._newPolygon(
points, stackedOnPoints
points, stackedOnPoints, closed
);
}// 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, closed
);
}
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>, closed: 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,
closed
},
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>, closed: 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,
closed,
stackedOnPoints: stackedOnPoints
},
segmentIgnoreThreshold: 2
Expand Down
28 changes: 23 additions & 5 deletions src/chart/line/poly.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ class ECPolylineShape {
smoothConstraint = true;
smoothMonotone: 'x' | 'y' | 'none';
connectNulls: boolean;
closed = false;
}

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

buildPath(ctx: PathProxy, shape: ECPolylineShape) {
const points = shape.points;
const points = shape.closed
? [
shape.points[shape.points.length - 2],
Copy link
Contributor

Choose a reason for hiding this comment

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

What if shape.points.length is less than 2?

Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to consider the situation when some of the data is null and when connectNulls is set to either true or false. Please add test cases.

shape.points[shape.points.length - 1],
...shape.points as number[],
shape.points[0],
shape.points[1]
]
: shape.points;

let i = 0;
let len = points.length / 2;
Expand All @@ -267,7 +276,7 @@ export class ECPolyline extends Path<ECPolylineProps> {
}
while (i < len) {
i += drawSegment(
ctx, points, i, len, len,
ctx, points, shape.closed ? i - 1 : i, len, shape.closed ? len - 1 : len,
1,
shape.smooth,
shape.smoothMonotone, shape.connectNulls
Expand Down Expand Up @@ -369,7 +378,15 @@ export class ECPolygon extends Path {
}

buildPath(ctx: PathProxy, shape: ECPolygonShape) {
const points = shape.points;
const points = shape.closed
? [
shape.points[shape.points.length - 2],
shape.points[shape.points.length - 1],
...shape.points as number[],
shape.points[0],
shape.points[1]
]
: shape.points;
const stackedOnPoints = shape.stackedOnPoints;

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