feat: allow closing a curve on polar line series by yassilah · Pull Request #17720 · apache/echarts

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

            }

-            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


    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

            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

 }

+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!

Ovilia

Sorry for the late reply!

Ovilia · 2024-11-08T02:41:07Z

src/chart/line/poly.ts

+        points = nonNull;
+    }
+
+    return [points[points.length - 2], points[points.length - 1], ...points, points[0], points[1]];


What if points.length is less than 5?

Ovilia · 2024-11-08T02:43:08Z

src/chart/line/poly.ts

    buildPath(ctx: PathProxy, shape: ECPolylineShape) {
-        const points = shape.points;
+        if (shape.loop) {
+            return ECPolygon.prototype.buildPath.call(this, ctx, {


I didn't mean implement loop inside Polyline by calling Polygon, because this will cause dependency on the later. Instead, we should decide to call Polygon or Polyline in LineView.

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

yassilah requested a review from Ovilia October 23, 2024 12:35

Ovilia requested changes Nov 8, 2024

View reviewed changes

Ovilia modified the milestones: 5.5.2, 6.0.0 Nov 8, 2024

Ovilia added PR: revision needed and removed PR: awaiting review labels Jun 20, 2025

Ovilia modified the milestones: 6.0.0, 6.x Jun 20, 2025

Conversation

yassilah commented Oct 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

echarts-bot bot commented Oct 1, 2022

Uh oh!

Ovilia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yassilah commented Nov 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yassilah commented Sep 10, 2023

Uh oh!

Ovilia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ovilia left a comment

Choose a reason for hiding this comment

Uh oh!

yassilah commented Nov 16, 2023

Uh oh!

Ovilia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yassilah commented Oct 1, 2022 •

edited

Loading