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(lines): discard extraneous data-points during lines animation #13638

Merged
merged 2 commits into from Mar 30, 2021

Conversation

vially
Copy link
Contributor

@vially vially commented Nov 17, 2020

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

Discards data-points that were erroneously added to the chart during the lines animation.

Fixed issues

There are multiple issues currently open about this problem and they might all be related.

Details

Add a test to easily reproduce the bug where points are erroneously added to the chart during the lines animation and make a naive attempt at fixing it.

Before: What was the problem?

This bug happens during line-animations and it makes the chart display some invalid data-points after the animation is complete.

These erroneous data-points seem to be transient and they seem to go away after some interactions with the chart (e.g.: by manually changing the data-zoom).

This is a screenshot of the bug from the added test case (there are only two points in the series so there should only be one segment):

erroneous_line

Reproduction link: https://jsfiddle.net/8kv71j0w/7/ (also see added test case in test/lines-extraneous.html).

After: How is it fixed in this PR?

Due to some items in the old series having been filtered out (e.g.: by the data-zoom component), the rawIndex of the removed items is different than the index. Therefore, this condition evaluates to true which results in this particular code path being executed. This in turn has the effect of keeping these removed points in the line during (and after) the animation.

The naive fix as part of this pull-request was to remove that code-block all together.

Warning: I don't have enough context to understand the full-implications of this change and it's very likely that this is not a good fix for this problem.

The main reason for opening this pull-request was to start a discussion and get some feedback from the developers about what would a proper fix look like.

That being said, this proposed change does seem to produce the expected result in the attached test case:

no_erroneous_line

Usage

Are there any API changes?

  • The API has been changed.

Related test cases or examples to use the new APIs

See test case in: test/lines-extraneous.html

Others

Merging options

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

Other information

See the linked issues for more details about other instances where this bug happens.

@echarts-bot
Copy link

echarts-bot bot commented Nov 17, 2020

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.

@ThomasBower
Copy link

+1 to this fix. We are seeing similar issues on line chart animations/zooming.

@pissang pissang added this to the 5.1.0 milestone Jan 25, 2021
@pissang pissang merged commit d80deef into apache:master Mar 30, 2021
@echarts-bot
Copy link

echarts-bot bot commented Mar 30, 2021

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

@pissang
Copy link
Contributor

pissang commented Mar 30, 2021

Thanks so much! Have to admit I'm very surprised this issue that bothers us for a long time can be fixed by simply removing this branch

@vially
Copy link
Contributor Author

vially commented Mar 30, 2021

Thank you for this awesome library!

I'm a little surprised too and to be honest I'm still a bit afraid of this change having unintended side-effects because I guess that branch must have been there for a reason.

In any case, let's see what happens and if it turns out it's a bad fix it can always be reverted.

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

Successfully merging this pull request may close these issues.

None yet

3 participants