-
-
Notifications
You must be signed in to change notification settings - Fork 41
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
EMF file support #152
EMF file support #152
Conversation
.circleci/config.yml
Outdated
@@ -61,7 +61,7 @@ jobs: | |||
name: Install deps | |||
command: | | |||
npm install | |||
sudo apt-get install poppler-utils libgconf-2-4 | |||
sudo apt-get update && sudo apt-get install inkscape poppler-utils libgconf-2-4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor 🐄
sudo apt-get update
sudo apt-get install inkscape poppler-utils libgconf-2-4
package-lock.json
Outdated
@@ -143,7 +143,7 @@ | |||
"string-width": { | |||
"version": "2.1.1", | |||
"resolved": "https://registry.npmjs.org/string-width/-/string-width-2.1.1.tgz", | |||
"integrity": "sha512-nOqH59deCq9SRHlxq1Aw85Jnt4w6KvLKqWVik6oA9ZklXLNIOlqg4F2yrT1MVaTjAqvVwdfeZ7w7aCvJD7ugkw==", | |||
"integrity": "sha1-q5Pyeo3BPSjKyBXEYhQ6bZASrp4=", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Which npm version are you using?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using 6.2.0. I should probably upgrade. What are you using?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/util/inkscape.js
Outdated
}) | ||
} | ||
|
||
cleanSvg (svg) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow. That's some fancy regex work!
How you tried using something like xml2.js
to loop over all those SVG nodes? It should make things easier to maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was pondering whether to use regex vs parsing the XML. The latter is certainly less error-prone.
Now that I know what cleaning needs to be done, I will look into xml2.js
!
@antoinerg Thanks for making https://gist.github.com/antoinerg/680ce690b039fbcb49b976de1d28e1c6 - the results look pretty good! I've noticed that fill from carpets traces and some contour traces (the ones coloring constraint regions?) appear black. Is that something that can be fixed? Ideally, I would prefer not using regexes to do all the heavy compatibility work before the SVG -> EMF conversion. Using something like We could even move this compatibility logic to plotly.js' Thinking about this again, I think adding a |
@etpinard thank you for the review!
I decided to wait for your feedback before looking into those. There are also some
I agree that regexes feel like a hack and are error-prone. I'll look into
I was wondering the same thing: how much cleaning should be done in plotly.js vs Orca. A
Yes, some of the fixes for Inkscape's SVG-to-EMF glitches should definitely stay in Orca (ex. changing how we define SVG |
CarpetsAfter removing the semi-transparent fills, the carpets now look like: ContoursAbout conversion problems with contours, are you referring to the result below? If so, I need to investigate the clipping. I suspect it has to do with the way we define our units (similarly to how we fixed the gl2dUnfortunatley, those one are much harder to deal with because they were designed with transparency in mind: they have SVG axes on top of which there are a transparent PNGs of the WebGL contexts. As you know by now, EMF doesn't support transparency very well. Not sure how we could handle this without significant work/modification to the SVG. See this comment for details on EMF and transparency in Inkscape's bug tracker: https://bugs.launchpad.net/inkscape/+bug/1419686/comments/3 |
Results from commit 1ecb2f4: https://gist.github.com/antoinerg/e3b7d7b7b97bbff9e0d84c769f0a4e91 |
Results for commit 5432b59: https://gist.github.com/antoinerg/1637f08d26664965b6d942e53d56e7a3 Improvement to |
@antoinerg Thanks for fixing our tests! Now, it looks like the AppVeyor runs have been failing since 7dddce5 where |
Commit 212ed21 (which has a bumped down |
Ok. Something is up with AppVeyor. I pushed an empty commit off master, and AppVeyor fails there too: https://ci.appveyor.com/project/AppVeyorDashAdmin/image-exporter/builds/20823193 So, I would be ok merging this thing as is. Let me review it one last time. |
@etpinard There's also one last important to do: update the deployment's Dockerfile. Should I submit a separate PR for this one? |
Ah good point. Let's do this in this PR! |
src/util/inkscape.js
Outdated
|
||
// Get background color from figure's definition | ||
var bgColor | ||
if (opts.figure.layout.paper_bgcolor) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
figure.layout
might not always be defined (unless orca is doing something hacky I don't remember), so I would prefer:
if ((opts.figure.layout || {}).paper_bgcolor) { /* */ }
to stay safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change made in commit e4ab53d
} | ||
|
||
cleanSvg (svg, bgColor) { | ||
const fragment = JSDOM.fragment(svg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find! This is probably faster than DOMParser
src/util/inkscape.js
Outdated
var m = style.match(fillOpacityZero) | ||
|
||
if (m) { | ||
var rgbFill = m[1] + `fill: rgb(${bgColor[0]},${bgColor[1]},${bgColor[2]});` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐄 Let's make this one single ES6 string: ${m[1]} fill: rgb ... ;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in e76c52b
Nice job. The new jsdom-based logic looks fantastic. https://gist.github.com/antoinerg/1637f08d26664965b6d942e53d56e7a3 is giving me 500 though, is that just Github acting up? |
It's probably Github acting up because the gist is large. Maybe git clone it:
By the way, I really like |
Results in PNG for latest commit e76c52b:
Basically, |
deployment/Dockerfile
Outdated
@@ -1,4 +1,4 @@ | |||
FROM ubuntu:xenial | |||
FROM ubuntu:bionic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. We'll have to test that thoroughly on stage. Is this required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's definitely the path of least resistance to get the latest version of Inkscape (0.92.3 which is also the version I used in my testing). Otherwise we'd have to compile Inkscape in the container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a ppa shouldn't be too bad:
http://ubuntuhandbook.org/index.php/2017/01/install-inkscape-0-92-ppa-ubuntu-16-04-16-10-14-04/
I'm not against bumping Ubuntu, but we can't just casually bump it here w/o some thorough manual testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand your concern. Let me try to compile Inkscape or install from a deb package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a ppa shouldn't be too bad:
That did the trick thank you 👍
Commit b6daae1 reverted our container to ubuntu:xenial
and install Inkscape from a PPA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
amazing 🎉
Thanks very much!
deployment/Dockerfile
Outdated
@@ -90,7 +90,7 @@ RUN apt-get update -y && apt-get install -y subversion && \ | |||
RUN wget -q -O - https://dl-ssl.google.com/linux/linux_signing_key.pub | apt-key add - && \ | |||
sh -c 'echo "deb [arch=amd64] http://dl.google.com/linux/chrome/deb/ stable main" >> /etc/apt/sources.list.d/google-chrome.list' && \ | |||
apt-get update -y && \ | |||
apt-get install -y google-chrome-stable xvfb poppler-utils git && \ | |||
apt-get install -y google-chrome-stable xvfb poppler-utils inkscape git && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried running that locally?
If I remember correctly:
docker build . -f deployment/Dockerfile -t tmp-inkscape
# check if ok with
docker images
docker run tmp-inkscape -d -p 9010:9010 tmp-inkscape
# then
curl localhost:9010 ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default port is 9091
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following works for me!
docker build . -f deployment/Dockerfile -t tmp-inkscape
# check if ok with
docker images
docker run tmp-inkscape -d -p 9091:9091 tmp-inkscape
# then
cd plotly/plotly.js/test/image/mocks/
curl -XPOST -d @14.json localhost:9091 > /tmp/14.png
Thanks! The results are very impressive! Apart from all gl2d and splom mocks, I noticed:
All in all, out of 600+ mocks, that's not bad. I'm ready to 💃 @antoinerg you can either:
About the AppVeyor failure, we should try running the |
@etpinard, I think I will go with the second course of action so others can benefit as soon as possible from the tests that we fixed in this branch. For example this PR @tarzzz just submitted #165 fails :(
Is this blocking? If so, I will test it first thing tomorrow! Thank you for the thorough review and for finding all the discrepancies 🕵️♂️! I will fix them in a separate issue as you suggested 🎉 |
Not blocking for this PR, but we should try to fix it ASAP to not delay any upcoming orca releases. Opening an issue for now would be 👌 |
This PR is a work in progress adding EMF support to Orca. After testing Inkscape, unoconv and svg2vector I went with Inkscape because it gives the best results.
The biggest challenge with converting SVG to EMF have to do with opacity. Although EMF supports full transparency, it does not support opacity between 0 and 1. This is a problem because:
fill: rgba(0,0,0,0)
. Inkscape throws away the alpha channel and is then left with a black rectangle instead of a transparent one! Example:Finally, Inkscape doesn't handle well the way we define our SVG linear gradients:
Those issues can be addressed by cleaning the SVG prior to feeding it to Inkscape as I demonstrate here.
In commit 035ed22, I fix most of the black rectangles by removing transparent rectangles, properly convert the gradient in EMF by changing the
gradientUnits
of our SVGlinearGradient
element and finally manually do alpha compositing for each pixel of the PNG images of WebGL traces.You can find a comparison of Orca's PNG and Orca's EMF converted to PNG at https://gist.github.com/antoinerg/680ce690b039fbcb49b976de1d28e1c6
To showcase the progress so far, the above figures now looks like this in EMF: