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.

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

fix(splitLine): fix chart throws errors when splitLine is enabled in radius axis. #16736

Merged
merged 1 commit into from
Mar 24, 2022

Conversation

plainheart
Copy link
Member

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

The chart throws errors when splitLine is enabled in the radius axis, resolves #16731.

Fixed issues

Details

Before: What was the problem?

It will throw illegal argument errors as zrender basic shape Circle doesn't handle the value less than 0, unlike the Sector shape, it normalizes such an unexpected radius value.

After: How is it fixed in this PR?

Ensure the circle radius is non-negative in ECharts.

Comparison

image

Misc

  • The API has been changed (apache/echarts-doc#xxx).
  • This PR depends on ZRender changes (ecomfe/zrender#xxx).

Related test cases or examples to use the new APIs

Please refer to test/splitLine.html

Others

Merging options

  • Please squash the commits into a single one when merging.

Other information

@echarts-bot
Copy link

echarts-bot bot commented Mar 24, 2022

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.

The pull request is marked to be PR: author is committer because you are a committer of this project.

@pissang pissang added this to the 5.3.2 milestone Mar 24, 2022
@plainheart plainheart merged commit c2f4ac6 into master Mar 24, 2022
@plainheart plainheart deleted the fix-radius-splitLine branch March 24, 2022 05:38
@echarts-bot
Copy link

echarts-bot bot commented Mar 24, 2022

Congratulations! Your PR has been merged. Thanks for your contribution! 👍

@jiawulin001
Copy link
Member

jiawulin001 commented Mar 24, 2022

@plainheart @pissang Hi, good to see you've found a solution. Actually I have another thought about where the real problem is. At:

echarts/src/coord/Axis.ts

Lines 311 to 313 in ad3a07a

each(ticksCoords, function (ticksItem) {
ticksItem.coord -= shift / 2;
});

The statement is dealing with tickItem.coord without judging the sign of it. In the case of radius axis, ticksItem.coord is equal to shift/2, but a very tiny error is introduced by binary storage when executing tickItem.coord -= shift/2. This makes ticksItem.coord a negative value when the error is negative, and BOOM, the painter won't accept a negative radius. This is why #16731 is happening.
The current solution only makes sure tickCoords.coord is positive when creating a new circle at:
splitLines[colorIndex].push(new graphic.Circle({
shape: {
cx: polar.cx,
cy: polar.cy,
// ensure circle radius >= 0
r: Math.max(ticksCoords[i].coord, 0)
}
}));

But there are other places where tickCoords.coord is used like:
let prevRadius = ticksCoords[0].coord;

splitAreas[colorIndex].push(new graphic.Sector({
shape: {
cx: polar.cx,
cy: polar.cy,
r0: prevRadius,
r: ticksCoords[i].coord,
startAngle: 0,
endAngle: Math.PI * 2
},

prevRadius = ticksCoords[i].coord;

This can cause the same problem in the future.

So my suggestion would be:
Adding the one line in current PR is not enough, but instead a line should be added to

echarts/src/coord/Axis.ts

Lines 311 to 313 in ad3a07a

each(ticksCoords, function (ticksItem) {
ticksItem.coord -= shift / 2;
});

and makes it look like:

        each(ticksCoords, function (ticksItem) {
            ticksItem.coord -= shift / 2;
            ticksItem.coord = Math.max(ticksItem.coord, 0)
        });

Please tell me if I got anything wrong or unclear.

@plainheart
Copy link
Member Author

@jiawulin001 So much thanks for your deep dig. I've mentioned a bit in the MR description what you are concerned about [1]. That's why I didn't apply the changes to other places.

[1]

It will throw illegal argument errors as zrender basic shape Circle doesn't handle the value less than 0, unlike the Sector shape, it normalizes such an unexpected radius value.

@jiawulin001
Copy link
Member

@plainheart Thanks for your explanation! Literally a lesson to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]极坐标柱状图半径多次改变后报错
3 participants