Skip to content

Refactor Signals#585

Merged
swharden merged 18 commits intoScottPlot:masterfrom
StendProg:SignalsConstructorSimplify
Oct 22, 2020
Merged

Refactor Signals#585
swharden merged 18 commits intoScottPlot:masterfrom
StendProg:SignalsConstructorSimplify

Conversation

@StendProg
Copy link
Contributor

Work In Progress.

Purpose:
Part of #578

New Functionality:
In progress.

@StendProg
Copy link
Contributor Author

StendProg commented Oct 7, 2020

Hi @swharden,
This is very early stage, so do not pay attention if you feel sorry for the time.

I decided to try to do it through Properties and Promises first. I'll see what happens.
In any case, it will not be difficult to return to public fields and validation, I do not disclaim responsibility.

The main meaning of such an early PR, a lot of small questions that have arisen and will arise, I will write them here. Access to Azure also will be usefull.

Here is some questions:

  1. Color? fillColor1 and Color? fillColor2 allows setting null, but at the same time there is a check (associated with the fill type ) for null with exception. It might make sense to just set the default color for this property Color fillColor1 = Color.Green.
  2. penLD and penHD both created in constructor and got same color. At the same time, there is a Fill demo, which changes the PenHD directly. Is it possible to discard these properties and create pens in Render loop based on one Color, or will it be necessary to enter two different colors ColorLD and ColorHD?

@swharden
Copy link
Member

swharden commented Oct 7, 2020

Hi @StendProg, this is very interesting work! Here are my thoughts based on the modified files and your comments.

Nullable Colors

Presently these colors are nullable

https://github.com/swharden/ScottPlot/blob/0fb684146dceb2c4963c9b2289ba9e8494d160df/src/ScottPlot/plottables/PlottableSignalBase.cs#L43-L46

It might make sense to just set the default color for this property Color fillColor1 = Color.Green.

I agree! Nullable value types can be confusing and introduce errors, and it would be nice to eliminate them if possible.

I notice that the null logic for gradientFillColor1 and gradientFillColor2 is actually used though, so it may be a good idea to let those stay nullable.

Exposed System.Drawing Objects

penLD and penHD both created in constructor and got same color. At the same time, there is a Fill demo, which changes the PenHD directly. Is it possible to discard these properties and create pens in Render loop based on one Color, or will it be necessary to enter two different colors ColorLD and ColorHD?

Currently pens are created in the constructor and stored at the class level as public fields:

https://github.com/swharden/ScottPlot/blob/0fb684146dceb2c4963c9b2289ba9e8494d160df/src/ScottPlot/plottables/PlottableSignalBase.cs#L77-L78

I think the best way to proceed is to ensure that System.Drawing objects that implement IDisposable (like pens and brushes) are all created in the render loop with using statements. Probably it would be best to delete the penLD and penHD fields and switch to this new design. I know this is a small breaking API change, but I think it is a good change. The demo can set color, lineWidth, and lineStyle to customize the line.

Capitalization

When I first created ScottPlot I made all public fields lowercase because that is the naming convention of Python (the language I was most familiar with at the time). I now recognize the C# Capitalization Conventions says public fields should be capitalized.

I can see you've capitalized public fields this with this PR. This change will modify a large number of files (especially if it is done for every plottable), so it may be a good idea to keep existing public fields with the same name for now (simplifying this PR), and we can make all fields capitalized in 4.1 at the same time we make lots of other breaking changes.

Thank you @StendProg for your excellent work on this!

@StendProg
Copy link
Contributor Author

This test not culture invariant and always fail for me on local computer.
https://github.com/swharden/ScottPlot/blob/02fb5530798543cdc54d7dfa671c52b2f78ab054/tests/Axis/AxisLimits.cs#L34-L41

Expected result is x1=11.000, x2=22.000, y1=33.000, y2=44.000 but actual x1=11,000, x2=22,000, y1=33,000, y2=44,000

@StendProg
Copy link
Contributor Author

StendProg commented Oct 8, 2020

SignalBase class need access to settings.axes.x.min, settings.axes.x.span, settings.dataSize.Width, settings.dataSize.Height.
PlottableDimensions provide only GetPixel() methods.

@swharden

This comment has been minimized.

swharden added a commit that referenced this pull request Oct 8, 2020
@swharden
Copy link
Member

swharden commented Oct 8, 2020

Following-up, I too had this need while I was refactoring other plottables (#578) so I exposed those fields in 31658bd and updated the master branch. This should make it easier for you to refactor the Signal plots 👍

https://github.com/swharden/ScottPlot/blob/727b24550461ce445273bd19f9f34089f9cbbbf2/src/ScottPlot/Drawing/PlotDimensions.cs#L19-L38

@swharden swharden mentioned this pull request Oct 8, 2020
38 tasks
@StendProg StendProg force-pushed the SignalsConstructorSimplify branch from 966fce1 to 61e1d21 Compare October 9, 2020 15:23
@StendProg
Copy link
Contributor Author

StendProg commented Oct 9, 2020

What parameters should the new signal constructor contain?

  1. PlottableSignal(double[] ys)
  2. PlottableSignal() - it easy make empty with correct handling ys property.

I chose the second option, everything works fine.
It won't be difficult to return to the first option if necessary.

@swharden
Copy link
Member

Hi @StendProg, thank you for your hard work on this! I know from reviewing this PR that the new rendering system took a lot of time and thought to implement.

There are some improvements I still wish to make, but I'm going to merge this in now so I can improve the validation system for all plottables at the same time. I'll discuss these topics in #578

@swharden swharden merged commit c17aca4 into ScottPlot:master Oct 22, 2020
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.

2 participants