Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/Diagrams/TwoD/Path.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ import Data.AffineSpace
import Data.Default.Class
import Data.VectorSpace

import Data.Colour (transparent)

import Diagrams.Coordinates
import Diagrams.Core
import Diagrams.Located (Located, mapLoc, unLoc)
Expand All @@ -65,6 +67,7 @@ import Diagrams.Segment
import Diagrams.Solve
import Diagrams.Trail
import Diagrams.TrailLike
import Diagrams.Attributes (fcA)
import Diagrams.TwoD.Segment ()
import Diagrams.TwoD.Types
import Diagrams.Util (tau)
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

| otherwise = (fcA transparent $ mkP pLines) <> mkP pLoops
where
(pLines,pLoops) = partitionPath (isLine . unLoc) path
mkP p
Expand Down