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

State #74

Merged
merged 9 commits into from Apr 1, 2015

Conversation

Projects
None yet
4 participants
@cchalmers
Copy link
Member

commented Mar 27, 2015

Closes #66 and partly closes #73.

Would like to close #32 but needs lucid-svg to change from the Show instance (or we could hack a newtype wrapper with the desired Show instance).

Please test! Would like to merge before 1.3.

cchalmers added some commits Mar 25, 2015

accumulate style in state
Still needs testing but the basic idea is there. The style is accumilated in the state and attributes are only rendered once they're needed. This should help solve the font size bug and reduce unnessessary attribute rendering.
@jeffreyrosenbluth

This comment has been minimized.

Copy link
Member

commented on 3187d3b Mar 27, 2015

While we are at it should we limit the number of decimal places we output to 4 or so?

This comment has been minimized.

Copy link
Member Author

replied Mar 27, 2015

Yeah, I think so. 4 should be enough, all paths are explicit and other things only have a single transform so it should be fine

This comment has been minimized.

Copy link
Member

replied Mar 27, 2015

I used

http://hackage.haskell.org/package/double-conversion-2.0.1.0/docs/Data-Double-Conversion-Text.html

for this purpose in the past, @cchalmers you might find it helpful for the above task

This comment has been minimized.

Copy link
Member Author

replied Mar 27, 2015

I don't want to use double-conversion because the older ghc linker doesn't work with it (bos/double-conversion#8). Data.Text.Lazy.Builder.RealFloat should be fast enough. I've already coded it up but path operations in Lucid.Svg.Path use the Show instance.

add defs tags for clips and gradients
I don't know if it's nessesary but it seems to be the recommented way to do it.
@jeffreyrosenbluth

This comment has been minimized.

Copy link
Member

commented on 6282b7b Mar 27, 2015

It would be nice to hoist all defs to the top of the file.

This comment has been minimized.

Copy link
Member

replied Mar 27, 2015

As far as I can tell, the only reason they recommend it is or code readabliity.

This comment has been minimized.

Copy link
Member Author

replied Mar 27, 2015

It would but I guess this should be done in the DTree/RTree stage. Otherwise we'd need to render the whole thing and prepend the defs after.

This comment has been minimized.

Copy link
Member Author

replied Mar 27, 2015

Maybe it won't be an issue, it would only affect really big svgs and we're not really optimised for that anyway.

@jeffreyrosenbluth

This comment has been minimized.

Copy link
Member

commented Mar 27, 2015

@cchalmers so you think i should leave lucid-svg as is or change the toText function?

@cchalmers

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2015

I'm not sure. You could limit everything to RealFloat and make it 4 d.p. It might not suit everyone's needs but it's better than the current Show instance. :)

@byorgey

This comment has been minimized.

Copy link
Member

commented Apr 1, 2015

Looks good to me, I will try building the whole website with this and see what happens.

@byorgey

This comment has been minimized.

Copy link
Member

commented Apr 1, 2015

What should this code produce?

cir = circle 1 # lw none # fc red
overlap = (cir <> cir # translateX 1)

example = hsep 1 [ overlap # opacity 0.3, overlap # opacityGroup 0.3 ]

Right now it produces this:

which for me (on Chrome) looks like two copies of the same thing. In particular I think the left-hand one is incorrect.

@cchalmers

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2015

Yeah, that looks wrong. The one on the left should have a visible overlap.

@byorgey

This comment has been minimized.

Copy link
Member

commented Apr 1, 2015

Normalized font size also seems not to work. i.e.

> eff = (text "F" <> square 1) # fontSize (normalized 0.1)
>
> example = hcat
>   [eff, eff # scale 2, eff # scaleX 2, eff # scaleY 2, eff # rotateBy (1/12)]

produces

@cchalmers

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2015

k, I'll have a look.

@cchalmers

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2015

Both these examples are working as expected for me. Are you sure you have the right version installed?

@byorgey

This comment has been minimized.

Copy link
Member

commented Apr 1, 2015

No, I'm not sure. I will double check.

On Wed, Apr 1, 2015 at 7:36 AM Chris notifications@github.com wrote:

Both these examples are working as expected for be. Are you sure you have
the right version installed?


Reply to this email directly or view it on GitHub
#74 (comment).

@byorgey

This comment has been minimized.

Copy link
Member

commented Apr 1, 2015

Sorry, false alarm, it seems I had somehow indeed built diagrams-svg master instead of this PR.

@byorgey

This comment has been minimized.

Copy link
Member

commented Apr 1, 2015

Tests all look good, is this ready to be merged?

@cchalmers

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2015

Yeah. We can do #32 later.

cchalmers added a commit that referenced this pull request Apr 1, 2015

@cchalmers cchalmers merged commit 5d9711b into master Apr 1, 2015

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details

@cchalmers cchalmers deleted the state branch Apr 1, 2015

@byorgey

This comment has been minimized.

Copy link
Member

commented Apr 1, 2015

Do you know what's up with the build failure on GHC 7.6? It looks like text failed to install but with no obvious errors. Also, I think we need to bump the base upper bound for monoid-extras?

@bergey

This comment has been minimized.

Copy link
Member

commented Apr 1, 2015

The bump already happened in monoid-extras. Looks like diagrams-svg/.travis.yml needs to have monoid-extras in HEAD_DEPS.

What build failure with GHC 7.6? I only see failure in 7.10:
https://travis-ci.org/diagrams/diagrams-svg/builds/56733583

@byorgey

This comment has been minimized.

Copy link
Member

commented Apr 1, 2015

@byorgey

This comment has been minimized.

Copy link
Member

commented Apr 1, 2015

And sorry, I meant 7.8.

@cchalmers

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2015

Hmm, some times they just fail, the other one passed.

I'll add monoid-extras to travis.

@bergey

This comment has been minimized.

Copy link
Member

commented Apr 1, 2015

Ah. That's a very useless log tail. Alas, I don't have any idea what's going on.

@byorgey

This comment has been minimized.

Copy link
Member

commented Apr 1, 2015

OK, let's not worry about it unless it keeps happening.

@jeffreyrosenbluth

This comment has been minimized.

Copy link
Member

commented Jul 9, 2015

lucid-svg-0.5 removes the use of the show instance for RealFloats hence closing #32

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.