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.

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

Fixed bug that miter line joins are cut of early and one with stacked clippings. #37

Closed
wants to merge 5 commits into from

Conversation

jbracker
Copy link
Member

@jbracker jbracker commented Sep 2, 2013

Fixed bug that miter line joins are cut of early by explicitly setting a high miter limit.

I noticed that the miter limit was left at its default value by the SVG backend while looking at the Charts drawing test for lines. I don't know if a even higher value would be good here or an actual miter limit attribute for Diagrams, but this value works for my use cases.

I also fixed a issue with stacked clippings. Before this several stacked clips were just printed as several paths in the clipPath element. But according to the SVG spec this means they are OR'ed together, although we want them to be intersected. I changed this by building them up around the clipped element hierarchical, which leads to intersection according to the SVG spec. I tested this in the chart drawing tests and it seems to work.

Jan Bracker added 2 commits September 2, 2013 13:12
Before this several stacked clips were just printed as several paths in the clipPath element.
But according to the SVG standard this means they are OR'ed together, although we want them to be intersected.
I changed this by building them up around the clipped element hierarchical, which leads to intersection according to the SVG spec.
I tested this in the chart drawing tests and it seems to work.
@jbracker
Copy link
Member Author

jbracker commented Sep 2, 2013

In my perspective: If you can't change it manually try to emulate as if there was none. So maybe it would be useful to add a diagrams attribute for this.

@byorgey
Copy link
Member

byorgey commented Sep 2, 2013

See diagrams/diagrams-lib@7be5cdf . Should we also change setDefault2DAttributes (https://github.com/diagrams/diagrams-lib/blob/master/src/Diagrams/TwoD/Adjust.hs#L49) to explicitly set a default miter limit?

@jbracker
Copy link
Member Author

jbracker commented Sep 2, 2013

I think it would be good to change setDefault2DAttributes too.

The given miter limit attribute misses semantics. Can someone write them done properly?

@byorgey
Copy link
Member

byorgey commented Sep 2, 2013

Postscript and cairo both use a default miter limit of 10:

I can't find anything officially specifying the default miter limit used by SVG. To keep things simple we should probably just specify 10 as the default miter limit for diagrams too.

@jbracker
Copy link
Member Author

jbracker commented Sep 2, 2013

The default for SVG is 4.

Spec: miter limit = 1/sin(angle/2)
This seems to be resonable because SVG and Cairo use the same definition.

@@ -193,7 +198,7 @@ instance Backend SVG R2 where
-- primitives before passing them to render.
renderDia SVG opts d =
doRender SVG opts' . mconcat . map renderOne . prims $ d'
where (opts', d') = adjustDia SVG opts d
where (opts', d') = adjustDia SVG opts $ setDefault2DAttributes d
Copy link
Member

Choose a reason for hiding this comment

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

adjustDia already calls setDefault2DAttributes (via adjustDia2D) so this is calling setDefault2DAttributes twice.

Copy link
Member Author

Choose a reason for hiding this comment

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

My bad I will remove that again then.

@jbracker
Copy link
Member Author

jbracker commented Sep 4, 2013

I hope this can be merged now, because after this I will send the pull requests for the font-embedding stuff.

@byorgey
Copy link
Member

byorgey commented Sep 4, 2013

So is this now superseded by #38 ?

@jbracker
Copy link
Member Author

jbracker commented Sep 4, 2013

Yes

@byorgey
Copy link
Member

byorgey commented Sep 4, 2013

Closing in favor of #38.

@byorgey byorgey closed this Sep 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants