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

ofxSvg improved compatibility #6303

Merged
merged 7 commits into from Jun 5, 2019
Merged

ofxSvg improved compatibility #6303

merged 7 commits into from Jun 5, 2019

Conversation

sebleedelisle
Copy link
Contributor

Hi team!

Some fixes for ofxSVG here. I hope they're helpful, and let me know your thoughts / feedback etc.

  • added support for the SVG "use" command, a common feature that allows multiple instances of the same graphic elements. This is commonly found in SVGs exported with vvvv and Flash.

  • fixed an issue where line weights less than 1 would not render.

  • fixed an issue where elements marked "display:none" would still render.

  • separated the parsing from the loading for increased flexibility (my use case was parsing SVG files being sent over a socket connection from vvvv).

It works by expanding out the original SVG text before passing it into lib_svgtiny so there is somewhat of a performance hit - so I've added the ability to disable it if required by using the ofxSVG::setImprovedCompatibilityMode(bool mode) method. It also required a minor addition to ofXML as outlined below.

ofXML - provided access to the underlying pugi::xml_node method "getParent()"

ofxSVG
- added support for the SVG "use" command, a common feature that allows multiple instances of the same graphic elements. Also fixed an issue where line weights less that 1 unit would not render. This increased compatibility can be disabled if required by using the ofxSVG::setImprovedCompatibilityMode(bool mode) method.

ofXML - provided access to the underlying pugi::xml_node method "getParent()"
@arturoc
Copy link
Member

arturoc commented May 30, 2019

have you meassured how bad is the performance loss? if it's not too much i think we can just remove this new method to disable the full parser

In any case instead of having a flag that sets a state it would be better to have an additional method like loadLimitedSvg() or something like that

@sebleedelisle
Copy link
Contributor Author

sebleedelisle commented May 30, 2019

Hey Arturo! Performance hit will vary depending on the size of the SVG file, and the amount of fixing it needs but it seems negligible. For the tiger image in the SVG example, it actually seemed to load a bit faster than before (which I don't actually understand!). So maybe we don't need the fallback? Happy to remove it and re-commit if you're happy. :)

@arturoc
Copy link
Member

arturoc commented Jun 1, 2019

mmh not sure what to do then, have you seen any case where loading a file would take way longer than before?

@sebleedelisle
Copy link
Contributor Author

After more testing I can confirm that there is no measurable performance degradation for SVG files that do not need fixing. So it only takes longer if the files do need fixing, in which case they wouldn't have worked at all before, and the performance decrease is marginal in most SVGs... so I think we're good.

Let me know if you're happy to take out the flag and I'll do another pull request.

@arturoc
Copy link
Member

arturoc commented Jun 3, 2019

Sure, if you can remove the flag I think it0s ready to merge, no need for another PR just push the new commit to your branch

Now SVGs are “fixed” by default as there wasn’t a noticable performance degradation for files that don’t need fixing. Added some documentation.
@sebleedelisle
Copy link
Contributor Author

Ah great, new version pushed... I also added some more documentation to the file. Does this automatically get pushed out to the ofSite or do we need to do that separately?

@arturoc
Copy link
Member

arturoc commented Jun 4, 2019

documentation only updates when we do a major release which will happen soon and it happens automatically so nothing else to do.

there's a problem right now with some of the tests, nothing related to this PR just a general problem, i'll look into it and merge it as soon as that is solved

thanks!

@sebleedelisle
Copy link
Contributor Author

Great thanks Arturo!

@arturoc arturoc merged commit 1c1e15a into openframeworks:master Jun 5, 2019
@arturoc
Copy link
Member

arturoc commented Jun 5, 2019

thanks!

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