TAChart: improper calculations in TAxisCoeffHelper.CalcScale
Original Reporter info from Mantis: Marcin Wiazowski
-
Reporter name:
Original Reporter info from Mantis: Marcin Wiazowski
- Reporter name:
Description:
Problems described here are related to #34819 (closed), but have been described separately here, to avoid mess in the original report.
Let's make some initial notes first. When Chart.BottomAxis and Chart.LeftAxis are to be drawn, two transformation coefficients must be calculated for each axis: Chart.FScale and Chart.FOffset. They are used to convert coordinates in pixels into coordinates in axis units - just by using an usual linear equation:
AxisValue := (PixelValue - Chart.FOffset) / Chart.FScale
which can be also written as:
PixelValue := Chart.FScale * AxisValue + Chart.FOffset
By default, horizontal axis' values grow along with pixel numbers (axis is directed from left to right, and image coordinates also grow from left to right). As a consequence, Chart.FScale.X is > 0.
By default, vertical axis' values grow in opposite direction to pixel numbers (axis is directed from bottom to top, but image coordinates grow from top to bottom). As a consequence, Chart.FScale.Y is < 0.
This must be inverted when Axis.IsFlipped is set: in this case, Chart.FScale must be changed to -Chart.FScale.
So we can now easily determine, if axis values should grow along with pixel numbers (so Chart.FScale is > 0) or rather in opposite direction (so Chart.FScale is < 0):
FAxisIsFlipped := FAxis.IsFlipped xor AxisIsVertical
Assuming, that we just calculated the Chart.FScale value (it will be always positive initially, due to the calculation algorithm), we must do the following:
Chart.FScale := ...
if FAxisIsFlipped then
Chart.FScale := -Chart.FScale;
================
Now let's take a look at the CalcScale function:
function TAxisCoeffHelper.CalcScale(ASign: Integer): Double;
begin
if (FMax^ = FMin^) or (Sign(FHi - FLo) <> ASign) then exit(1.0);
if (FAxis <> nil) and FAxis.IsFlipped then
Exchange(FLo, FHi);
Result := (FHi - FLo) / (FMax^ - FMin^);
end;
In the current implementation, there is no FAxisIsFlipped variable - its functionality is split between reading FAxis.IsFlipped property, and using ASign as an axis direction information (ASign = 1 for horizontal axis and ASign = -1 for vertical axis).
First, we can easily verify, that the only purpose of the "Exchange(FLo, FHi)" clause is to change sign of the result. Although FLo and FHi variables are still used after the CalcScale call, only their sum is calculated - so not exchanging them is not a problem. So we can transform the code into:
function TAxisCoeffHelper.CalcScale(ASign: Integer): Double;
begin
if (FMax^ = FMin^) or (Sign(FHi - FLo) <> ASign) then exit(1.0);
Result := (FHi - FLo) / (FMax^ - FMin^);
if (FAxis <> nil) and FAxis.IsFlipped then
Result := -Result;
end;
Note: as we can see here, sign of the Result is changed only when FAxis.IsFlipped = True, but NOT when the axis is vertical (ASign = -1) - this may seem wrong, but it is NOT a bug - instead of multiplying Result by ASign, the FHi and FLo variables are initialized inversely for vertical axis, so the "(FHi - FLo)" operation has already its sign changed.
Now let's take a look at the case, when "(FMax^ = FMin^) or (Sign(FHi - FLo) <> ASign)" - there are following problems here:
a) The returned result is always equal to 1.0 - i.e. positive! It neither depends on FAxis.IsFlipped value, nor on ASign value. As a consequence, axis values will get reverse order - this happens for horizontal axis with IsFlipped set, and for vertical axis with IsFlipped not set, when chart margins are so big, or chart window became so small, that there is no room for painting the series.
b) FMax^ and FMin^ are floating point values, effectively read from FCurrentExtent; FCurrentExtent is calculated, and - due to finite precision of floating point operations - there may be a case, when FMax^ has lower value than FMin^, because it differs at the 15th decimal point - for example when FMax^ = 1 and FMin^ = 1.000000000000001; in this case, the FMax^ = FMin^ condition will NOT be met. Since both FMax^ = FMin^ and FMax^ < FMin^ cases are invalid for extent ranges, the best choice is to change the condition to "(FMax^ <= FMin^)". This is still fully valid, and also protects us from the finite precision of floating point operations.
So, the contition should finally be: "if (FMax^ <= FMin^) or (Sign(FHi - FLo) <> ASign) then exit(IfThen((FAxis <> nil) and FAxis.IsFlipped, -ASign, ASign)"
function TAxisCoeffHelper.CalcScale(ASign: Integer): Double;
begin
if (FMax^ <= FMin^) or (Sign(FHi - FLo) <> ASign) then
exit(IfThen((FAxis <> nil) and FAxis.IsFlipped, -ASign, ASign);
Result := (FHi - FLo) / (FMax^ - FMin^);
if (FAxis <> nil) and FAxis.IsFlipped then
Result := -Result;
end;
We can still improve the readability:
function TAxisCoeffHelper.CalcScale(ASign: Integer): Double;
begin
if (FMax^ <= FMin^) or (Sign(FHi - FLo) <> ASign) then
Result := ASign
else
Result := (FHi - FLo) / (FMax^ - FMin^);
if (FAxis <> nil) and FAxis.IsFlipped then
Result := -Result;
end;
================
There is also one more, related issue in TChart.Create: vertical axis should have negative initial FScale.Y value (since it's not flipped by default), so the FScale should be initialized as:
FScale := DoublePoint(1, -1);
not as:
FScale := DoublePoint(1, 1);
================
By adjusting the code a bit more, we can make it fully clear and understandable. Since it's much easier to explain this with formatted code, I attached a PDF to this bug report.
The attached patch.diff makes all the changes described here and in the PDF.
Regards