View Issue Details

IDProjectCategoryView StatusLast Update
0035200LazarusTAChartpublic2019-03-08 00:37
ReporterMarcin WiazowskiAssigned Towp 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version2.1 (SVN)Product Build60606 
Target Version2.2Fixed in Version 
Summary0035200: TAChart: some fixes for TAFuncSeries unit
DescriptionI'm attaching a patch, that solves a group of related problems in TAFuncSeries unit.



Test 1: Load the attached Reproduce application and start it. An exception 'External: SIGSEGV' will be raised immediately due to a null pointer dereference - this is because TFuncSeries.OnCalculate handler is not assigned.

So there is no validation for the handler - although there should be.



Test 2: In Object Inspector, set Chart2FuncSeries' OnCalculate handler to Chart2FuncSeriesCalculate (so application will be able to work properly). Now take a look at the designed form, at the middle (TFuncSeries) chart - it's X axis range is -1 .. 1 (it uses ExtentAutoY functionaliy, so Y axis range is same). Now launch the application - X axis range is changed to 0 .. 500.

For both left chart (holding TExpressionSeries) and middle chart (holding TFuncSeries), series' Extent property is defined in the same way, forcing X range to be 0 .. 500. At design time, this works properly only for TExpressionSeries. But at runtime, this works properly for both TExpressionSeries and TFuncSeries.

This is because TFuncSeries.GetBounds() is implemented in a strange way:

  procedure TFuncSeries.GetBounds(var ABounds: TDoubleRect);
  begin
    if (csDesigning in ComponentState) then
      exit;
    inherited GetBounds(ABounds);
  end;

Maybe, some time ago, there was a reason for doing this, but currently this can work properly also at design time - exactly as it is in TExpressionSeries.



Now some note: TFuncSeries and TParametricCurveSeries use their OnCalculate events to draw the series. But, at design time, OnCalculate handler cannot be used - so, in order to show some visual feedback to the user, an internal DoCalcIdentity() function is used - so a straight line is drawn.





The attached patch:

1) Removes TFuncSeries.GetBounds() method, so ancestor's GetBounds() is used - thanks to that, proper X axis range is shown also at design time. This solves the problem described in Test 2.


2) Now the TFuncSeries.DoCalculate() and TParametricCurveSeries.DoCalculate() methods encapsulate the "DoCalcIdentity()" behavior, making the code concise. And, to prevent null pointer dereferences (as described in Test 1), an exception is raised in case of uninitialized OnCalculate event.


3) The TFuncSeries.IsEmpty() and TParametricCurveSeries.IsEmpty() methods work improperly now. They are implemented as:

  function XXX.IsEmpty: Boolean;
  begin
    Result := not Assigned(OnCalculate);
  end;

As described above, at design time, an internal "DoCalcIdentity()" behavior is used, so series is NOT in fact empty. So "and not (csDesigning in ComponentState)" expressions have been added to fix this.


4) A needless "if (csDesigning in ComponentState)" expression has been removed in TColorMapSeries.FunctionValue().


5) Since TExpressionSeries and TFuncSeries use TIntervalList helper object internally, improper validation in TIntervalList.Intersect() has also been fixed.
TagsNo tags attached.
Fixed in Revision60609, 60610, 60611, 60615, 60618
LazTarget2.2
WidgetsetWin32/Win64
Attached Files
  • Reproduce.zip (2,471 bytes)
  • patch.diff (5,459 bytes)
    Index: tachartutils.pas
    ===================================================================
    --- tachartutils.pas	(revision 60606)
    +++ tachartutils.pas	(working copy)
    @@ -774,7 +774,7 @@
       Result := false;
       if Length(FIntervals) = 0 then exit;
     
    -  AHint := Min(High(FIntervals), AHint);
    +  AHint := Min(High(FIntervals), Max(0, AHint));
       while (AHint > 0) and (FIntervals[AHint].FStart > ARight) do
         Dec(AHint);
     
    Index: tafuncseries.pas
    ===================================================================
    --- tafuncseries.pas	(revision 60606)
    +++ tafuncseries.pas	(working copy)
    @@ -1,4 +1,4 @@
    -{
    +{
     
      Function series for TAChart.
     
    @@ -77,9 +77,8 @@
         FOnCalculate: TFuncCalculateEvent;
         procedure SetOnCalculate(AValue: TFuncCalculateEvent);
       protected
    -    function DoCalcIdentity(AX: Double): Double;
    +    function DoCalcIdentity(AX: Double): Double; inline;
         function DoCalculate(AX: Double): Double; override;
    -    procedure GetBounds(var ABounds: TDoubleRect); override;
       public
         procedure Assign(ASource: TPersistent); override;
         procedure Draw(ADrawer: IChartDrawer); override;
    @@ -104,7 +103,7 @@
         FPen: TChartPen;
         FStep: TFuncSeriesStep;
     
    -    function DoCalcIdentity(AT: Double): TDoublePoint;
    +    function DoCalcIdentity(AT: Double): TDoublePoint; inline;
         function DoCalculate(AT: Double): TDoublePoint;
         function ParamMaxIsStored: Boolean;
         function ParamMaxStepIsStored: Boolean;
    @@ -509,6 +508,7 @@
       DEF_PARAM_MIN = 0.0;
       DEF_PARAM_MAX = 1.0;
     
    +  SEventHandlerNotAssigned = '[%s.%s] Event handler is not assigned.';
       SIndexOutOfRange = '[%s.%s] Index out of range.';
     
     type
    @@ -531,8 +531,6 @@
         procedure Draw(ADrawer: IChartDrawer; const ARect: TRect); override;
       end;
     
    -  TParametricFunc = function (A: Double): TDoublePoint of object;
    -
     function ParamsToEquation(
       AEquation: TFitEquation; const AParams: array of Double;
       ANumFormat, AXText, AYText: String): String;
    @@ -720,22 +718,20 @@
     
     function TFuncSeries.DoCalculate(AX: Double): Double;
     begin
    -  OnCalculate(AX, Result)
    +  if Assigned(FOnCalculate) then
    +    FOnCalculate(AX, Result)
    +  else if csDesigning in ComponentState then
    +    Result := DoCalcIdentity(AX)
    +  else
    +    raise EChartError.CreateFmt(SEventHandlerNotAssigned, [NameOrClassName(Self), 'OnCalculate']);
     end;
     
     procedure TFuncSeries.Draw(ADrawer: IChartDrawer);
    -var
    -  calc: TTransformFunc;
     begin
    -  if Assigned(OnCalculate) then
    -    calc := @DoCalculate
    -  else if csDesigning in ComponentState then
    -    calc := @DoCalcIdentity
    -  else
    -    exit;
    +  if IsEmpty then exit;
       ADrawer.SetBrushParams(bsClear, clTAColor);
       ADrawer.Pen := Pen;
    -  with TDrawFuncHelper.Create(Self, DomainExclusions, calc, Step) do
    +  with TDrawFuncHelper.Create(Self, DomainExclusions, @DoCalculate, Step) do
         try
           DrawFunction(ADrawer);
         finally
    @@ -743,26 +739,18 @@
         end;
     end;
     
    -procedure TFuncSeries.GetBounds(var ABounds: TDoubleRect);
    -begin
    -  if (csDesigning in ComponentState) then
    -    exit;
    -  inherited GetBounds(ABounds);
    -end;
    -
     function TFuncSeries.GetNearestPoint(
       const AParams: TNearestPointParams;
       out AResults: TNearestPointResults): Boolean;
     begin
       AResults.FIndex := -1;
    -  if not Assigned(OnCalculate) then
    -    exit(false);
    +  if IsEmpty then exit(false);
       Result := inherited;
     end;
     
     function TFuncSeries.IsEmpty: Boolean;
     begin
    -  Result := not Assigned(OnCalculate);
    +  Result := not Assigned(FOnCalculate) and not (csDesigning in ComponentState);
     end;
     
     procedure TFuncSeries.SetOnCalculate(AValue: TFuncCalculateEvent);
    @@ -811,16 +799,19 @@
     
     function TParametricCurveSeries.DoCalculate(AT: Double): TDoublePoint;
     begin
    -  OnCalculate(AT, Result.X, Result.Y);
    +  if Assigned(FOnCalculate) then
    +    FOnCalculate(AT, Result.X, Result.Y)
    +  else if csDesigning in ComponentState then
    +    Result := DoCalcIdentity(AT)
    +  else
    +    raise EChartError.CreateFmt(SEventHandlerNotAssigned, [NameOrClassName(Self), 'OnCalculate']);
     end;
     
     procedure TParametricCurveSeries.Draw(ADrawer: IChartDrawer);
    -var
    -  calc: TParametricFunc;
     
       function PointAt(AT: Double): TPoint;
       begin
    -    Result := ParentChart.GraphToImage(AxisToGraph(calc(AT)))
    +    Result := ParentChart.GraphToImage(AxisToGraph(DoCalculate(AT)))
       end;
     
     var
    @@ -827,12 +818,7 @@
       t, ts, ms: Double;
       p, pp: TPoint;
     begin
    -  if Assigned(OnCalculate) then
    -    calc := @DoCalculate
    -  else if csDesigning in ComponentState then
    -    calc := @DoCalcIdentity
    -  else
    -    exit;
    +  if IsEmpty then exit;
       ADrawer.SetBrushParams(bsClear, clTAColor);
       ADrawer.Pen := Pen;
     
    @@ -861,7 +847,7 @@
     
     function TParametricCurveSeries.IsEmpty: Boolean;
     begin
    -  Result := not Assigned(OnCalculate);
    +  Result := not Assigned(FOnCalculate) and not (csDesigning in ComponentState);
     end;
     
     function TParametricCurveSeries.ParamMaxIsStored: Boolean;
    @@ -2759,15 +2745,15 @@
     
     function TColorMapSeries.FunctionValue(AX, AY: Double): Double;
     begin
    -  if (csDesigning in ComponentState) or not Assigned(FOnCalculate) then
    -    Result := 0
    +  if Assigned(FOnCalculate) then
    +    FOnCalculate(AX, AY, Result)
       else
    -    OnCalculate(AX, AY, Result);
    +    Result := 0;
     end;
     
     function TColorMapSeries.IsEmpty: Boolean;
     begin
    -  Result := not Assigned(OnCalculate);
    +  Result := not Assigned(FOnCalculate);
     end;
     
     procedure TColorMapSeries.SetOnCalculate(AValue: TFuncCalculate3DEvent);
    
    patch.diff (5,459 bytes)
  • update.diff (3,769 bytes)
    Index: components/tachart/tafuncseries.pas
    ===================================================================
    --- components/tachart/tafuncseries.pas	(revision 60617)
    +++ components/tachart/tafuncseries.pas	(working copy)
    @@ -1,4 +1,4 @@
    -{
    +{
     
      Function series for TAChart.
     
    @@ -78,6 +78,7 @@
         procedure SetOnCalculate(AValue: TFuncCalculateEvent);
       protected
         function DoCalculate(AX: Double): Double; override;
    +    procedure GetBounds(var ABounds: TDoubleRect); override;
       public
         procedure Assign(ASource: TPersistent); override;
         procedure Draw(ADrawer: IChartDrawer); override;
    @@ -523,8 +524,6 @@
         procedure Draw(ADrawer: IChartDrawer; const ARect: TRect); override;
       end;
     
    -  TParametricFunc = function (A: Double): TDoublePoint of object;
    -
     function ParamsToEquation(
       AEquation: TFitEquation; const AParams: array of Double;
       ANumFormat, AXText, AYText: String): String;
    @@ -624,6 +623,7 @@
     
     procedure TCustomFuncSeries.Draw(ADrawer: IChartDrawer);
     begin
    +  if IsEmpty or (not Active) then exit;
       ADrawer.SetBrushParams(bsClear, clTAColor);
       ADrawer.Pen := Pen;
       with TDrawFuncHelper.Create(Self, DomainExclusions, @DoCalculate, Step) do
    @@ -711,7 +711,7 @@
     
     function TFuncSeries.DoCalculate(AX: Double): Double;
     begin
    -  OnCalculate(AX, Result)
    +  OnCalculate(AX, Result);
     end;
     
     procedure TFuncSeries.Draw(ADrawer: IChartDrawer);
    @@ -718,8 +718,7 @@
     var
       R: TRect;
     begin
    -  ADrawer.SetBrushParams(bsClear, clTAColor);
    -  ADrawer.Pen := Pen;
    +  if not Active then exit;
     
       if csDesigning in ComponentState then begin
         with ParentChart do begin
    @@ -727,19 +726,25 @@
           R.BottomRight := GraphToImage(CurrentExtent.b);
           NormalizeRect(R);
         end;
    +    ADrawer.SetBrushParams(bsClear, clTAColor);
    +    ADrawer.Pen := Pen;
         ADrawer.Line(R.Left, R.Bottom, R.Right, R.Top);
         exit;
       end;
     
    -  if IsEmpty then
    -    exit;
    +  inherited;
    +end;
     
    -  with TDrawFuncHelper.Create(Self, DomainExclusions, @DoCalculate, Step) do
    -    try
    -      DrawFunction(ADrawer);
    -    finally
    -      Free;
    -    end;
    +procedure TFuncSeries.GetBounds(var ABounds: TDoubleRect);
    +begin
    +  inherited GetBounds(ABounds);
    +  if not (csDesigning in ComponentState) or
    +     not Extent.UseXMin or not Extent.UseXMax or not ExtentAutoY then exit;
    +
    +  // When designing, an oblique line is drawn (see TFuncSeries.Draw),
    +  // so bounds should be adjusted when ExtentAutoY is True
    +  ABounds.a.Y := ABounds.a.X;
    +  ABounds.b.Y := ABounds.b.X;
     end;
     
     function TFuncSeries.IsEmpty: Boolean;
    @@ -803,22 +808,24 @@
       t, ts, ms: Double;
       p, pp: TPoint;
     begin
    -  ADrawer.SetBrushParams(bsClear, clTAColor);
    -  ADrawer.Pen := Pen;
    +  if not Active then exit;
     
       if csDesigning in ComponentState then begin
         with ParentChart do begin
    -      R.TopLeft := GraphToImage(LogicalExtent.a);
    -      R.BottomRight := GraphToImage(LogicalExtent.b);
    +      R.TopLeft := GraphToImage(CurrentExtent.a);
    +      R.BottomRight := GraphToImage(CurrentExtent.b);
           NormalizeRect(R);
         end;
    +    ADrawer.SetBrushParams(bsClear, clTAColor);
    +    ADrawer.Pen := Pen;
         ADrawer.Ellipse(R.Left, R.Bottom, R.Right, R.Top);
         exit;
       end;
     
    -  if IsEmpty then
    -    exit;
    +  if IsEmpty then exit;
     
    +  ADrawer.SetBrushParams(bsClear, clTAColor);
    +  ADrawer.Pen := Pen;
       t := ParamMin;
       pp := PointAt(ParamMin);
       ADrawer.MoveTo(pp);
    @@ -2742,10 +2749,10 @@
     
     function TColorMapSeries.FunctionValue(AX, AY: Double): Double;
     begin
    -  if (csDesigning in ComponentState) or not Assigned(FOnCalculate) then
    -    Result := 0
    +  if Assigned(OnCalculate) then
    +    OnCalculate(AX, AY, Result)
       else
    -    OnCalculate(AX, AY, Result);
    +    Result := 0;
     end;
     
     function TColorMapSeries.IsEmpty: Boolean;
    
    update.diff (3,769 bytes)

Activities

Marcin Wiazowski

2019-03-07 00:15

reporter  

Reproduce.zip (2,471 bytes)

Marcin Wiazowski

2019-03-07 00:15

reporter  

patch.diff (5,459 bytes)
Index: tachartutils.pas
===================================================================
--- tachartutils.pas	(revision 60606)
+++ tachartutils.pas	(working copy)
@@ -774,7 +774,7 @@
   Result := false;
   if Length(FIntervals) = 0 then exit;
 
-  AHint := Min(High(FIntervals), AHint);
+  AHint := Min(High(FIntervals), Max(0, AHint));
   while (AHint > 0) and (FIntervals[AHint].FStart > ARight) do
     Dec(AHint);
 
Index: tafuncseries.pas
===================================================================
--- tafuncseries.pas	(revision 60606)
+++ tafuncseries.pas	(working copy)
@@ -1,4 +1,4 @@
-{
+{
 
  Function series for TAChart.
 
@@ -77,9 +77,8 @@
     FOnCalculate: TFuncCalculateEvent;
     procedure SetOnCalculate(AValue: TFuncCalculateEvent);
   protected
-    function DoCalcIdentity(AX: Double): Double;
+    function DoCalcIdentity(AX: Double): Double; inline;
     function DoCalculate(AX: Double): Double; override;
-    procedure GetBounds(var ABounds: TDoubleRect); override;
   public
     procedure Assign(ASource: TPersistent); override;
     procedure Draw(ADrawer: IChartDrawer); override;
@@ -104,7 +103,7 @@
     FPen: TChartPen;
     FStep: TFuncSeriesStep;
 
-    function DoCalcIdentity(AT: Double): TDoublePoint;
+    function DoCalcIdentity(AT: Double): TDoublePoint; inline;
     function DoCalculate(AT: Double): TDoublePoint;
     function ParamMaxIsStored: Boolean;
     function ParamMaxStepIsStored: Boolean;
@@ -509,6 +508,7 @@
   DEF_PARAM_MIN = 0.0;
   DEF_PARAM_MAX = 1.0;
 
+  SEventHandlerNotAssigned = '[%s.%s] Event handler is not assigned.';
   SIndexOutOfRange = '[%s.%s] Index out of range.';
 
 type
@@ -531,8 +531,6 @@
     procedure Draw(ADrawer: IChartDrawer; const ARect: TRect); override;
   end;
 
-  TParametricFunc = function (A: Double): TDoublePoint of object;
-
 function ParamsToEquation(
   AEquation: TFitEquation; const AParams: array of Double;
   ANumFormat, AXText, AYText: String): String;
@@ -720,22 +718,20 @@
 
 function TFuncSeries.DoCalculate(AX: Double): Double;
 begin
-  OnCalculate(AX, Result)
+  if Assigned(FOnCalculate) then
+    FOnCalculate(AX, Result)
+  else if csDesigning in ComponentState then
+    Result := DoCalcIdentity(AX)
+  else
+    raise EChartError.CreateFmt(SEventHandlerNotAssigned, [NameOrClassName(Self), 'OnCalculate']);
 end;
 
 procedure TFuncSeries.Draw(ADrawer: IChartDrawer);
-var
-  calc: TTransformFunc;
 begin
-  if Assigned(OnCalculate) then
-    calc := @DoCalculate
-  else if csDesigning in ComponentState then
-    calc := @DoCalcIdentity
-  else
-    exit;
+  if IsEmpty then exit;
   ADrawer.SetBrushParams(bsClear, clTAColor);
   ADrawer.Pen := Pen;
-  with TDrawFuncHelper.Create(Self, DomainExclusions, calc, Step) do
+  with TDrawFuncHelper.Create(Self, DomainExclusions, @DoCalculate, Step) do
     try
       DrawFunction(ADrawer);
     finally
@@ -743,26 +739,18 @@
     end;
 end;
 
-procedure TFuncSeries.GetBounds(var ABounds: TDoubleRect);
-begin
-  if (csDesigning in ComponentState) then
-    exit;
-  inherited GetBounds(ABounds);
-end;
-
 function TFuncSeries.GetNearestPoint(
   const AParams: TNearestPointParams;
   out AResults: TNearestPointResults): Boolean;
 begin
   AResults.FIndex := -1;
-  if not Assigned(OnCalculate) then
-    exit(false);
+  if IsEmpty then exit(false);
   Result := inherited;
 end;
 
 function TFuncSeries.IsEmpty: Boolean;
 begin
-  Result := not Assigned(OnCalculate);
+  Result := not Assigned(FOnCalculate) and not (csDesigning in ComponentState);
 end;
 
 procedure TFuncSeries.SetOnCalculate(AValue: TFuncCalculateEvent);
@@ -811,16 +799,19 @@
 
 function TParametricCurveSeries.DoCalculate(AT: Double): TDoublePoint;
 begin
-  OnCalculate(AT, Result.X, Result.Y);
+  if Assigned(FOnCalculate) then
+    FOnCalculate(AT, Result.X, Result.Y)
+  else if csDesigning in ComponentState then
+    Result := DoCalcIdentity(AT)
+  else
+    raise EChartError.CreateFmt(SEventHandlerNotAssigned, [NameOrClassName(Self), 'OnCalculate']);
 end;
 
 procedure TParametricCurveSeries.Draw(ADrawer: IChartDrawer);
-var
-  calc: TParametricFunc;
 
   function PointAt(AT: Double): TPoint;
   begin
-    Result := ParentChart.GraphToImage(AxisToGraph(calc(AT)))
+    Result := ParentChart.GraphToImage(AxisToGraph(DoCalculate(AT)))
   end;
 
 var
@@ -827,12 +818,7 @@
   t, ts, ms: Double;
   p, pp: TPoint;
 begin
-  if Assigned(OnCalculate) then
-    calc := @DoCalculate
-  else if csDesigning in ComponentState then
-    calc := @DoCalcIdentity
-  else
-    exit;
+  if IsEmpty then exit;
   ADrawer.SetBrushParams(bsClear, clTAColor);
   ADrawer.Pen := Pen;
 
@@ -861,7 +847,7 @@
 
 function TParametricCurveSeries.IsEmpty: Boolean;
 begin
-  Result := not Assigned(OnCalculate);
+  Result := not Assigned(FOnCalculate) and not (csDesigning in ComponentState);
 end;
 
 function TParametricCurveSeries.ParamMaxIsStored: Boolean;
@@ -2759,15 +2745,15 @@
 
 function TColorMapSeries.FunctionValue(AX, AY: Double): Double;
 begin
-  if (csDesigning in ComponentState) or not Assigned(FOnCalculate) then
-    Result := 0
+  if Assigned(FOnCalculate) then
+    FOnCalculate(AX, AY, Result)
   else
-    OnCalculate(AX, AY, Result);
+    Result := 0;
 end;
 
 function TColorMapSeries.IsEmpty: Boolean;
 begin
-  Result := not Assigned(OnCalculate);
+  Result := not Assigned(FOnCalculate);
 end;
 
 procedure TColorMapSeries.SetOnCalculate(AValue: TFuncCalculate3DEvent);
patch.diff (5,459 bytes)

wp

2019-03-07 16:28

developer   ~0114701

Thank you. Applied with modifications:

- Raising an exception here is not good: Maybe the user just wants to have a TFuncSeries on the form and provides an OnCalculate handler later at runtime. This is the same as if a TLineSeries would raise an exception when its chartsource does not provide any data.

- I also did not use the DoCalculate calling DoCalcIdentity because now there is an IF inside the calculation routine; the old code avoided this by assigning different function variables to the DrawHelper depending on the situation.

- Since after fixing the GetBounds issue the designtime curve depends on the Extent property I decided to just paint a diagonal at designtime without calling any function at all. And the parameteric series got an ellipse which, for me, is the prototype of a parametric curve.

Marcin Wiazowski

2019-03-07 20:49

reporter  

update.diff (3,769 bytes)
Index: components/tachart/tafuncseries.pas
===================================================================
--- components/tachart/tafuncseries.pas	(revision 60617)
+++ components/tachart/tafuncseries.pas	(working copy)
@@ -1,4 +1,4 @@
-{
+{
 
  Function series for TAChart.
 
@@ -78,6 +78,7 @@
     procedure SetOnCalculate(AValue: TFuncCalculateEvent);
   protected
     function DoCalculate(AX: Double): Double; override;
+    procedure GetBounds(var ABounds: TDoubleRect); override;
   public
     procedure Assign(ASource: TPersistent); override;
     procedure Draw(ADrawer: IChartDrawer); override;
@@ -523,8 +524,6 @@
     procedure Draw(ADrawer: IChartDrawer; const ARect: TRect); override;
   end;
 
-  TParametricFunc = function (A: Double): TDoublePoint of object;
-
 function ParamsToEquation(
   AEquation: TFitEquation; const AParams: array of Double;
   ANumFormat, AXText, AYText: String): String;
@@ -624,6 +623,7 @@
 
 procedure TCustomFuncSeries.Draw(ADrawer: IChartDrawer);
 begin
+  if IsEmpty or (not Active) then exit;
   ADrawer.SetBrushParams(bsClear, clTAColor);
   ADrawer.Pen := Pen;
   with TDrawFuncHelper.Create(Self, DomainExclusions, @DoCalculate, Step) do
@@ -711,7 +711,7 @@
 
 function TFuncSeries.DoCalculate(AX: Double): Double;
 begin
-  OnCalculate(AX, Result)
+  OnCalculate(AX, Result);
 end;
 
 procedure TFuncSeries.Draw(ADrawer: IChartDrawer);
@@ -718,8 +718,7 @@
 var
   R: TRect;
 begin
-  ADrawer.SetBrushParams(bsClear, clTAColor);
-  ADrawer.Pen := Pen;
+  if not Active then exit;
 
   if csDesigning in ComponentState then begin
     with ParentChart do begin
@@ -727,19 +726,25 @@
       R.BottomRight := GraphToImage(CurrentExtent.b);
       NormalizeRect(R);
     end;
+    ADrawer.SetBrushParams(bsClear, clTAColor);
+    ADrawer.Pen := Pen;
     ADrawer.Line(R.Left, R.Bottom, R.Right, R.Top);
     exit;
   end;
 
-  if IsEmpty then
-    exit;
+  inherited;
+end;
 
-  with TDrawFuncHelper.Create(Self, DomainExclusions, @DoCalculate, Step) do
-    try
-      DrawFunction(ADrawer);
-    finally
-      Free;
-    end;
+procedure TFuncSeries.GetBounds(var ABounds: TDoubleRect);
+begin
+  inherited GetBounds(ABounds);
+  if not (csDesigning in ComponentState) or
+     not Extent.UseXMin or not Extent.UseXMax or not ExtentAutoY then exit;
+
+  // When designing, an oblique line is drawn (see TFuncSeries.Draw),
+  // so bounds should be adjusted when ExtentAutoY is True
+  ABounds.a.Y := ABounds.a.X;
+  ABounds.b.Y := ABounds.b.X;
 end;
 
 function TFuncSeries.IsEmpty: Boolean;
@@ -803,22 +808,24 @@
   t, ts, ms: Double;
   p, pp: TPoint;
 begin
-  ADrawer.SetBrushParams(bsClear, clTAColor);
-  ADrawer.Pen := Pen;
+  if not Active then exit;
 
   if csDesigning in ComponentState then begin
     with ParentChart do begin
-      R.TopLeft := GraphToImage(LogicalExtent.a);
-      R.BottomRight := GraphToImage(LogicalExtent.b);
+      R.TopLeft := GraphToImage(CurrentExtent.a);
+      R.BottomRight := GraphToImage(CurrentExtent.b);
       NormalizeRect(R);
     end;
+    ADrawer.SetBrushParams(bsClear, clTAColor);
+    ADrawer.Pen := Pen;
     ADrawer.Ellipse(R.Left, R.Bottom, R.Right, R.Top);
     exit;
   end;
 
-  if IsEmpty then
-    exit;
+  if IsEmpty then exit;
 
+  ADrawer.SetBrushParams(bsClear, clTAColor);
+  ADrawer.Pen := Pen;
   t := ParamMin;
   pp := PointAt(ParamMin);
   ADrawer.MoveTo(pp);
@@ -2742,10 +2749,10 @@
 
 function TColorMapSeries.FunctionValue(AX, AY: Double): Double;
 begin
-  if (csDesigning in ComponentState) or not Assigned(FOnCalculate) then
-    Result := 0
+  if Assigned(OnCalculate) then
+    OnCalculate(AX, AY, Result)
   else
-    OnCalculate(AX, AY, Result);
+    Result := 0;
 end;
 
 function TColorMapSeries.IsEmpty: Boolean;
update.diff (3,769 bytes)

Marcin Wiazowski

2019-03-07 20:51

reporter   ~0114702

> Raising an exception here is not good: Maybe the user just wants to have a TFuncSeries on the form and provides an OnCalculate handler later at runtime. This is the same as if a TLineSeries would raise an exception when its chartsource does not provide any data.

Right.



> I also did not use the DoCalculate calling DoCalcIdentity because now there is an IF inside the calculation routine; the old code avoided this by assigning different function variables to the DrawHelper depending on the situation

Well, I was fully aware of this; however, for chart's width equal to 2000 pixels, and default series' setting Step = 2, we get 1000 calls to DoCalculate - so "if Assigned(OnCalculate)" expression gives 1000 additional executions of a "test eax, eax; jz SomeOffset" sequence (on 32-bit Intel). This should give a total execution time about 1-2 microseconds on a 2GHz CPU - so not a big problem.



> Since after fixing the GetBounds issue the designtime curve depends on the Extent property I decided to just paint a diagonal at designtime without calling any function at all. And the parameteric series got an ellipse which, for me, is the prototype of a parametric curve.

Very nice!




I'm attaching update.diff, that fixes some last issues:

- Currently, when designing, TFuncSeries does not provide any visual feedback to the user, when switching its ExtentAutoY property - so GetBounds() has been overriden to provide this. There is no such problem with TParametricCurveSeries, because it doesn't implement ExtentAutoY

- An internal declaration of TParametricFunc is no longer needed - has been removed

- In TCustomFuncSeries.Draw(), I added "if IsEmpty or (not Active) then exit" to avoid calls to DoCalculate() in TCustomFuncSeries descendants (in particular in TFuncSeries, which - if IsEmpty - would call DoCalculate() with uninitialized calculation handler)

- In TFuncSeries.Draw(), I added "if not Active then exit" to avoid any further useless operations

- In TFuncSeries.Draw(), a piece of code was just a copy of the TCustomFuncSeries.Draw() method - so I replaced it with an "inherited" call

- In TParametricCurveSeries.Draw(), I added "if not Active then exit" to avoid any further useless operations

- In TParametricCurveSeries.Draw(), I moved ADrawer.SetBrushParams/Pen initialization to places, where it is really needed - GDI operations are time-consuming, so should be avoided when not necessary

- In TParametricCurveSeries.Draw(), I changed references from LogicalExtent to CurrentExtent - to be consistent with the similar code in TFuncSeries.Draw(), and also because CurrentExtent seems to be more general(?)

- In TColorMapSeries.FunctionValue(), a forgotten optimization has been made.

wp

2019-03-07 23:21

developer   ~0114704

> for chart's width equal to 2000 pixels, and default series' setting Step = 2,
> we get 1000 calls to DoCalculate - so "if Assigned(OnCalculate)" expression
> gives 1000 additional executions of a "test eax, eax; jz SomeOffset" sequence
> (on 32-bit Intel). This should give a total execution time about 1-2 microseconds
> on a 2GHz CPU - so not a big problem.

Who is talking here about optimization?

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

> In TParametricCurveSeries.Draw(), I moved ADrawer.SetBrushParams/Pen
> initialization to places, where it is really needed - GDI operations are time-
> consuming, so should be avoided when not necessary

These calls are needed in both cases, that's why they are at the beginning. They are useless only when IsEmpty is true - ok, I am wasting a fraction of the microseconds that I saved above...

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

> In TParametricCurveSeries.Draw(), I changed references from LogicalExtent to
> CurrentExtent - to be consistent with the similar code in TFuncSeries.Draw(),
> and also because CurrentExtent seems to be more general(?)

The FuncSeries runs across the entire x axis including the Margins; therefore we need the CurrentExtent. The range covered by the ParametricSeries, however, is not determined by the CurrentExtent at all, but by the parametric equations and the parameter range. When you define an OnCalculate handler with AX := cos(AT) and AY := sin(AT), AT = 0..2*pi, the plot is contained in a rectangle -1/-1 to +1/+1, and the data range defines the LogicalExtent.

Marcin Wiazowski

2019-03-08 00:37

reporter   ~0114707

> Who is talking here about optimization?

It seems that I just misunderstood your words.



> ok, I am wasting a fraction of the microseconds that I saved above...

I just checked this and - in fact - this code executes in about 0.3 of microsecond (on Windows). However, in general, it's good to avoid needless GDI calls - simple assignment, like Font1 := Font2, may potentially make a lot of GDI operations under the hood. This may be especially problematic, when performed in a loop.



Thanks for explanations about LogicalExtent and CurrentExtent.

Issue History

Date Modified Username Field Change
2019-03-07 00:15 Marcin Wiazowski New Issue
2019-03-07 00:15 Marcin Wiazowski File Added: Reproduce.zip
2019-03-07 00:15 Marcin Wiazowski File Added: patch.diff
2019-03-07 10:04 wp Assigned To => wp
2019-03-07 10:04 wp Status new => assigned
2019-03-07 16:28 wp Note Added: 0114701
2019-03-07 16:36 wp Fixed in Revision => 60609, 60610, 60611, 60615
2019-03-07 16:36 wp LazTarget => 2.2
2019-03-07 16:36 wp Status assigned => resolved
2019-03-07 16:36 wp Resolution open => fixed
2019-03-07 16:36 wp Target Version => 2.2
2019-03-07 20:49 Marcin Wiazowski File Added: update.diff
2019-03-07 20:51 Marcin Wiazowski Note Added: 0114702
2019-03-07 20:51 Marcin Wiazowski Status resolved => assigned
2019-03-07 20:51 Marcin Wiazowski Resolution fixed => reopened
2019-03-07 23:21 wp Note Added: 0114704
2019-03-07 23:21 wp Fixed in Revision 60609, 60610, 60611, 60615 => 60609, 60610, 60611, 60615, 60618
2019-03-07 23:21 wp Status assigned => resolved
2019-03-07 23:21 wp Resolution reopened => fixed
2019-03-08 00:37 Marcin Wiazowski Note Added: 0114707
2019-03-08 00:37 Marcin Wiazowski Status resolved => closed