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

Push/pop attributes for SVG element. Add support for x and y attributes. #131

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mindbrix
Copy link

Hi. This fixes the attached SVG.

material_design_icons.svg.zip

@FitzerIRL
Copy link

This is nice !

@mindbrix
Copy link
Author

mindbrix commented Aug 15, 2018

On closer inspection, attribute parsing needs to be element specific is some cases, e.g. x and y for concatenates a translation matrix, but sets the lower point for the rect element

Also, transforms attributes need priority for (and possible others), but they are currently parsed in the order they are found. The attached file illustrates this issue.

Accordion.svg.zip

@SergeySlice
Copy link

Such picture?
2018-08-15 19 24 52
:)

@mindbrix
Copy link
Author

Yes. Fixed, but it needs more work.

image

@mindbrix
Copy link
Author

mindbrix commented Aug 15, 2018

f8d80bc fixes rects while still supporting x and y attributes for the svg element

Repository owner deleted a comment from mindbrix Aug 16, 2018
@SergeySlice
Copy link

OK. One moment else. Should we perform parsing "transform" after parsing "x" and "y" in nsvg__parseAttr?
The order is significant.

@SergeySlice
Copy link

O, I see you already said this
"Also, transforms attributes need priority for (and possible others), but they are currently parsed in the order they are found."
Somehow reorder attributes?

@SergeySlice
Copy link

Or just don't multiply matrices until parseAttr will finish.

@mindbrix
Copy link
Author

mindbrix commented Aug 16, 2018

@SergeySlice my analysis was incorrect. The problem was that for rects the x and y attributes were being consumed twice and concatenating an unnecessary translation. This is now fixed.

@SergeySlice
Copy link

All is good now.
2018-08-16 19 16 23

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.

3 participants