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

[Bug] Series-line is clipped incorrectly at the rightmost position #19051

Closed
RexSkz opened this issue Aug 29, 2023 · 3 comments
Closed

[Bug] Series-line is clipped incorrectly at the rightmost position #19051

RexSkz opened this issue Aug 29, 2023 · 3 comments
Labels
bug en This issue is in English pending We are not sure about whether this is a bug/new feature.

Comments

@RexSkz
Copy link
Contributor

RexSkz commented Aug 29, 2023

Version

5.4.3

Link to Minimal Reproduction

https://codepen.io/rexskz/pen/WNLweyw

Steps to Reproduce

  1. Open the codepen link.
  2. Look at the rightmost position of the line.

Current Behavior

It's clipped incorrectly, but the leftmost position of the line is okay.

image

Expected Behavior

The rightmost position should not be clipped, just like the leftmost one.

Environment

- OS: macOS Ventura
- Browser: Chrome 116.0.5845.110
- Framework: /

Any additional comments?

The root cause is that ECharts uses Math.round in createClipPathFromCoordSys.ts:

const rect = cartesian.getArea();
let width = rect.width;
width = Math.round(width);

The width here can be a float with the decimal part less than 0.5:

image

Finally, it results in the incorrect clipPath, just 1px smaller than expected:

image

This issue can only be reproduced in a container with a specific width (503px in the codepen link).

This issue might be related to #11369; considering it meant to extend the clipPath and use Math.floor on x, I think it's okay to use Math.ceil on width to ensure the clipPath is large enough to contain the whole line (with cap).

@RexSkz RexSkz added the bug label Aug 29, 2023
@echarts-bot echarts-bot bot added en This issue is in English pending We are not sure about whether this is a bug/new feature. labels Aug 29, 2023
@RexSkz
Copy link
Contributor Author

RexSkz commented Aug 29, 2023

It seems that using Math.ceil is not enough, the devicePixelRatio should be considered. The full code might be:

- const lineWidth = seriesModel.get(['lineStyle', 'width']) || 2;
+ const _lineWidth = seriesModel.get(['lineStyle', 'width']) || 2;

+ // I don't know where to find the dpr from `echarts.init`, so I
+ // use `window.devicePixelRatio` as example
+ const lineWidth = _lineWidth * window.devicePixelRatio;

  // Expand the clip path a bit to avoid the border is clipped and looks thinner
  x -= lineWidth / 2;
  y -= lineWidth / 2;
  width += lineWidth;
  height += lineWidth;

  // fix: https://github.com/apache/incubator-echarts/issues/11369
  x = Math.floor(x);
- width = Math.round(width);
+ width = Math.ceil(width);

@RexSkz
Copy link
Contributor Author

RexSkz commented Aug 30, 2023

I just found that DPR can be retrieved from zrender instance:

const zr = cartesian.model.ecModel.scheduler.ecInstance.getZr();
const dpr = zr.painter instanceof CanvasPainter
    ? zr.painter.dpr
    : window.devicePixelRatio; /* global window */

const _lineWidth = seriesModel.get(['lineStyle', 'width']) || 2;
const lineWidth = _lineWidth * (dpr || 1);

And after multiplying DPR, there are no such numbers as x.333, only x.00009 or x.99996, which are the round-off error. Keeping Math.round should be fine.

PR is on the way...

@RexSkz
Copy link
Contributor Author

RexSkz commented Sep 15, 2023

It seems that using Math.ceil is not enough, the devicePixelRatio should be considered. The full code might be:

- const lineWidth = seriesModel.get(['lineStyle', 'width']) || 2;
+ const _lineWidth = seriesModel.get(['lineStyle', 'width']) || 2;

+ // I don't know where to find the dpr from `echarts.init`, so I
+ // use `window.devicePixelRatio` as example
+ const lineWidth = _lineWidth * window.devicePixelRatio;

  // Expand the clip path a bit to avoid the border is clipped and looks thinner
  x -= lineWidth / 2;
  y -= lineWidth / 2;
  width += lineWidth;
  height += lineWidth;

  // fix: https://github.com/apache/incubator-echarts/issues/11369
  x = Math.floor(x);
- width = Math.round(width);
+ width = Math.ceil(width);

It's ok to use only Math.ceil, but the width should add an extra 1px if x is floored.

@Ovilia Ovilia closed this as completed in a57202d Sep 19, 2023
Ovilia added a commit that referenced this issue Sep 19, 2023
fix(clip): add an extra space to the clip-path width to prevent unexpected clip. close #19051
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug en This issue is in English pending We are not sure about whether this is a bug/new feature.
Projects
None yet
Development

No branches or pull requests

1 participant