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

Avoid passing NaN to CoreGraphics API (Fixes #1626) #2568

Merged
merged 1 commit into from
Aug 7, 2019

Conversation

chiahan1123
Copy link
Contributor

Fix issue #1626
The Highlight object generated in the ChartViewBase class contains a Double.NaN y-value when calling the highlightValue functions that does not require a y-value.

In the LineChartRenderer class, the drawHighlighted function passes the NaN y-value into the Transformer pixelForValues to apply the valueToPixelMatrix, which will result in the x-value being a NaN; therefore, causing the LineScatterCandleRadarRenderer drawHighlightLines to move to a CGPoint with a NaN x-value.

To fix this, the y-value being passed into pixelForValues should be from the entryForXValue.

@codecov-io
Copy link

codecov-io commented Jun 29, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@6b730a0). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2568   +/-   ##
=========================================
  Coverage          ?   19.65%           
=========================================
  Files             ?      112           
  Lines             ?    13717           
  Branches          ?        0           
=========================================
  Hits              ?     2696           
  Misses            ?    11021           
  Partials          ?        0
Impacted Files Coverage Δ
Source/Charts/Renderers/LineChartRenderer.swift 55.72% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b730a0...3aa542a. Read the comment docs.

@jjatie
Copy link
Collaborator

jjatie commented Jan 12, 2018

Thank you for the description! I'd like to have this reviewed this this week.

I'm unclear as to why we should be using the y value from entryForXValue instead of the one from the Highlight. @chiahan1123 @liuxuan30 is it safe to assume that let e = set.entryForXValue(high.x, closestToY: high.y) will always give the coordinates we want? I haven't looked into highlighting much, but it seems like this should be true. If I can get this verified, I can merge right away.

@liuxuan30
Copy link
Member

liuxuan30 commented Jan 15, 2018

I think we should first figure out why there is NaN during the process, rather than bypass it.

Any NaN directly reflects something wrong with the data, or the interval(min/max value) we calculated based on the data, otherwise it should be safe.

NaN generated by user's invalid data should not be our responsibilities, they should be warned. The interval/min/max value we have are ours, like the one we merged: #2377

BTW, will #2377 fix your issues? @chiahan1123

Copy link
Member

@liuxuan30 liuxuan30 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rejecting for the moment.
We should know what's causing NaN

@liuxuan30
Copy link
Member

liuxuan30 commented Aug 7, 2019

Hi there, sorry it took so long to realize this is a good catch! Thank you!

@liuxuan30 liuxuan30 merged commit 8a98ee2 into ChartsOrg:master Aug 7, 2019
Copy link
Member

@liuxuan30 liuxuan30 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approve

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

Successfully merging this pull request may close these issues.

4 participants