View Issue Details

IDProjectCategoryView StatusLast Update
0034896LazarusTAChartpublic2019-02-25 19:11
ReporterMarcin WiazowskiAssigned Towp 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version2.1 (SVN)Product Build60103 
Target Version2.0.2Fixed in Version 
Summary0034896: TAChart: improper calculations in TAxisCoeffHelper.CalcScale
DescriptionProblems described here are related to 0034819, 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
TagsNo tags attached.
Fixed in Revisionin v2.0: 60129 and 60309, in v2.02: 60311, 60334
LazTarget2.0.2
WidgetsetWin32/Win64
Attached Files
  • Calculations.pdf (20,896 bytes)
  • patch.diff (3,438 bytes)
    Index: components/tachart/tachartaxis.pas
    ===================================================================
    --- components/tachart/tachartaxis.pas	(revision 60103)
    +++ components/tachart/tachartaxis.pas	(working copy)
    @@ -244,15 +244,15 @@
       { TAxisCoeffHelper }
     
       TAxisCoeffHelper = object
    -    FAxis: TChartAxis;
    +    FAxisIsFlipped: Boolean;
         FImageLo, FImageHi: Integer;
         FLo, FHi: Integer;
         FMin, FMax: PDouble;
         function CalcOffset(AScale: Double): Double;
    -    function CalcScale(ASign: Integer): Double;
    +    function CalcScale: Double;
         constructor Init(
           AAxis: TChartAxis; AImageLo, AImageHi, AMarginLo, AMarginHi: Integer;
    -      AMin, AMax: PDouble);
    +      AMin, AMax: PDouble; AAxisIsVertical: Boolean);
         procedure UpdateMinMax(AConv: TAxisConvFunc);
       end;
     
    @@ -1217,9 +1217,9 @@
     
     constructor TAxisCoeffHelper.Init(
       AAxis: TChartAxis; AImageLo, AImageHi, AMarginLo, AMarginHi: Integer;
    -  AMin, AMax: PDouble);
    +  AMin, AMax: PDouble; AAxisIsVertical: Boolean);
     begin
    -  FAxis := AAxis;
    +  FAxisIsFlipped := ((AAxis <> nil) and AAxis.IsFlipped) xor AAxisIsVertical;
       FImageLo := AImageLo;
       FImageHi := AImageHi;
       FMin := AMin;
    @@ -1228,17 +1228,19 @@
       FHi := FImageHi + AMarginHi;
     end;
     
    -function TAxisCoeffHelper.CalcScale(ASign: Integer): Double;
    +function TAxisCoeffHelper.CalcScale: 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^);
    +  if (FMax^ <= FMin^) or (FHi <= FLo) then
    +    Result := 1
    +  else
    +    Result := (FHi - FLo) / (FMax^ - FMin^);
    +  if FAxisIsFlipped then
    +    Result := -Result;
     end;
     
     function TAxisCoeffHelper.CalcOffset(AScale: Double): Double;
     begin
    -  Result := (FLo + FHi) / 2 - AScale * (FMin^ + FMax^) / 2;
    +  Result := ((FLo + FHi) - AScale * (FMin^ + FMax^)) * 0.5;
     end;
     
     procedure TAxisCoeffHelper.UpdateMinMax(AConv: TAxisConvFunc);
    @@ -1245,7 +1247,7 @@
     begin
       FMin^ := AConv(FImageLo);
       FMax^ := AConv(FImageHi);
    -  if (FAxis <> nil) and FAxis.IsFlipped then
    +  if FAxisIsFlipped then
         Exchange(FMin^, FMax^);
     end;
     
    Index: components/tachart/tagraph.pas
    ===================================================================
    --- components/tachart/tagraph.pas	(revision 60103)
    +++ components/tachart/tagraph.pas	(working copy)
    @@ -583,12 +583,12 @@
     begin
       rX.Init(
         BottomAxis, FClipRect.Left, FClipRect.Right, AMargin.Left, -AMargin.Right,
    -    @FCurrentExtent.a.X, @FCurrentExtent.b.X);
    +    @FCurrentExtent.a.X, @FCurrentExtent.b.X, False);
       rY.Init(
    -    LeftAxis, FClipRect.Bottom, FClipRect.Top, -AMargin.Bottom, AMargin.Top,
    -    @FCurrentExtent.a.Y, @FCurrentExtent.b.Y);
    -  FScale.X := rX.CalcScale(1);
    -  FScale.Y := rY.CalcScale(-1);
    +    LeftAxis, FClipRect.Top, FClipRect.Bottom, AMargin.Top, -AMargin.Bottom,
    +    @FCurrentExtent.a.Y, @FCurrentExtent.b.Y, True);
    +  FScale.X := rX.CalcScale;
    +  FScale.Y := rY.CalcScale;
       if Proportional then begin
         if Abs(FScale.X) > Abs(FScale.Y) then
           FScale.X := Abs(FScale.Y) * Sign(FScale.X)
    @@ -693,7 +693,7 @@
       FDefaultGUIConnector.CreateDrawer(FConnectorData);
       FGUIConnectorListener := TListener.Create(@FGUIConnector, @StyleChanged);
     
    -  FScale := DoublePoint(1, 1);
    +  FScale := DoublePoint(1, -1);
     
       Width := DEFAULT_CHART_WIDTH;
       Height := DEFAULT_CHART_HEIGHT;
    
    patch.diff (3,438 bytes)
  • Animation.gif (70,761 bytes)
    Animation.gif (70,761 bytes)
  • Reproduce.zip (3,462 bytes)
  • Reproduce2.zip (3,811 bytes)
  • patch_extended.diff (8,088 bytes)
    Index: components/tachart/tachartaxis.pas
    ===================================================================
    --- components/tachart/tachartaxis.pas	(revision 60116)
    +++ components/tachart/tachartaxis.pas	(working copy)
    @@ -244,15 +244,17 @@
       { TAxisCoeffHelper }
     
       TAxisCoeffHelper = object
    -    FAxis: TChartAxis;
    +    FAxisIsFlipped: Boolean;
         FImageLo, FImageHi: Integer;
         FLo, FHi: Integer;
         FMin, FMax: PDouble;
    +    FRangeIsInvalid: Boolean;
         function CalcOffset(AScale: Double): Double;
    -    function CalcScale(ASign: Integer): Double;
    +    function CalcScale: Double;
         constructor Init(
    -      AAxis: TChartAxis; AImageLo, AImageHi, AMarginLo, AMarginHi: Integer;
    -      AMin, AMax: PDouble);
    +      AAxis: TChartAxis; AImageLo, AImageHi, AMarginLo, AMarginHi,
    +      ARequiredMarginLo, ARequiredMarginHi: Integer;
    +      AMin, AMax: PDouble; AAxisIsVertical: Boolean);
         procedure UpdateMinMax(AConv: TAxisConvFunc);
       end;
     
    @@ -1216,10 +1218,13 @@
     { TAxisCoeffHelper }
     
     constructor TAxisCoeffHelper.Init(
    -  AAxis: TChartAxis; AImageLo, AImageHi, AMarginLo, AMarginHi: Integer;
    -  AMin, AMax: PDouble);
    +  AAxis: TChartAxis; AImageLo, AImageHi, AMarginLo, AMarginHi,
    +  ARequiredMarginLo, ARequiredMarginHi: Integer;
    +  AMin, AMax: PDouble; AAxisIsVertical: Boolean);
    +const
    +  GUARANTEED_SPACE = 9; // Currently must be an odd number
     begin
    -  FAxis := AAxis;
    +  FAxisIsFlipped := ((AAxis <> nil) and AAxis.IsFlipped) xor AAxisIsVertical;
       FImageLo := AImageLo;
       FImageHi := AImageHi;
       FMin := AMin;
    @@ -1226,27 +1231,83 @@
       FMax := AMax;
       FLo := FImageLo + AMarginLo;
       FHi := FImageHi + AMarginHi;
    +
    +  FRangeIsInvalid :=
    +    (FImageHi <= FImageLo) {Chart.FClipRect has (after adjusting by required
    +      margins) empty or reversed range - this means that chart's window become
    +      so small (due to resizing), that there is no image area to paint on it}
    +    or
    +    (FMax^ <= FMin^); {Chart.FCurrentExtent has empty or reversed range -
    +      this should never happen, even when there is no data to paint}
    +
    +  if FRangeIsInvalid then exit;
    +
    +  if FHi <= FLo + GUARANTEED_SPACE - 1 then
    +  begin
    +    {Don't allow too small (or even negative) width/height of the image area,
    +     that will be used for painting the series: provide GUARANTEED_SPACE image
    +     units (usually pixel) in such cases}
    +    if (FLo + FHi) mod 2 <> 0 then
    +    begin
    +      FLo := (FLo + FHi) div 2 - GUARANTEED_SPACE div 2;
    +      FHi := FLo + 1 + GUARANTEED_SPACE div 2;
    +    end else
    +    if Abs(AMarginHi) < Abs(AMarginLo) then
    +    begin
    +      FLo := (FLo + FHi) div 2 - GUARANTEED_SPACE div 2;
    +      FHi := FLo + 1 + GUARANTEED_SPACE div 2;
    +    end else
    +    begin
    +      FHi := (FLo + FHi) div 2 + GUARANTEED_SPACE div 2;
    +      FLo := FHi - 1 - GUARANTEED_SPACE div 2;
    +    end;
    +
    +    {If space, provided above, is located ouside the image area, move it to
    +     the nearest area's egde}
    +    if FHi > FImageHi + ARequiredMarginHi then
    +    begin
    +      FHi := FImageHi + ARequiredMarginHi;
    +      FLo := FHi - GUARANTEED_SPACE;
    +    end else
    +    if FLo < FImageLo + ARequiredMarginLo then
    +    begin
    +      FLo := FImageLo + ARequiredMarginLo;
    +      FHi := FLo + GUARANTEED_SPACE;
    +    end;
    +  end;
     end;
     
    -function TAxisCoeffHelper.CalcScale(ASign: Integer): Double;
    +function TAxisCoeffHelper.CalcScale: 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^);
    +  if FRangeIsInvalid then
    +    Result := 1
    +  else
    +    Result := (FHi - FLo) / (FMax^ - FMin^);
    +  if FAxisIsFlipped then
    +    Result := -Result;
     end;
     
     function TAxisCoeffHelper.CalcOffset(AScale: Double): Double;
     begin
    -  Result := (FLo + FHi) / 2 - AScale * (FMin^ + FMax^) / 2;
    +  if FRangeIsInvalid then
    +    Result := 0
    +  else
    +    Result := ((FLo + FHi) - AScale * (FMin^ + FMax^)) * 0.5;
     end;
     
     procedure TAxisCoeffHelper.UpdateMinMax(AConv: TAxisConvFunc);
     begin
    -  FMin^ := AConv(FImageLo);
    -  FMax^ := AConv(FImageHi);
    -  if (FAxis <> nil) and FAxis.IsFlipped then
    -    Exchange(FMin^, FMax^);
    +  if FRangeIsInvalid then
    +  begin
    +    FMin^ := -Infinity;
    +    FMax^ := Infinity;
    +  end else
    +  begin
    +    FMin^ := AConv(FImageLo);
    +    FMax^ := AConv(FImageHi);
    +    if FAxisIsFlipped then
    +      Exchange(FMin^, FMax^);
    +  end;
     end;
     
     procedure SkipObsoleteAxisProperties;
    Index: components/tachart/tagraph.pas
    ===================================================================
    --- components/tachart/tagraph.pas	(revision 60116)
    +++ components/tachart/tagraph.pas	(working copy)
    @@ -1,4 +1,4 @@
    -{
    +{
      /***************************************************************************
                                    TAGraph.pas
                                    -----------
    @@ -244,7 +244,7 @@
         FSavedClipRect: TRect;
         FClipRectLock: Integer;
     
    -    procedure CalculateTransformationCoeffs(const AMargin: TRect);
    +    procedure CalculateTransformationCoeffs(const AMargin, ARequiredMargin: TRect);
         procedure DrawReticule(ADrawer: IChartDrawer);  deprecated 'Use DatapointCrosshairTool instead';
         procedure FindComponentClass(
           AReader: TReader; const AClassName: String; var AClass: TComponentClass);
    @@ -577,18 +577,20 @@
       StyleChanged(ASeries);
     end;
     
    -procedure TChart.CalculateTransformationCoeffs(const AMargin: TRect);
    +procedure TChart.CalculateTransformationCoeffs(const AMargin, ARequiredMargin: TRect);
     var
       rX, rY: TAxisCoeffHelper;
     begin
       rX.Init(
         BottomAxis, FClipRect.Left, FClipRect.Right, AMargin.Left, -AMargin.Right,
    -    @FCurrentExtent.a.X, @FCurrentExtent.b.X);
    +    ARequiredMargin.Left, -ARequiredMargin.Right,
    +    @FCurrentExtent.a.X, @FCurrentExtent.b.X, False);
       rY.Init(
    -    LeftAxis, FClipRect.Bottom, FClipRect.Top, -AMargin.Bottom, AMargin.Top,
    -    @FCurrentExtent.a.Y, @FCurrentExtent.b.Y);
    -  FScale.X := rX.CalcScale(1);
    -  FScale.Y := rY.CalcScale(-1);
    +    LeftAxis, FClipRect.Top, FClipRect.Bottom, AMargin.Top, -AMargin.Bottom,
    +    ARequiredMargin.Top, -ARequiredMargin.Bottom,
    +    @FCurrentExtent.a.Y, @FCurrentExtent.b.Y, True);
    +  FScale.X := rX.CalcScale;
    +  FScale.Y := rY.CalcScale;
       if Proportional then begin
         if Abs(FScale.X) > Abs(FScale.Y) then
           FScale.X := Abs(FScale.Y) * Sign(FScale.X)
    @@ -693,7 +695,7 @@
       FDefaultGUIConnector.CreateDrawer(FConnectorData);
       FGUIConnectorListener := TListener.Create(@FGUIConnector, @StyleChanged);
     
    -  FScale := DoublePoint(1, 1);
    +  FScale := DoublePoint(1, -1);
     
       Width := DEFAULT_CHART_WIDTH;
       Height := DEFAULT_CHART_HEIGHT;
    @@ -1436,12 +1438,18 @@
       prevExt: TDoubleRect;
       axis: TChartAxis;
       scaled_depth: Integer;
    +  requiredMargin: TRect;
     begin
    +  requiredMargin.Top:=ADrawer.Scale(Margins.Top);
    +  requiredMargin.Left:=ADrawer.Scale(Margins.Left);
    +  requiredMargin.Bottom:=ADrawer.Scale(Margins.Bottom);
    +  requiredMargin.Right:=ADrawer.Scale(Margins.Right);
    +
       scaled_depth := ADrawer.Scale(Depth);
       if not AxisVisible then begin
         FClipRect.Left += scaled_depth;
         FClipRect.Bottom -= scaled_depth;
    -    CalculateTransformationCoeffs(GetMargins(ADrawer));
    +    CalculateTransformationCoeffs(GetMargins(ADrawer), requiredMargin);
         exit;
       end;
     
    @@ -1451,7 +1459,7 @@
     
       // There is a cyclic dependency: extent -> visible marks -> margins.
       // We recalculate them iteratively hoping that the process converges.
    -  CalculateTransformationCoeffs(ZeroRect);
    +  CalculateTransformationCoeffs(ZeroRect, requiredMargin);
       cr := FClipRect;
       for tries := 1 to 10 do begin
         axisMargin := AxisList.Measure(CurrentExtent, scaled_depth);
    @@ -1462,7 +1470,7 @@
           SideByAlignment(FClipRect, aa, -axisMargin[aa]);
         prevExt := FCurrentExtent;
         FCurrentExtent := FLogicalExtent;
    -    CalculateTransformationCoeffs(GetMargins(ADrawer));
    +    CalculateTransformationCoeffs(GetMargins(ADrawer), requiredMargin);
         if prevExt = FCurrentExtent then break;
         prevExt := FCurrentExtent;
       end;
    
    patch_extended.diff (8,088 bytes)
  • LongSeriesLabels_Test.zip (4,722 bytes)
  • Crash.zip (2,860 bytes)
  • AnimationTest.gif (71,455 bytes)
    AnimationTest.gif (71,455 bytes)
  • Test.zip (3,725 bytes)
  • test.diff (3,616 bytes)
    Index: components/tachart/tachartaxis.pas
    ===================================================================
    --- components/tachart/tachartaxis.pas	(revision 60310)
    +++ components/tachart/tachartaxis.pas	(working copy)
    @@ -252,7 +252,7 @@
         function CalcScale(ASign: Integer): Double;
         constructor Init(AAxis: TChartAxis; AImageLo, AImageHi: Integer;
           AMarginLo, AMarginHi, ARequiredMarginLo, ARequiredMarginHi, AMinDataSpace: Integer;
    -      AHasMarksInMargin, AAxisIsVertical: Boolean; AMin, AMax: PDouble);
    +      AAxisIsVertical: Boolean; AMin, AMax: PDouble);
         procedure UpdateMinMax(AConv: TAxisConvFunc);
       end;
     
    @@ -1218,14 +1218,19 @@
     procedure EnsureGuaranteedSpace(var AValue1, AValue2: Integer;
       AImage1, AImage2, AMargin1, AMargin2, AGuaranteed: Integer);
     var
    +  HasMarksInMargin1, HasMarksInMargin2: Boolean;
       delta1, delta2: Integer;
     begin
    -  if (AValue2 = AImage2 - AMargin2) then
    +  HasMarksInMargin1 := (AValue1 = AImage1 + AMargin1);
    +  HasMarksInMargin2 := (AValue2 = AImage2 - AMargin2);
    +
    +  if HasMarksInMargin2 and not HasMarksInMargin1 then
         AValue1 := AValue2 - AGuaranteed
       else
    -  if (AValue1 = AImage1 + AMargin1) then
    +  if HasMarksInMargin1 and not HasMarksInMargin2 then
         AValue2 := AValue1 + AGuaranteed
    -  else begin
    +  else
    +  begin
         delta1 := AImage1 - AValue1;
         delta2 := AImage2 - AValue2;
         AValue1 := (AImage1 + AImage2 - AGuaranteed) div 2;
    @@ -1243,7 +1248,7 @@
     
     constructor TAxisCoeffHelper.Init(AAxis: TChartAxis; AImageLo, AImageHi: Integer;
       AMarginLo, AMarginHi, ARequiredMarginLo, ARequiredMarginHi, AMinDataSpace: Integer;
    -  AHasMarksInMargin, AAxisIsVertical: Boolean; AMin, AMax: PDouble);
    +  AAxisIsVertical: Boolean; AMin, AMax: PDouble);
     begin
       FAxisIsFlipped := (AAxis <> nil) and AAxis.IsFlipped;
       FImageLo := AImageLo;
    @@ -1253,16 +1258,14 @@
       FLo := FImageLo + AMarginLo;
       FHi := FImageHi + AMarginHi;
     
    -  if AHasMarksInMargin then begin
    -    if AAxisIsVertical then begin
    -      if (FHi + AMinDataSpace >= FLo) then
    -        EnsureGuaranteedSpace(FHi, FLo, FImageHi, FImageLo,
    -          ARequiredMarginHi, ARequiredMarginLo, AMinDataSpace);
    -    end else begin
    -      if (FLo + AMinDataSpace >= FHi) then
    -        EnsureGuaranteedSpace(FLo, FHi, FImageLo, FImageHi,
    -          ARequiredMarginLo, ARequiredMarginHi, AMinDataSpace);
    -    end;
    +  if AAxisIsVertical then begin
    +    if (FHi + AMinDataSpace >= FLo) then
    +      EnsureGuaranteedSpace(FHi, FLo, FImageHi, FImageLo,
    +        ARequiredMarginHi, ARequiredMarginLo, AMinDataSpace);
    +  end else begin
    +    if (FLo + AMinDataSpace >= FHi) then
    +      EnsureGuaranteedSpace(FLo, FHi, FImageLo, FImageHi,
    +        ARequiredMarginLo, ARequiredMarginHi, AMinDataSpace);
       end;
     end;
     
    Index: components/tachart/tagraph.pas
    ===================================================================
    --- components/tachart/tagraph.pas	(revision 60310)
    +++ components/tachart/tagraph.pas	(working copy)
    @@ -594,12 +594,10 @@
       rX.Init(
         HorAxis, FClipRect.Left, FClipRect.Right, AMargin.Left, -AMargin.Right,
         AChartMargins.Left, AChartMargins.Right, AMinDataSpace,
    -    (AMargin.Left <> AChartMargins.Left) or (AMargin.Right <> AChartMargins.Right),
         false, @FCurrentExtent.a.X, @FCurrentExtent.b.X);
       rY.Init(
         VertAxis, FClipRect.Bottom, FClipRect.Top, -AMargin.Bottom, AMargin.Top,
         AChartMargins.Bottom, AChartMargins.Top, AMinDataSpace,
    -    (AMargin.Top <> AChartMargins.Top) or (AMargin.Bottom <> AChartMargins.Bottom),
         true, @FCurrentExtent.a.Y, @FCurrentExtent.b.Y);
     
       FScale.X := rX.CalcScale(1);
    
    test.diff (3,616 bytes)
  • Panning1.mp4 (625,626 bytes)
  • Panning2.mp4 (249,447 bytes)
  • Zooming1.mp4 (304,529 bytes)
  • Zooming2.mp4 (255,004 bytes)

Relationships

related to 0034819 closedwp Packages Series marks are sometimes invisible 

Activities

Marcin Wiazowski

2019-01-18 21:14

reporter  

Calculations.pdf (20,896 bytes)

Marcin Wiazowski

2019-01-18 21:14

reporter  

patch.diff (3,438 bytes)
Index: components/tachart/tachartaxis.pas
===================================================================
--- components/tachart/tachartaxis.pas	(revision 60103)
+++ components/tachart/tachartaxis.pas	(working copy)
@@ -244,15 +244,15 @@
   { TAxisCoeffHelper }
 
   TAxisCoeffHelper = object
-    FAxis: TChartAxis;
+    FAxisIsFlipped: Boolean;
     FImageLo, FImageHi: Integer;
     FLo, FHi: Integer;
     FMin, FMax: PDouble;
     function CalcOffset(AScale: Double): Double;
-    function CalcScale(ASign: Integer): Double;
+    function CalcScale: Double;
     constructor Init(
       AAxis: TChartAxis; AImageLo, AImageHi, AMarginLo, AMarginHi: Integer;
-      AMin, AMax: PDouble);
+      AMin, AMax: PDouble; AAxisIsVertical: Boolean);
     procedure UpdateMinMax(AConv: TAxisConvFunc);
   end;
 
@@ -1217,9 +1217,9 @@
 
 constructor TAxisCoeffHelper.Init(
   AAxis: TChartAxis; AImageLo, AImageHi, AMarginLo, AMarginHi: Integer;
-  AMin, AMax: PDouble);
+  AMin, AMax: PDouble; AAxisIsVertical: Boolean);
 begin
-  FAxis := AAxis;
+  FAxisIsFlipped := ((AAxis <> nil) and AAxis.IsFlipped) xor AAxisIsVertical;
   FImageLo := AImageLo;
   FImageHi := AImageHi;
   FMin := AMin;
@@ -1228,17 +1228,19 @@
   FHi := FImageHi + AMarginHi;
 end;
 
-function TAxisCoeffHelper.CalcScale(ASign: Integer): Double;
+function TAxisCoeffHelper.CalcScale: 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^);
+  if (FMax^ <= FMin^) or (FHi <= FLo) then
+    Result := 1
+  else
+    Result := (FHi - FLo) / (FMax^ - FMin^);
+  if FAxisIsFlipped then
+    Result := -Result;
 end;
 
 function TAxisCoeffHelper.CalcOffset(AScale: Double): Double;
 begin
-  Result := (FLo + FHi) / 2 - AScale * (FMin^ + FMax^) / 2;
+  Result := ((FLo + FHi) - AScale * (FMin^ + FMax^)) * 0.5;
 end;
 
 procedure TAxisCoeffHelper.UpdateMinMax(AConv: TAxisConvFunc);
@@ -1245,7 +1247,7 @@
 begin
   FMin^ := AConv(FImageLo);
   FMax^ := AConv(FImageHi);
-  if (FAxis <> nil) and FAxis.IsFlipped then
+  if FAxisIsFlipped then
     Exchange(FMin^, FMax^);
 end;
 
Index: components/tachart/tagraph.pas
===================================================================
--- components/tachart/tagraph.pas	(revision 60103)
+++ components/tachart/tagraph.pas	(working copy)
@@ -583,12 +583,12 @@
 begin
   rX.Init(
     BottomAxis, FClipRect.Left, FClipRect.Right, AMargin.Left, -AMargin.Right,
-    @FCurrentExtent.a.X, @FCurrentExtent.b.X);
+    @FCurrentExtent.a.X, @FCurrentExtent.b.X, False);
   rY.Init(
-    LeftAxis, FClipRect.Bottom, FClipRect.Top, -AMargin.Bottom, AMargin.Top,
-    @FCurrentExtent.a.Y, @FCurrentExtent.b.Y);
-  FScale.X := rX.CalcScale(1);
-  FScale.Y := rY.CalcScale(-1);
+    LeftAxis, FClipRect.Top, FClipRect.Bottom, AMargin.Top, -AMargin.Bottom,
+    @FCurrentExtent.a.Y, @FCurrentExtent.b.Y, True);
+  FScale.X := rX.CalcScale;
+  FScale.Y := rY.CalcScale;
   if Proportional then begin
     if Abs(FScale.X) > Abs(FScale.Y) then
       FScale.X := Abs(FScale.Y) * Sign(FScale.X)
@@ -693,7 +693,7 @@
   FDefaultGUIConnector.CreateDrawer(FConnectorData);
   FGUIConnectorListener := TListener.Create(@FGUIConnector, @StyleChanged);
 
-  FScale := DoublePoint(1, 1);
+  FScale := DoublePoint(1, -1);
 
   Width := DEFAULT_CHART_WIDTH;
   Height := DEFAULT_CHART_HEIGHT;
patch.diff (3,438 bytes)

wp

2019-01-19 00:51

developer   ~0113471

Hmm...

Is there any specific bug that you have in mind why you want to change routines which have been working correctly for years?

If there is a bug please post a demo so that I can see the effect of the bug and the improvement by your patch.

Just for the fun of having a routine which appears to be more understandable to you I will not touch anything in the heart of TAChart. The risk of breaking something is too high.

Marcin Wiazowski

2019-01-19 02:55

reporter   ~0113472

Well, you are of course right. Apparently I wasn't clear enough, that the patch solves the problem described in 0034819, in Note 14.

I'm attaching a test application here, and also an animation, that shows the difference before and after applying the patch (tested with r60103).

As can be observed, without the patch there is a moment, when Y axis gets reversed, so bars start to be drawn in the opposite direction. Some other chart's algorithm gets confused, so marks are not drawn anymore.




Additional note: after applying the patch, things are still not ideal - one of the bars starts to be drawn partially outside the chart, so it's mark is not painted anymore. After clicking the button few more times, both bars are drawn fully outside the chart (and their marks are not painted). But Y axis is not reversed anymore, so this problem can be now solved - and I already solved it, but I'll create another report for this.

Regards

Marcin Wiazowski

2019-01-19 02:56

reporter  

Animation.gif (70,761 bytes)
Animation.gif (70,761 bytes)

Marcin Wiazowski

2019-01-19 02:56

reporter  

Reproduce.zip (3,462 bytes)

wp

2019-01-19 17:19

developer   ~0113486

OK, I had suspected that this is what you mean. The problem with this example is that it corresponds to an unnormal usage of TAChart because the height of the series labels becomes larger than the height of the plot area. The correct fix must detect the error situation and bring the code back into a safe zone, but it should not, by all means, alter the backbone of the code needed by all other use cases. The risk to damage something for almost nothing is too high.

When you step through the code with the debugger you will notice that the CurrentExtent becomes inverted when the axis labeling direction changes. "Inverted" means that Extent.b.y (the uppper end of the extent) becomes smaller than the lower end, Extent.a.y. So, the fix must detect this error situation, for example in the "tries" loop of TChart.PrepareAxis, and restore a "good" extent. This does avoid inverting the axis labels, but pushes the chart out of view in favor of the super-large labels - this is not yet exactly what I want: I'd like the chart to be visible all the time and the label to be truncated.

To see this, you must add the lines indicated to TChart.PrepareAxis:

  for tries := 1 to 10 do begin
    axisMargin := AxisList.Measure(CurrentExtent, scaled_depth);
    axisMargin[calLeft] := Max(axisMargin[calLeft], scaled_depth);
    axisMargin[calBottom] := Max(axisMargin[calBottom], scaled_depth);
    FClipRect := cr;
    for aa := Low(aa) to High(aa) do
      SideByAlignment(FClipRect, aa, -axisMargin[aa]);
    prevExt := FCurrentExtent;
    FCurrentExtent := FLogicalExtent;
    CalculateTransformationCoeffs(GetMargins(ADrawer));
    if prevExt = FCurrentExtent then break;
    
    // --- to be added from here... -----------------------
    if (FCurrentExtent.b.Y < FCurrentExtent.a.Y) or
       (FCurrentExtent.b.X < FCurrentExtent.a.X) then
    begin
      FCurrentExtent := prevExt;
      FClipRect := cr;
      break;
    end;
    // --- ... to here ------------------------------------
    
    prevExt := FCurrentExtent;
  end;

Marcin Wiazowski

2019-01-19 21:40

reporter  

Reproduce2.zip (3,811 bytes)

Marcin Wiazowski

2019-01-19 21:41

reporter  

patch_extended.diff (8,088 bytes)
Index: components/tachart/tachartaxis.pas
===================================================================
--- components/tachart/tachartaxis.pas	(revision 60116)
+++ components/tachart/tachartaxis.pas	(working copy)
@@ -244,15 +244,17 @@
   { TAxisCoeffHelper }
 
   TAxisCoeffHelper = object
-    FAxis: TChartAxis;
+    FAxisIsFlipped: Boolean;
     FImageLo, FImageHi: Integer;
     FLo, FHi: Integer;
     FMin, FMax: PDouble;
+    FRangeIsInvalid: Boolean;
     function CalcOffset(AScale: Double): Double;
-    function CalcScale(ASign: Integer): Double;
+    function CalcScale: Double;
     constructor Init(
-      AAxis: TChartAxis; AImageLo, AImageHi, AMarginLo, AMarginHi: Integer;
-      AMin, AMax: PDouble);
+      AAxis: TChartAxis; AImageLo, AImageHi, AMarginLo, AMarginHi,
+      ARequiredMarginLo, ARequiredMarginHi: Integer;
+      AMin, AMax: PDouble; AAxisIsVertical: Boolean);
     procedure UpdateMinMax(AConv: TAxisConvFunc);
   end;
 
@@ -1216,10 +1218,13 @@
 { TAxisCoeffHelper }
 
 constructor TAxisCoeffHelper.Init(
-  AAxis: TChartAxis; AImageLo, AImageHi, AMarginLo, AMarginHi: Integer;
-  AMin, AMax: PDouble);
+  AAxis: TChartAxis; AImageLo, AImageHi, AMarginLo, AMarginHi,
+  ARequiredMarginLo, ARequiredMarginHi: Integer;
+  AMin, AMax: PDouble; AAxisIsVertical: Boolean);
+const
+  GUARANTEED_SPACE = 9; // Currently must be an odd number
 begin
-  FAxis := AAxis;
+  FAxisIsFlipped := ((AAxis <> nil) and AAxis.IsFlipped) xor AAxisIsVertical;
   FImageLo := AImageLo;
   FImageHi := AImageHi;
   FMin := AMin;
@@ -1226,27 +1231,83 @@
   FMax := AMax;
   FLo := FImageLo + AMarginLo;
   FHi := FImageHi + AMarginHi;
+
+  FRangeIsInvalid :=
+    (FImageHi <= FImageLo) {Chart.FClipRect has (after adjusting by required
+      margins) empty or reversed range - this means that chart's window become
+      so small (due to resizing), that there is no image area to paint on it}
+    or
+    (FMax^ <= FMin^); {Chart.FCurrentExtent has empty or reversed range -
+      this should never happen, even when there is no data to paint}
+
+  if FRangeIsInvalid then exit;
+
+  if FHi <= FLo + GUARANTEED_SPACE - 1 then
+  begin
+    {Don't allow too small (or even negative) width/height of the image area,
+     that will be used for painting the series: provide GUARANTEED_SPACE image
+     units (usually pixel) in such cases}
+    if (FLo + FHi) mod 2 <> 0 then
+    begin
+      FLo := (FLo + FHi) div 2 - GUARANTEED_SPACE div 2;
+      FHi := FLo + 1 + GUARANTEED_SPACE div 2;
+    end else
+    if Abs(AMarginHi) < Abs(AMarginLo) then
+    begin
+      FLo := (FLo + FHi) div 2 - GUARANTEED_SPACE div 2;
+      FHi := FLo + 1 + GUARANTEED_SPACE div 2;
+    end else
+    begin
+      FHi := (FLo + FHi) div 2 + GUARANTEED_SPACE div 2;
+      FLo := FHi - 1 - GUARANTEED_SPACE div 2;
+    end;
+
+    {If space, provided above, is located ouside the image area, move it to
+     the nearest area's egde}
+    if FHi > FImageHi + ARequiredMarginHi then
+    begin
+      FHi := FImageHi + ARequiredMarginHi;
+      FLo := FHi - GUARANTEED_SPACE;
+    end else
+    if FLo < FImageLo + ARequiredMarginLo then
+    begin
+      FLo := FImageLo + ARequiredMarginLo;
+      FHi := FLo + GUARANTEED_SPACE;
+    end;
+  end;
 end;
 
-function TAxisCoeffHelper.CalcScale(ASign: Integer): Double;
+function TAxisCoeffHelper.CalcScale: 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^);
+  if FRangeIsInvalid then
+    Result := 1
+  else
+    Result := (FHi - FLo) / (FMax^ - FMin^);
+  if FAxisIsFlipped then
+    Result := -Result;
 end;
 
 function TAxisCoeffHelper.CalcOffset(AScale: Double): Double;
 begin
-  Result := (FLo + FHi) / 2 - AScale * (FMin^ + FMax^) / 2;
+  if FRangeIsInvalid then
+    Result := 0
+  else
+    Result := ((FLo + FHi) - AScale * (FMin^ + FMax^)) * 0.5;
 end;
 
 procedure TAxisCoeffHelper.UpdateMinMax(AConv: TAxisConvFunc);
 begin
-  FMin^ := AConv(FImageLo);
-  FMax^ := AConv(FImageHi);
-  if (FAxis <> nil) and FAxis.IsFlipped then
-    Exchange(FMin^, FMax^);
+  if FRangeIsInvalid then
+  begin
+    FMin^ := -Infinity;
+    FMax^ := Infinity;
+  end else
+  begin
+    FMin^ := AConv(FImageLo);
+    FMax^ := AConv(FImageHi);
+    if FAxisIsFlipped then
+      Exchange(FMin^, FMax^);
+  end;
 end;
 
 procedure SkipObsoleteAxisProperties;
Index: components/tachart/tagraph.pas
===================================================================
--- components/tachart/tagraph.pas	(revision 60116)
+++ components/tachart/tagraph.pas	(working copy)
@@ -1,4 +1,4 @@
-{
+{
  /***************************************************************************
                                TAGraph.pas
                                -----------
@@ -244,7 +244,7 @@
     FSavedClipRect: TRect;
     FClipRectLock: Integer;
 
-    procedure CalculateTransformationCoeffs(const AMargin: TRect);
+    procedure CalculateTransformationCoeffs(const AMargin, ARequiredMargin: TRect);
     procedure DrawReticule(ADrawer: IChartDrawer);  deprecated 'Use DatapointCrosshairTool instead';
     procedure FindComponentClass(
       AReader: TReader; const AClassName: String; var AClass: TComponentClass);
@@ -577,18 +577,20 @@
   StyleChanged(ASeries);
 end;
 
-procedure TChart.CalculateTransformationCoeffs(const AMargin: TRect);
+procedure TChart.CalculateTransformationCoeffs(const AMargin, ARequiredMargin: TRect);
 var
   rX, rY: TAxisCoeffHelper;
 begin
   rX.Init(
     BottomAxis, FClipRect.Left, FClipRect.Right, AMargin.Left, -AMargin.Right,
-    @FCurrentExtent.a.X, @FCurrentExtent.b.X);
+    ARequiredMargin.Left, -ARequiredMargin.Right,
+    @FCurrentExtent.a.X, @FCurrentExtent.b.X, False);
   rY.Init(
-    LeftAxis, FClipRect.Bottom, FClipRect.Top, -AMargin.Bottom, AMargin.Top,
-    @FCurrentExtent.a.Y, @FCurrentExtent.b.Y);
-  FScale.X := rX.CalcScale(1);
-  FScale.Y := rY.CalcScale(-1);
+    LeftAxis, FClipRect.Top, FClipRect.Bottom, AMargin.Top, -AMargin.Bottom,
+    ARequiredMargin.Top, -ARequiredMargin.Bottom,
+    @FCurrentExtent.a.Y, @FCurrentExtent.b.Y, True);
+  FScale.X := rX.CalcScale;
+  FScale.Y := rY.CalcScale;
   if Proportional then begin
     if Abs(FScale.X) > Abs(FScale.Y) then
       FScale.X := Abs(FScale.Y) * Sign(FScale.X)
@@ -693,7 +695,7 @@
   FDefaultGUIConnector.CreateDrawer(FConnectorData);
   FGUIConnectorListener := TListener.Create(@FGUIConnector, @StyleChanged);
 
-  FScale := DoublePoint(1, 1);
+  FScale := DoublePoint(1, -1);
 
   Width := DEFAULT_CHART_WIDTH;
   Height := DEFAULT_CHART_HEIGHT;
@@ -1436,12 +1438,18 @@
   prevExt: TDoubleRect;
   axis: TChartAxis;
   scaled_depth: Integer;
+  requiredMargin: TRect;
 begin
+  requiredMargin.Top:=ADrawer.Scale(Margins.Top);
+  requiredMargin.Left:=ADrawer.Scale(Margins.Left);
+  requiredMargin.Bottom:=ADrawer.Scale(Margins.Bottom);
+  requiredMargin.Right:=ADrawer.Scale(Margins.Right);
+
   scaled_depth := ADrawer.Scale(Depth);
   if not AxisVisible then begin
     FClipRect.Left += scaled_depth;
     FClipRect.Bottom -= scaled_depth;
-    CalculateTransformationCoeffs(GetMargins(ADrawer));
+    CalculateTransformationCoeffs(GetMargins(ADrawer), requiredMargin);
     exit;
   end;
 
@@ -1451,7 +1459,7 @@
 
   // There is a cyclic dependency: extent -> visible marks -> margins.
   // We recalculate them iteratively hoping that the process converges.
-  CalculateTransformationCoeffs(ZeroRect);
+  CalculateTransformationCoeffs(ZeroRect, requiredMargin);
   cr := FClipRect;
   for tries := 1 to 10 do begin
     axisMargin := AxisList.Measure(CurrentExtent, scaled_depth);
@@ -1462,7 +1470,7 @@
       SideByAlignment(FClipRect, aa, -axisMargin[aa]);
     prevExt := FCurrentExtent;
     FCurrentExtent := FLogicalExtent;
-    CalculateTransformationCoeffs(GetMargins(ADrawer));
+    CalculateTransformationCoeffs(GetMargins(ADrawer), requiredMargin);
     if prevExt = FCurrentExtent then break;
     prevExt := FCurrentExtent;
   end;
patch_extended.diff (8,088 bytes)

Marcin Wiazowski

2019-01-19 21:45

reporter   ~0113495

The solution with adding additional code to TChart.PrepareAxis could work, but there is a drawback - it would prevent achieving the true convergence: FCurrentExtent may be changed in the next loop iteration, so it will no longer be inverted - and breaking the loop may prevent this. Please note, that this would be also treating the symptoms, instead of the cause - the cause is in TAxisCoeffHelper.CalcScale... And, after all, this is also touching the code, that has been working for years.

Well, I'm also not a fan of turning a big piece of the working code inside out, just to make something 0.002% faster or 0.03% better...

It's always a choice between risk and advantages.

I personally think, that in this specific case the risk is worth it - but only because the only thing, that is really going to be changed, is an old bug, that has been always in the code, and which manifests itself only in case of long marks (or big margins) - so nobody reported it before. Maybe long, multiline marks don't occur every day, but, from the other side, I discovered the problem, because mark contents come from the application's users and they can be multiline - well, this is a real life scenario. I could cut texts, but - to which number of lines? There is no obvious method of calculating this. And it's also not a good solution - the user may zoom the chart in, so the same text will no longer be too long...



I attached another test application here (Reproduce2.zip). It shows the nature of the bug (i.e. inverting the axis because of the improper sign of the FScale value, coming from the TAxisCoeffHelper.CalcScale - i.e. always +1.0). In other words, Reproduce2 shows, that for horizontal axis (when Inverted = True) and for vertical axis (when Inverted = False), the Inverted setting is being ignored when labels become long enough. Even if this is an intended bevahior (I don't believe it) - does it make any sense? It doesn't respect user's choice (expressed by seting, or not setting, the Inverted property).



Please let me be very clear: To fix the cause of the problem, the only thing, that is really needed, is changing TAxisCoeffHelper.CalcScale to:

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;

It works exactly in the same manner for all non-too-big-margins-or-too-long-labels cases - i.e. in almost all cases. If you wish, you could - for testing purposes - use it along with the old version, and raise an exception when results are different. This will happen only for
long marks / big margins.

All the additional changes, that are made by the patch.diff, are just boolean and arithmetical transformations (and I think that one could agree, that their correctness can be verified by looking into Calculations.pdf). But even patching only the TAxisCoeffHelper.CalcScale function, as shown here, will make things better (i.e correct...).




> I'd like the chart to be visible all the time and the label to be truncated.

Ok: I'm attaching my solution here, just for testing purposes - please see the patch_extended.diff. It makes all the changes that the patch.diff makes, plus some additional changes - just to make your wish the reality ;) When you'll look at changes introduced by patch_extended.diff, you will see why I needed all these code transformations - without them, extending the patch would be a big pain (in particular because of swapped Top-Bottom parameters, passed into rY.Init).

I still need to take a look at this solution and tune it a bit - then I will be also able to explain what the particular changes are for. Please apply the patch just for testing purposes and try with Reproduce.zip and with Reproduce2.zip attached here - everything will be working properly at last, just like it should be from the beginning.



Regards

wp

2019-01-19 23:38

developer   ~0113498

I am inclined to use the CalcScale modification which seems to be reasonable.

But I refrain from using the other modifications. Why do you think, as expressed in your pdf, that passing ClipRect.Bottom and .Top - in this order - to the parameters AImageLo and AImageHi of the AxisCoeffHelper.Init is not the natural order? Bottom=Lo, Top=Hi - what's wrong with it? And the Sign passed to the Scale function must be -1 for the y axis because image and graph coordinates run in opposite directions. The rest of the document only contains fixes for this fundamental misunderstanding.

The GUARANTEED_SPACE in your extended patch is what I was looking for. Now the chart behaves correctly just like an unbiased user would expect.

Please rewrite the patch without the reordering of the parameters of the Init method. Also omit the optimizations for the moment (e.g. multiplications by 0.5 instead of division by 2) - they can be introduced in the next step. CalcScale must be the version with the Sign parameter, otherwise I cannot compare with the current version.

Marcin Wiazowski

2019-01-20 00:44

reporter   ~0113504

> Why do you think, as expressed in your pdf, that passing ClipRect.Bottom and .Top - in this order - to the parameters AImageLo and AImageHi of the AxisCoeffHelper.Init is not the natural order? Bottom=Lo, Top=Hi - what's wrong with it? And the Sign passed to the Scale function must be -1

Ok, I haven't explained that. In fact, when thinking about image coordinates for the Y axis, natural order may be seen as Bottom to Top. However, by using the "natural order" term, I was thinking rather about mathematics - i.e. having number of pixels (to draw) always given as a positive number. Here we have sample FClipRect rect, that I grabbed under the debugger:

  LEFT = 4
  TOP = 22
  RIGHT = 444
  BOTTOM = 200



Lets's assume that we pass this FClipRect record to the rX.Init() and rY.Init() calls in the order, that looks nice to me - i.e.:

  rX.Init(..., FClipRect.Left, FClipRect.Right, ...)
  rY.Init(..., FClipRect.Top, FClipRect.Bottom, ...)


A) Inside of the rX.Init() call we get:

  AImageLo := FClipRect.Left := 4
  AImageHi := FClipRect.Right := 444

so number of available pixels is: (AImageHi - AImageLo) = 444 - 4 = 440
which is POSITIVE (this is important).



B) Inside of the rY.Init() call we get:

  AImageLo := FClipRect.Top := 22
  AImageHi := FClipRect.Bottom := 200

so number of available pixels is: (AImageHi - AImageLo) = 220 - 22 = 198
which also is POSITIVE (this is important).



Now let's pass the FClipRect to the rY.Init() call in the order currently used:

rY.Init(..., FClipRect.Bottom, FClipRect.Top, ...)

  AImageLo := FClipRect.Bottom := 22
  AImageHi := FClipRect.Top := 200

so number of available pixels is: (AImageHi - AImageLo) = 22 - 220 = -198
which is NEGATIVE.


And here comes our ASign = -1 parameter for the Y axis - since our number of pixels is currently negative, we must multiply it by ASign to get the usable value.


So, by passing (..., FClipRect.Top, FClipRect.Bottom, ...) instead of the currently used (..., FClipRect.Bottom, FClipRect.Top, ...), we make the number of the available pixels always POSITIVE - thanks to that, we can eliminate ASign parameter in all calculations, which makes the calculations easier.





So my idea is:


1) Please apply the simplest possible patch for the TAxisCoeffHelper.CalcScale function, i.e.:

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;


2) Just to make me a favor, please replace the "FAxis: TChartAxis" variable with the "FAxisIsFlipped: Boolean" variable, initializd in Init() as "FAxisIsFlipped := (FAxis <> nil) and FAxis.IsFlipped" - so my code will be more simple


3) I'll make a final tuning of the extended patch


4) If your current decision will be still to keep the ASign parameter, I'll try to adapt my extended patch; if you could leave without it, it would be even better. After all, ASign can always be eliminated later, for example during development for the final Lazarus 2.2 version


5) Just an idea: since chart's margins are definable by properties, guaranteed space for series could also be definable.


Regards

wp

2019-01-20 00:58

developer   ~0113505

No, maybe it's not the best word for it, but "image" coordinates are generalized "screen" coordinates which run from top to bottom. Well, not exactly "screen" coordinates because there is another transformation (a multiplication) which converts the "image" coordinates to "device" coordinates, such as screen pixels, or printer pixels.

It is documented like this (http://wiki.lazarus.freepascal.org/TAChart_documentation#Coordinates_and_axes) and mentioned in numerous forum discussions. This definition cannot be changed.

Marcin Wiazowski

2019-01-20 01:32

reporter   ~0113506

I think that I was misunderstood. I have nothing against the current terminology or FClipRect contents - I suppose that I (unintentionally) used some phrase like "image coordinates" in a general meaning, but it has also some specific meaning in TAChart, and it was understood just in this specific meaning.

Numbering vertical image/screen/bitmap/etc. coordinates from top to bottom is natural for me - it's exactly as Windows does for screens and bitmaps. In any case my intention was to suggest that FClipRect.Top and FClipRect.Bottom ***contents*** should be exchanged in the their values, in TRect structure - my intention was only ***to pass them*** in the swapped order to the TAxisCoeffHelper.Init() call - so only their values will be written to the TAxisCoeffHelper record in the opposite order; the consequences of this change should never, and will never affect anything outside the TChart.CalculateTransformationCoeffs internals.

wp

2019-01-21 00:42

developer   ~0113540

Last edited: 2019-01-21 00:46

View 2 revisions

Fixed in r60129 motivated by your extended patch. I am attaching my test program for reference ("LongSeriesLabels_Test"). The only things which I found not to work are zooming and panning (in your original patch, they are not working either).

Resolving this report. Please test and close if ok.

Open another report if zooming and panning are important for your application.

wp

2019-01-21 00:43

developer  

LongSeriesLabels_Test.zip (4,722 bytes)

Marcin Wiazowski

2019-01-21 02:13

reporter   ~0113541

Nice to see.


I can see some issues:


A) The TChart.CalculateTransformationCoeffs(const AMargin: TRect) procedure gets the AMargin rect parameter - which is obtained by calling "GetMargins(ADrawer)" in the CalculateTransformationCoeffs' caller (i.e. in TChart.PrepareAxis - this is the only place of calling the CalculateTransformationCoeffs procedure).

The GetMargins(ADrawer) call returns margins, that are scaled by calling ADrawer.Scale() - so CalculateTransformationCoeffs receives scaled margins. This means, that guaranteed margins, that are passed to the rX.Init() and rY.Init() calls, should also be scaled (since we can't compare/add/subtract scaled values with not scaled ones). This could be done be passing the ADrawer parameter to the CalculateTransformationCoeffs procedure and calling:

  rX.Init(..., ADrawer.Scale(FMargins.Left), ADrawer.Scale(FMargins.Right), ...);
  rY.Init(..., ADrawer.Scale((FMargins.Bottom), ADrawer.Scale(FMargins.Top), ... );

However, CalculateTransformationCoeffs is called in a loop, and guaranteed margins don't change between loop iterations, so it's faster to precalculate them and just pass to the CalculateTransformationCoeffs call (and this is the reason why I did this in my extended patch).


B) Well, GUARANTEED_SPACE should also be passed to the ADrawer.Scale() call before using it (and, well, I haven't done this in my patch...). In fact, GUARANTEED_SPACE could be replaced with some new TChart's properties, like MinDataWidth / MinDataHeight...


C) The "if Assigned(AAxis) and AAxis.IsVertical then" clause is unfortunate, because a vertical axis, that hasn't its AAxis assigned, will be treated as if it were a horizontal axis... The solution could be similar as it is already in the TAxisCoeffHelper.CalcScale(ASign: Integer) call - the ASign constant, passed as 1 or -1, de facto says about the axis' direction (so AAxis = nil doesn't hurt us). So this could be as follows:

  rX.Init(..., False);
  rY.Init(..., True);

  constructor TAxisCoeffHelper.Init(..., AAxisIsVertical: Boolean); // No need to pass ASign here - simple Boolean constant is enough
  begin
    if AAxisIsVertical then
      ...
    else
      ...
  end;


Regards

wp

2019-01-23 00:19

developer   ~0113588

Added in r60151.

The new property is called MinDataSpace, it is only public because I don't want users to play with it too easily: it can create some unreasonable effects when the value is too large and applied to a chart with "normal" labels and, most of all, when the user does not know what it is good for. There are also some safety measures which prevent the chart to enter these routines in the "normal" case.

Marcin Wiazowski

2019-01-23 14:49

reporter   ~0113596

Well, nice work!


It seems that the last issue arisen: please take a look at the attached Crash.zip. Assumption that AAxis <> nil may not be true. The Crash application works properly under r60150 and crashes under r60151.


> The only things which I found not to work are zooming and panning

I could not reproduce this, at least with default, built-in zooming and panning. Are there any special steps needed to reproduce the problem?


Regards

Marcin Wiazowski

2019-01-23 14:50

reporter  

Crash.zip (2,860 bytes)

Marcin Wiazowski

2019-01-23 15:21

reporter   ~0113597

Additional note: it seems, that the code could be easily improved: currently, the CalculateTransformationCoeffs procedure uses only BottomAxis and LeftAxis for its calculations. But, for example, if the user prefers to have the horizontal axis above the chart (not below, as it is by default), he will use Top axis instead of the Bottom one - and this axis will NOT be taken into account by the CalculateTransformationCoeffs. So the solution could be:


procedure TChart.CalculateTransformationCoeffs(...);
var
  Axis: TChartAxis;
  rX, rY: TAxisCoeffHelper;
begin
  Axis:= BottomAxis;
  if Axis = nil then
    Axis:= GetAxisByAlign(calTop); // There is no TopAxis property
  rX.Init(
    Axis, ...);

  Axis:= LeftAxis;
  if Axis = nil then
    Axis:= GetAxisByAlign(calRight); // There is no RightAxis property
  rY.Init(
    Axis, ...);


As far as I tested, not having any vertical and/or horizontal axis is still fully valid, so the code should still expect, that Axis may be null.

wp

2019-01-24 00:47

developer   ~0113603

I highly disapprove destroying the default axes of a chart, there are numerous places within the library where they are assumed to exist. But as I found out, to my surprise, a simple chart with a series survives when all its axes are destroyed. Therefore, you are right, the Axis in CalculatedTransformationCoeffs CAN be nil.

As for usage of BottomAxis and LeftAxis in this routine I added new public properties HorAxis and VertAxis which are the BottomAxis/LeftAxis if they exist, or the top/right axes otherwise.

I performed several tests, and it seems to work now even with axes destroyed and axes at any side.

--------------

To demonstrate the panning issue, you can use any of your demos. Add as many rows until the longest label overflows. Drag the chart with the right mouse pressed to a different location. After a short vertical movement of the mouse the series disappears. A horizontal movement is ok.

For zooming, again add as many rows until the longest label overflows. Drag with the left mouse button pressed, from left/top to bottom/right, a rectangle tighly around a bar of the series. When you release the mouse button the rectangle should be blown up to fill the chart area, it does not. Depending on the size of the zoom rect, the bar can even become smaller.

I did not investigate into this bug so far. And I don't have the resources to do so for the rarely used feature of the huge series labels. Probably it is a consequence of the fact that the data area of the chart is now so small. Because zooming/panning is effectively setting the LogicalExtent of the chart, and this is now only the narrow rectangle defined by MinDataSpace.

If you want to debug by yourself you find the zooming/panning code in the TATools unit (TZoomDragTool, TZoomPanTool). The other zoom/pan tools are probably affected as well.

If you submit a patch open new issue.

Marcin Wiazowski

2019-02-02 19:49

reporter   ~0113804

It seems that there are some issues in EnsureGuaranteedSpace():



1) There is:

    AValue1 := (AImage1 + AImage2 + AGuaranteed) div 2;
    AValue2 := AValue1 + AGuaranteed;

where AImage1 and AImage2 give a number of the first and the last pixel in our range to be painted.

I suppose that the intention was to find a middle of this pixel range, i.e. "(AImage1 + AImage2)/2", and then enlarge it in both sides by "AGuaranteed/2" - just an idea like:

    AValue1 := (AImage1 + AImage2)/2 - AGuaranteed/2
    AValue2 := (AImage1 + AImage2)/2 + AGuaranteed/2

or:

    AValue1 := (AImage1 + AImage2)/2 - AGuaranteed/2
    AValue2 := AValue1 + AGuaranteed

or:

    AValue1 := (AImage1 + AImage2 - AGuaranteed)/2
    AValue2 := AValue1 + AGuaranteed

but we need integer values, not floating point ones:

    AValue1 := (AImage1 + AImage2 - AGuaranteed) div 2; <======== "+" CHANGED TO "-" HERE
    AValue2 := AValue1 + AGuaranteed

So it seems, that - when comparing to the original formula - "- AGuaranteed" should be used instead of "+ AGuaranteed".



2) Not a bug, only a possible improvement: Instead of calculating delta1 and delta2 at the beginning of the procedure, they might be calculated later - when it is already known, that they are really needed (see the code below).



3) Not a bug, only a possible improvement:

  if (AValue2 = AImage2 - AMargin2) then begin
    AValue2 := AImage2 - AMargin2; <======== WE JUST CHECKED that AValue2 is equal to AImage2 - AMargin2 -> no need to make this assignment, this line can be removed
    AValue1 := AValue2 - AGuaranteed;
  end else
  if (AValue1 = AImage1 + AMargin1) then begin
    AValue1 := AImage1 + AMargin1; <======== WE JUST CHECKED that AValue1 is equal to AImage1 + AMargin1 -> no need to make this assignment, this line can be removed
    AValue2 := AValue1 + AGuaranteed;
  end else



After incorporating these changes, the code could be:


procedure EnsureGuaranteedSpace(var AValue1, AValue2: Integer;
  AImage1, AImage2, AMargin1, AMargin2, AGuaranteed: Integer);
var
  delta1, delta2: Integer;
begin
  if (AValue2 = AImage2 - AMargin2) then
    AValue1 := AValue2 - AGuaranteed <======== useless line removed above
  else
  if (AValue1 = AImage1 + AMargin1) then
    AValue2 := AValue1 + AGuaranteed <======== useless line removed above
  else
  begin
    delta1 := AImage1 - AValue1; <======== assigning delta1 and delta2 moved here
    delta2 := AImage2 - AValue2;
    AValue1 := (AImage1 + AImage2 - AGuaranteed) div 2; <======== "+" changed to "-" here
    AValue2 := AValue1 + AGuaranteed;
    if AValue1 + delta1 >= AImage1 then begin
      AValue1 := AImage1 - delta1;
      AValue2 := AValue1 + AGuaranteed;
    end else
    if AValue2 + delta2 <= AImage2 then begin
      AValue2 := AImage2 - delta2;
      AValue1 := AValue2 - AGuaranteed;
    end;
  end;
end;


Regards

wp

2019-02-02 23:47

developer   ~0113808

Thank you, applied. A nice example that the number of bugs in software having reached some level of "perfection" cannot be reduced any more: when one bug is fixed another one is introduced.

Marcin Wiazowski

2019-02-03 02:00

reporter  

AnimationTest.gif (71,455 bytes)
AnimationTest.gif (71,455 bytes)

Marcin Wiazowski

2019-02-03 02:01

reporter  

Test.zip (3,725 bytes)

Marcin Wiazowski

2019-02-03 02:02

reporter  

test.diff (3,616 bytes)
Index: components/tachart/tachartaxis.pas
===================================================================
--- components/tachart/tachartaxis.pas	(revision 60310)
+++ components/tachart/tachartaxis.pas	(working copy)
@@ -252,7 +252,7 @@
     function CalcScale(ASign: Integer): Double;
     constructor Init(AAxis: TChartAxis; AImageLo, AImageHi: Integer;
       AMarginLo, AMarginHi, ARequiredMarginLo, ARequiredMarginHi, AMinDataSpace: Integer;
-      AHasMarksInMargin, AAxisIsVertical: Boolean; AMin, AMax: PDouble);
+      AAxisIsVertical: Boolean; AMin, AMax: PDouble);
     procedure UpdateMinMax(AConv: TAxisConvFunc);
   end;
 
@@ -1218,14 +1218,19 @@
 procedure EnsureGuaranteedSpace(var AValue1, AValue2: Integer;
   AImage1, AImage2, AMargin1, AMargin2, AGuaranteed: Integer);
 var
+  HasMarksInMargin1, HasMarksInMargin2: Boolean;
   delta1, delta2: Integer;
 begin
-  if (AValue2 = AImage2 - AMargin2) then
+  HasMarksInMargin1 := (AValue1 = AImage1 + AMargin1);
+  HasMarksInMargin2 := (AValue2 = AImage2 - AMargin2);
+
+  if HasMarksInMargin2 and not HasMarksInMargin1 then
     AValue1 := AValue2 - AGuaranteed
   else
-  if (AValue1 = AImage1 + AMargin1) then
+  if HasMarksInMargin1 and not HasMarksInMargin2 then
     AValue2 := AValue1 + AGuaranteed
-  else begin
+  else
+  begin
     delta1 := AImage1 - AValue1;
     delta2 := AImage2 - AValue2;
     AValue1 := (AImage1 + AImage2 - AGuaranteed) div 2;
@@ -1243,7 +1248,7 @@
 
 constructor TAxisCoeffHelper.Init(AAxis: TChartAxis; AImageLo, AImageHi: Integer;
   AMarginLo, AMarginHi, ARequiredMarginLo, ARequiredMarginHi, AMinDataSpace: Integer;
-  AHasMarksInMargin, AAxisIsVertical: Boolean; AMin, AMax: PDouble);
+  AAxisIsVertical: Boolean; AMin, AMax: PDouble);
 begin
   FAxisIsFlipped := (AAxis <> nil) and AAxis.IsFlipped;
   FImageLo := AImageLo;
@@ -1253,16 +1258,14 @@
   FLo := FImageLo + AMarginLo;
   FHi := FImageHi + AMarginHi;
 
-  if AHasMarksInMargin then begin
-    if AAxisIsVertical then begin
-      if (FHi + AMinDataSpace >= FLo) then
-        EnsureGuaranteedSpace(FHi, FLo, FImageHi, FImageLo,
-          ARequiredMarginHi, ARequiredMarginLo, AMinDataSpace);
-    end else begin
-      if (FLo + AMinDataSpace >= FHi) then
-        EnsureGuaranteedSpace(FLo, FHi, FImageLo, FImageHi,
-          ARequiredMarginLo, ARequiredMarginHi, AMinDataSpace);
-    end;
+  if AAxisIsVertical then begin
+    if (FHi + AMinDataSpace >= FLo) then
+      EnsureGuaranteedSpace(FHi, FLo, FImageHi, FImageLo,
+        ARequiredMarginHi, ARequiredMarginLo, AMinDataSpace);
+  end else begin
+    if (FLo + AMinDataSpace >= FHi) then
+      EnsureGuaranteedSpace(FLo, FHi, FImageLo, FImageHi,
+        ARequiredMarginLo, ARequiredMarginHi, AMinDataSpace);
   end;
 end;
 
Index: components/tachart/tagraph.pas
===================================================================
--- components/tachart/tagraph.pas	(revision 60310)
+++ components/tachart/tagraph.pas	(working copy)
@@ -594,12 +594,10 @@
   rX.Init(
     HorAxis, FClipRect.Left, FClipRect.Right, AMargin.Left, -AMargin.Right,
     AChartMargins.Left, AChartMargins.Right, AMinDataSpace,
-    (AMargin.Left <> AChartMargins.Left) or (AMargin.Right <> AChartMargins.Right),
     false, @FCurrentExtent.a.X, @FCurrentExtent.b.X);
   rY.Init(
     VertAxis, FClipRect.Bottom, FClipRect.Top, -AMargin.Bottom, AMargin.Top,
     AChartMargins.Bottom, AChartMargins.Top, AMinDataSpace,
-    (AMargin.Top <> AChartMargins.Top) or (AMargin.Bottom <> AChartMargins.Bottom),
     true, @FCurrentExtent.a.Y, @FCurrentExtent.b.Y);
 
   FScale.X := rX.CalcScale(1);
test.diff (3,616 bytes)

Marcin Wiazowski

2019-02-03 02:03

reporter   ~0113809

I just realized, that your code also gracefully handles situations, when no marks are visible - only slight modification is needed, so cases when there are no marks are treated as cases when marks are visible both above and below bars (i.e. when MarkPositions = lmpOutside).

Please take a look at Test application, AnimationTest.gif and test.diff and see if you like it. In test application, pink background represents chart margins' area - I made margins larger than usual, to make the behavior better visible. You may also play with buttons to add some marks, but - in this case - there is no difference after applying the test.diff.

wp

2019-02-03 12:00

developer   ~0113813

Yes, behaves more consistently. Applied in r60311.

Marcin Wiazowski

2019-02-03 19:11

reporter  

Panning1.mp4 (625,626 bytes)

Marcin Wiazowski

2019-02-03 19:11

reporter  

Panning2.mp4 (249,447 bytes)

Marcin Wiazowski

2019-02-03 19:11

reporter  

Zooming1.mp4 (304,529 bytes)

Marcin Wiazowski

2019-02-03 19:12

reporter  

Zooming2.mp4 (255,004 bytes)

Marcin Wiazowski

2019-02-03 19:13

reporter   ~0113837

I need a bit of your help.

I tried to reproduce the zooming and panning issues, that you described - this time with the latest patches, i.e. with r60312. I used the Test.zip application attached here, with one modification - after clicking the button, 5 lines of mark's text are added.



> To demonstrate the panning issue, you can use any of your demos. Add as many rows until the longest label overflows. Drag the chart with the right mouse pressed to a different location. After a short vertical movement of the mouse the series disappears. A horizontal movement is ok.

I can't currently confirm - nothing disappears. Although, there is a bit strange behavior, which can be seen even for 1-line marks - although for multiline marks it's stronger, so better visible. Please see the attached videos:

a) Panning1.mp4: initially, there is enough space for bars and marks.

b) Panning2.mp4: initially, there is NOT enough space for bars and marks.



> For zooming, again add as many rows until the longest label overflows. Drag with the left mouse button pressed, from left/top to bottom/right, a rectangle tightly around a bar of the series. When you release the mouse button the rectangle should be blown up to fill the chart area, it does not. Depending on the size of the zoom rect, the bar can even become smaller.

Currently, I can't see any strange a behavior here. The only thing that I can see is: when I select a rectangle tightly around the bar:

a) Zooming1.mp4: there is enough space, so the bar (plus its mark, if exists) is enlarged in such way, that it (plus its mark, if exists) fills the whole available height - i.e. only pink margins (above and below) remain preserved.

b) Zooming2.mp4: there is NOT enough space, so the bar (plus its mark, if exists) is enlarged in such way, that it (plus its mark, if exists) fills the whole available height, plus also uses some space of pink margins - but it is as designed, just EnsureGuaranteedSpace() makes its work.



I just want to ask you to take a look at this and confirm, or deny, my observations - and tell me if you can see any behavior, that you recognize as incorrect (or that could be just improved).

Regards

wp

2019-02-03 22:25

developer   ~0113839

Last edited: 2019-02-03 22:29

View 3 revisions

According to documentation, zooming and panning are intended to do nothing else than setting the LogicalExtent
(http://wiki.lazarus.freepascal.org/TAChart_documentation#Extents):

* For zooming the user can paint the new LogicalExtent on the chart by dragging a rectangle from the top/left to bottom/right corners (using the left mouse button). When the mouse is released this rectangle will become the new LogicalExtent.

* For Panning the user drags the viewport to a new location (using the right mouse button).

Panning
=======
* Add some more lines to the 2nd mark in your demo "Test":

  procedure TForm1.ButtonShowHideMark2Click(Sender: TObject);
  begin
    with ListChartSource1 do
    if Item[1]^.Text = '' then
      SetText(1,'bbbb'0000013#10'bbbb'0000013'ddd'0000013'eee'0000013'fff'0000013'ggggg'0000013 +
        'hhhh'0000013'iiii'0000013'jj'0000013'kkkk'0000013'lll'0000013'mmmm')
    else
      SetText(1,'');
  end;
  
* Reset the chart's top and bottom Margins to something normal, such as 4.

* Press the RIGHT mouse button somewhere in the chart and drag it around.

* When marks are off everything behaves as expected, the entire chart nicely follows the mouse, no size changes.

* Click into the chart (with left button) to restore the original position.

* Now turn on the 2nd (huge) mark. Drag the mouse (right button) a bit towards the top --> suddenly the bar becomes much larger (because the guaranteed_space mechanism is turned off). Or drag the mouse down so that the top of the bar disappears below the bottom axis --> suddenly the mark disappears (because the Chart.IsPointInViewPort is no longer true)
  
* Both effects are not expected by the user and should be avoided.


Zooming
=======
I must say that the effect was more drastic before today's update because in the old revision the LogicalExtent could become very small, now it cannot become smaller than the MinDataSpace.

* At first, in order to see the effect of "correct" zooming have both marks turned off and drag the zoom rect closely around the first bar --> when the mouse button is released the bar grows to closely fill the viewport. That's correct.

* Restore the original unzoomed state by clicking with the left button into the chart.

* Turn on the marks of the first bar. Drag a rectangle again closely around the bar without the mark --> the bar increases in size, but the mark is visible. Well - basically I would not expect to see the mark, but with AutoMargins active that's probably how it should be because the chart always tries to show the mark; and since normally the marks are small the zooming effect is clear and appears to be correct.

* But: after resetting the zoom drag a larger rectangle which tightly encloses the bar and the mark --> the bottom part of the mark is expected to be near the x axis, but it turns out to be somewhere in the center of the chart. This is disturbing although this behaviour has been there all the time.

* Reset the zoom again and turn on the marks of the second bar, the one with the very high marks. Again drag the
zoom rect around the bar only --> the size of the bar does not change because due the size reserved for the margin it can only be drawn in the narrow MinDataSpace. This is not expected.

* Drag a larger zoom rect which encloses the last mark row ('mmmm') and reaches down to the lower end of the bar --> the bar becomes smaller. Again, not expected, some kind of "anti-zooming"...

I did not spend much work on these phenomena, but is seems to me that they are a consequence of the fact that the zoomed region is defined by the LogicalExtent, not by the CurrentExtent.

Marcin Wiazowski

2019-02-04 00:05

reporter   ~0113844

Thanks for your detailed description. I was able to reproduce all the problems, that you described above. To continue discussion about zooming and panning, I created a separate report 0035002.



So this report might be basically closed, but I found the last two issues:



1) In TChart.PrepareAxis(), there is:

  CalculateTransformationCoeffs(ZeroRect, scChartMargins, scMinDataSpace);
  ...
  for tries := 1 to 10 do begin
    ...
    CalculateTransformationCoeffs(scSeriesMargins, scChartMargins, scMinDataSpace);
    ...
  end;

As can be seen, ZeroRect is passed to the first call to CalculateTransformationCoeffs() as series' margins. This makes no sense, because series' margins are either equal to chart's margins, or larger (when some additional space for marks is needed) - but they are never smaller. So passing ZeroRect only disturbs in achieving a convergence in the loop that is below. The intention of this call was to make an initialization as if no ADDITIONAL margins were needed (for marks) - so scChartMargins should be passed as series' margins, instead of ZeroRect:

  CalculateTransformationCoeffs(ZeroRect, scChartMargins, scMinDataSpace);

should be changed to:

  CalculateTransformationCoeffs(scChartMargins, scChartMargins, scMinDataSpace);

This makes the convergence (in the loop below) faster / better.




2) In TChart.Create(), FScale is initialized as:

  FScale := DoublePoint(1, 1);

but Y axis is directed in the opposite order than vertical image coordinates, so it should be initialized as:

  FScale := DoublePoint(1, -1);

wp

2019-02-04 19:04

developer   ~0113853

Fixed, thank you.

Marcin Wiazowski

2019-02-25 19:11

reporter   ~0114423

All the problems are solved now.



Just to note: I was talking about:

  CalculateTransformationCoeffs(scChartMargins, scChartMargins, ...

however, you changed the code to:

  CalculateTransformationCoeffs(scSeriesMargins, scChartMargins, ...



But your update is even better. And - as we can see - it also works properly. So it should stay as it is now.

Issue History

Date Modified Username Field Change
2019-01-18 21:14 Marcin Wiazowski New Issue
2019-01-18 21:14 Marcin Wiazowski File Added: Calculations.pdf
2019-01-18 21:14 Marcin Wiazowski File Added: patch.diff
2019-01-19 00:44 wp Assigned To => wp
2019-01-19 00:44 wp Status new => assigned
2019-01-19 00:51 wp Note Added: 0113471
2019-01-19 00:52 wp LazTarget => -
2019-01-19 00:52 wp Status assigned => feedback
2019-01-19 02:55 Marcin Wiazowski Note Added: 0113472
2019-01-19 02:55 Marcin Wiazowski Status feedback => assigned
2019-01-19 02:56 Marcin Wiazowski File Added: Animation.gif
2019-01-19 02:56 Marcin Wiazowski File Added: Reproduce.zip
2019-01-19 17:19 wp Note Added: 0113486
2019-01-19 17:19 wp Status assigned => feedback
2019-01-19 21:40 Marcin Wiazowski File Added: Reproduce2.zip
2019-01-19 21:41 Marcin Wiazowski File Added: patch_extended.diff
2019-01-19 21:45 Marcin Wiazowski Note Added: 0113495
2019-01-19 21:45 Marcin Wiazowski Status feedback => assigned
2019-01-19 23:38 wp Note Added: 0113498
2019-01-20 00:44 Marcin Wiazowski Note Added: 0113504
2019-01-20 00:58 wp Note Added: 0113505
2019-01-20 01:32 Marcin Wiazowski Note Added: 0113506
2019-01-21 00:42 wp Note Added: 0113540
2019-01-21 00:43 wp File Added: LongSeriesLabels_Test.zip
2019-01-21 00:44 wp Relationship added related to 0034819
2019-01-21 00:46 wp Note Edited: 0113540 View Revisions
2019-01-21 00:47 wp Fixed in Revision => 60129
2019-01-21 00:47 wp LazTarget - => 2.0
2019-01-21 00:47 wp Status assigned => resolved
2019-01-21 00:47 wp Resolution open => fixed
2019-01-21 00:47 wp Target Version => 2.0
2019-01-21 02:13 Marcin Wiazowski Note Added: 0113541
2019-01-23 00:19 wp Note Added: 0113588
2019-01-23 14:49 Marcin Wiazowski Note Added: 0113596
2019-01-23 14:50 Marcin Wiazowski File Added: Crash.zip
2019-01-23 15:21 Marcin Wiazowski Note Added: 0113597
2019-01-24 00:47 wp Note Added: 0113603
2019-02-02 19:49 Marcin Wiazowski Note Added: 0113804
2019-02-02 19:49 Marcin Wiazowski Status resolved => assigned
2019-02-02 19:49 Marcin Wiazowski Resolution fixed => reopened
2019-02-02 23:47 wp Note Added: 0113808
2019-02-02 23:56 wp Fixed in Revision 60129 => 60129, 60309
2019-02-02 23:56 wp Status assigned => resolved
2019-02-02 23:56 wp Resolution reopened => fixed
2019-02-03 02:00 Marcin Wiazowski File Added: AnimationTest.gif
2019-02-03 02:01 Marcin Wiazowski File Added: Test.zip
2019-02-03 02:02 Marcin Wiazowski File Added: test.diff
2019-02-03 02:03 Marcin Wiazowski Note Added: 0113809
2019-02-03 02:03 Marcin Wiazowski Status resolved => assigned
2019-02-03 02:03 Marcin Wiazowski Resolution fixed => reopened
2019-02-03 12:00 wp Fixed in Revision 60129, 60309 => 60129, 60309, 60311
2019-02-03 12:00 wp Note Added: 0113813
2019-02-03 12:00 wp Status assigned => resolved
2019-02-03 12:00 wp Resolution reopened => fixed
2019-02-03 19:11 Marcin Wiazowski File Added: Panning1.mp4
2019-02-03 19:11 Marcin Wiazowski File Added: Panning2.mp4
2019-02-03 19:11 Marcin Wiazowski File Added: Zooming1.mp4
2019-02-03 19:12 Marcin Wiazowski File Added: Zooming2.mp4
2019-02-03 19:13 Marcin Wiazowski Note Added: 0113837
2019-02-03 19:13 Marcin Wiazowski Status resolved => assigned
2019-02-03 19:13 Marcin Wiazowski Resolution fixed => reopened
2019-02-03 22:25 wp Note Added: 0113839
2019-02-03 22:28 wp Note Edited: 0113839 View Revisions
2019-02-03 22:29 wp Note Edited: 0113839 View Revisions
2019-02-04 00:05 Marcin Wiazowski Note Added: 0113844
2019-02-04 19:04 wp Fixed in Revision 60129, 60309, 60311 => 60129, 60309, 60311, 60334
2019-02-04 19:04 wp Note Added: 0113853
2019-02-04 19:04 wp Status assigned => resolved
2019-02-04 19:04 wp Resolution reopened => fixed
2019-02-04 19:04 wp Status resolved => assigned
2019-02-04 19:04 wp Resolution fixed => reopened
2019-02-04 19:05 wp Fixed in Revision 60129, 60309, 60311, 60334 => in v2.0: 60129 and 60309, in v2.02: 60311, 60334
2019-02-04 19:05 wp LazTarget 2.0 => 2.0.2
2019-02-04 19:05 wp Status assigned => resolved
2019-02-04 19:05 wp Resolution reopened => fixed
2019-02-04 19:05 wp Target Version 2.0 => 2.0.2
2019-02-25 19:11 Marcin Wiazowski Note Added: 0114423
2019-02-25 19:11 Marcin Wiazowski Status resolved => closed