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

New version of bug #13 fix #15

Merged
merged 2 commits into from
Dec 21, 2015
Merged

New version of bug #13 fix #15

merged 2 commits into from
Dec 21, 2015

Conversation

kpym
Copy link
Contributor

@kpym kpym commented Dec 20, 2015

I have created a new module ellipse where the hard part of the calculation is.
I have reverted my changes to matrix module, because you don't like my copy method ;)

@kpym
Copy link
Contributor Author

kpym commented Dec 20, 2015

@puzrin Now if rx~=0 or ry~=0 the arc is replaced by a line segment as you suggested, but I can't check if this is consistent with what chrome is doing because if the transform is not reversible, like scale=(0,2) the path is not displayed (even with vector-effect="non-scaling-stroke").
I don't know if this is a good idea to do this...

@puzrin
Copy link
Member

puzrin commented Dec 20, 2015

It's better to squash into single comit & update codig style to make tests pass (npm test). Also see https://github.com/fontello/svgpath/blob/master/lib/a2c.js#L9 disabled rule, convenient for files with math. Please update coding style to be similar.

The second. I don't see radii correction as in https://github.com/fontello/svgpath/blob/master/lib/a2c.js#L131-L140. That's important. I think, after you try to add those, your signatures can change, because you do no pass size info now ((x2-x1)/2 and (y2-y1)/2).

@puzrin
Copy link
Member

puzrin commented Dec 20, 2015

Now if rx~=0 or ry~=0 the arc is replaced by a line segment as you suggested, but I can't check if this is consistent with what chrome is doing because if the transform is not reversible

We do not rely on chrome. We follow SVG implementation notes http://www.w3.org/TR/SVG/implnote.html#ArcCorrectionOutOfRangeRadii. It says arc should be replaced with line if rx or ry = 0. It also says in other chapters that arc should be dropped if end point === start point.

@kpym
Copy link
Contributor Author

kpym commented Dec 20, 2015

It's better to squash into single comit & update codig style to make tests pass (npm test). Also see https://github.com/fontello/svgpath/blob/master/lib/a2c.js#L9 disabled rule, convenient for files with math. Please update coding style to be similar.

Sorry I don't have so much time right now ... I'll move to other things probably.

The second. I don't see radii correction as in https://github.com/fontello/svgpath/blob/master/lib/a2c.js#L131-L140. That's important. I think, after you try to add those, your signatures can change, because you do no pass size info now ((x2-x1)/2 and (y2-y1)/2).

The resulting radii are positif by construction and if they are almost the same they are corrected to be the same (circle case).

if some other corrections should be done, you can probably add them. Thanks.

@puzrin
Copy link
Member

puzrin commented Dec 20, 2015

No problem. Let's keep it open until we do the rest. That will be complete at the start of nearest week.

@kpym kpym force-pushed the arctransformfix branch 4 times, most recently from 81de040 to c17ff59 Compare December 21, 2015 14:09
@puzrin
Copy link
Member

puzrin commented Dec 21, 2015

Could you squash this PR to single commit (git rebase --interactive HEAD~6 ...)? I think it's ok to merge

@puzrin
Copy link
Member

puzrin commented Dec 21, 2015

@kpym could you join all to single commit?

@kpym
Copy link
Contributor Author

kpym commented Dec 21, 2015

Is it ok now ?

puzrin pushed a commit that referenced this pull request Dec 21, 2015
@puzrin puzrin merged commit eb4dc56 into fontello:master Dec 21, 2015
@puzrin
Copy link
Member

puzrin commented Dec 21, 2015

Yes, thank you for you patience!

Did you checked arc transforms results visually or i should do it for sure?

@kpym
Copy link
Contributor Author

kpym commented Dec 21, 2015

I have checked a lot, but better you check some also.

@puzrin
Copy link
Member

puzrin commented Dec 21, 2015

Ok. Can i take 2 days before release or it's urgent?

@kpym
Copy link
Contributor Author

kpym commented Dec 21, 2015

Nothing is urgent for me ;)

@kpym kpym deleted the arctransformfix branch December 22, 2015 07:13
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

2 participants