Skip to content

SignalPlotXY: defer ascending X check to ValidateData and only validate between min/max render index#1777

Merged
swharden merged 7 commits intoScottPlot:mainfrom
BAUR-GmbH:1771-signalxy-only-check-x-values-between-min-and-max-render-index
Apr 30, 2022
Merged

SignalPlotXY: defer ascending X check to ValidateData and only validate between min/max render index#1777
swharden merged 7 commits intoScottPlot:mainfrom
BAUR-GmbH:1771-signalxy-only-check-x-values-between-min-and-max-render-index

Conversation

@bernhardbreuss
Copy link
Contributor

Hi @swharden,

I have tried to solve #1771 by validating X only between min/max render index.
Doing the check only between min/max render index in the setter didn't worked out very well:

  • min/max render index must be set before
  • before setting max render index Ys must be set
  • min/max render index would have been added to AddSignalXY which then already started to look like the obsolete PlotSignalXY

Therefore I decided to only use the already present check in ValidateData.

@swharden
Copy link
Member

Hi @bernhardbreuss, sorry it took so long for me to get to this!

This is a hard problem, but I think your solution is a good one. Unfortunately I can't edit contents of this pull request. Can you edit it to give me permissions to make changes, then I'll make some small updates and merge it in? Thanks! 🚀

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

@bernhardbreuss
Copy link
Contributor Author

Hi @swharden, I am afraid but the information in the provided link seems only to work for forks owned by a personal account. However I have added you as a collaborator. Hopefully this way you can also edit the PR branch.

@swharden
Copy link
Member

added you as a collaborator

That worked, thanks! I'll make a few changes then merge this in shortly. I recommend not committing new changes on your end while I'm actively working on it 👍

Thanks again for starting this PR! This will be a great improvement to the library 🚀

Following discussion in #1777 #1771, this strategy confirms Xs will always be checked for ascension at least once without having to manually call ValidateData(), but also allows time for the user to define min/max render indexes before validation occurs.
@swharden
Copy link
Member

I reached a good compromise in my last few commits.

  • There's now a private flag variable which indicates whether Xs have been validated or not.

  • Assigning Xs resets this flag, and the validation isn't performed until Render() is called.

  • This gives the user time to assign data and set min/max indexes before validation occurs.

  • This strategy also ensures Xs is always checked at least once before the first render, hopefully catching the common user of attempting to plot data with non-ascending X values.

@swharden swharden enabled auto-merge April 30, 2022 12:44
@swharden swharden merged commit 9e61c87 into ScottPlot:main Apr 30, 2022
@swharden swharden deleted the 1771-signalxy-only-check-x-values-between-min-and-max-render-index branch April 30, 2022 12:49
@bernhardbreuss
Copy link
Contributor Author

Hi @swharden, thank you for your hard work. ScottPlot is a great lib, easy and straight forward to use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SignalXY: Only check for ascending X values between min/max render index

2 participants