SVG backend doesn't fill loops if they occur in the same subtree as a line

[jeffreyrosenbluth]

This goes with its companion branch of SVG
Please have a careful look before merging and only merge with fill/SVG

[byorgey]

This makes me a bit uneasy, though I'm willing to be convinced. Are we sure that a transparent fill is going to be the same as no fill for all backends, in all situations? This is also going to result in a bunch of extra SVG nodes specifying a transparent fill even when the user did not set any fill colors anyway.

[byorgey]

The alternative I was thinking about was to add an RTree -> RTree pass during compilation which splits and removes fill attributes as appropriate. However, Jeff correctly points out that given the way we currently have things organized, this would mean that the compilation code in diagrams-core would have to depend on attributes defined in diagrams-lib. We would either have to move some attributes into diagrams-core (seems wrong), or move the compilation code into diagrams-lib --- which actually might be the right thing to do, I am not sure.

[jeffreyrosenbluth]

I'm assuming (perhaps incorrectly) that it's only SVG amongst our current
backends that fills open paths. I'm basing this on the fact the the others
all render the backend-tests correctly. If this is correct we only have to
worry about whether transparent fill == no fill for backends we do not yet
have, also it seems a shame to have to reorganize and add complexity to
our code. Unless of course this is the right thing to do irrespective of
this problem.

I think the extra SVG nodes could be removed in an optimization pass on the
RTree so I'm not too worried about this one, since we plan on optimizing
the RTree anyway.

Lastly, somehow I feel that having to add an RTree -> RTree pass for
reasons other than optimization, may signal a defect in design, my
intuition is that the issue should be handled earlier in the process. This
is just my own opinion - I have no formal way to back it up.

On Wed, Nov 27, 2013 at 2:42 PM, Brent Yorgey notifications@github.comwrote:

In src/Diagrams/TwoD/Path.hs:

@@ -171,9 +174,9 @@ instance Renderable (Path R2) b => TrailLike (QDiagram b R2 Any) where
-- ... }@ syntax may be used.
stroke' :: (Renderable (Path R2) b, IsName a) => StrokeOpts a -> Path R2 -> Diagram b R2
stroke' opts path

• | null (pLines ^. unwrapped) = mkP pLoops
• | null (pLoops ^. unwrapped) = mkP pLines
• | otherwise = mkP pLines <> mkP pLoops
• | null (pLines ^. unwrapped) = mkP pLoops
• | null (pLoops ^. unwrapped) = fcA transparent $ mkP pLines

The alternative I was thinking about was to add an RTree -> RTree pass
during compilation which splits and removes fill attributes as appropriate.
However, Jeff correctly points out that given the way we currently have
things organized, this would mean that the compilation code diagrams-corewould have to depend on attributes defined in
diagrams-lib. We would either have to move some attributes into
diagrams-core (seems wrong), or move the compilation code into
diagrams-lib --- which actually might be the right thing to do, I am not
sure.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/141/files#r7967673
.

[jeffreyrosenbluth]

Perhaps a compromise is to add a NoColor attribute value for FillColor, something like

data SomeColor = forall c. Color c => SomeColor c | NoColor

and use this instead of transparent. Then the SVG backend could handle NoColor with fcA transparent and other backends would do whatever they need to, which for existing backends is nothing. I would have to put some more thought into whether this would work.

[byorgey]

Re: assuming that only SVG fills open paths: that's true, but it's something of an implementation detail/accident. Reorganization of other backends might produce the same problem (in particular, postscript has not been ported to the backend tree stuff yet). And in any case, even so it's not true that we only have to worry about what transparent fill means for backends we don't yet have, since (1) we certainly have to worry about what it means for SVG, and (2) actually we have to worry about it for all backends, since if we are going to be adding transparent fill attributes in diagrams-lib then all backends are going to see and process them.

"I think the extra SVG nodes could be removed in an optimization pass on the RTree" -- but that would again require moving the compilation/optimization stuff into diagrams-lib.

Re: adding RTree passes for reasons other than optimization, I don't feel the same as you. The point of the DTree and RTree stuff is not just to optimize, but more generally to do whatever centralized hard work we can in order to make life easier for backends. We take diagrams and produce simpler RTrees with some invariant guarantees that backends can take advantage of in order to make their job easier. One such guarantee could be, for example, that fill color attributes have no lines in the subtree underneath them.

[byorgey]

Re: adding NoColor, I thought of that, but I don't think it's a very good solution. If the SVG backend implements it by fcA transparent then we still have to worry about whether this is the same as no fill (it seems like it should be, of course, but I still don't know this for certain); and if other backends can't do that, then they might have to do some whole-tree analysis to figure out what to do---exactly what we are trying to avoid.

[jeffreyrosenbluth]

I'm don't love either of the two proposed solutions so perhaps we should keep thinking about it.

[byorgey]

Sure. As I mentioned earlier, I think users are very unlikely to run into this often in practice (and it's very easy to work around if they do), so there's no particular rush to fix this. It's definitely worth sitting on it for a while until we have something we're all happy with.