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

yassilah · 2022-10-01T14:10:28Z

Brief Information

This pull request is in the type of:

bug fixing
new feature
others

What does this PR do?

This PR allows to close a curve on a line series with a polar coord system.

Fixed issues

Details

Before: What was the problem?

After: How does it behave after the fixing?

Document Info

One of the following should be checked.

This PR doesn't relate to document changes
The document should be updated later
The document changes have been made in apache/echarts-doc

Misc

ZRender Changes

This PR depends on ZRender changes (ecomfe/zrender#xxx).

Related test cases or examples to use the new APIs

N.A.

Others

Merging options

Please squash the commits into a single one when merging.

Other information

echarts-bot · 2022-10-01T14:10:33Z

Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

Document changes are required in this PR. Please also make a PR to apache/echarts-doc for document changes and update the issue id in the PR description. When the doc PR is merged, the maintainers will remove the PR: awaiting doc label.

Ovilia

I think this would be a very useful feature.

Please also test the cases with areaStyle.

Ovilia · 2022-10-18T03:30:45Z

src/chart/line/LineView.ts

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

- polyline = this._newPolyline(points);
+ polyline = this._newPolyline(points, closed);


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.

Ovilia · 2022-10-18T03:34:17Z

src/chart/line/poly.ts

- const points = shape.points;
+ const points = shape.closed
+ ? [
+ shape.points[shape.points.length - 2],


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

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.

Ovilia · 2022-10-18T03:48:43Z

src/chart/line/LineSeries.ts

@@ -120,6 +120,8 @@ export interface LineSeriesOption extends SeriesOption<LineStateOption<CallbackD
 data?: (LineDataValue | LineDataItemOption)[]

 triggerLineEvent?: boolean
+
+ closed?: boolean


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.

yassilah · 2022-11-23T09:55:02Z

hi! sorry for the delay but I finally added some tests, they did help in avoiding some issues and avoiding code repetition so I hope it's enough :)

Ovilia · 2023-02-01T08:48:55Z

src/chart/line/LineView.ts

@@ -1028,7 +1030,8 @@ class LineView extends ChartView {

 polyline = new ECPolyline({
 shape: {
- points
+ points,
+ loop


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

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

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

yassilah · 2023-09-10T17:26:50Z

Hi, is there anything else I can do to get this PR merged?

Ovilia

Sorry for the late reply. I think the overall logic is fine. Just a small problem.

Ovilia · 2023-10-10T09:36:29Z

src/chart/line/poly.ts

@@ -211,12 +211,34 @@ function drawSegment(
 return k;
 }

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


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

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?

Ovilia

I would suggest using ECPolygon when loop so that you don't have to write extra code to make the looped array.

yassilah · 2023-11-16T09:52:51Z

Hi! So I got rid of the loop implementation for ECPolyline, which will use the ECPolygon buildPath method when loop: true instead. I tried to get rid of the getPoints function entirely and instead implement the logic in drawSegment directly to avoid unnecessary array manipulation but I actually can't because I still need it to work when smooth: true. So, instead I added a check to see whether the extra before/after points were necessary (when both loop and smooth are truthy), which should only trigger array manipulation when visually necessary. Hope that makes sense!

yassilah added 2 commits October 1, 2022 15:55

feat: allow closing a curve on polar line series

9b306ba

chore: remove pnpm lockfile

ba21d32

echarts-bot bot added PR: awaiting doc Document changes is required for this PR. PR: awaiting review labels Oct 1, 2022

pull-request-size bot added the size/M label Oct 1, 2022

Ovilia requested changes Oct 18, 2022

View reviewed changes

echarts-bot bot added PR: revision needed and removed PR: awaiting review labels Oct 18, 2022

Ovilia reviewed Oct 18, 2022

View reviewed changes

Ovilia added this to the 5.4.1 milestone Oct 18, 2022

Merge branch 'apache:master' into feat-closed-line-series-polar

2bb3988

echarts-bot bot added PR: awaiting review and removed PR: revision needed labels Nov 10, 2022

plainheart modified the milestones: 5.4.1, 5.5.0 Nov 14, 2022

yassilah added 2 commits November 23, 2022 09:01

fix: change to

f21cadf

test(line): add tests for loop option with various cases

ef5f29e

Merge branch 'apache:master' into feat-closed-line-series-polar

8bf567d

Ovilia requested changes Feb 1, 2023

View reviewed changes

echarts-bot bot added PR: revision needed and removed PR: awaiting review labels Feb 1, 2023

Ovilia requested changes Oct 10, 2023

View reviewed changes

Merge branch 'apache:master' into feat-closed-line-series-polar

0c1fb9e

echarts-bot bot added PR: awaiting review and removed PR: revision needed labels Oct 10, 2023

yassilah added 2 commits October 10, 2023 12:55

fix: clean code and skip process if no loop required

0653a08

fix: remove unnecessary if statement

175d8cd

yassilah mentioned this pull request Oct 12, 2023

docs(line-series): add loop option apache/echarts-doc#366

Open

echarts-bot bot added PR: doc ready and removed PR: awaiting doc Document changes is required for this PR. labels Oct 12, 2023

Ovilia requested changes Nov 16, 2023

View reviewed changes

echarts-bot bot added PR: revision needed and removed PR: awaiting review labels Nov 16, 2023

refactor(poly): use ECPolygon when Polyline is looped

9cf4c8c

echarts-bot bot added PR: awaiting review and removed PR: revision needed labels Nov 16, 2023

yassilah added 2 commits November 16, 2023 10:43

refactor(poly): getLoopedPoints

60c8e93

refactor(poly): getSmoothableLoopedPoints

8c2a932

Ovilia modified the milestones: 5.5.0, 5.5.1 Jan 18, 2024

Ovilia modified the milestones: 5.5.1, 5.5.2 Jun 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

yassilah commented Oct 1, 2022 •

edited

Loading

echarts-bot bot commented Oct 1, 2022

Ovilia left a comment

Ovilia Oct 18, 2022

Ovilia Oct 18, 2022

Ovilia Oct 18, 2022

Ovilia Oct 18, 2022

yassilah commented Nov 23, 2022

Ovilia Feb 1, 2023

yassilah Feb 1, 2023

Ovilia Feb 2, 2023

yassilah commented Sep 10, 2023

Ovilia left a comment

Ovilia Oct 10, 2023

yassilah Oct 10, 2023

Ovilia left a comment

yassilah commented Nov 16, 2023

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

Are you sure you want to change the base?

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

Conversation

yassilah commented Oct 1, 2022 • edited Loading

Brief Information

What does this PR do?

Fixed issues

Details

Before: What was the problem?

After: How does it behave after the fixing?

Document Info

Misc

ZRender Changes

Related test cases or examples to use the new APIs

Others

Merging options

Other information

echarts-bot bot commented Oct 1, 2022

Ovilia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yassilah commented Nov 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yassilah commented Sep 10, 2023

Ovilia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ovilia left a comment

Choose a reason for hiding this comment

yassilah commented Nov 16, 2023

yassilah commented Oct 1, 2022 •

edited

Loading