View Issue Details

IDProjectCategoryView StatusLast Update
0034935LazarusTAChartpublic2019-01-25 22:45
ReporterMarcin WiazowskiAssigned Towp 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version2.1 (SVN)Product Build60161 
Target Version2.0Fixed in Version 
Summary0034935: TAChart: TBasicPointSeries uses uninitialized variables
Description    TAChart: TBasicPointSeries uses uninitialized variables


Please launch the attached Reproduce application.

a) After start, bars' marks are invisible. Minimize the application, and maximize it again (so chart will be repainted) - right bar's mark will become visible then (left bar's mark should stay invisible - this is OK).

b) Press "Add data" button - a bar will be added, but its mark will be truncated. Minimize the application, and maximize it again (so chart will be repainted) - bar will not be truncated anymore.



The problem manifests itself in TBasicPointSeries.UpdateMargins() - there is a loop:

  for i := FLoBound to FUpBound do begin

FLoBound and FUpBound are used as a range of data, that is potentially visible on the chart - it's much faster to perform painting through FLoBound..FUpBound than through 0..Source.Count-1. They are initialized in a TBasicPointSeries.FindExtentInterval call, but - interestingly - at every chart painting, TBasicPointSeries.FindExtentInterval() is called AFTER TBasicPointSeries.UpdateMargins() - so, each time, TBasicPointSeries.UpdateMargins() uses out-of-date FLoBound and FUpBound!



Some symptoms:

1) FLoBound and FUpBound are NOT initialized in TBasicPointSeries.Create, so they are both 0 - although they should reflect default data source's state.

2) FLoBound and FUpBound are NOT reinitialized when data source is changed (for example some data points have been deleted) - so they may be out of bounds.

3) FLoBound and FUpBound are NOT reinitialized when a new data source is assigned to TBasicPointSeries.Source - so they may be out of bounds.



The attached patch sets FLoBound to 0 and FUpBound to Source.Count-1 each time, when data source changes, and also in TBasicPointSeries.Create - so iterations will be slow, but, at least, there is no more danger of out-of-bounds memory accesses. The patch is very rough, and should be only treated as a reference (there was a need to access strict protected fields from an other class, so I changed declarations and made some hacking - some code redesigning should solve this).



But a long-term solution should also, during the chart painting, call the TBasicPointSeries.FindExtentInterval() before the TBasicPointSeries.UpdateMargins(), to make painting as fast as it should be.

Regards
TagsNo tags attached.
Fixed in Revision60163, 60218
LazTarget2.0
WidgetsetWin32/Win64
Attached Files
  • Reproduce.zip (3,362 bytes)
  • patch.diff (1,431 bytes)
    Index: components/tachart/tacustomseries.pas
    ===================================================================
    --- components/tachart/tacustomseries.pas	(revision 60162)
    +++ components/tachart/tacustomseries.pas	(working copy)
    @@ -270,13 +270,14 @@
         procedure SetPointer(AValue: TSeriesPointer);
         procedure SetStacked(AValue: Boolean);
         procedure SetUseReticule(AValue: Boolean); deprecated 'Use DatapointCrosshairTool instead';
    +  protected
    +    FLoBound: Integer;
    +    FUpBound: Integer;
       strict protected
         FGraphPoints: array of TDoublePoint;
    -    FLoBound: Integer;
         FMinXRange: Double;
         FPointer: TSeriesPointer;
         FStacked: Boolean;
    -    FUpBound: Integer;
         FUseReticule: Boolean;
         FOptimizeX: Boolean;
         FSupportsZeroLevel: Boolean;
    @@ -1089,8 +1090,17 @@
         ListSource.Item[AIndex]^.YList[AYIndex - 1] := AValue;
     end;
     
    +type
    +  THookBasicPointSeries = class(TBasicPointSeries)
    +  end;
    +
     procedure TChartSeries.SourceChanged(ASender: TObject);
     begin
    +  if Self is TBasicPointSeries then
    +  begin
    +    THookBasicPointSeries(Self).FLoBound := 0;
    +    THookBasicPointSeries(Self).FUpBound := Count - 1;
    +  end;
       StyleChanged(ASender);
     end;
     
    @@ -1144,6 +1154,8 @@
       FErrorBars[1] := TChartErrorBar.Create(FChart);
       FOptimizeX := true;
       ToolTargets := [nptPoint, nptYList];
    +  FLoBound := 0;
    +  FUpBound := Count - 1;
     end;
     
     destructor TBasicPointSeries.Destroy;
    
    patch.diff (1,431 bytes)
  • patch_ver2.diff (1,059 bytes)
    Index: components/tachart/tacustomseries.pas
    ===================================================================
    --- components/tachart/tacustomseries.pas	(revision 60162)
    +++ components/tachart/tacustomseries.pas	(working copy)
    @@ -308,6 +308,7 @@
     
       protected
         procedure AfterAdd; override;
    +    procedure SourceChanged(ASender: TObject); override;
         procedure UpdateMargins(ADrawer: IChartDrawer; var AMargins: TRect); override;
     
         property MarkPositionCentered: Boolean
    @@ -1143,6 +1144,8 @@
       FErrorBars[0] := TChartErrorBar.Create(FChart);
       FErrorBars[1] := TChartErrorBar.Create(FChart);
       FOptimizeX := true;
    +  FLoBound := 0;
    +  FUpBound := Count - 1;
       ToolTargets := [nptPoint, nptYList];
     end;
     
    @@ -1770,6 +1773,13 @@
       UpdateGraphPoints(AIndex, FLoBound, FUpBound, ACumulative);
     end;
     
    +procedure TBasicPointSeries.SourceChanged(ASender: TObject);
    +begin
    +  FLoBound := 0;
    +  FUpBound := Count - 1;
    +  inherited;
    +end;
    +
     procedure TBasicPointSeries.UpdateMargins(
       ADrawer: IChartDrawer; var AMargins: TRect);
     var
    
    patch_ver2.diff (1,059 bytes)
  • Reproduce2.zip (3,426 bytes)
  • patchB.diff (3,157 bytes)
    Index: components/tachart/tacustomseries.pas
    ===================================================================
    --- components/tachart/tacustomseries.pas	(revision 60207)
    +++ components/tachart/tacustomseries.pas	(working copy)
    @@ -1792,6 +1792,19 @@
       if not Marks.IsMarkLabelsVisible or not Marks.AutoMargins then exit;
       if Count = 0 then exit;
     
    +  {FLoBound and FUpBound fields may be outdated here (if axis' range has been
    +   changed after the last series' painting). FLoBound and FUpBound will be fully
    +   updated later, in a PrepareGraphPoints() call. But we need them now. If data
    +   source is sorted, obtaining FLoBound and FUpBound is very fast (binary search) -
    +   so we call FindExtentInterval() with True as the second parameter. If data
    +   source is not sorted, obtaining FLoBound and FUpBound requires enumerating all
    +   the data points to see, if they are in the current chart's viewport. But this
    +   is exactly what we are going to do in the loop below, so obtaining true FLoBound
    +   and FUpBound values makes no sense in this case - so we call FindExtentInterval()
    +   with False as the second parameter, thus setting FLoBound to 0 and FUpBound to
    +   Count-1}
    +  FindExtentInterval(ParentChart.CurrentExtent, Source.IsSorted);
    +
       for i := FLoBound to FUpBound do begin
         gp := GetGraphPoint(i);
         if not ParentChart.IsPointInViewPort(gp) then continue;
    Index: components/tachart/tagraph.pas
    ===================================================================
    --- components/tachart/tagraph.pas	(revision 60207)
    +++ components/tachart/tagraph.pas	(working copy)
    @@ -322,7 +322,7 @@
         {$ENDIF}
         procedure Notification(
           AComponent: TComponent; AOperation: TOperation); override;
    -    procedure PrepareAxis(ADrawer: IChartDrawer);
    +      procedure PrepareAxis(ADrawer: IChartDrawer);
         function PrepareLegend(
           ADrawer: IChartDrawer; var AClipRect: TRect): TChartLegendDrawingData;
         procedure SetBiDiMode(AValue: TBiDiMode); override;
    @@ -1465,10 +1465,12 @@
       prevExt: TDoubleRect;
       axis: TChartAxis;
       scDepth: Integer;
    +  scSeriesMargins: TRect;
       scChartMargins: TRect;
       scMinDataSpace: Integer;
     begin
       scDepth := ADrawer.Scale(Depth);
    +  scSeriesMargins := GetMargins(ADrawer);
       scChartMargins.Left := ADrawer.Scale(Margins.Left);
       scChartMargins.Right := ADrawer.Scale(Margins.Right);
       scChartMargins.Top := ADrawer.Scale(Margins.Top);
    @@ -1478,7 +1480,7 @@
       if not AxisVisible then begin
         FClipRect.Left += scDepth;
         FClipRect.Bottom -= scDepth;
    -    CalculateTransformationCoeffs(GetMargins(ADrawer), scChartMargins, scMinDataSpace);
    +    CalculateTransformationCoeffs(scSeriesMargins, scChartMargins, scMinDataSpace);
         exit;
       end;
     
    @@ -1499,7 +1501,7 @@
           SideByAlignment(FClipRect, aa, -axisMargin[aa]);
         prevExt := FCurrentExtent;
         FCurrentExtent := FLogicalExtent;
    -    CalculateTransformationCoeffs(GetMargins(ADrawer), scChartMargins, scMinDataSpace);
    +    CalculateTransformationCoeffs(scSeriesMargins, scChartMargins, scMinDataSpace);
         if prevExt = FCurrentExtent then break;
         prevExt := FCurrentExtent;
       end;
    
    patchB.diff (3,157 bytes)

Activities

Marcin Wiazowski

2019-01-24 00:25

reporter  

Reproduce.zip (3,362 bytes)

Marcin Wiazowski

2019-01-24 00:31

reporter   ~0113602

The attached patch is for newest r60162.

Marcin Wiazowski

2019-01-24 00:31

reporter  

patch.diff (1,431 bytes)
Index: components/tachart/tacustomseries.pas
===================================================================
--- components/tachart/tacustomseries.pas	(revision 60162)
+++ components/tachart/tacustomseries.pas	(working copy)
@@ -270,13 +270,14 @@
     procedure SetPointer(AValue: TSeriesPointer);
     procedure SetStacked(AValue: Boolean);
     procedure SetUseReticule(AValue: Boolean); deprecated 'Use DatapointCrosshairTool instead';
+  protected
+    FLoBound: Integer;
+    FUpBound: Integer;
   strict protected
     FGraphPoints: array of TDoublePoint;
-    FLoBound: Integer;
     FMinXRange: Double;
     FPointer: TSeriesPointer;
     FStacked: Boolean;
-    FUpBound: Integer;
     FUseReticule: Boolean;
     FOptimizeX: Boolean;
     FSupportsZeroLevel: Boolean;
@@ -1089,8 +1090,17 @@
     ListSource.Item[AIndex]^.YList[AYIndex - 1] := AValue;
 end;
 
+type
+  THookBasicPointSeries = class(TBasicPointSeries)
+  end;
+
 procedure TChartSeries.SourceChanged(ASender: TObject);
 begin
+  if Self is TBasicPointSeries then
+  begin
+    THookBasicPointSeries(Self).FLoBound := 0;
+    THookBasicPointSeries(Self).FUpBound := Count - 1;
+  end;
   StyleChanged(ASender);
 end;
 
@@ -1144,6 +1154,8 @@
   FErrorBars[1] := TChartErrorBar.Create(FChart);
   FOptimizeX := true;
   ToolTargets := [nptPoint, nptYList];
+  FLoBound := 0;
+  FUpBound := Count - 1;
 end;
 
 destructor TBasicPointSeries.Destroy;
patch.diff (1,431 bytes)

Marcin Wiazowski

2019-01-24 00:53

reporter  

patch_ver2.diff (1,059 bytes)
Index: components/tachart/tacustomseries.pas
===================================================================
--- components/tachart/tacustomseries.pas	(revision 60162)
+++ components/tachart/tacustomseries.pas	(working copy)
@@ -308,6 +308,7 @@
 
   protected
     procedure AfterAdd; override;
+    procedure SourceChanged(ASender: TObject); override;
     procedure UpdateMargins(ADrawer: IChartDrawer; var AMargins: TRect); override;
 
     property MarkPositionCentered: Boolean
@@ -1143,6 +1144,8 @@
   FErrorBars[0] := TChartErrorBar.Create(FChart);
   FErrorBars[1] := TChartErrorBar.Create(FChart);
   FOptimizeX := true;
+  FLoBound := 0;
+  FUpBound := Count - 1;
   ToolTargets := [nptPoint, nptYList];
 end;
 
@@ -1770,6 +1773,13 @@
   UpdateGraphPoints(AIndex, FLoBound, FUpBound, ACumulative);
 end;
 
+procedure TBasicPointSeries.SourceChanged(ASender: TObject);
+begin
+  FLoBound := 0;
+  FUpBound := Count - 1;
+  inherited;
+end;
+
 procedure TBasicPointSeries.UpdateMargins(
   ADrawer: IChartDrawer; var AMargins: TRect);
 var
patch_ver2.diff (1,059 bytes)

Marcin Wiazowski

2019-01-24 00:54

reporter   ~0113604

The attached patch_ver2.diff does not use hacking anymore, and is ready to apply.

wp

2019-01-24 01:15

developer   ~0113606

Fixed. Thank you.

Marcin Wiazowski

2019-01-24 21:27

reporter   ~0113611

Now it's time to patch the root cause of the problem. The TChart.Draw procedure looks like:


procedure TChart.Draw(...);
begin
  Prepare; <=== FCurrentExtent is initialized here as "FCurrentExtent := FLogicalExtent"
  ...
  PrepareAxis(ADrawer); <=== FLoBound and FUpBound are used here (in TBasicPointSeries.UpdateMargins)
  ...

  ...
  DisplaySeries(ADrawer); <=== FLoBound and FUpBound are initialized here (in TBasicPointSeries.PrepareGraphPoints)
  ...
end;


As we can see, the TBasicPointSeries.UpdateMargins() call uses stale FLoBound and FUpBound values. Please launch the attached Reproduce2 application, then press a "Show more data" button. Horizontal axis' range will be widened, so additional data points will be shown - but last point will have its mark truncated. Now minimize the application, and maximize it again (so chart will be repainted) - mark will be properly shown then. This is because - initially - the last point has been outside of the stale FLoBound and FUpBound range, so it hasn't been used in the margin calculation process.


Solution: in the TBasicPointSeries.UpdateMargins() call, FLoBound and FUpBound should be recalculated. If data source is sorted, obtaining FLoBound and FUpBound is very fast (binary search), so we can get true FLoBound and FUpBound values. If data source is NOT sorted, obtaining FLoBound and FUpBound requires enumerating all the data points to see, if they are in the current chart's viewport. But this is exactly what is done in UpdateMargins(), so there is no need to make this work twice - so, for unsorted sources, it's enough to initialize FLoBound to 0 and FUpBound to Count-1, so all data points will be used for margin calculation.



One more observation: UpdateMargins() is called from TChart.GetMargins(), which is called in a loop in TChart.PrepareAxis():


  for tries := 1 to 10 do begin
    axisMargin := AxisList.Measure(CurrentExtent, scDepth);
    axisMargin[calLeft] := Max(axisMargin[calLeft], scDepth);
    axisMargin[calBottom] := Max(axisMargin[calBottom], scDepth);
    FClipRect := cr;
    for aa := Low(aa) to High(aa) do
      SideByAlignment(FClipRect, aa, -axisMargin[aa]);
    prevExt := FCurrentExtent;


HERE:
    FCurrentExtent := FLogicalExtent;
    CalculateTransformationCoeffs(GetMargins(ADrawer), scChartMargins, scMinDataSpace);


    if prevExt = FCurrentExtent then break;
    prevExt := FCurrentExtent;
  end;


Calling "GetMargins(ADrawer)" in a loop makes no sense, since one line above we have "FCurrentExtent := FLogicalExtent" - this ensures that FCurrentExtent has constant value between iterations, so the "GetMargins(ADrawer)" call returns always same result. And GetMargins() is time-consuming, since it enumerates all the visible series and - in case of unsorted data sources - all series' data points. Solution is obvious - the GetMargins() call should be cached, similarly to scChartMargins and scMinDataSpace. The FLogicalExtent variable has same value during the whole TChart.PrepareAxis() execution, so GetMargins() can be cached just at the beginning of the procedure, and cached value can be used in both places when "GetMargins(ADrawer)" is curently called.



The attached patchB.diff solves both described problems.

Marcin Wiazowski

2019-01-24 21:27

reporter  

Reproduce2.zip (3,426 bytes)

Marcin Wiazowski

2019-01-24 21:28

reporter  

patchB.diff (3,157 bytes)
Index: components/tachart/tacustomseries.pas
===================================================================
--- components/tachart/tacustomseries.pas	(revision 60207)
+++ components/tachart/tacustomseries.pas	(working copy)
@@ -1792,6 +1792,19 @@
   if not Marks.IsMarkLabelsVisible or not Marks.AutoMargins then exit;
   if Count = 0 then exit;
 
+  {FLoBound and FUpBound fields may be outdated here (if axis' range has been
+   changed after the last series' painting). FLoBound and FUpBound will be fully
+   updated later, in a PrepareGraphPoints() call. But we need them now. If data
+   source is sorted, obtaining FLoBound and FUpBound is very fast (binary search) -
+   so we call FindExtentInterval() with True as the second parameter. If data
+   source is not sorted, obtaining FLoBound and FUpBound requires enumerating all
+   the data points to see, if they are in the current chart's viewport. But this
+   is exactly what we are going to do in the loop below, so obtaining true FLoBound
+   and FUpBound values makes no sense in this case - so we call FindExtentInterval()
+   with False as the second parameter, thus setting FLoBound to 0 and FUpBound to
+   Count-1}
+  FindExtentInterval(ParentChart.CurrentExtent, Source.IsSorted);
+
   for i := FLoBound to FUpBound do begin
     gp := GetGraphPoint(i);
     if not ParentChart.IsPointInViewPort(gp) then continue;
Index: components/tachart/tagraph.pas
===================================================================
--- components/tachart/tagraph.pas	(revision 60207)
+++ components/tachart/tagraph.pas	(working copy)
@@ -322,7 +322,7 @@
     {$ENDIF}
     procedure Notification(
       AComponent: TComponent; AOperation: TOperation); override;
-    procedure PrepareAxis(ADrawer: IChartDrawer);
+      procedure PrepareAxis(ADrawer: IChartDrawer);
     function PrepareLegend(
       ADrawer: IChartDrawer; var AClipRect: TRect): TChartLegendDrawingData;
     procedure SetBiDiMode(AValue: TBiDiMode); override;
@@ -1465,10 +1465,12 @@
   prevExt: TDoubleRect;
   axis: TChartAxis;
   scDepth: Integer;
+  scSeriesMargins: TRect;
   scChartMargins: TRect;
   scMinDataSpace: Integer;
 begin
   scDepth := ADrawer.Scale(Depth);
+  scSeriesMargins := GetMargins(ADrawer);
   scChartMargins.Left := ADrawer.Scale(Margins.Left);
   scChartMargins.Right := ADrawer.Scale(Margins.Right);
   scChartMargins.Top := ADrawer.Scale(Margins.Top);
@@ -1478,7 +1480,7 @@
   if not AxisVisible then begin
     FClipRect.Left += scDepth;
     FClipRect.Bottom -= scDepth;
-    CalculateTransformationCoeffs(GetMargins(ADrawer), scChartMargins, scMinDataSpace);
+    CalculateTransformationCoeffs(scSeriesMargins, scChartMargins, scMinDataSpace);
     exit;
   end;
 
@@ -1499,7 +1501,7 @@
       SideByAlignment(FClipRect, aa, -axisMargin[aa]);
     prevExt := FCurrentExtent;
     FCurrentExtent := FLogicalExtent;
-    CalculateTransformationCoeffs(GetMargins(ADrawer), scChartMargins, scMinDataSpace);
+    CalculateTransformationCoeffs(scSeriesMargins, scChartMargins, scMinDataSpace);
     if prevExt = FCurrentExtent then break;
     prevExt := FCurrentExtent;
   end;
patchB.diff (3,157 bytes)

Marcin Wiazowski

2019-01-24 21:31

reporter   ~0113612

Additional note: I checked all the other places, where FLoBound and FUpBound are used, and it seems, that they are properly initialized there.

wp

2019-01-25 18:30

developer   ~0113618

Fixed, thank you. Please close if ok.

Marcin Wiazowski

2019-01-25 22:45

reporter   ~0113622

Fixed, thanks!

Issue History

Date Modified Username Field Change
2019-01-24 00:25 Marcin Wiazowski New Issue
2019-01-24 00:25 Marcin Wiazowski File Added: Reproduce.zip
2019-01-24 00:31 Marcin Wiazowski Note Added: 0113602
2019-01-24 00:31 Marcin Wiazowski File Added: patch.diff
2019-01-24 00:53 Marcin Wiazowski File Added: patch_ver2.diff
2019-01-24 00:54 Marcin Wiazowski Note Added: 0113604
2019-01-24 01:11 wp Assigned To => wp
2019-01-24 01:11 wp Status new => assigned
2019-01-24 01:15 wp Fixed in Revision => 60163
2019-01-24 01:15 wp LazTarget => 2.0
2019-01-24 01:15 wp Note Added: 0113606
2019-01-24 01:15 wp Status assigned => resolved
2019-01-24 01:15 wp Resolution open => fixed
2019-01-24 01:15 wp Target Version => 2.0
2019-01-24 21:27 Marcin Wiazowski Note Added: 0113611
2019-01-24 21:27 Marcin Wiazowski File Added: Reproduce2.zip
2019-01-24 21:28 Marcin Wiazowski File Added: patchB.diff
2019-01-24 21:31 Marcin Wiazowski Note Added: 0113612
2019-01-25 18:29 wp Status resolved => acknowledged
2019-01-25 18:30 wp Fixed in Revision 60163 => 60163, 60218
2019-01-25 18:30 wp Note Added: 0113618
2019-01-25 18:30 wp Status acknowledged => resolved
2019-01-25 22:45 Marcin Wiazowski Note Added: 0113622
2019-01-25 22:45 Marcin Wiazowski Status resolved => closed