View Issue Details

IDProjectCategoryView StatusLast Update
0034898LazarusTAChartpublic2019-01-19 03:16
ReporterMarcin Wiazowski Assigned Towp  
Status closedResolutionfixed 
Product Version2.1 (SVN) 
Target Version2.0 
Summary0034898: TAChart: improper calculations in TChart.YImageToGraph
DescriptionProblem described here is related to 0034819, but has been described separately here, to avoid mess in the original report.

In TAGraph.pas, there are two functions declared:

function TChart.XImageToGraph(AX: Integer): Double;
  if AX >= MAX_COORD then
    Result := Infinity * SgnInt(FScale.X)
  else if AX <= -MAX_COORD then
    Result := -Infinity * SgnInt(FScale.X)
    Result := (AX - FOffset.X) / FScale.X;

function TChart.YImageToGraph(AY: Integer): Double;
  if AY >= MAX_COORD then
    Result := Infinity * SgnInt(FScale.Y)
  else if AY <= -MAX_COORD then
    Result := +Infinity * SgnInt(FScale.Y)
    Result := (AY - FOffset.Y) / FScale.Y;

The idea is:
- for very large, positive values, return +INF,
- for very large, negative values, return -INF,
- in other cases perfrom the calculations.

There is a mistake in TChart.YImageToGraph: "+Infinity" should be changed to "-Infinity".

TagsNo tags attached.
Fixed in Revision60104, 60105
Attached Files



2019-01-18 23:03

developer   ~0113464

> There is a mistake in TChart.YImageToGraph: "+Infinity" should be changed to "-Infinity".

Why? On the y axis, FScale.Y is negative, and therefore, the expression

  Result := +Infinity * SgnInt(FScale.Y)

is negative.

Marcin Wiazowski

2019-01-18 23:32

reporter   ~0113465

Let's assume for the moment, that FOffset = 0 and that FScale = -2 (so it is negative).

Let's assume first, that AY is equal to -1000. In this case, it is in the -MAX_COORD .. MAX_COORD range, so result will be:

  Result := (AY - FOffset.Y) / FScale.Y


  Result := (-1000 - 0) / (-2)


  Result := 500

so Result is positive.

For AY = -1000000000, condition "AY <= -MAX_COORD" is met, so calculation currently is:

  Result := +Infinity * SgnInt(FScale.Y)


  Result := +Infinity * (-1)


  Result := -Infinity

so Result is negative, although it still should be positive.

Please note, that there is no such problem with AY = +1000 and AY = +1000000000 - both results will be negative in this case.


Marcin Wiazowski

2019-01-18 23:42

reporter   ~0113467

But I can see another problem now: Result's sign depends not only on FScale's sign, but also on FOffset value - in particular, (AY - FOffset.Y) may give either positive, or negative value. So functions should be like:

function TChart.XImageToGraph(AX: Integer): Double;
  if (AX < -MAX_COORD) or (AX > MAX_COORD) then
    Result := Infinity * SgnInt(AX - FOffset.X) * SgnInt(FScale.X)
    Result := (AX - FOffset.X) / FScale.X;

function TChart.YImageToGraph(AY: Integer): Double;
  if (AY < -MAX_COORD) or (AY > MAX_COORD) then
    Result := Infinity * SgnInt(AY - FOffset.Y) * SgnInt(FScale.Y)
    Result := (AY - FOffset.Y) / FScale.Y;


2019-01-19 00:29

developer   ~0113469

Last edited: 2019-01-19 00:33

View 2 revisions

No, too many thoughts for something which never will happen. These changes were introduced in r60075 where I fixed the bug that the zero level of an area series was suddenly drawn at the top of the chart when the axis was logarithmic - it should have been at -Inf because log(0) is -Inf. The coordinate transformation made this to MaxInt pixels which is correct. But the canvas does not seem to like coordinate > about 100 millions because the area was drawn inverted. From this reason I truncated the the pixel coordinate at the MAX_COORD which still works correctly. For the opposite conversion, screen to graph coordinate, I thought that the Inf value should be restored, but I already knew that this case will never happen: the screen coordinates will already be clipped somewhere before they become this large.

I think it's not worth to put many thoughts into this case, and I'll revert ImageToGraph to the former code (GraphToImage, of course, will remain changed).

And the coordinate transformations are called again and again, so we should not waste cycles in them.

Marcin Wiazowski

2019-01-19 00:36

reporter   ~0113470

Well, I don't have so impressive knowledge about TAChart's internals, so I can only agree.

Marcin Wiazowski

2019-01-19 03:16

reporter   ~0113473

Fixed in r60105.

Issue History

Date Modified Username Field Change
2019-01-18 21:54 Marcin Wiazowski New Issue
2019-01-18 23:01 wp Assigned To => wp
2019-01-18 23:01 wp Status new => assigned
2019-01-18 23:03 wp Note Added: 0113464
2019-01-18 23:13 wp LazTarget => -
2019-01-18 23:13 wp Status assigned => feedback
2019-01-18 23:32 Marcin Wiazowski Note Added: 0113465
2019-01-18 23:32 Marcin Wiazowski Status feedback => assigned
2019-01-18 23:42 Marcin Wiazowski Note Added: 0113467
2019-01-19 00:29 wp Note Added: 0113469
2019-01-19 00:32 wp Fixed in Revision => 60104, 60105
2019-01-19 00:32 wp LazTarget - => 2.0
2019-01-19 00:32 wp Status assigned => resolved
2019-01-19 00:32 wp Resolution open => fixed
2019-01-19 00:32 wp Target Version => 2.0
2019-01-19 00:33 wp Note Edited: 0113469 View Revisions
2019-01-19 00:36 Marcin Wiazowski Note Added: 0113470
2019-01-19 03:16 Marcin Wiazowski Note Added: 0113473
2019-01-19 03:16 Marcin Wiazowski Status resolved => closed