Allow solid colored glyphs have a different colored outline

[zaddok]

Here is an initial go at a patch that allows solid coloured Glyphs to have an outline.

A keen observer may note that an update to allow a PyramidGlyph to be solid coloured with an outline, is almost the same as if we had allowed TriangleGlyph to have a fill. (ditto for Box/Square, and Circle,Round).

There is probably a case for arguing that we don't need PyramidGlyph and TriangleGlyph as they are both the same except with a different fill or stroke colour, but I don't want such arguments to hold up a useful patch :D Can we patch first and argue how to present the API later?

Where in the past we may have done:

     p, err := plot.New()
     ....
     lpPoints.Shape = draw.CircleGlyph{}
     lpPoints.Color = color.RGBA{R: 255, G: 200, B: 200, A: 255}
     p.Add(lpLine, lpPoints)

We can now do:

     p, err := plot.New()
     ....
     lpPoints.Shape = draw.CircleGlyph{}
     lpPoints.Color = color.RGBA{R: 255, G: 200, B: 200, A: 255}
     lpPoints.Width = vg.Points(1.0)
     lpPoints.LineColor = lpLine.Color
     p.Add(lpLine, lpPoints)

[kortschak]

How about using a LineStyle here? Comment would be:

// LineStyle is the style of the glyph outline.
// If LineStyle.Width is zero no outline is drawn.

[zaddok]

Great suggestion, I would like that idea, except for the fact that LineStyle also includes a Dashes and a DashOffs field—they would be effectively unused. I'm happy to switch the patch over to use LineStyle if that is what people prefer.

PS: Yikes, I need to fix the wording of that comment as well.

[kortschak]

There's no reason not to allow dashed outlines for glyphs.

[kortschak]

I'm not sure about this. I'm inclined to think the place for this kind of configuration is in the GlyphDrawer implementations rather than in a style.

[zaddok]

Shouldmt the object that holds the full color also hold the line/stroke color?

Perhaps I misunderstand? Are you saying the fill color should go in the GlyphStyle object and the line/stroke color should go in the GlyphDrawer object?

[kortschak]

What I'm saying is that you could write an OutlineCircle implementation of GlyphDrawer that has all the relevant style information in the type.

[zaddok]

Aaah, ok so your suggesting we create either:

1. OutlineCircleGlyph, OutlineBoxGlyph and a OutlinePyramidGlyph or
2. FilledRoundGlyph, FilledSquareGlyph and a FilledTriangleGlyph

That I could also do, that would be more following the spirit of the existing code. Would that get the pull request approved

If we go down that route, someone could argue that OutlineCircleGlyph, CircleGlyph and RoundGlyph are technically all just circles with different style attributes.

[kortschak]

The argument could be made that there is an obvious redundancy if the glyphs are configurable. The issue is that they just work. Functions of similar names that return appropriately configured GlyphDrawers would probably be satisfactory though.

All up, this is more complicated than it first looks. I'd like to wait until @eaburns has a chance to comment.

[zaddok]

In summary the choice is between

1. This patch, Make CircleGlyph, BoxGlyph, PyramidGlyph configurable
2. Add new types of glyphs for each shape, i.e.
   • OutlineCircleGlyph, CircleGlyph, and RoundGlyph
   • OutlineBoxGlyph, BoxGlyph, and SquareGlyph
   • OutlinePyramidGlyph, PyramidGlyph, and TriangleGlyph
3. Merge CircleGlyph and RoundGlyph into a single object where you can specify both stroke and fill.

[eaburns]

I haven't had a chance to look at the code yet, but from what I've read here, I prefer option 3.

[zaddok]

Great. In the morning I'll fgo ahead and create another patch that it implements option three.

Do we want to merge CircleGlyph into RoundGlyph, or visa versa? (Same goes for square/box and Triangle/Pyramid)

[eaburns]

Any news on this? We are trying to clean up our PRs. It seems like a nice simple change, but still has some outstanding comments. Do you plan to address them? If not, we plan to close this soon.

[eaburns]

I am closing this, because we are cleaning up inactive pull requests. See https://groups.google.com/d/topic/gonum-dev/Dr8t8WdvB_4/discussion for more details.