View Issue Details
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0035634||Lazarus||TAChart||public||2019-05-25 22:01||2019-06-15 00:03|
|Reporter||Marcin Wiazowski||Assigned To||wp|
|Product Version||2.1 (SVN)|
|Summary||0035634: TAChart: lack of cooperation between zooming tools and chart's ExtentSizeLimit|
|Description||I'm attaching a patch, that solves some problem with the (lack of) cooperation between chart's ExtentSizeLimit and zooming tools.|
Let's make an experiment first: launch the attached "extent_limit_test_mod3" application, enable "Use XMin = 0.9" and "Use XMax = 1.1" options, and try to use zoom click or zoom wheel tool - nothing happens. Then enable also "Animation (all zoom tools)" option - now some zooming occurs.
Explanation: when we look into TChart.SetLogicalExtent() we can see, that chart's ExtentSizeLimit works in the following way: when trying to assign some new value to LogicalExtent, width and height of the requested new extent are verified - and, if some of them is below or above the allowed limit, no change in the logical extent is performed at all - the whole request is rejected.
The general idea of zooming tools is quite simple: they set LogicalExtent to needed values - thus performing zooming in or out. As a consequence, if the requested LogicalExtent doesn't conform chart's ExtentSizeLimit, zooming just fails.
In our case, the initial horizontal extent is 0 .. 1 (so its width is 1), the allowed minimum width is 0.9, and the allowed maximum width is 1.1. Zoom click and zoom wheel tools have ZoomFactor set to 1.2, so - when zooming out, if animations are off - they just try to set the new horizontal extent to -0.1 .. 1.1 (so width is 1.2). This operation fails.
When animation is enabled, 10 steps of animation are used - this means, that the following consecutive extents are requested:
0) 0 .. 1 (initial extent)
1) -0.01 .. 1.01 (width = 1.02)
2) -0.02 .. 1.02 (width = 1.04)
3) -0.03 .. 1.03 (width = 1.06)
4) -0.04 .. 1.04 (width = 1.08)
5) -0.05 .. 1.05 (width = 1.1)
6) -0.06 .. 1.06 (width = 1.12)
7) -0.07 .. 1.07 (width = 1.14)
8) -0.08 .. 1.08 (width = 1.16)
9) -0.09 .. 1.09 (width = 1.18)
10) -0.1 .. 1.1 (width = 1.2)
In this case, requests 1) .. 5) are within the allowed range, so these animation steps can be applied and seen, and requests 6) .. 10) are above the allowed range, so they are ignored (and the animation finishes prematurely).
The solution is: we should check, if the requested new extent is within the allowed range; if not, we should downscale it - in our case this means, that our one step (if animation is off) or 10 steps (if animation is on) should be applied to achieve not transformation from 0 .. 1 to -0.1 .. 1.1, but from 0 .. 1 to -0.05 .. 1.05.
In the example above, some final animation steps fail. It is also possible, that some *initial* steps may fail - this can occur, for example, when we are zooming in, and ExtentSizeLimit's maximum width is *smaller* than the full extent (full extent can be successfully applied always, regardless of the ExtentSizeLimit settings) - well, this isn't very clever setting, but can occur.
So, finally, the patch should verify both initial and final extents, and - if needed - adjust both of them, to be within the allowed ExtentSizeLimit's minimum and maximum ranges.
In report 0035344, LimitToExtent functionality has been introduced in zooming tools. The patch attached here is a bit similar - in both cases, maximum extent's width is applied if needed. Differences are:
- here, we must take care not only about the maximum extent's width, but also minimum extent's width,
- when dealing with ExtentSizeLimit, the requested (i.e. assigned to LogicalExtent) ranges must be contained strictly within ExtentSizeLimit - if our request is under/oversized even only due to lack of precision of the floating-point calculations, our request will be rejected.
The attached code isn't very complicated, although it must handle four limits (XMin, XMax, YMin, YMax), both of them when zooming in and out - so similar code must be used several times.
After applying the patch, zooming just works - in one step (if animation is off) or in the requested number of steps (if animation is on) - zooming no longer can finish prematurely (or start with some delay, which could occur without the patch, when failing to apply some initial animation steps).
Similarly as in the 0035344 patch, there is a special handling for zooming out, to allow showing the full extent back - if one of the dimensions can no longer be zoomed out, the proportionality rule is relaxed, to allow achieving the full extent's size also by the another dimension.
|Tags||No tags attached.|
|Fixed in Revision|
extent_limit_test_mod3.zip (3,836 bytes)
patch.diff (9,223 bytes)
Index: components/tachart/tatools.pas =================================================================== --- components/tachart/tatools.pas (revision 61292) +++ components/tachart/tatools.pas (working copy) @@ -1164,7 +1164,7 @@ procedure TBasicZoomTool.DoZoom(ANewExtent: TDoubleRect; AFull: Boolean); - procedure ValidateNewSize(LimitLo, LimitHi: TZoomDirection; + procedure LTEValidateNewSize(LimitLo, LimitHi: TZoomDirection; const PrevSize, NewSize, MaxSize, ImageMaxSize: Double; out Scale: Double; out AllowProportionalAdjustment: Boolean); begin @@ -1206,7 +1206,7 @@ Scale := (MaxSize - PrevSize) / (NewSize - PrevSize); end; - procedure AdjustNewSizeAndPosition(LimitLo, LimitHi: TZoomDirection; + procedure LTEAdjustNewSizeAndPosition(LimitLo, LimitHi: TZoomDirection; var NewSizeLo, NewSizeHi: Double; const MaxSizeLo, MaxSizeHi: Double); var Diff: Double; @@ -1240,26 +1240,109 @@ end; end; + function ESLValidateSizes(const PrevSize, NewSize, MinSize, MaxSize, + ImageMaxSize: Double; out ScaleFrom, ScaleTo: Double; + out AllowProportionalAdjustment: Boolean): Boolean; + begin + ScaleFrom := 0; + ScaleTo := 1; + AllowProportionalAdjustment := true; + + if PrevSize > NewSize then begin + Result := PrevSize >= MinSize; + if Result and (NewSize < MinSize) then + ScaleTo := Min(ScaleTo, (MinSize - PrevSize) / (NewSize - PrevSize)); + end else + begin + Result := NewSize >= MinSize; + if Result and (PrevSize < MinSize) then + ScaleFrom := Max(ScaleFrom, (MinSize - PrevSize) / (NewSize - PrevSize)); + end; + if not Result then exit; + + if PrevSize < NewSize then begin + Result := PrevSize <= MaxSize; + if Result and (NewSize > MaxSize) then begin + ScaleTo := Min(ScaleTo, (MaxSize - PrevSize) / (NewSize - PrevSize)); + + // if max size is only a bit greater than previous size, this may be due to + // limited precision of floating-point calculations, so - if change in size + // is smaller than half of the pixel - disable proportional adjustments; in + // this case, adjusting the other dimension will be performed independently + if MaxSize < PrevSize * (1 + 0.5 / abs(ImageMaxSize)) then + AllowProportionalAdjustment := false; + end; + end else + begin + Result := NewSize <= MaxSize; + if Result and (PrevSize > MaxSize) then + ScaleFrom := Max(ScaleFrom, (MaxSize - PrevSize) / (NewSize - PrevSize)); + end; + if not Result then exit; + + Result := ScaleFrom <= ScaleTo; + end; + + procedure ESLAdjustSize(var a, b: Double; const SizeMin, SizeMax: Double); + var + Size: Double; + SaveMode: TFPURoundingMode; + begin + // At this stage of calculations, size, that is smaller or grater than + // allowed, can occur only due to limited precision of the already performed + // floating-point calculations (we could take care of this earlier, but it's + // just much easier to correct this now, at the end of calculations). Size + // must be strictly within the chart's ExtentSizeLimit bounds, otherwise + // TChart.SetLogicalExtent() call will fail. + Size := b - a; + if Size < SizeMin then begin + a := (a + b - SizeMin) * 0.5; + SaveMode := SetRoundMode(rmUp); + try + b := a + SizeMin; + finally + SetRoundMode(SaveMode); + end; + end; + if Size > SizeMax then begin + a := (a + b - SizeMax) * 0.5; + SaveMode := SetRoundMode(rmDown); + try + b := a + SizeMax; + finally + SetRoundMode(SaveMode); + end; + end; + end; + var FullExt: TDoubleRect; + ImageMaxSizeX, ImageMaxSizeY: Double; ScaleX, ScaleY: Double; + ScaleXFrom, ScaleXTo, ScaleYFrom, ScaleYTo: Double; + SizeXMin, SizeXMax, SizeYMin, SizeYMax: Double; AllowProportionalAdjustmentX, AllowProportionalAdjustmentY: Boolean; + ZoomPossible: Boolean; begin - if not AFull then + FullExt := FChart.GetFullExtent; + + ImageMaxSizeX := FChart.XGraphToImage(FullExt.b.X) - FChart.XGraphToImage(FullExt.a.X); + ImageMaxSizeY := FChart.YGraphToImage(FullExt.b.Y) - FChart.YGraphToImage(FullExt.a.Y); + + // Handle LimitToExtent + if not AFull then begin // perform the actions below even when LimitToExtent is empty - this will // correct sub-pixel changes in viewport size (occuring due to limited // precision of floating-point calculations), which will result in a more // smooth visual behavior with ANewExtent do begin - FullExt := FChart.GetFullExtent; - - ValidateNewSize(zdLeft, zdRight, FChart.LogicalExtent.b.X - FChart.LogicalExtent.a.X, - b.X - a.X, FullExt.b.X - FullExt.a.X, - FChart.XGraphToImage(FullExt.b.X) - FChart.XGraphToImage(FullExt.a.X), + LTEValidateNewSize(zdLeft, zdRight, + FChart.LogicalExtent.b.X - FChart.LogicalExtent.a.X, + b.X - a.X, FullExt.b.X - FullExt.a.X, ImageMaxSizeX, ScaleX, AllowProportionalAdjustmentX); - ValidateNewSize(zdDown, zdUp, FChart.LogicalExtent.b.Y - FChart.LogicalExtent.a.Y, - b.Y - a.Y, FullExt.b.Y - FullExt.a.Y, - FChart.YGraphToImage(FullExt.b.Y) - FChart.YGraphToImage(FullExt.a.Y), + LTEValidateNewSize(zdDown, zdUp, + FChart.LogicalExtent.b.Y - FChart.LogicalExtent.a.Y, + b.Y - a.Y, FullExt.b.Y - FullExt.a.Y, ImageMaxSizeY, ScaleY, AllowProportionalAdjustmentY); if AllowProportionalAdjustmentX and AllowProportionalAdjustmentY and @@ -1273,15 +1356,67 @@ a.Y := WeightedAverage(FChart.LogicalExtent.a.Y, a.Y, ScaleY); b.Y := WeightedAverage(FChart.LogicalExtent.b.Y, b.Y, ScaleY); - AdjustNewSizeAndPosition(zdLeft, zdRight, a.X, b.X, FullExt.a.X, FullExt.b.X); - AdjustNewSizeAndPosition(zdDown, zdUp, a.Y, b.Y, FullExt.a.Y, FullExt.b.Y); + LTEAdjustNewSizeAndPosition(zdLeft, zdRight, a.X, b.X, FullExt.a.X, FullExt.b.X); + LTEAdjustNewSizeAndPosition(zdDown, zdUp, a.Y, b.Y, FullExt.a.Y, FullExt.b.Y); end; + end else + ANewExtent := FullExt; - if (AnimationInterval = 0) or (AnimationSteps = 0) then begin + // Handle FChart.ExtentSizeLimit + with ANewExtent do begin + with FChart.ExtentSizeLimit do begin + SizeXMin := IfThen(UseXMin, XMin, -MaxDouble); + SizeXMax := IfThen(UseXMax, XMax, MaxDouble); + SizeYMin := IfThen(UseYMin, YMin, -MaxDouble); + SizeYMax := IfThen(UseYMax, YMax, MaxDouble); + end; + + ZoomPossible := + ESLValidateSizes( + FChart.LogicalExtent.b.X - FChart.LogicalExtent.a.X, b.X - a.X, SizeXMin, + SizeXMax, ImageMaxSizeX, ScaleXFrom, ScaleXTo, AllowProportionalAdjustmentX) and + ESLValidateSizes( + FChart.LogicalExtent.b.Y - FChart.LogicalExtent.a.Y, b.Y - a.Y, SizeYMin, + SizeYMax, ImageMaxSizeY, ScaleYFrom, ScaleYTo, AllowProportionalAdjustmentY); + + if ZoomPossible and AllowProportionalAdjustmentX and + AllowProportionalAdjustmentY and IsProportional then begin + ScaleXFrom := Max(ScaleXFrom, ScaleYFrom); + ScaleYFrom := ScaleXFrom; + + ScaleXTo := Min(ScaleXTo, ScaleYTo); + ScaleYTo := ScaleXTo; + + ZoomPossible := (ScaleYFrom <= ScaleYTo); + end; + + if ZoomPossible then begin + FExtSrc.a.X := WeightedAverage(FChart.LogicalExtent.a.X, a.X, ScaleXFrom); + FExtSrc.b.X := WeightedAverage(FChart.LogicalExtent.b.X, b.X, ScaleXFrom); + FExtSrc.a.Y := WeightedAverage(FChart.LogicalExtent.a.Y, a.Y, ScaleYFrom); + FExtSrc.b.Y := WeightedAverage(FChart.LogicalExtent.b.Y, b.Y, ScaleYFrom); + ESLAdjustSize(FExtSrc.a.X, FExtSrc.b.X, SizeXMin, SizeXMax); + ESLAdjustSize(FExtSrc.a.Y, FExtSrc.b.Y, SizeYMin, SizeYMax); + + FExtDst.a.X := WeightedAverage(FChart.LogicalExtent.a.X, a.X, ScaleXTo); + FExtDst.b.X := WeightedAverage(FChart.LogicalExtent.b.X, b.X, ScaleXTo); + FExtDst.a.Y := WeightedAverage(FChart.LogicalExtent.a.Y, a.Y, ScaleYTo); + FExtDst.b.Y := WeightedAverage(FChart.LogicalExtent.b.Y, b.Y, ScaleYTo); + ESLAdjustSize(FExtDst.a.X, FExtDst.b.X, SizeXMin, SizeXMax); + ESLAdjustSize(FExtDst.a.Y, FExtDst.b.Y, SizeYMin, SizeYMax); + end else begin + FExtSrc := FullExt; + FExtDst := FullExt; + end; + end; + + if (AnimationInterval = 0) or (AnimationSteps = 0) or + (FExtSrc = FExtDst) or (not ZoomPossible) then begin if AFull then FChart.ZoomFull else - FChart.LogicalExtent := ANewExtent; + if ZoomPossible then + FChart.LogicalExtent := FExtDst; if IsActive then Deactivate; exit; @@ -1288,8 +1423,6 @@ end; if not IsActive then Activate; - FExtSrc := FChart.LogicalExtent; - FExtDst := ANewExtent; FFullZoom := AFull; FCurrentStep := 0; FTimer.Interval := AnimationInterval; @@ -1470,7 +1603,7 @@ if not Chart.IsZoomed and (EffectiveDrawingMode = tdmNormal) then // ZoomFull will not cause redraw, force it to erase the tool. Chart.StyleChanged(Self); - DoZoom(FChart.GetFullExtent, true); + DoZoom(EmptyExtent, true); Handled; exit; end;
patch.diff (9,223 bytes)
I am hesitant, again... The patch seems to be ok, but: do we still need "ExtentSizeLimit"? I never understood why it only limits the "Size", and not the Extent itself. You recently added LimitToExtent properties to the chart extent tools. They are working fine. We could even go a step further and introduce a chart-global "ExtentLimit" (without "Size"), it could replace all the individual LimitToExtent settings of combined extent tools, because I don't see a reason why a ZoomDrag and a ZoomMouseWheel tool should use different extents at all. Maybe having a single extent validation in Chart.SetLogicalExtent would also fix the missing solution for extent changes in the NavPanel. (The OnExtentValidate event should override the ExtentLimit settings).
So, essentailly I like to deprecate "ExtentSizeLimit". Of course, this will take some time until it will disappear from the released product, therefore, it is maybe necessary to apply the patch anyway for the intermediate time.
Is there anything that I am missing?
> The patch seems to be ok, but: do we still need "ExtentSizeLimit"? I never understood why it only limits the "Size", and not the Extent itself. You recently added LimitToExtent properties to the chart extent tools. They are working fine. [...] So, essentailly I like to deprecate "ExtentSizeLimit". Of course, this will take some time until it will disappear from the released product, therefore, it is maybe necessary to apply the patch anyway for the intermediate time. Is there anything that I am missing?
I created - and attached here - a small test application. I performed some tests and it turns out, that ExtentSizeLimit and LimitToExtent are complementary, rather than replaceable:
Test 1 (r61353):
- move mouse to X=5, Y=0.5
- use mouse wheel to zoom in again, and again - we finally end up with some very large zoom, with Y range like 0.3 .. 0.6; no series is visible with such an enormous zoom - so not experienced users may get confused
- now try the same after making "use ExtentSizeLimit.YMin = 1" checked - this time, zooming stops and some series is always visible - which should prevent users from being confused
Test 2 (r61353):
- use zoom drag tool to select region X=5..6, Y=-1..-2.25
- after zooming, no series is visible, and we are partially below the full extent
- now try the same after making "use LimitToExtent Up/Down" checked - this time, selection is moved back to the full extent's area, so some series is always visible, which should prevent users from being confused
So both ExtentSizeLimit and LimitToExtent functionalities have their separate tasks - and LimitToExtent can't be used instead of ExtentSizeLimit in Test 1, also ExtentSizeLimit can't be used instead of LimitToExtent in Test 2. I also found Test 1 case described in http://wiki.freepascal.org/TAChart_Tutorial:_Chart_Tools#Using_the_ExtentSizeLimit.
There is currently some issue:
Test 3 (r61353):
- make "use ExtentSizeLimit.YMin = 1" checked
- use zoom drag tool to select region X=5..6, Y=0.3..0.5
- nothing happens; this is because the requested vertical range is 0.5-0.3 = 0.2, which is smaller than 1
After applying the attached patch, selected range is adjusted, so vertical range becomes 1. This means, that zooming works even when the selected range is too tight - the range is adjusted, and some series is always visible.
Some small summary:
* ExtentSizeLimit XMin/YMin can be used to avoid zooming in too much (so we may end up somewhere between series, so no series is visible)
* ExtentSizeLimit XMax/YMax can be used to avoid zooming out too much (so our data becomes very small, and we can see mainly empty chart's area)
* LimitToExtent can be used to avoid zooming beyond the full extent at chart egdes
* the attached patch fixes the animation problem
* by its nature, the attached patch fixes also the Test 3 problem
> We could even go a step further and introduce a chart-global "ExtentLimit" (without "Size"), it could replace all the individual LimitToExtent settings of combined extent tools, because I don't see a reason why a ZoomDrag and a ZoomMouseWheel tool should use different extents at all.
> Maybe having a single extent validation in Chart.SetLogicalExtent would also fix the missing solution for extent changes in the NavPanel. (The OnExtentValidate event should override the ExtentLimit settings).
This is a very good idea, I think.
Maybe this should be done in the following way:
1) The attached patch is applied
2) A new, public method in TChart is introduced:
function TChart.GetExtentTransition(const AExtentRequested: TDoubleRect; AFull, AProportional: Boolean; out AExtentForm, AExtentTo: TDoubleRect): Boolean;
3) The code from TBasicZoomTool.DoZoom(), between "begin" and "if (AnimationInterval = 0) or (AnimationSteps = 0)" is moved to TChart.GetExtentTransition and adapted as needed; since this code refers to FChart, adapting should be easy; the returned result should be taken from the current ZoomPossible variable.
4) TBasicZoomTool.LimitToExtent shoult be migrated to a new property: TChart.ExtentLimit
5) TBasicZoomTool.DoZoom() should now look like:
ZoomPossible := FChart.GetExtentTransition(ANewExtent, AFull, IsProportional, FExtSrc, FExtDst);
if (AnimationInterval = 0) or (AnimationSteps = 0) ...
This way, all the extent validation is now located in TChart.GetExtentTransition(), which handles both the currently existing TChart.ExtentSizeLimit and the newly introduced TChart.ExtentLimit.
6) In TChart.SetLogicalExtent(), GetExtentTransition() call should be made after OnExtentValidate handling:
if Assigned(OnExtentValidate) then begin
if not AllowChange then exit;
if not GetExtentTransition(AValue, false, false, ExtSrc, ExtDst) then
AValue := ExtDst;
if FLogicalExtent = AValue then exit;
This should automatically add extent limit handling to TChartNavPanel, because TChartNavPanel just sets chart's logical extent in its TChartNavPanel.MouseMove().
I don't have currently time for this, but implementing this shouldn't be very complicated.
Test.zip (2,800 bytes)
On the contrary, I think these tests are confusing because when the user is zooming around some area he is not expecting that the zoomed area is different from that he selected with the drag tool. I don't see much use in limiting the smallest zoomed range just for the reason to see some part of the series left. When the user zooms in deeper and deeper and sees the curve disappearing why should he be confused, even when he is inexperienced?
It may occur that two data points are very close together and the user must zoom deeply to separate them - but he is out of luck when the programmer has set up a too-high minimum extent size limit.
Setting up a minimum extent size limit only makes sense to protect to user to zoom in too much so that the program reaches an instable state, for example in the MandelBrot tutorial when the magnification has become so large to produce a floating point overflow - this may take some time though, and I wonder if someone ever ran into this problem. But ok, maybe there are better examples.
So, I am back at the point: Is this a relevant patch?
|2019-05-25 22:01||Marcin Wiazowski||New Issue|
|2019-05-25 22:01||Marcin Wiazowski||File Added: extent_limit_test_mod3.zip|
|2019-05-25 22:01||Marcin Wiazowski||File Added: patch.diff|
|2019-05-26 15:11||wp||Assigned To||=> wp|
|2019-05-26 15:11||wp||Status||new => assigned|
|2019-06-05 12:52||wp||Status||assigned => feedback|
|2019-06-05 12:52||wp||LazTarget||=> -|
|2019-06-05 12:54||wp||Note Added: 0116574|
|2019-06-10 22:46||Marcin Wiazowski||File Added: Test.zip|
|2019-06-10 22:46||Marcin Wiazowski||Note Added: 0116670|
|2019-06-10 22:46||Marcin Wiazowski||Status||feedback => assigned|
|2019-06-14 14:37||wp||Relationship added||related to 0035344|
|2019-06-15 00:03||wp||Note Added: 0116725|
|2019-06-15 00:03||wp||Status||assigned => feedback|