SignalXY: fix exception thrown when data runs outside the view window by RFBomb · Pull Request #4286 · ScottPlot/ScottPlot

RFBomb · 2024-09-23T14:55:13Z

Resolves #4261 by adding a validation check to check results of GetFirstPoint and GetLastPoint.

For some reason, I was only able to reproduce this bug when a plot had both a scatter and a signalXY.

When zooming into the plot, and the signalXY was outside of the zoomed area, the application would crash because the first index was greater than the last index when attempting to draw the SignalXY (which had nothing to draw because it was outside the viewing window)

Code to reproduce ( I pasted this into the SignalXY tester in the WPF demo and called it during constructor)

private void CreateTheProblem()
    {
        var scatterX = Generate.Consecutive(20);
        var scatterY = Generate.SquareWave(20, 2);

        var signalX = new double[5] { 16, 17, 18, 19, 20 };
        var signalY = new double[5] { -20, 1, 15, 15, 10 };

        var sig = WpfPlot1.Plot.Add.SignalXY(signalX, signalY, ScottPlot.Colors.Blue);
        sig.Axes.YAxis = WpfPlot1.Plot.Axes.Left;
        sig.Axes.XAxis = WpfPlot1.Plot.Axes.Bottom;

        var scat = WpfPlot1.Plot.Add.Scatter(scatterX, scatterY, ScottPlot.Colors.Red);
        scat.ConnectStyle = ConnectStyle.StepHorizontal;
        scat.Axes.YAxis = WpfPlot1.Plot.Axes.Right;
        scat.Axes.XAxis = WpfPlot1.Plot.Axes.Bottom;

        WpfPlot1.Refresh();
        // once view is loaded, simply zoom into an area of the plot that does not have any SignalXY drawn
    }

- Adds a validation check to check results of GetFirstPoint and GetLastPoint

RFBomb · 2024-09-25T22:31:53Z

DataLoggerSource may also have this bug present! As such, it may be wise to move the validation logic into a static utility class (such as the one I provided over in #4270

ScottPlot/src/ScottPlot5/ScottPlot5/DataSources/DataLoggerSource.cs

Line 75 in 743c942

IndexRange visibleRange = new(dataIndexFirst, dataIndexLast);

fix would be required for both horizontal and vertical functions

PTA00 · 2024-09-27T07:33:59Z

我也发现了这个bug，希望快点发布新版

Edit (Google Translate): "I also found this bug and hope to release a new version soon"

StendProg · 2024-09-27T18:22:13Z

ScottPlot/src/ScottPlot5/ScottPlot5/DataSources/SignalXYSourceDoubleArray.cs

Lines 100 to 102 in 743c942

    
           if (Xs[dataIndexFirst] > Xs[dataIndexLast]) 
        
               throw new InvalidDataException("Xs must contain only ascending values. " + 
        
                   $"The value at index {dataIndexFirst} ({Xs[dataIndexFirst]}) is greater than the value at index {dataIndexLast} ({Xs[dataIndexLast]})");

That's the whole problem with this check. It is really unnecessary and does not check anything completely. We already used binary search before it, and only after it we check that the array is sorted (which allows us to use binary search), and only by 2 points, which are relatively randomly selected depending on the Plot display settings. So we may realize that the array is not sorted, or we may not realize it, it depends on how lucky we are.

The further code is resistant to wrong index order and correctly returns an empty list, so you don't need to make an early return.

To summarize: you have added additional checks to prevent errors in the check, which is not really necessary (IMHO).

@swharden had some considerations when he added this check. But personally I'm in favor of just removing it.

RFBomb · 2024-09-27T18:35:59Z

This PR fixed two issues though:

it was crashing applications (including demo) if scatter and SignalXY exist on same plot and you zoom in where no signalXY is displayed. The crash was due to one of two scenarios:

incorrectly throwing an out of range exception (how is index 0 out of range when collection has 10 items?!). When it throws here, both first and last are equal to 0.
the functions to determine start/stop indexes were throwing the exception you want removed. Not because collection is out of order, but because the first was a larger index than the second somehow. I don't understand how though.

Either way, some validation needs to occur here to squash the bug, as I don't believe it has to do with the index search.

The bug is 100% reproduceable.

StendProg · 2024-09-27T19:33:57Z

Here is you reproduce code (but in Winforms)
I fixed only this:

var scatterY = Generate.SquareWave(/*20*/ 10, 2);

This PR:

WithValidateFix.mp4

This PR, but if (ValidateVisibleRange(dataIndexFirst, dataIndexLast) is false) return []; commented out:

WithoutValidateFix.mp4

It looks like your fix breaks the interpolation on the outer points due to the early return.

Not because collection is out of order, but because the first was a larger index than the second somehow. I don't understand how though.

If the start index is larger than the end index, it is an indicator that the number of points inside is empty. Further code handles this correctly. Moreover, it correctly handles external points and interpolation by them.
The accending check itself is incorrect, it does not take into account the fact that the start index may be larger.

incorrectly throwing an out of range exception (how is index 0 out of range when collection has 10 items?!). When it throws here, both first and last are equal to 0.

I have not been able to reproduce this.

RFBomb · 2024-09-27T22:10:47Z

I will retest and upload a video of it crashing, along with a demo in winforms and wpf that is reproducible. I was able to reproduce this on two separate computers with fresh pulls of the library.

You also zoom in on the signal area, that may be why yours is successful. I was zooming to the left of the start of the area, where the scatter exists but there are no values for the signalXY.

Also, if it weren't an issue I shouldn't have been hitting the exceptions screenshotted in the related issue

RFBomb · 2024-09-27T23:01:07Z

SignalXY.Zoom.Bug.Demo.2024-09-27.19-00-18.mp4

StendProg · 2024-09-28T08:32:24Z

A very handy example, thank you.

I'm not arguing that there is a bug that throws an exception in the original version.
I'm just saying that it's caused by incorrect ascending check.
If you comment out the check in the original version, everything will work fine.
If you comment out ValidateVisibleRange from this PR, which also contains this check, you will also get error-free results.

If we decide to keep this check, then your PR should be adjusted to bypass the accending check in case of “invalid” indexes, but still let them go further down the code. (the missing lines when zooming deep inside the interval are present in your PR example).
Although personally I am in favor of just removing this accending check.

incorrectly throwing an out of range exception (how is index 0 out of range when collection has 10 items?!). When it throws here, both first and last are equal to 0

I was able to reproduce the Exception from the first case as you describe. I don't know why it displays dataIndexLast == 0 in the debugger. In fact dataIndexLast == -1 if you catch that point before the Exception. (Which is also correct for the code that follows, but the check doesn't take this into account)

RFBomb · 2024-09-28T12:34:16Z

@StendProg I understand now, sorry for being abrasive/defensive there. Removing the exception does resolve the issue, and I was able to see after some additional testing that the lastIndex = -1 under that first scenario. (I did check for <0 in my validation)

I was also able to reproduce the interpolation issue you mentioned while my early escape was included.

I'm uploading a new commit that removes the check altogether. Additionally, if the visible range length is 0, the binary search will always return index 0, which results in GetColumnPixels returning an array of 0. The VisiblePoints value will never be populated while visibleRange.Length == 0, so I put in a check for that.

Finally, I suspected that DataLogger may have the same problem because it is so similar, so I tested it. DataLogger does not have the same issue, because (unsure why this occurs) it frames the plot and prevents adjustment of X/Y. (datalogger is green in video)

SignalXY.Zoom.Bug.Demo.2024-09-28.08-33-02.mp4

edit: It seems that validation exception is unit tested. So maybe it only checks for the exception when visibleRange.Length > 0

previously it would throw an exception if the signal xy plot was outside the data area

swharden · 2024-10-09T21:05:34Z

Hi @RFBomb and @StendProg, thanks for your feedback!

This issue/PR reveals that the ascending value checker could fail if SignalXY data ran outside the data area.

The original solution recommended by this PR was to remove the ascending value checker. Indeed, it's not a thorough check (because inspecting every X value in the entire dataset on every frame render would be to costly). While not completely thorough, it catches many cases. Without such a check, users who feed non-ascending X values into a SignalXY plot will have no negative feedback indicating there was a problem, and the plot may have confusing artifacts without a clear explanation. I found this from personal use when I accidentally used a SignalXY instead of a Scatter plot.

If there's a better way to check for ascending values that isn't a negative performance impact, I'm happy to implement such an improvement! To correct the short term issue, I added tests that replicated the bug and added a check to avoid it, while preserving the original exception for users who accidentally use SignalXY to plot non-ascending X values.

RFBomb · 2024-10-09T21:23:16Z

Yea, other than brute-forcing a check across the collection, which is really inefficient, I don't see a better check than the one that was originally implemented, or the one I had put in with the validation method initially (which was non-exhaustive).

But i like the IsValid property being added, it's an elegant solution to various issues.

RFBomb added 2 commits September 23, 2024 10:52

Adds a validation check to SignalXY data sources

76967dc

- Adds a validation check to check results of GetFirstPoint and GetLastPoint

Update SignalXYSourceGenericArray.cs

1683524

Proof

c9ab278

Remove Validation

e0a96c1

swharden added 3 commits October 9, 2024 16:44

Merge branch 'main' into pr/4286

c2654d4

SignalXY sources: add original ascending value check with failing tests

38921cb

SignalXY sources: fix ascending value check

6d4570a

previously it would throw an exception if the signal xy plot was outside the data area

swharden added 2 commits October 9, 2024 17:12

Test: additional SignalXY off-screen configurations

18d92d3

WinForms demo: remove SignalXY ZoomBug window

e6b7dd1

swharden changed the title ~~Adds a validation check to SignalXY data sources~~ SignalXY: fix exception thrown when data runs outside the view window Oct 9, 2024

swharden added 2 commits October 9, 2024 17:16

csproj: fix formatting

b2fe223

Update CHANGELOG.md

bb42186

swharden merged commit fe8c2c8 into ScottPlot:main Oct 9, 2024

StendProg mentioned this pull request Oct 10, 2024

signalXY move out of view throw an exception) #4314

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SignalXY: fix exception thrown when data runs outside the view window#4286

SignalXY: fix exception thrown when data runs outside the view window#4286
swharden merged 11 commits intoScottPlot:mainfrom
RFBomb:FixSignalXYZoomBug

RFBomb commented Sep 23, 2024 •

edited

Loading

Uh oh!

RFBomb commented Sep 25, 2024 •

edited

Loading

Uh oh!

PTA00 commented Sep 27, 2024 •

edited by swharden

Loading

Uh oh!

StendProg commented Sep 27, 2024

Uh oh!

RFBomb commented Sep 27, 2024 •

edited

Loading

Uh oh!

StendProg commented Sep 27, 2024

Uh oh!

RFBomb commented Sep 27, 2024 •

edited

Loading

Uh oh!

RFBomb commented Sep 27, 2024

Uh oh!

StendProg commented Sep 28, 2024 •

edited

Loading

Uh oh!

RFBomb commented Sep 28, 2024 •

edited

Loading

Uh oh!

swharden commented Oct 9, 2024

Uh oh!

RFBomb commented Oct 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

RFBomb commented Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RFBomb commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PTA00 commented Sep 27, 2024 • edited by swharden Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StendProg commented Sep 27, 2024

Uh oh!

RFBomb commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StendProg commented Sep 27, 2024

Uh oh!

RFBomb commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RFBomb commented Sep 27, 2024

Uh oh!

StendProg commented Sep 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RFBomb commented Sep 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swharden commented Oct 9, 2024

Uh oh!

RFBomb commented Oct 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RFBomb commented Sep 23, 2024 •

edited

Loading

RFBomb commented Sep 25, 2024 •

edited

Loading

PTA00 commented Sep 27, 2024 •

edited by swharden

Loading

RFBomb commented Sep 27, 2024 •

edited

Loading

RFBomb commented Sep 27, 2024 •

edited

Loading

StendProg commented Sep 28, 2024 •

edited

Loading

RFBomb commented Sep 28, 2024 •

edited

Loading