View Issue Details

IDProjectCategoryView StatusLast Update
0035077LazarusTAChartpublic2019-03-03 23:54
ReporterMarcin WiazowskiAssigned Towp 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version2.1 (SVN)Product Build60414 
Target Version2.2Fixed in Version 
Summary0035077: TAChart: line series is drawn improperly for multi-value data sources
DescriptionThere is a problem in TLineSeries, when using multi-value data source: every label and every error bar is drawn multiple times, overwriting other, already drawn chart elements - in particular pointers.

In TLineSeries.Draw(), there is a loop:

  DrawSingleLineInStack(ADrawer, 0); <==== FIRST CALL (FOR Y0)
  for i := 0 to Source.YCount - 2 do begin
    if Source.IsYErrorIndex(i+1) then Continue;
    UpdateGraphPoints(i, FStacked);
    DrawSingleLineInStack(ADrawer, i + 1); <==== NEXT CALLS (FOR Y1, Y2, ...)
  end;



In each call to TLineSeries.DrawSingleLineInStack(..., AIndex) we have:

  case FColorEach of
    ceNone, cePoint:
      DrawDefaultLines; <==== draws a line requested by the AIndex parameter
    else
      DrawColoredLines; <==== draws a line requested by the AIndex parameter
  end;

  DrawErrorBars(ADrawer); <==== draws ALL error bars EACH TIME, regardless of the AIndex parameter

  DrawLabels(ADrawer); <==== draws ALL marks EACH TIME, regardless of the AIndex parameter

  if ShowPoints then <==== draws a pointer requested by the AIndex parameter
    DrawPointers(ADrawer, AIndex, FColorEach in [cePoint, cePointAndLineBefore, cePointAndLineAfter]);



The attached "Reproduce" application uses DoubleBuffered = False for the chart, so each drawing phase can be seen immediately.

To see the problem:
- launch the application and move its window to such a location, that the window is fully visible
- place a breakpoint at the beginning of the TLineSeries.DrawSingleLineInStack() method
- minimize the application's window and restore it again - it will be repainted, so breakpoint will be reached
- step over (F8) the TLineSeries.DrawSingleLineInStack() code (it will be called 4 times) and observe chart's behavior.

Drawing phases will be:
1) pink horizontal line is drawn for Y0 = 2; error bars are drawn for Y0 = 2; marks are drawn for ALL Y values (should be drawn only for Y0 = 2); two green pointers are drawn for Y0 = 2
2) pink horizontal line is drawn for Y1 = 4; error bars are drawn for Y0 = 2 (should not be drawn anymore), so pink horizontal line for Y1 = 4 and green pointers for Y0 = 2 become partially overwritten; marks are drawn for ALL Y values (should be drawn only for Y1 = 4); two green pointers are drawn for Y1 = 4
3) similarly to 2)
4) similarly to 2)

Final result can be seen on the attached Reproduce.png image:
- mark links (short red lines) are (re)drawn OVER green pointers for Y = 2, 4 and 6 (but not 8) - although all of them should be always located BELOW the pointers
- error bars (black lines) are (re)drawn OVER green pointers for Y = 2 and 4 - although they should be located BELOW the pointers
- error bars (black lines) are (re)drawn OVER the pink horizontal line for Y = 4 - although they should be located BELOW the line

To see a line series that is drawn properly, change data source's YCount from 4 to 1.

The cause of the problem is: "DrawErrorBars(ADrawer)" and "DrawLabels(ADrawer)" calls internally ignore the AIndex parameter.



Regards
TagsNo tags attached.
Fixed in Revision60420, 60423, 60448
LazTarget2.2
WidgetsetWin32/Win64
Attached Files

Activities

Marcin Wiazowski

2019-02-13 18:47

reporter  

Reproduce.zip (3,419 bytes)

Marcin Wiazowski

2019-02-13 18:47

reporter  

Reproduce.png (6,130 bytes)
Reproduce.png (6,130 bytes)

wp

2019-02-13 23:48

developer   ~0114095

No, all this is correct. Although DrawErrorBars and DrawLabels do not have AIndex as a parameter they get different plot data in FGraphPoints from the preceding UpdateGraphPoints call which filters the y values belonging to AIndex. Not very clear code, I know...

The main problem is a documentation error: You simply should not do this. If you use a line series with 4 y values you'll get error bars only for y0 - that's the way it is implemented, the functions TCustomChartSource.GetYErrorBarValues and -Limits only consider y0. I am not saying that it is not possible to extend it to error bars for all y values, but then the user might get the idea to stack error bar series, who knows, and so the entire stacking story would have to be re-written, and so I decided to stay with a single y value for error bars only to make life easier. I do not consider this to be a serious drawback because the user still can split a multi-y-value lineseries into several single-valued series.

I just updated the wiki saying that only y0 can be displayed by the current code.

I am resolving this as "won't fix".

Marcin Wiazowski

2019-02-14 00:26

reporter   ~0114097

Maybe I wasn't clear enough... I also think that error bars should be drawn only for Y0 - using same error values for different Y values doesn't make much sense. Since Y0 is always in use, and Y1, Y2, Y3 etc. are optional, error bars should be drawn for Y0.


All I wanted to say is that:

- error bars should be drawn only once, i.e. only when handling Y0 value - currently they are drawn 4 times, but each time at Y0 level! This causes drawing issues. The simplest fix I can see would be just:

  if AIndex = 0 then <==== ADDED
    DrawErrorBars(ADrawer);

- similarly, each mark (label) should be drawn only once - currently, each mark (for example "aaa 2") is drawn 4 times! When handling Y0 (AIndex = 0), all 8 marks are drawn (i.e. two at Y0 level, two at Y1 level, two at Y2 level and two at Y3 level). When handling Y1 (AIndex = 1) - once again, all 8 marks are drawn. Similarly for Y2 and Y3. So, in total, mark drawing is executed 32 times instead of 8 times. This repainting alters already drawn pointers (green squares) - because mark links (red short lines) overwrite the pointers, that have been drawn in the meantime.


In other words, each chart element that should be drawn is drawn, but some of them - i.e. error bars and labels - are drawn multiple times instead of just once - and ONLY THIS should be fixed. Drawing same elements multiple times causes visual issues (and slows drawing down). The "To see the problem" steps, that I listed above, seem to be the easiest way to see the visual issues immediately when they are appearing.

wp

2019-02-14 12:09

developer   ~0114106

Thanks for clarification.

Marcin Wiazowski

2019-02-14 22:26

reporter   ~0114125

Almost fixed - there is still one problem: "aaa 2", "aaa 4", "aaa 6", "aaa 8" and "bbb 8" are drawn as expected - but "bbb 2", "bbb 4" and "bbb 6" are NOT drawn at all.

In TBasicPointSeries.DrawLabels(), there is:

        if (AYIndex >= 0) then begin
          if si < AYIndex then
            Continue
          else if si > AYIndex then
            exit; <==== EXITS THE OUTER LOOP PREMATURELY
        end;

The "exit" clause exits TWO loops, and this causes the problem. Try changing just to:

        if (AYIndex >= 0) and (si <> AYIndex) then
          Continue;

wp

2019-02-14 22:51

developer   ~0114126

Done. Of course the "exit" is nonsense, it should be "break" because I don't want the calculations for the upper levels of the same data point be done.

Marcin Wiazowski

2019-02-15 02:08

reporter   ~0114132

All the problems described above are fixed now. Thanks to your patch, execution time of a SpeedTest application, attached to 0035031, has been dramatically reduced - from about 1860 ms to about 610 ms here!

Marcin Wiazowski

2019-02-15 02:10

reporter  

Reproduce2.zip (3,724 bytes)

Marcin Wiazowski

2019-02-15 02:10

reporter  

Reproduce2.png (17,033 bytes)
Reproduce2.png (17,033 bytes)

Marcin Wiazowski

2019-02-15 02:11

reporter   ~0114133

I found some additional, related problems - please see the attached Reproduce2 application and the image.


Problem 1: For TBSplineSeries, TCubicSplineSeries, TFitSeries and TBubbleSeries, only a line/bubbles for Y0 is/are drawn - although marks are drawn for all Y0, Y1, Y2 and Y3.


Problem 2: In TFitSeries.Draw(), there is:

    DrawLabels(ADrawer);
    DrawErrorBars(ADrawer);
    DrawPointers(ADrawer);

  so error bars are painted OVER marks - for example, at the attached Reproduce2 image, at Chart 3, the "aaa 2" mark is overwritten. The code should be changed to:

    DrawErrorBars(ADrawer);
    DrawLabels(ADrawer);
    DrawPointers(ADrawer);


Problem 3: In TBSplineSeries, TCubicSplineSeries, TFitSeries and TBubbleSeries, the "MarkPositions" property is public - although it should be PUBLISHED.

wp

2019-02-15 09:23

developer   ~0114134

> execution time of a SpeedTest application, attached to 0035031, has been
> dramatically reduced - from about 1860 ms to about 610 ms here!


Can't resist: Are you a sales man? In a "real" application this reduction will not be noticable...

Marcin Wiazowski

2019-02-15 14:58

reporter   ~0114149

:D


Of course, I'm not a sales man:

- I'm a born engineer, so it's just nice for me to see, that solving some problems give us - just for free - 5.5x faster execution time (reduction from 3370 ms to 610 ms),

- About real-life applications: on my chart, I have currently about 15 kinds of possible events - each kind using its own Y value on the chart. So, when using multi-value data source with YCount = 15, thanks to your last patch, I can reduce chart drawing time about 15x - starting from about 400 - 1500 ms (depending on the machine). Reducing drawing time from 1500 ms to about 100 - 150 ms *IS* a noticeable improvement, isn't it?


Regards!

wp

2019-02-16 00:57

developer   ~0114164

- BSplineSeries: done (r60430), open issue with ignoring Style.

- CubicSplineSeries: r60433, draws only the 1st y value (there is no reason for this limitation but my laziness to dive into the spline stuff again...)

- FitSeries: r60433, 60434. Also uses only the 1st y value. I will probably stick to that because having several fit curves would require making the statistical analysis of the fit results available for each curve, and it's rather confusing already now with a single curve.

- BubbleSeries: r60437 fixes positioning of the 2nd y value label (which is the bubble radius) at the bubble perimeter. MarkPositions does not work logically, the inside/outside options should refer to the center of the bubble, not the center of the viewport. AutoMargin must be updated for the BubbleRadiusUnit variants - but I don't know if it's worth the effort.

- r60432: Unpublished Marks and MarkPositions in the elemental series and published them again wherever useful, well, almost everywhere, but now I have better control over these properties.

- To do: check marks for Polar, OHLC and Box/Whisker.

Marcin Wiazowski

2019-02-18 01:35

reporter   ~0114235

r60443: there are two issues in TBSplineSeries.Draw():

- a call to "DrawErrorBars(ADrawer)" in a loop seems to be redundant and should be removed

- cosmetic: "splineend" -> "splineEnd"

wp

2019-02-18 23:34

developer   ~0114251

Done

Marcin Wiazowski

2019-02-25 18:52

reporter  

Reproduce4.zip (3,457 bytes)

Marcin Wiazowski

2019-02-25 18:52

reporter  

Reproduce4.png (7,451 bytes)
Reproduce4.png (7,451 bytes)

Marcin Wiazowski

2019-02-25 18:52

reporter  

Reproduce5.zip (3,490 bytes)

Marcin Wiazowski

2019-02-25 18:52

reporter  

Reproduce5.png (13,766 bytes)
Reproduce5.png (13,766 bytes)

Marcin Wiazowski

2019-02-25 18:53

reporter   ~0114422

Side note first: please take a look at 0035125, I attached an additional patch, and asked some questions there (I couldn't reopen, because it's your report, not mine).




Problem 1, 2 and 3: fixed.




> BSplineSeries: done (r60430), open issue with ignoring Style.

PROBLEM 4: Style is now properly applied to lines and marks, but for pointers only partially - see gray pointers in Reproduce 4.




> CubicSplineSeries: r60433, draws only the 1st y value [...]
> FitSeries: r60433, 60434. Also uses only the 1st y value [...]

Good choice.




> BubbleSeries [...]

PROBLEM 5: Marks for bubble series, for lmpInside and lmpPositive, are currently drawn in a weird and unpredictable way - see Reproduce 5.




> r60443: there are two issues in TBSplineSeries.Draw() [...]

Fixed.

wp

2019-02-25 22:28

developer   ~0114433

Last edited: 2019-02-25 22:37

View 2 revisions

> PROBLEM 4: Style is now properly applied to lines and marks, but for pointers
> only partially - see gray pointers in Reproduce 4.

Fixed

-----

> PROBLEM 5: Marks for bubble series, for lmpInside and lmpPositive, are currently
> drawn in a weird and unpredictable way - see Reproduce 5.

That's ok. Your bubbles are too big, you don't see the starting points of the marks because of the overlaps. Select a smaller radius (Y2 column in DataPoints editor) to avoid the overlap. The first data point (X, Y1) refers to the bubble centers - that's where the marker starts. The second data point (Y2) is the radius, its label begins at the perimeter of the circle/ellipse. Because I wanted to avoid overlapping labels in the lmpInside mode I decided to move the Y2 mark to the opposite position on the perimeter. Therefore, the words "inside" and "outside" are a bit out of place here because one of the labels is always outside and the other one inside the bubble. And there is an issue with margin calculation in one of the BubbleRadiusUnits options which I am too lazy to fix ATM.

Marcin Wiazowski

2019-02-26 00:03

reporter  

Reproduce6.zip (3,726 bytes)

Marcin Wiazowski

2019-02-26 00:03

reporter  

Reproduce6.gif (34,626 bytes)
Reproduce6.gif (34,626 bytes)

Marcin Wiazowski

2019-02-26 00:04

reporter   ~0114436

Again, side note first: still some issue in 0035125.



Problem 4: fixed.

Problem 5: nothing to fix.



Still some issues - see Reproduce6. All series share the same ListChartSource, having XCount = 2 and YCount = 4.



Problem 6a: On Chart1 and Chart2, X2 values from data source are taken into account for horizontal extent calculation. Interestingly, after clicking the "Stacked" checkbox above the Chart2, problem disappears there.



Problem 6b: On Chart3 and Chart4, move the mouse pointer, for example, to coordinates X=2 Y=8 or X=3 Y=9. Apparently, invisible points (having Y = Y2, Y3, Y4) are selected with CrosshairTool.

The piece of code below seems to solve the problem, but I'm not sure, if this is a right solution: in TCubicSplineSeries.GetNearestPoint() and TFitSeries.GetNearestPoint(), place this:

  Result := inherited GetNearestPoint(AParams, AResults);

  // ADDED:
  if Result and (AResults.FYIndex <> 0) then begin
    Result := false;
    AResults.FYIndex := -1; // should be here?
    exit;
  end;

Marcin Wiazowski

2019-02-26 00:39

reporter   ~0114438

Ad problem 6a:

In TCustomChartSource.ExtentCumulative(), used for stacked series, there is:

  Result := Extent;
  if YCount < 2 then exit;

In TCustomChartSource.ExtentList(), used for non-stacked series, there is:

  Result := Extent;
  if (YCount < 2) and (XCount < 2) then exit;

This causes the difference when toggling Chart2's checkbox.



BTW: Currently, there is a TCustomChartSource.BasicExtent() function, which uses only X0 and Y0 for calculations.

Maybe a general solution would be useful, something like: TCustomChartSource.Extent(AXCount, AYCount : Integer).



So, for TBSplineSeries and TLineSeries, something like this could be used:

XXX.ExtentCumulative():
  Result := Extent(1, YCount);

XXX.ExtentList():
  Result := Extent(1, YCount);

wp

2019-02-26 00:51

developer   ~0114441

The title of this report was "TLineSeries does not draw properly for multi-value data sources", and this has been fixed according to your original decription. Normally I am willing to handle related small bugs within the same report. But after dealing with TBSplineSeries, TCubicSplineSeries, TBubbleSeries, TFitSeries, we entered now the area of ChartTools. I must say STOP here, before this thread (like others) will never end: Please open new reports for other bugs. Nobody will ever be able to understand what's going on here.

To complete this thread and to answer your points before I resolve this report hopefully for the last time:

- TLineSeries and all other series (except for TFieldSeries) were never thought to work correctly with XCount > 1.

- The series have a property ToolTargets. When you include nptXList and nptYList in the set the tools will use the value in these lists as data. This behavior therefore is intended (well, not very nice for those series which do not display multiple values, I agree)

I will not fix these issues for the moment, there are more important things to do. (And if they were in separate reports there were a high chance that I would find them again, but having them at the end of a long thread I am not very optimistic...)

wp

2019-02-27 01:12

developer   ~0114479

Fixed 6a.

I think I keep 6b. Since the effect can easily be avoided by using XCount = 1 or ToolTargets without XLists and YLists this may also be considered to be a feature. Maybe somebody can take advantage of it (it requires some imagination, though).

Marcin Wiazowski

2019-03-03 23:54

reporter   ~0114609

Problem 6a: fix confirmed.

Problem 6b: ok.

Indeed, I made many of my reports large, by reopening them again and again with related issues - I'll try to avoid this.

Thanks!

Issue History

Date Modified Username Field Change
2019-02-13 18:47 Marcin Wiazowski New Issue
2019-02-13 18:47 Marcin Wiazowski File Added: Reproduce.zip
2019-02-13 18:47 Marcin Wiazowski File Added: Reproduce.png
2019-02-13 22:10 wp Assigned To => wp
2019-02-13 22:10 wp Status new => assigned
2019-02-13 23:48 wp LazTarget => -
2019-02-13 23:48 wp Note Added: 0114095
2019-02-13 23:48 wp Status assigned => resolved
2019-02-13 23:48 wp Resolution open => won't fix
2019-02-14 00:26 Marcin Wiazowski Note Added: 0114097
2019-02-14 00:26 Marcin Wiazowski Status resolved => assigned
2019-02-14 00:26 Marcin Wiazowski Resolution won't fix => reopened
2019-02-14 12:09 wp Fixed in Revision => 60420
2019-02-14 12:09 wp Note Added: 0114106
2019-02-14 12:09 wp Status assigned => resolved
2019-02-14 12:09 wp Resolution reopened => fixed
2019-02-14 22:26 Marcin Wiazowski Note Added: 0114125
2019-02-14 22:26 Marcin Wiazowski Status resolved => assigned
2019-02-14 22:26 Marcin Wiazowski Resolution fixed => reopened
2019-02-14 22:51 wp Fixed in Revision 60420 => 60420, 60423
2019-02-14 22:51 wp Note Added: 0114126
2019-02-14 22:51 wp Status assigned => resolved
2019-02-14 22:51 wp Resolution reopened => fixed
2019-02-15 02:08 Marcin Wiazowski Note Added: 0114132
2019-02-15 02:10 Marcin Wiazowski File Added: Reproduce2.zip
2019-02-15 02:10 Marcin Wiazowski File Added: Reproduce2.png
2019-02-15 02:11 Marcin Wiazowski Note Added: 0114133
2019-02-15 02:11 Marcin Wiazowski Status resolved => assigned
2019-02-15 02:11 Marcin Wiazowski Resolution fixed => reopened
2019-02-15 09:23 wp Note Added: 0114134
2019-02-15 14:58 Marcin Wiazowski Note Added: 0114149
2019-02-16 00:57 wp Note Added: 0114164
2019-02-18 01:35 Marcin Wiazowski Note Added: 0114235
2019-02-18 23:34 wp Fixed in Revision 60420, 60423 => 60420, 60423, 60448
2019-02-18 23:34 wp LazTarget - => 2.2
2019-02-18 23:34 wp Note Added: 0114251
2019-02-18 23:34 wp Status assigned => resolved
2019-02-18 23:34 wp Resolution reopened => fixed
2019-02-18 23:34 wp Target Version => 2.2
2019-02-25 18:52 Marcin Wiazowski File Added: Reproduce4.zip
2019-02-25 18:52 Marcin Wiazowski File Added: Reproduce4.png
2019-02-25 18:52 Marcin Wiazowski File Added: Reproduce5.zip
2019-02-25 18:52 Marcin Wiazowski File Added: Reproduce5.png
2019-02-25 18:53 Marcin Wiazowski Note Added: 0114422
2019-02-25 18:53 Marcin Wiazowski Status resolved => assigned
2019-02-25 18:53 Marcin Wiazowski Resolution fixed => reopened
2019-02-25 22:28 wp Note Added: 0114433
2019-02-25 22:28 wp Status assigned => resolved
2019-02-25 22:28 wp Resolution reopened => fixed
2019-02-25 22:37 wp Note Edited: 0114433 View Revisions
2019-02-26 00:03 Marcin Wiazowski File Added: Reproduce6.zip
2019-02-26 00:03 Marcin Wiazowski File Added: Reproduce6.gif
2019-02-26 00:04 Marcin Wiazowski Note Added: 0114436
2019-02-26 00:04 Marcin Wiazowski Status resolved => assigned
2019-02-26 00:04 Marcin Wiazowski Resolution fixed => reopened
2019-02-26 00:39 Marcin Wiazowski Note Added: 0114438
2019-02-26 00:51 wp Note Added: 0114441
2019-02-26 00:51 wp Status assigned => resolved
2019-02-26 00:51 wp Resolution reopened => fixed
2019-02-27 01:12 wp Note Added: 0114479
2019-03-03 23:54 Marcin Wiazowski Note Added: 0114609
2019-03-03 23:54 Marcin Wiazowski Status resolved => closed