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

node image functions [WIP] #57

Merged
merged 28 commits into from Jan 10, 2019
Merged

Conversation

AliciaSchep
Copy link
Contributor

@AliciaSchep AliciaSchep commented Jan 8, 2019

Opening up a PR to maybe make it easier to discuss the differences between 'node' versions of image functions and the previous versions.

Some differences:

  • No vw_to_png or wv_png_bin, instead a vw_to_bitmap that produces a bitmap array
  • Dependency on node + rsvg, no dependency on webdriver or v8 (other than for shinytest)
  • vw_to_png has a dpi parameter. I don't really see an effect of changing it though, so I am a bit confused. From png::writePNG function...
  • No scale parameter. Potentially this could be added and scale would just multiply width and height by that number? I guess I don't fully understand 'scale' yet
  • No embed parameter -- as the functions only use spec and not the widget itself
  • A seed and base_url argument -- these enable setting a random seed and specifying a base_url for local data files
  • Added an as_vegaspec method for vegawidget to be able to pull spec from widget
  • Some functions that were S3 methods are now regular functions at the moment where there didn't seem to be a benefit, but that can be easily reverted if desired

Given the current vignette focuses a lot on size & resolution issues, I think figuring out how to harmonize that somewhat would be helpful, but I must admit that I've gotten a bit confused about the 'scale' parameter and retina resolution

@AliciaSchep AliciaSchep mentioned this pull request Jan 8, 2019
16 tasks
@ijlyttle
Copy link
Contributor

ijlyttle commented Jan 9, 2019

Hey @AliciaSchep,

I have poked around the code a wee bit, and I like this!

For me, I had to roxygenize a couple of times to get the namespace worked out; that seems to have made the CI happier, but I am seeing that the tests are not passing - it looks like there is some difference in how node creates an svg in travis, vs. our machines (tests pass on my Mac) ...

I have run the svg function and tried to think about how the rest of it might fit together.

If I am thinking about this right, the central function will be vw_to_svg(), with friends: vw_to_bitmap(), vw_write_svg(), and vw_write_png().

I have a half-formed idea of how we might be able to use a scale argument. I think this would be useful in creating png for display on retina displays - if you specify scale = 2, you get twice the number of pixels in each dimension, but it's up to you to display it using the "regular" number of pixels.

I think we could get there with a helper function that scrapes the svg for its "native" dimensions. It could be used by vw_to_bitmap() before calling rsvg::rsvg() with a width and height multiplied by the scale. Let me see if I can build the scraper on Wednesday...

Otherwise I have minor questions:

Do you have an example (because I am curious myself) of using a local file as a data source?

What sorts of situations would call for you to set the seed for a Vega rendering? I have no doubt they exist, but I am uneducated...

Thanks!

R/to-image.R Outdated Show resolved Hide resolved
@AliciaSchep
Copy link
Contributor Author

AliciaSchep commented Jan 9, 2019

@ijlyttle sorry about the NAMESPACE issue -- I had not committed the post-build version of that file before putting in the PR!

Yes the central function is 'vw_to_svg', as the other image functions depend on that

Re scale I was thinking something along the same lines (if I am following correctly), although don't quite get what is meant by "it's up to you to display it using the "regular" number of pixels."

I will add an example for the local file -- also maybe as an example for possibly an issue where it doesn't work when making a widget itself (I have some ideas why this might be and how maybe to fix it).

Re 'seed' I don't really know... including it because I was adapting vega's vg2svg and that has a seed argument. Maybe for specs that use sample or random?

@ijlyttle
Copy link
Contributor

ijlyttle commented Jan 9, 2019

One limitation I am seeing is that vw_to_svg() does not work for specs where the there are URLs that have to go through a proxy to be accessed. At the root, it seems, is that node-fetch is not aware (nor does it want to be aware) of environment variables http_proxy, https_proxy and noproxy. See node-fetch/node-fetch#79

There are workarounds, such as https://github.com/TooTallNate/node-https-proxy-agent and https://github.com/TooTallNate/node-http-proxy-agent, but it puts a huge logic burden on us to decide when to use proxy agent, and which to use. Further, if there is a spec that has some URLs that use the proxy, and others that don't, we (at the vegawidget level) are helpless.

It almost seems that this should be addressed at the vega-loader level, so that it can be proxy-aware when it uses node-fetch.

For now, I think we just note the limitation that you cannot go through a proxy when using vw_to_svg().

@ijlyttle
Copy link
Contributor

I am not given to using profanity online, but if I were, this would be the occasion:

https://travis-ci.org/vegawidget/vegawidget/jobs/477604916

@ijlyttle
Copy link
Contributor

ijlyttle commented Jan 10, 2019

Thanks for the local-data demo! When I tried it out, I was reminded of an earlier frustration I had with Vega - that in regular use, you cannot not use a local data file unless it is exposed through a web server. I get why it is, but I remember being a little 🤔 at the time.

I added a scale argument to vw_to_bitmap() and vw_write_png().

Here's my understanding of how you would use scale: if you want a 400x300 chart, and you want a png that will have retina resolution, you would specify scale = 2. You would get an image that is 800x600 pixels, then specify that the image be displayed with dimension 400x300. Am I answering the right question? I end up referring to this Zev Ross post a lot.

I'm guessing the functions are working as advertised (please let me know if you concur - or don't). I updated the image article, and I think all that remains is touching up the documentation and writing out the limitations.

I am happy to move forward with this into CRAN, particularly because I think we can continue to support this API regardless of what is happening behind-the-scenes.

I do not want this to take away from my ongoing appreciation of the miracles you (@AliciaSchep) produce on a regular basis to "keep the lights on" for this project. In that spirit, here's where I see the gaps:

We do not yet support using proxies for URLs in data; and I fear we cannot "get there from here", ourselves, using node. This is on top of not being able to use random URLs with the RStudio IDE (which is a different issue).

In the "image" article, the PNGs appear different from the browser rendering - the colors are a little different and the sizes are a bit different. I suspect that librsvg does things differently from Chome and PhantomJS.

Perhaps, at some point in the future, we will have "easy" access to an ES6 headless webdriver (one that uses the environment-variables for proxies), and we could revisit the implementation. That being said, I am very glad we are covered using node for the immediate future.

Any objections to me merging this, @AliciaSchep?

@AliciaSchep
Copy link
Contributor Author

Whitespace! Ack! Thanks for tracking down the travis issue.

The scale argument seems reasonable, thanks for the explanation & the helpful reference! I think I need a nicer screen 😄

I pushed another commit with a minor update to vegaspec vignette (to swap V8 with node when describing dependency of vw_to_vega).

I noticed the PNG color issue as well -- I was hoping it might be a quirk of my computer, but seems not ☹️ I think some other solutions for doing svg -> png use phantom-js, so we could in theory use node to do the svg conversion (so that it is compatible with ES6 vega) and then use phantom-js based approach to do the png conversion. There is actually an R package that uses phantom to do svg -> png conversion: https://cran.r-project.org/web/packages/convertGraph/convertGraph.pdf but have not tried it out yet.

Re the proxy, I think that might be a limitation for initial release to be articulated in documentation. Not very familiar with proxies, so don't have a sense how difficult it would be to achieve support... but sounds complicated. My understanding though is that the old approach wasn't working for any url's, so this limitation shouldn't be a reason to favor the old versus the new?

Re merging, do you have access to a windows computer? I think pre-merging (or at least pre-cran submission) it would be a good idea to test drive this functionality on windows... I can try to get a hold of one, but I haven't yet tested on windows.

I'm going to open a separate issue/pr about the local data file for vega htmlwidgets because I believe that it should be possible!

@ijlyttle
Copy link
Contributor

Short version - I agree with everything you said, and thanks for all the improvements!

I have access to a windows computer I can use to make sure all the image-stuff works. I submitted this to r-hub the other day, and it passed - I'll do that again.

In the old approach, I don't know if the issue was the proxy or not. My only experience was to use Vega-example URLs on a computer that uses a proxy. However, like you said, we are no worse off with the new approach, and we dodge the ES6 bullet. I think I could get my thoughts together and put a question to the Vega folks about proxy usage with node-fetch. Nothing to stop us from moving forward now.

As for the svg-rendering issue, it's inconvenient, but, again, I don't think it's a show-stopper. I see this CRAN release, among other things, as a "plant-the-flag" exercise, so I think we have the chance to improve things going forward. As you pointed out, we needed to converge on an API, and I think we have.

@ijlyttle
Copy link
Contributor

I made some small changes to the call to node; now it appears to work on all three systems. I also had to tweak the test of the SVG, thanks to \r\n on windows; luckily I had some recent experience 😅

I ran the "image" article on windows, and it rendered as I expected. I think any remaining changes will be on the documentation, and I'll be happier to make those in master, making it easier to update pkgdown and make sure things are just-so. (I know there is a better way to manage pkgdown, but I'll leave that for next week...)

Here goes, and thanks again!

@ijlyttle ijlyttle merged commit f863f59 into vegawidget:master Jan 10, 2019
@AliciaSchep
Copy link
Contributor Author

Woohoo! Thanks for testing on windows & getting it to work cross-platform 🎉

@ijlyttle
Copy link
Contributor

No problem! Thanks for writing the functions!

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