View Issue Details

IDProjectCategoryView StatusLast Update
0035207LazarusTAChartpublic2019-03-09 20:05
ReporterMarcin WiazowskiAssigned Towp 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version2.1 (SVN)Product Build60623 
Target Version2.2Fixed in Version 
Summary0035207: TAChart: some series still use uninitialized scaling
DescriptionTest 1: Load the attached Reproduce application in IDE - when displaying application's form, IDE will hang for about 10 seconds.

This is because the form contains two charts having their X axis ranges from 0 to 100000000. Because chart scaling is used although not yet initialized properly, this leads to about 2*100000000 calls to some calculation function - which takes a lot of time.




Test 2: Go to tacustomfuncseries.pas and, at the beginning of TDrawFuncHelper.Create(), insert:

  if not ASeries.ParentChart.ScalingValid then
    raise EChartError.Create('Test failed');

Then start the Reproduce application under the debugger. In FormCreate(), there are 11 calls to functions, that internally create an TDrawFuncHelper object (which requires valid chart scaling) - 10 of them will raise a 'Test failed' exception. This means, that uninitialized chart scaling has been used.




Currently, there exist 11 different ways of using an internal TDrawFuncHelper object - this is as in FormCreate():
- TExpressionSeries.Draw()
- TExpressionSeries.GetGraphBounds()
- TExpressionSeries.GetNearestPoint()

- TFuncSeries.Draw()
- TFuncSeries.GetGraphBounds()
- TFuncSeries.GetNearestPoint()

- TCubicSplineSeries.Draw()
- TCubicSplineSeries.GetNearestPoint()

- TFitSeries.Draw()
- TFitSeries.Extent()
- TFitSeries.GetNearestPoint()

All of them are public functions, so they are allowed to fail, but not to hang. So all of them should validate the chart scaling before use.




The solution is as follows:

A) GetGraphBounds() and Extent() methods are mostly called in a scaling calculation loop, where - at the first iteration - scaling is not available. So:
- multi-pass scaling must be requested,
- it must be verified that scaling is valid before further actions.

B) Draw() and GetNearestPoint() methods are, under normal circumstances, called when scaling is already initialized. So the only needed action is:
- it must be verified that scaling is valid before further actions.

But, in B) case, it is also allowed to request multi-pass scaling - this doesn't hurt us in any way (this will be just ignored if performed beyond the scale calculation loop). Thanks to that, the code for case A) can also be used in case B).




Since it's not good to copy same code to 11 places, I created a function for checking / multi-pass scaling requesting. This function must work on series' FChart field, which is implemented in TBasicChartSeries - so I implemented an "RequestValidChartScaling" function there.

The TChart's "MultiPassScalingNeeded" method, that has been introduced few days ago, I renamed to "RequestMultiPassScaling" - just to be more similar to "RequestValidChartScaling".

To avoid further problems with uninitialized scaling, I also added exceptions where the scaling is used at the lowest level - i.e. to:
- TChart.XGraphToImage()
- TChart.XImageToGraph()
- TChart.YGraphToImage()
- TChart.YImageToGraph()
As I checked, this adds only microseconds to chart painting, so should not be a problem.

Thanks to these new exceptions, I also found references to uninitialized scaling in TBasicPointSeries.GetNearestPoint() - so I fixed the problem.

And, of course, the attached patch solves also problems in TExpressionSeries, TFuncSeries, TCubicSplineSeries and TFitSeries.
TagsNo tags attached.
Fixed in Revision60636
LazTarget2.2
WidgetsetWin32/Win64
Attached Files
  • Reproduce.zip (2,950 bytes)
  • patch.diff (7,389 bytes)
    Index: components/tachart/tacustomseries.pas
    ===================================================================
    --- components/tachart/tacustomseries.pas	(revision 60623)
    +++ components/tachart/tacustomseries.pas	(working copy)
    @@ -1563,6 +1563,9 @@
       AResults.FIndex := -1;
       AResults.FXIndex := 0;
       AResults.FYIndex := 0;
    +  if IsEmpty then exit(false);
    +  if not RequestValidChartScaling then exit(false);
    +
       if FOptimizeX and AParams.FOptimizeX then
         Source.FindBounds(
           GetGrabBound(-AParams.FRadius),
    Index: components/tachart/tafuncseries.pas
    ===================================================================
    --- components/tachart/tafuncseries.pas	(revision 60623)
    +++ components/tachart/tafuncseries.pas	(working copy)
    @@ -624,6 +624,8 @@
     procedure TCustomFuncSeries.Draw(ADrawer: IChartDrawer);
     begin
       if IsEmpty or (not Active) then exit;
    +  if not RequestValidChartScaling then exit;
    +
       ADrawer.SetBrushParams(bsClear, clTAColor);
       ADrawer.Pen := Pen;
       with TDrawFuncHelper.Create(Self, DomainExclusions, @DoCalculate, Step) do
    @@ -639,12 +641,15 @@
       ymin, ymax: Double;
     begin
       inherited GetBounds(ABounds);
    -  if not Extent.UseXMin or not Extent.UseXMax or not ExtentAutoY or IsEmpty then
    +  if not Extent.UseXMin or not Extent.UseXMax or not ExtentAutoY then
         exit;
    -  ymin := SafeInfinity;
    -  ymax := NegInfinity;
    +  if IsEmpty or (not Active) then exit;
    +  if not RequestValidChartScaling then exit;
    +
       with TDrawFuncHelper.Create(Self, DomainExclusions, @DoCalculate, Step) do
         try
    +      ymin := SafeInfinity;
    +      ymax := NegInfinity;
           CalcAxisExtentY(ABounds.a.X, ABounds.b.X, ymin, ymax);
           if not Extent.UseYMin or (ymin > Extent.YMin) then
             ABounds.a.Y := ymin;
    @@ -664,10 +669,14 @@
       const AParams: TNearestPointParams;
       out AResults: TNearestPointResults): Boolean;
     begin
    -  if IsEmpty then begin
    -    AResults.FIndex := -1;
    -    exit(false);
    -  end;
    +  // As in TBasicPointSeries.GetNearestPoint()
    +  AResults.FDist := Sqr(AParams.FRadius) + 1;
    +  AResults.FIndex := -1;
    +  AResults.FXIndex := 0;
    +  AResults.FYIndex := 0;
    +  if IsEmpty then exit(false);
    +  if not RequestValidChartScaling then exit(false);
    +
       with TDrawFuncHelper.Create(Self, DomainExclusions, @DoCalculate, Step) do
         try
           Result := GetNearestPoint(AParams, AResults);
    @@ -1388,7 +1397,9 @@
     var
       s: TSpline;
     begin
    -  if IsEmpty then exit;
    +  if IsEmpty or (not Active) then exit;
    +  if not RequestValidChartScaling then exit;
    +
       if FSplines = nil then
         PrepareCoeffs;
     
    @@ -1467,7 +1478,10 @@
     begin
       Result := inherited GetNearestPoint(AParams, AResults);
       if (not Result) and (nptCustom in ToolTargets) and (nptCustom in AParams.FTargets)
    -  then
    +  then begin
    +    if IsEmpty then exit;
    +    if not RequestValidChartScaling then exit;
    +
         for s in FSplines do begin
           if s.IsFewPoints or (s.FIsUnorderedX and not IsUnorderedVisible) then
             continue;
    @@ -1484,6 +1498,7 @@
               Free;
             end;
         end;
    +  end;
     end;
     
     function TCubicSplineSeries.IsUnorderedVisible: Boolean;
    @@ -1660,6 +1675,8 @@
       de : TIntervalList;
     begin
       if IsEmpty or (not Active) then exit;
    +  if not RequestValidChartScaling then exit;
    +
       if FAutoFit then ExecFit;
       ADrawer.SetBrushParams(bsClear, clTAColor);
       ADrawer.Pen := Pen;
    @@ -1810,16 +1827,11 @@
       de : TIntervalList;
     begin
       Result := Source.BasicExtent;
    +  if not FUseCombinedExtentY then exit;
       if IsEmpty or (not Active) then exit;
    -  if not FUseCombinedExtentY then exit;
    +  if not RequestValidChartScaling then exit;
     
    -  // TDrawFuncHelper needs a valid image-to-graph conversion
    -  if ParentChart = nil then exit;
    -  ParentChart.MultiPassScalingNeeded;
    -  if not ParentChart.ScalingValid then exit;
    -
       if FAutoFit then ExecFit;
    -
       if (FState = fpsValid) and (FErrCode = fitOK) then begin
         de := PrepareIntervals;
         try
    @@ -1955,6 +1967,9 @@
       Result := inherited GetNearestPoint(AParams, AResults);
       if (not Result) and (nptCustom in ToolTargets) and (nptCustom in AParams.FTargets)
       then begin
    +    if IsEmpty then exit;
    +    if not RequestValidChartScaling then exit;
    +
         ExecFit;
         if State <> fpsValid then exit(false);
         de := PrepareIntervals;
    Index: components/tachart/tagraph.pas
    ===================================================================
    --- components/tachart/tagraph.pas	(revision 60623)
    +++ components/tachart/tagraph.pas	(working copy)
    @@ -52,6 +52,7 @@
         procedure BeforeDraw; virtual;
         procedure GetLegendItemsBasic(AItems: TChartLegendItems); virtual; abstract;
         function GetShowInLegend: Boolean; virtual; abstract;
    +    function RequestValidChartScaling: Boolean; virtual;
         procedure SetActive(AValue: Boolean); virtual; abstract;
         procedure SetDepth(AValue: TChartDistance); virtual; abstract;
         procedure SetShadow(AValue: TChartShadow); virtual; abstract;
    @@ -361,7 +362,7 @@
         function XImageToGraph(AX: Integer): Double; inline;
         function YGraphToImage(AY: Double): Integer; inline;
         function YImageToGraph(AY: Integer): Double; inline;
    -    procedure MultiPassScalingNeeded; inline;
    +    procedure RequestMultiPassScaling; inline;
         property ScalingValid: Boolean read FScalingValid;
     
       public
    @@ -593,9 +594,9 @@
       end;
       FOffset.X := rX.CalcOffset(FScale.X);
       FOffset.Y := rY.CalcOffset(FScale.Y);
    +  FScalingValid := True;
       rX.UpdateMinMax(@XImageToGraph);
       rY.UpdateMinMax(@YImageToGraph);
    -  FScalingValid := True;
     end;
     
     procedure TChart.Clear(ADrawer: IChartDrawer; const ARect: TRect);
    @@ -1866,25 +1867,33 @@
     
     function TChart.XGraphToImage(AX: Double): Integer;
     begin
    +  if not FScalingValid then
    +    raise EChartError.CreateFmt('%s: chart''s scaling is not yet initialized.', ['XGraphToImage']);
       Result := ImgRoundChecked(FScale.X * AX + FOffset.X);
     end;
     
     function TChart.XImageToGraph(AX: Integer): Double;
     begin
    +  if not FScalingValid then
    +    raise EChartError.CreateFmt('%s: chart''s scaling is not yet initialized.', ['XImageToGraph']);
       Result := (AX - FOffset.X) / FScale.X;
     end;
     
     function TChart.YGraphToImage(AY: Double): Integer;
     begin
    -   Result := ImgRoundChecked(FScale.Y * AY + FOffset.Y);
    +  if not FScalingValid then
    +    raise EChartError.CreateFmt('%s: chart''s scaling is not yet initialized.', ['YGraphToImage']);
    +  Result := ImgRoundChecked(FScale.Y * AY + FOffset.Y);
     end;
     
     function TChart.YImageToGraph(AY: Integer): Double;
     begin
    +  if not FScalingValid then
    +    raise EChartError.CreateFmt('%s: chart''s scaling is not yet initialized.', ['YImageToGraph']);
       Result := (AY - FOffset.Y) / FScale.Y;
     end;
     
    -procedure TChart.MultiPassScalingNeeded;
    +procedure TChart.RequestMultiPassScaling;
     begin
       FMultiPassScalingNeeded := True;
     end;
    @@ -1930,6 +1939,20 @@
       // empty
     end;
     
    +function TBasicChartSeries.RequestValidChartScaling: Boolean;
    +begin
    +  Result := false;
    +  if not Assigned(FChart) then exit;
    +
    +  // We request a valid scaling from FChart. Scaling is not initialized during
    +  // the first phase of scaling calculation, so we request more phases. If we
    +  // are already beyond the scaling calculation loop, our request will be just
    +  // ignored.
    +  FChart.RequestMultiPassScaling;
    +
    +  Result := FChart.ScalingValid;
    +end;
    +
     destructor TBasicChartSeries.Destroy;
     begin
       if FChart <> nil then
    
    patch.diff (7,389 bytes)

Activities

Marcin Wiazowski

2019-03-08 22:10

reporter  

Reproduce.zip (2,950 bytes)

Marcin Wiazowski

2019-03-08 22:10

reporter  

patch.diff (7,389 bytes)
Index: components/tachart/tacustomseries.pas
===================================================================
--- components/tachart/tacustomseries.pas	(revision 60623)
+++ components/tachart/tacustomseries.pas	(working copy)
@@ -1563,6 +1563,9 @@
   AResults.FIndex := -1;
   AResults.FXIndex := 0;
   AResults.FYIndex := 0;
+  if IsEmpty then exit(false);
+  if not RequestValidChartScaling then exit(false);
+
   if FOptimizeX and AParams.FOptimizeX then
     Source.FindBounds(
       GetGrabBound(-AParams.FRadius),
Index: components/tachart/tafuncseries.pas
===================================================================
--- components/tachart/tafuncseries.pas	(revision 60623)
+++ components/tachart/tafuncseries.pas	(working copy)
@@ -624,6 +624,8 @@
 procedure TCustomFuncSeries.Draw(ADrawer: IChartDrawer);
 begin
   if IsEmpty or (not Active) then exit;
+  if not RequestValidChartScaling then exit;
+
   ADrawer.SetBrushParams(bsClear, clTAColor);
   ADrawer.Pen := Pen;
   with TDrawFuncHelper.Create(Self, DomainExclusions, @DoCalculate, Step) do
@@ -639,12 +641,15 @@
   ymin, ymax: Double;
 begin
   inherited GetBounds(ABounds);
-  if not Extent.UseXMin or not Extent.UseXMax or not ExtentAutoY or IsEmpty then
+  if not Extent.UseXMin or not Extent.UseXMax or not ExtentAutoY then
     exit;
-  ymin := SafeInfinity;
-  ymax := NegInfinity;
+  if IsEmpty or (not Active) then exit;
+  if not RequestValidChartScaling then exit;
+
   with TDrawFuncHelper.Create(Self, DomainExclusions, @DoCalculate, Step) do
     try
+      ymin := SafeInfinity;
+      ymax := NegInfinity;
       CalcAxisExtentY(ABounds.a.X, ABounds.b.X, ymin, ymax);
       if not Extent.UseYMin or (ymin > Extent.YMin) then
         ABounds.a.Y := ymin;
@@ -664,10 +669,14 @@
   const AParams: TNearestPointParams;
   out AResults: TNearestPointResults): Boolean;
 begin
-  if IsEmpty then begin
-    AResults.FIndex := -1;
-    exit(false);
-  end;
+  // As in TBasicPointSeries.GetNearestPoint()
+  AResults.FDist := Sqr(AParams.FRadius) + 1;
+  AResults.FIndex := -1;
+  AResults.FXIndex := 0;
+  AResults.FYIndex := 0;
+  if IsEmpty then exit(false);
+  if not RequestValidChartScaling then exit(false);
+
   with TDrawFuncHelper.Create(Self, DomainExclusions, @DoCalculate, Step) do
     try
       Result := GetNearestPoint(AParams, AResults);
@@ -1388,7 +1397,9 @@
 var
   s: TSpline;
 begin
-  if IsEmpty then exit;
+  if IsEmpty or (not Active) then exit;
+  if not RequestValidChartScaling then exit;
+
   if FSplines = nil then
     PrepareCoeffs;
 
@@ -1467,7 +1478,10 @@
 begin
   Result := inherited GetNearestPoint(AParams, AResults);
   if (not Result) and (nptCustom in ToolTargets) and (nptCustom in AParams.FTargets)
-  then
+  then begin
+    if IsEmpty then exit;
+    if not RequestValidChartScaling then exit;
+
     for s in FSplines do begin
       if s.IsFewPoints or (s.FIsUnorderedX and not IsUnorderedVisible) then
         continue;
@@ -1484,6 +1498,7 @@
           Free;
         end;
     end;
+  end;
 end;
 
 function TCubicSplineSeries.IsUnorderedVisible: Boolean;
@@ -1660,6 +1675,8 @@
   de : TIntervalList;
 begin
   if IsEmpty or (not Active) then exit;
+  if not RequestValidChartScaling then exit;
+
   if FAutoFit then ExecFit;
   ADrawer.SetBrushParams(bsClear, clTAColor);
   ADrawer.Pen := Pen;
@@ -1810,16 +1827,11 @@
   de : TIntervalList;
 begin
   Result := Source.BasicExtent;
+  if not FUseCombinedExtentY then exit;
   if IsEmpty or (not Active) then exit;
-  if not FUseCombinedExtentY then exit;
+  if not RequestValidChartScaling then exit;
 
-  // TDrawFuncHelper needs a valid image-to-graph conversion
-  if ParentChart = nil then exit;
-  ParentChart.MultiPassScalingNeeded;
-  if not ParentChart.ScalingValid then exit;
-
   if FAutoFit then ExecFit;
-
   if (FState = fpsValid) and (FErrCode = fitOK) then begin
     de := PrepareIntervals;
     try
@@ -1955,6 +1967,9 @@
   Result := inherited GetNearestPoint(AParams, AResults);
   if (not Result) and (nptCustom in ToolTargets) and (nptCustom in AParams.FTargets)
   then begin
+    if IsEmpty then exit;
+    if not RequestValidChartScaling then exit;
+
     ExecFit;
     if State <> fpsValid then exit(false);
     de := PrepareIntervals;
Index: components/tachart/tagraph.pas
===================================================================
--- components/tachart/tagraph.pas	(revision 60623)
+++ components/tachart/tagraph.pas	(working copy)
@@ -52,6 +52,7 @@
     procedure BeforeDraw; virtual;
     procedure GetLegendItemsBasic(AItems: TChartLegendItems); virtual; abstract;
     function GetShowInLegend: Boolean; virtual; abstract;
+    function RequestValidChartScaling: Boolean; virtual;
     procedure SetActive(AValue: Boolean); virtual; abstract;
     procedure SetDepth(AValue: TChartDistance); virtual; abstract;
     procedure SetShadow(AValue: TChartShadow); virtual; abstract;
@@ -361,7 +362,7 @@
     function XImageToGraph(AX: Integer): Double; inline;
     function YGraphToImage(AY: Double): Integer; inline;
     function YImageToGraph(AY: Integer): Double; inline;
-    procedure MultiPassScalingNeeded; inline;
+    procedure RequestMultiPassScaling; inline;
     property ScalingValid: Boolean read FScalingValid;
 
   public
@@ -593,9 +594,9 @@
   end;
   FOffset.X := rX.CalcOffset(FScale.X);
   FOffset.Y := rY.CalcOffset(FScale.Y);
+  FScalingValid := True;
   rX.UpdateMinMax(@XImageToGraph);
   rY.UpdateMinMax(@YImageToGraph);
-  FScalingValid := True;
 end;
 
 procedure TChart.Clear(ADrawer: IChartDrawer; const ARect: TRect);
@@ -1866,25 +1867,33 @@
 
 function TChart.XGraphToImage(AX: Double): Integer;
 begin
+  if not FScalingValid then
+    raise EChartError.CreateFmt('%s: chart''s scaling is not yet initialized.', ['XGraphToImage']);
   Result := ImgRoundChecked(FScale.X * AX + FOffset.X);
 end;
 
 function TChart.XImageToGraph(AX: Integer): Double;
 begin
+  if not FScalingValid then
+    raise EChartError.CreateFmt('%s: chart''s scaling is not yet initialized.', ['XImageToGraph']);
   Result := (AX - FOffset.X) / FScale.X;
 end;
 
 function TChart.YGraphToImage(AY: Double): Integer;
 begin
-   Result := ImgRoundChecked(FScale.Y * AY + FOffset.Y);
+  if not FScalingValid then
+    raise EChartError.CreateFmt('%s: chart''s scaling is not yet initialized.', ['YGraphToImage']);
+  Result := ImgRoundChecked(FScale.Y * AY + FOffset.Y);
 end;
 
 function TChart.YImageToGraph(AY: Integer): Double;
 begin
+  if not FScalingValid then
+    raise EChartError.CreateFmt('%s: chart''s scaling is not yet initialized.', ['YImageToGraph']);
   Result := (AY - FOffset.Y) / FScale.Y;
 end;
 
-procedure TChart.MultiPassScalingNeeded;
+procedure TChart.RequestMultiPassScaling;
 begin
   FMultiPassScalingNeeded := True;
 end;
@@ -1930,6 +1939,20 @@
   // empty
 end;
 
+function TBasicChartSeries.RequestValidChartScaling: Boolean;
+begin
+  Result := false;
+  if not Assigned(FChart) then exit;
+
+  // We request a valid scaling from FChart. Scaling is not initialized during
+  // the first phase of scaling calculation, so we request more phases. If we
+  // are already beyond the scaling calculation loop, our request will be just
+  // ignored.
+  FChart.RequestMultiPassScaling;
+
+  Result := FChart.ScalingValid;
+end;
+
 destructor TBasicChartSeries.Destroy;
 begin
   if FChart <> nil then
patch.diff (7,389 bytes)

wp

2019-03-09 19:25

developer   ~0114745

Thanks, applied with these modifications:

- Moved TChart.MultiPassScaling to "protected" in order to avoid pollution of public methods by internal routines. This is possible now because the series does not call it directly any more but via its own RequestValidChartScaling which has access to the protected methods of TChart.

- Added conditional define CHECK_VALID_SCALING in order to allow turning off this check for the very low-level ImageToGraph and GraphToImage conversions.

- Applied the changes also to TBubbleSeries which calls the image-graph conversions.


Remarks:
- Kept the changes in GetNearestPoint although I am not convinced that they are needed: A user can apply a ChartTool only after the chart has been painted.

- Not sure whether all series should check Active again because this already has been done by TChart.DisplaySeries. But Series.Draw is public and thus may be called by user code. So, better to keep it. (Maybe some series are still missing it).

Marcin Wiazowski

2019-03-09 20:05

reporter   ~0114746

Moving TChart.MultiPassScaling() to "protected" section is a good idea.


In fact, GetNearestPoint() is - under normal circumstances - used only by chart tools, that work when the chart is already drawn - so scaling is initialized. However, GetNearestPoint() is a public method, so it's better to validate the scaling - without this, GetNearestPoint() might hang for several seconds, if called when scaling is not yet initialized, and X axis range is large. And the verification doesn't cause any slow down or other side effects, so it's better to keep it.


Checking if "Active" in Draw() methods does not seem in fact to be necessary - it's only to avoid executing needless code; there are no side effects of making this check, so it's probably better to have this check (at least in most cases).


Thanks!

Issue History

Date Modified Username Field Change
2019-03-08 22:10 Marcin Wiazowski New Issue
2019-03-08 22:10 Marcin Wiazowski File Added: Reproduce.zip
2019-03-08 22:10 Marcin Wiazowski File Added: patch.diff
2019-03-09 12:32 wp Assigned To => wp
2019-03-09 12:32 wp Status new => assigned
2019-03-09 19:25 wp Note Added: 0114745
2019-03-09 19:25 wp Fixed in Revision => 60636
2019-03-09 19:25 wp LazTarget => 2.2
2019-03-09 19:25 wp Status assigned => resolved
2019-03-09 19:25 wp Resolution open => fixed
2019-03-09 19:25 wp Target Version => 2.2
2019-03-09 20:05 Marcin Wiazowski Note Added: 0114746
2019-03-09 20:05 Marcin Wiazowski Status resolved => closed