Plottable: IHasLine, IHasMarker, and IHlightable by BambOoxX · Pull Request #1660 · ScottPlot/ScottPlot

BambOoxX · 2022-02-13T15:20:16Z

This PR implements the interfaces discussed in #1655, which has been rebased on this branch (not sure if it was the right thing to do though).

Three new interfaces are created IHasLine, IHasMarker and IIsHighlightable.
A new marker plotting method DrawMarkers is proposed to support customizeable markers in SignalPlots, as well as new implementations for marker plotting avoiding DrawString when possible.
Two new MarkerShapes are implemented : openTriangle and filledTriangle

Hopefully this will have some use beyond just interactive plotting.

…ghtable` interfaces - Adatpation of the existing properties declarations - Added `IsHighlighted` and `HighlightCoefficient` properties to toggle higlight and set its weight on thicknesses - Modified `Render` to take highlight effect into account

…hlightable` interfaces - Adaptation of the existing properties declarations - Added `MarkerShape` property to authorize user to change signal Marker. - Added `IsHighlighted` and `HighlightCoefficient` properties to toggle higlight and set its weight on thicknesses - Modified `Render` to take highlight effect into account

…of `PointF` - Hopefully this allows to have customizeable markers in `SignalPlots` without much performance impact (to be verified, but first test do not seem to make much difference) - In `DrawMarkers` , `eks`, `cross`, `asterisk`, `verticalbar` are plotted with `DrawLine` instead of `DrawString`. A performance improvement is expected but is to be verified

…PlotBase`

BambOoxX · 2022-02-15T12:29:45Z

Here are some views of the new functionalities

new markers (legend items are still ploted with the old functionnality)

test.mp4

SignalPlots with changeable markers

test2.mp4

swharden · 2022-02-16T03:31:00Z

This PR implements the interfaces discussed in #1655, which has been rebased on this branch (not sure if it was the right thing to do though)

I'm not 100% on this either ... Inter-dependent PRs can get pretty complicated, especially if I decide to refactor the PR this one depends on.

My strategy will be to focus solely on #1660, refactor as necessary, merge it, then come back here, cross my fingers, and hold my breath and see what broke 😅 I recommend not making new commits on either of these PRs until I'm done working on them.

In the future, smallest-possible PRs are probably the solution to this 👍 E.g., refactoring the DrawMarkers() system, and also adding new marker types, each could have been their own PR (independent of adding interfaces to plottables).

You already did a lot of work (fantastic work!) here and in #1655, so I'll pick it up where it is and try to get it merged today or tomorrow 🤞

PS: In the next major version of ScottPlot (in a magical world with rainbows and unicorns, lol) each Marker would be a class and implement IMarker meaning it has a Draw() method. When each marker can draw itself, it's so much easier to add new markers, and there would be no switch statements! I look forward to this day 😄

Removes logical optimizations introduced in ScottPlot#1667 These optimizations will be added in a future issue/PR ScottPlot#1668

swharden · 2022-02-16T04:59:57Z

I ran out of time and have to put this down tonight 😝

To be honest I got super overwhelmed with the modifications to the shape rendering code, so I took all of that out of this PR and stored it in #1660 so we can review it later when I have more availability to review/test it.

I think this PR is in a better/simpler place now, but I haven't had time to adequately test it yet, and testing on Linux and MacOS will be extremely important since we are twiddling with marker alignment and it's been a problem in the past (#340) 👍

PS: Marker rendering will be significantly reworked in ScottPlot 5, so I'm not highly motivated to micro-optimize System.Drawing marker rendering routines at this time. My main goal is to avoid introducing new bugs or or changes to existing behavior.

Thanks again for your work on this @BambOoxX! Hopefully I can merge this tomorrow night 🚀

BambOoxX · 2022-02-16T12:01:06Z

My strategy will be to focus solely on #1660, refactor as necessary, merge it, then come back here, cross my fingers, and hold my breath and see what broke 😅 I recommend not making new commits on either of these PRs until I'm done working on them.

Not problem with me, I was just surprised that SignalPlots could not change markers. So I just went on with it... Maybe a bit too enthusiastic on this one 😅...

You already did a lot of work (fantastic work!) here and in #1655, so I'll pick it up where it is and try to get it merged today or tomorrow 🤞

Thanks, I'm just trying to help when I can. I saw an opportunity to try something...

PS: In the next major version of ScottPlot (in a magical world with rainbows and unicorns, lol) each Marker would be a class and implement IMarker meaning it has a Draw() method. When each marker can draw itself, it's so much easier to add new markers, and there would be no switch statements! I look forward to this day 😄

Wouldn't that be problematic ? Some plots may have different markers depending on the point in the dataset, but for ScatterPlot and SignalPlots it seems better to have a single marker that is used throughout the dataset, no ?

swharden · 2022-02-16T14:11:03Z

Thanks, I'm just trying to help when I can. I saw an opportunity to try something...

It's a good improvement! Lots of this work will translate to the next version of ScottPlot too 👍🚀

it seems better to have a single marker that is used throughout the dataset

This is true.... but perhaps I didn't describe my proposed change well here.

Overall the the new behavior will be the essentially the same.

I'll implement it soon in ScottPlot5 (e.g., /dev/ScottPlot5 and #1647), then as we practice using the new design we will get an idea of whether it feels most natural or not

Since I'll be working in Marker land in #1668, it may be a good time to practice lightly refactoring ScottPlot 4's Marker rendering system to see if I can make it more modular without any breaking changes. I'll probably prioritize this over ScottPlot 5 development, and hopefully it'll be a quick refactor! Famous last words 🙃

swharden · 2022-02-17T01:21:31Z

@BambOoxX are you familiar with Docker? I'm struggling to run the unit tests in Linux in local docker container
1a04aa8 😅 This would be helpful for confirming that marker rendering is accurate in Linux.

I can use WSL to run the tests locally for now, but a Docker solution would be much more elegant 🐋

swharden · 2022-02-17T04:26:49Z

I confirmed the pixel offset is still required when drawing circles on Linux 😝 #340

Looks like 0.5f offset is better aligned than the 1.0f used previously

Linux (no offset)	Linux (with offset)

ScottPlot/src/tests/Misc/OS.cs

Lines 12 to 30 in 21b11c9

    
           public void Test_Plot_Basic() 
        
           { 
        
               int pointCount = 51; 
        
               double[] x = ScottPlot.DataGen.Consecutive(pointCount); 
        
               double[] sin = ScottPlot.DataGen.Sin(pointCount); 
        
               double[] cos = ScottPlot.DataGen.Cos(pointCount); 
        
               var plt = new ScottPlot.Plot(600, 400); 
        
               plt.AddScatter(x, sin, label: "sin"); 
        
               plt.AddScatter(x, cos, label: "cos"); 
        
               plt.YLabel("vertical units"); 
        
               plt.XLabel("horizontal units"); 
        
               plt.Title(ScottPlot.Tools.GetOsName()); 
        
               plt.Legend(); 
        
               TestTools.SaveFig(plt, artifact: true); 
        
           }

Tested using WSL/ubuntu

Improves alignment between lines and markers This may because Linux/MacOS define pixel locations as the center of pixels vs. the top-left corner 🤷‍♀️

BambOoxX · 2022-02-17T07:40:38Z

@swharden No, I have no clue about docker images. These always feel like magic to me 😅

swharden · 2022-02-17T15:29:30Z

no clue about docker images

It was worth a shot!

@bclehmann your suggestion to run tests using WSL has been helpful. Do you have enough experience with Docker to know why the tests won't run in the dotnet SDK Docker image? (#1677)

bclehmann · 2022-02-17T15:59:27Z

Do you have enough experience with Docker to know why the tests won't run in the dotnet SDK Docker image? (#1677)

The only things I can think of is either a native dependency isn't installed (i.e. libgdiplus) or if the dockerfile has conflicting dependency version requirements.

BambOoxX added 9 commits February 13, 2022 13:01

Implement IHasLine interface

d270fe9

Implement IHasMarker interface

63fabd0

Implement IIsHighlightable interface

2c4f796

Replaced DrawMarker with DrawMarkers in ScatterPlot and `Signal…

afa6ba1

…PlotBase`

Fix wrong location of highlight coefficient application

0a0901e

Applied code formatting

e556808

swharden added 2 commits February 15, 2022 20:12

Merge branch 'master' into add-plottable-interfaces

3960c07

Merge branch 'master' into add-plottable-interfaces

99017ea

swharden mentioned this pull request Feb 16, 2022

Tests: image hash tests for cookbook recipes #1666

Closed

Merge branch 'master' into add-plottable-interfaces

6376f16

swharden mentioned this pull request Feb 16, 2022

Markers: Render without using DrawString() #1668

Closed

swharden added 7 commits February 15, 2022 23:20

Marker: DrawMarkers() iteratively calls DrawMarker()

cc739ab

Removes logical optimizations introduced in ScottPlot#1667 These optimizations will be added in a future issue/PR ScottPlot#1668

Marker: remove unused usings

520e4a6

Plottable: IIsHighlightable -> IHighlightable

3aaefdc

Scatter: use properties for highlight scaling

d712762

Signal Plot Base: use properties for highlight scaling

756fdd1

Signal Plot: use original marker rendering code

5e0ca64

Signal Plot: Add support for custom markers

ceb1673

Scatter and Signal Plot: inline properties

f0b8b5e

Tests: add failed docker notes

1a04aa8

swharden added 12 commits February 16, 2022 23:31

Marker: half-pixel offset on non-Windows systems

9abc67d

Improves alignment between lines and markers This may because Linux/MacOS define pixel locations as the center of pixels vs. the top-left corner 🤷‍♀️

Update changelog.md

ce4fb45

IHasLine: Color -> LineColor

f032eeb

IHasMarker: add MarkerColor property

51e107e

Arrow: implement IHasLine

5aac55e

AxisLine: implement IHasLine

28bac10

Crosshair: implement IHasLine

4b62001

AxisLineVector: implement IHasLine

da90b63

Crosshair: fully implement IHasLine

728d2b6

Marker: implement IHasMarker

414d62e

Errorbar: implement IHasMarker and IHasLine

b6117eb

FunctionPlot: implement IHasLine

ff945a0

swharden changed the title ~~Add plottable interfaces~~ Plottable: IHasLine, IHasMarker, and IHlightable Feb 17, 2022

swharden enabled auto-merge February 17, 2022 05:03

swharden merged commit 8c81330 into ScottPlot:master Feb 17, 2022

swharden mentioned this pull request Feb 17, 2022

Plottable: IHasColor #1678

Closed

BambOoxX deleted the add-plottable-interfaces branch February 17, 2022 16:45

swharden mentioned this pull request Feb 19, 2022

Markers: refactor #1688

Merged

BambOoxX mentioned this pull request Feb 19, 2022

Marker #1689

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plottable: IHasLine, IHasMarker, and IHlightable#1660

Plottable: IHasLine, IHasMarker, and IHlightable#1660
swharden merged 33 commits intoScottPlot:masterfrom
BambOoxX:add-plottable-interfaces

BambOoxX commented Feb 13, 2022 •

edited

Loading

Uh oh!

BambOoxX commented Feb 15, 2022 •

edited

Loading

Uh oh!

swharden commented Feb 16, 2022 •

edited

Loading

Uh oh!

swharden commented Feb 16, 2022

Uh oh!

BambOoxX commented Feb 16, 2022

Uh oh!

swharden commented Feb 16, 2022

Uh oh!

swharden commented Feb 17, 2022

Uh oh!

swharden commented Feb 17, 2022

Uh oh!

BambOoxX commented Feb 17, 2022

Uh oh!

swharden commented Feb 17, 2022

Uh oh!

bclehmann commented Feb 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

BambOoxX commented Feb 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BambOoxX commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swharden commented Feb 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swharden commented Feb 16, 2022

Uh oh!

BambOoxX commented Feb 16, 2022

Uh oh!

swharden commented Feb 16, 2022

Uh oh!

swharden commented Feb 17, 2022

Uh oh!

swharden commented Feb 17, 2022

Uh oh!

BambOoxX commented Feb 17, 2022

Uh oh!

swharden commented Feb 17, 2022

Uh oh!

bclehmann commented Feb 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

BambOoxX commented Feb 13, 2022 •

edited

Loading

BambOoxX commented Feb 15, 2022 •

edited

Loading

swharden commented Feb 16, 2022 •

edited

Loading