View Issue Details

IDProjectCategoryView StatusLast Update
0035459LazarusTAChartpublic2019-05-07 00:59
ReporterMarcin WiazowskiAssigned Towp 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionreopened 
Product Version2.1 (SVN)Product Build61066 
Target VersionFixed in Version2.2 
Summary0035459: TAChart: patch to minimize flickering in zoom drag tool
DescriptionLet's make some experiment:

1) Launch "extent_limit_test_mod2" test application, that is attached to 0035344

2) Set RatioLimit to zrlFixedX

3) Start making a selection, by using the zoom drag tool

4) As can be seen, selected rectangle occupies the whole chart space from left to right, regardless of the current mouse's X position

5) This means, that - when moving the mouse ideally horizontally - selection rectangle does not change in any way

6) But some flickering of the selection rectangle is sometimes visible


This is because each mouse move redraws the selection - even when it's shape is not changed.

The attached tiny patch detects such cases and avoids useless selection redrawing.
TagsNo tags attached.
Fixed in Revision61166
LazTarget2.2
WidgetsetWin32/Win64
Attached Files
  • patch.diff (773 bytes)
    Index: components/tachart/tatools.pas
    ===================================================================
    --- components/tachart/tatools.pas	(revision 61066)
    +++ components/tachart/tatools.pas	(working copy)
    @@ -1391,6 +1391,14 @@
     procedure TZoomDragTool.SetSelectionRect(AValue: TRect);
     begin
       if (FSelectionRect = AValue) or not IsActive or IsAnimating then exit;
    +  if AdjustSelection and
    +    (((RatioLimit = zrlFixedX) and
    +      (FSelectionRect.Top = AValue.Top) and
    +      (FSelectionRect.Bottom = AValue.Bottom)) or
    +     ((RatioLimit = zrlFixedY) and
    +      (FSelectionRect.Left = AValue.Left) and
    +      (FSelectionRect.Right = AValue.Right))) then exit;
    +
       case EffectiveDrawingMode of
         tdmXor: with GetCurrentDrawer do begin
           SetXor(true);
    
    patch.diff (773 bytes)
  • flickering.mp4 (382,118 bytes)

Activities

Marcin Wiazowski

2019-04-28 02:17

reporter  

patch.diff (773 bytes)
Index: components/tachart/tatools.pas
===================================================================
--- components/tachart/tatools.pas	(revision 61066)
+++ components/tachart/tatools.pas	(working copy)
@@ -1391,6 +1391,14 @@
 procedure TZoomDragTool.SetSelectionRect(AValue: TRect);
 begin
   if (FSelectionRect = AValue) or not IsActive or IsAnimating then exit;
+  if AdjustSelection and
+    (((RatioLimit = zrlFixedX) and
+      (FSelectionRect.Top = AValue.Top) and
+      (FSelectionRect.Bottom = AValue.Bottom)) or
+     ((RatioLimit = zrlFixedY) and
+      (FSelectionRect.Left = AValue.Left) and
+      (FSelectionRect.Right = AValue.Right))) then exit;
+
   case EffectiveDrawingMode of
     tdmXor: with GetCurrentDrawer do begin
       SetXor(true);
patch.diff (773 bytes)

wp

2019-04-30 10:55

developer   ~0115915

Last edited: 2019-04-30 11:02

View 3 revisions

This is micro-optimization for a case with little practical relevance. I was not able to move the mouse exactly horizontally to see this flicker.

Marcin Wiazowski

2019-05-01 01:55

reporter   ~0115936

Flickering - or not - depends probably on graphics card kind and/or video drivers.

I made a short video for you - this is how it looks on my machine. When watching, please focus not at the mouse cursor, but try to observe the whole selection frame - so flickering can be more easily observed. The attached video has only 15 fps, so flickering is much less visible than when looking at the real monitor.

Since I'm not a master of horizontal mouse moving, there are moments, that mouse's Y coordinate is changed, so - of course - selection frame must be redrawn in these cases. But, regardless of that, applying the patch gives much better visual behavior on my machine, even when making natural (not artificially horizontal) mouse movements.

So the attached patch would be useful for users like me.

Marcin Wiazowski

2019-05-01 01:55

reporter  

flickering.mp4 (382,118 bytes)

wp

2019-05-01 12:23

developer   ~0115942

I see the flicker now, and it is gone with the patch. But the selection rect is normally dragged in the other direction, and this is where flickering is still present, of course.

This kind of flickering is caused by the XOR drawing mode of the zoom rectangle. Switch to tdmNormal and the flicker will be gone for dragging in any direction (although at the expense of full repainting of the chart).

I still think that the use case for the patch is not relevant enough to justify an increase of the TZoomDragTool.SetSelectionRect method's line count by almost 50%.

Marcin Wiazowski

2019-05-01 15:00

reporter   ~0115943

In fact, the patch adds 94 bytes of code (32-bit, with optimizations off) to TZoomDragTool.SetSelectionRect() - although this method isn't time critical, so there should be no efficiency problem here.

As I mentioned, when looking directly at the screen, the flickering is much more unpleasant than can be seen on the attached video. And double buffering does not help here - as you noticed, the problem is due to double xoring (which is the default mode, at least on Windows). So the user can do nothing in this case.

So I can see more advantages (i.e. much more visual behavior, although only on some machines) than disadvantages (94 additional bytes of code). And, as always, the final decision is yours.

wp

2019-05-01 18:21

developer   ~0115945

I was expecting this kind of response. 94 bytes. Am I nitpicking? But: "A penny saved is a penny earned". Why should I waste 94 bytes for a functionality which is not or rarely needed? When the user wants to drag a zoom rect for the case zlrFixedX he must drag the mouse vertically, not horizontally. And the flicker when dragging vertically is not affected by the patch, there is no better visual behavior in this case.

OK, nobody wants to be nitpicking. To acknowledge your work I could imagine to accept the patch if it were more general. Now it exits the SetSelectionRect method for a certain combination of parameters. What if at some time in the future another combination of parameters would lead to the same effect? Then we have to touch this method again, study the source code again (because this discussion will be forgotten), more discussions, the nitpicking argument will still be valid, etc... The root cause is (besides XOR painting which cannot be changed) unnecessary painting of the same rectangle for whatever reason. Why not simply check for this condition?

A quick test showed that the following code has the same effect like yours, but just rearranges the existing code (and probably costs even less than 94 bytes, but I did not test this):

  procedure TZoomDragTool.SetSelectionRect(AValue: TRect);
  var
    rOld, rNew: TRect;
  begin
    if (FSelectionRect = AValue) or not IsActive or IsAnimating then exit;
    case EffectiveDrawingMode of
      tdmXor: with GetCurrentDrawer do begin
        rOld := CalculateDrawRect;
        FSelectionRect := AValue;
        rNew := CalculateDrawRect;
        if rOld = rNew then exit; // repainting the same rectangle in XOR mode would create unnecessary flicker
        SetXor(true);
        Pen := Frame;
        Brush := Self.Brush;
        Rectangle(rOld);
        Rectangle(rNew);
        SetXor(false);
      end;
      tdmNormal: begin
        FSelectionRect := AValue;
        FChart.StyleChanged(Self);
      end;
    end;
  end;

Marcin Wiazowski

2019-05-05 20:43

reporter   ~0116025

Last edited: 2019-05-05 20:44

View 2 revisions

You made this in the right way; this is how it should be from the beginning.

I would vote for your solution (this is an example why such discussions can be useful - I was solving some specific problem, and I lost generality - but you looked from the other point of view, so you pointed this out).

wp

2019-05-06 18:28

developer   ~0116052

Done.

Marcin Wiazowski

2019-05-07 00:59

reporter   ~0116055

Thanks!

Issue History

Date Modified Username Field Change
2019-04-28 02:17 Marcin Wiazowski New Issue
2019-04-28 02:17 Marcin Wiazowski File Added: patch.diff
2019-04-29 14:46 Maxim Ganetsky Assigned To => wp
2019-04-29 14:46 Maxim Ganetsky Status new => assigned
2019-04-30 10:55 wp Note Added: 0115915
2019-04-30 10:55 wp Status assigned => resolved
2019-04-30 10:55 wp Resolution open => no change required
2019-04-30 10:55 wp LazTarget => -
2019-04-30 10:55 wp Widgetset Win32/Win64 => Win32/Win64
2019-04-30 11:02 wp Note Edited: 0115915 View Revisions
2019-04-30 11:02 wp Note Edited: 0115915 View Revisions
2019-05-01 01:55 Marcin Wiazowski Status resolved => assigned
2019-05-01 01:55 Marcin Wiazowski Resolution no change required => reopened
2019-05-01 01:55 Marcin Wiazowski Note Added: 0115936
2019-05-01 01:55 Marcin Wiazowski File Added: flickering.mp4
2019-05-01 12:23 wp Note Added: 0115942
2019-05-01 15:00 Marcin Wiazowski Note Added: 0115943
2019-05-01 18:21 wp Note Added: 0115945
2019-05-05 20:43 Marcin Wiazowski Note Added: 0116025
2019-05-05 20:44 Marcin Wiazowski Note Edited: 0116025 View Revisions
2019-05-06 18:28 wp Status assigned => resolved
2019-05-06 18:28 wp Fixed in Version => 2.2
2019-05-06 18:28 wp Fixed in Revision => 61166
2019-05-06 18:28 wp LazTarget - => 2.2
2019-05-06 18:28 wp Widgetset Win32/Win64 => Win32/Win64
2019-05-06 18:28 wp Note Added: 0116052
2019-05-07 00:59 Marcin Wiazowski Status resolved => closed
2019-05-07 00:59 Marcin Wiazowski Note Added: 0116055