Markers: refactor by swharden · Pull Request #1688 · ScottPlot/ScottPlot

swharden · 2022-02-19T04:53:43Z

Currently MarkerShape is an enum and DrawMarker() uses a nasty switch to draw each marker.

Eventually (either here or ScottPlot 5) I want Marker objects to be classes that inherit IMarker and can Draw() themselves. This will make the code easier to maintain, make it easier to add new markers, and allow users to add their own custom markers too.

This PR will start by breaking apart the RenderMarker() method into smaller static methods, and explore options for moving toward a class-based marker system that minimizes breaking changes. Drawing routines will be optimized where appropriate. Resolves #1668

swharden · 2022-02-19T05:24:40Z

@BambOoxX you might get a kick out of seeing where this is now.

I'm going to try some deeper refactoring tomorrow, but now your core suggestion from #1660 of discretely drawing lines instead of placing text has been implemented 🚀

before	after

I reduced the size where necessary to make sure all the markers are the same size

BambOoxX · 2022-02-19T07:48:02Z

@swharden Very nice! The only one I would modify is the asterisk. I believe reducing the length of the diagonal lines by a sqrt(2) factor would make the marker rounder.

BambOoxX · 2022-02-19T12:21:56Z

I think it would be good that hollow / thin markers inherit the line width by default, in order to maintain consistency.
See what happens if you (ridiculously) increase the line width.

I will try to add that and also the two marker types proposed in #1660 initially

…TriangleDownPoints` - Replaced `(float)x` with `xf` syntax

…enTriangleDown` markers and associated methods

BambOoxX · 2022-02-19T13:38:43Z

Here a the new markers from #1689

BambOoxX · 2022-02-19T13:55:41Z

Regarding the DrawMarkers method, my idea was focused at removing all the brush / pen creation from the loops as advised in
https://stackoverflow.com/a/1961966/9576551

Marker

swharden · 2022-02-19T15:29:32Z

my idea was focused at removing all the brush / pen creation from the loops as advised...

This is a good point indeed! I'll keep refactoring the Marker system today and favor brush/pen re-use where possible.

...but t be honest I'm not terribly motivated to micro-optimize GDI rendering at this time since my biggest active goal right now is cutting GDI out of this library entirely (#1036). My coding goal for now is aimed at making rendering methods as easy to port to another rendering system as possible.

BambOoxX · 2022-02-19T16:29:55Z

@swharden I see you are moving a lot of stuff around. I'll let you finish this then open a new PR with the following modifications (that I implemented at the moment) unless you don't think they are relevant.

Add a MarkerLineWidth property to IHasMarker (or the equivalent stuff handling markers), and correct the plottables implementing IHasMarker.
Improve DrawMarker and subsequent methods so that they take the MarkerLineWidth into account to plot the makers
improve LegendItem so that there is no overflow of markers or linewidths (currently this is handled by isRectangle but that leads to not plotting a maker ever if there is one for large LineWidth values)
Make MarkerSize and MarkerLineWidth depend on the LineWidth value in order to keep consistency (by default) between those values

...to support Visual Studio 2019 which is still used by the GitHub Actions build pipeline

swharden · 2022-02-19T19:42:29Z

This landed in a weird spot. All marker logic is in struct objects now which is a step in the right direction, but it wasn't easy to swap-out MarkerShape from an enum to a struct or class because lots of ScottPlot code uses MarkerShape arguments with default values (something you can only do with enums) so if I made the change it would break a lot of existing code... so I had to stop just short of my original goal.

The current solution is a little wonky but it works, doesn't break existing code, and these classes are ready to pick-up in ScottPlot 5 where breaking changes will be permitted.

ScottPlot/src/ScottPlot/Marker/Marker.cs

Lines 32 to 56 in d8af928

    
           public static IMarker Create(MarkerShape shape) 
        
           { 
        
               return shape switch 
        
               { 
        
                   MarkerShape.none => new MarkerShapes.None(), 
        
                   MarkerShape.filledCircle => new MarkerShapes.FilledCircle(), 
        
                   MarkerShape.filledSquare => new MarkerShapes.FilledSquare(), 
        
                   MarkerShape.openCircle => new MarkerShapes.OpenCircle(), 
        
                   MarkerShape.openSquare => new MarkerShapes.OpenSquare(), 
        
                   MarkerShape.filledDiamond => new MarkerShapes.FilledDiamond(), 
        
                   MarkerShape.openDiamond => new MarkerShapes.OpenDiamond(), 
        
                   MarkerShape.asterisk => new MarkerShapes.Asterisk(), 
        
                   MarkerShape.hashTag => new MarkerShapes.Hashtag(), 
        
                   MarkerShape.cross => new MarkerShapes.Cross(), 
        
                   MarkerShape.eks => new MarkerShapes.Eks(), 
        
                   MarkerShape.verticalBar => new MarkerShapes.VerticalBar(), 
        
                   MarkerShape.triUp => new MarkerShapes.TriStarUp(), 
        
                   MarkerShape.triDown => new MarkerShapes.TriStarDown(), 
        
                   MarkerShape.filledTriangleUp => new MarkerShapes.FilledTriangleUp(), 
        
                   MarkerShape.filledTriangleDown => new MarkerShapes.FilledTriangleDown(), 
        
                   MarkerShape.openTriangleUp => new MarkerShapes.OpenTriangleUp(), 
        
                   MarkerShape.openTriangleDown => new MarkerShapes.OpenTriangleDown(), 
        
                   _ => throw new NotImplementedException($"unsupported {shape.GetType()}: {shape}"), 
        
               }; 
        
           }

ScottPlot/src/ScottPlot/Marker/MarkerTools.cs

Lines 10 to 27 in d8af928

    
                   public static void DrawMarker(Graphics gfx, PointF pixelLocation, MarkerShape shape, float size, Brush brush, Pen pen) 
        
                   { 
        
                       if (size == 0 || shape == MarkerShape.none) 
        
                           return; 
        
                       float diameter = size; 
        
                       float radius = diameter / 2; 
        
                       /* Improve marker vs. line alignment on Linux and MacOS 
        
                        * https://github.com/ScottPlot/ScottPlot/issues/340 
        
                        * https://github.com/ScottPlot/ScottPlot/pull/1660 
        
                        */ 
        
                       if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) 
        
                           pixelLocation = new PointF(pixelLocation.X + .5f, pixelLocation.Y); 
        
                       IMarker marker = Marker.Create(shape); 
        
                       marker.Draw(gfx, pixelLocation, radius, brush, pen); 
        
                   }

I see you are moving a lot of stuff around. I'll let you finish this then open a new PR with the following modifications (that I implemented at the moment) unless you don't think they are relevant.

@BambOoxX I'm going to merge this now, and if you'd still like to make these changes given the new class structure you're welcome to in a new PR 👍 🚀

BambOoxX · 2022-02-19T20:08:02Z

I'm going to merge this now, and if you'd still like to make these changes given the new class structure you're welcome to in a new PR 👍 🚀

All right it's on !

swharden added 4 commits February 18, 2022 23:50

Markers: refactor DrawMarker()

36f6f7e

Markers: draw shapes using lines instead of strings

575527d

Scatter Plot: distinct LineColor and MarkerColor values

a7e042e

Legend: respect distinct line and marker colors

b70abba

BambOoxX added 3 commits February 19, 2022 14:28

Factored coordinates handling for triangles in TriangleUpPoints / `…

3fc4119

…TriangleDownPoints` - Replaced `(float)x` with `xf` syntax

Added FilledTriangleUp, FilledTriangleDown, OpenTriangleUp, `Op…

84f750e

…enTriangleDown` markers and associated methods

Modified DrawAsterisk method to maker marker rounder

142ce5a

Merge pull request #1689 from BambOoxX/marker

d956b76

Marker

swharden added 4 commits February 19, 2022 10:47

Marker: move source files

d99c19d

MarkerShapes: create a class for each marker

7477ea2

MarkerShapes: Eks

6cd461b

MarkerShapes: None

ca5ba3c

swharden added 5 commits February 19, 2022 11:50

Formatting: remove file-scoped namespaces

4a47fe8

...to support Visual Studio 2019 which is still used by the GitHub Actions build pipeline

Marker: remove obsolete class names

5fa9b95

Markers: refactor so Markers Draw() themselves

2c9c836

Marker: recycle Brush and Pen

0507158

MarkerShape: class -> struct

d8af928

swharden merged commit 9d0b936 into master Feb 19, 2022

swharden deleted the marker branch February 19, 2022 19:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Markers: refactor#1688

Markers: refactor#1688
swharden merged 17 commits intomasterfrom
marker

swharden commented Feb 19, 2022

Uh oh!

swharden commented Feb 19, 2022

Uh oh!

BambOoxX commented Feb 19, 2022

Uh oh!

BambOoxX commented Feb 19, 2022 •

edited

Loading

Uh oh!

BambOoxX commented Feb 19, 2022

Uh oh!

BambOoxX commented Feb 19, 2022

Uh oh!

swharden commented Feb 19, 2022

Uh oh!

BambOoxX commented Feb 19, 2022

Uh oh!

swharden commented Feb 19, 2022 •

edited

Loading

Uh oh!

BambOoxX commented Feb 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

swharden commented Feb 19, 2022

Uh oh!

swharden commented Feb 19, 2022

Uh oh!

BambOoxX commented Feb 19, 2022

Uh oh!

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

Uh oh!

Uh oh!

BambOoxX commented Feb 19, 2022

Uh oh!

BambOoxX commented Feb 19, 2022

Uh oh!

swharden commented Feb 19, 2022

Uh oh!

BambOoxX commented Feb 19, 2022

Uh oh!

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

Uh oh!

Uh oh!

BambOoxX commented Feb 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

BambOoxX commented Feb 19, 2022 •

edited

Loading

swharden commented Feb 19, 2022 •

edited

Loading