search

jimjam-slam / ggflags

A flag geom for ggplot2. Tweaks the original by using round flags (great for plotting as points).
91 stars 14 forks source link

Switch to HatScripts/circle-flags #17

Open jimjam-slam opened 2 years ago

jimjam-slam commented 2 years ago

The EmojiOne flag dataset has been frozen for a while now, and we've had some issues with inaccurate flags (eg. PR #16 fixes #14 and #7). https://github.com/HatScripts/circle-flags looks like it's had more recent updates and is MIT licensed.

jimjam-slam commented 2 years ago

Note I should do a comparison to catch any changes first! In particular:

jimjam-slam commented 2 years ago

Related: the {ggsvg} package, by @coolbutuseless, might be an alternate way to get the flags in that avoids some of the issues we've had with {grImport2}. Might be worth investigating rebuilding around these for a 1.0 release!

rempsyc commented 9 months ago

What would be the steps involved for switching to the HatScripts/circle-flags images?

jimjam-slam commented 9 months ago

The basic flow is:

  1. Create a directory in inst/, called svg
  2. Download the SVGs for the flags into inst/svg/, naming them according to their 2-letter ISO code in lowercase (eg. inst/svg/au.svg for the Australian flag)
  3. Run store_flags.r from inst/. This updates data/lflags.rda.
  4. (Optional) Running modify_flags.r opens data/lflags.rda, patches a couple of flags, and saves it back.
  5. Build the package with the new data/lflags.rda.

If there are problems with step 3 (especially if we switch data sources to HatScripts/circle-flags), it may be that the structure of some new SVGs doesn't fit well with grConvert or grImport2.

It may also be that I need to run it if grConvert can't be installed on Windows! This is about the point at which I'd be starting to look at a different SVG pipeline more broadly.

rempsyc commented 9 months ago

Is there a reason we don’t keep the flags on the repo (while adding them to .rbuildignore to avoid installing them with the package) for reproducibility? Instead of having to download and rename them manually? Is it that the files are too big?

jimjam-slam commented 9 months ago

It honestly just hadn't occured to me 😅 Ggflags was the first package I took maintenance on, and it definitely hasn't been put together as I would put a package together now with reproducibility (or even CRAN submission) in mind. I'd be happy for the flag SVGs to be bundled in separately!

I'm unfortunately having trouble running {grConvert} on my Mac too—something's going wrong with poppler. I'll have another try tonight, but the alternative solution might be to run it on a Linux cloud machine like Cloudspaces or GitHub Actions.

I've added the package to my R-Universe, but if we can't run {grConvert} on Windows or Mac, we can't add the flag bundling process itself to the package build process (which I think would be ideal). But if we could at least still have the flag SVGs in the inst folder alongside the bundling script for reproducibility, and automatically renaming them from their codepoints to the ISO codes, I think that's a great compromise.

(I'm actually not sure if the flags are too big! Definitely worth checking)

jimjam-slam commented 9 months ago

The specific problem I'm having installing grConvert is with the poppler dependency, which is used for PDFs rather than SVGs. librsvg is used for SVGs in grConvert::convertPicture. I wonder if we can just use librsvg directly...

jimjam-slam commented 9 months ago

In {grConvert}, the R code connects to a C function, svg_to_svg. We could strip down that code in {grConvert} and incorporate it in, although we'd need Simon to add a licence to the package that allowed that.

There's also the {rsvg} package, which also uses librsvg. It has a rsvg_svg function, although I don't 100% know if that would be a suitable replacement (I think we'd just have to try it and see). If it could be a drop-in replacement, that'd be a lot easier, as rsvg is still actively maintained!

jimjam-slam commented 9 months ago

Keen to have a crack at using {rsvg} as the replacement on Sunday (weekend time is a bit compressed at the moment unfortunately)!

rempsyc commented 9 months ago

If possible, I would rather have my PR merged on main first, so that my own R CMD check stops failing 😬

rempsyc commented 9 months ago

Right now all your changes from the last two years have been on the dev branch, and the main branch has not been updated for two years. But when people install ggflags, it installs from the main branch, not dev, so people have not been getting all the latest changes, it seems.

jimjam-slam commented 9 months ago

Okay, I've had a crack at this issue in #28, but it's not working because I'm having problems with {rsvg} (described in that PR).

But in the mean time, I've merged dev into main, so v0.0.3 is essentially the R CMD CHECK fixes and your authorship :)

jimjam-slam commented 9 months ago

Okay, the new HatScripts/circle-flags set is working—it needed some pre-processing because those SVGs use <mask>s for the circular frames rather than <clipPaths> (which librsvg didn't like), but otherwise they seem to render fine!

One thing I have held off on is introducing the non-country flags included with that set. I would like to, but there's a bit of a larger question about how we communicate licensing to people, as although circle-flags is MIT-licensed, the flags themselves have their own copyright. Obviously this is a larger problem even with the existing flags, but I'd like to do some thinking before I add them all in.

rempsyc commented 9 months ago

Oh, cool, great job! I understand for the non-country flags. Should we mention the licensing thing in the readme as well as in the data set documentation? It's possible to have different licenses for different components.

jimjam-slam commented 9 months ago

Yeah, I think it makes sense to say that circle-flags and the package code are both MIT-licensed, but the flags themselves may have their own copyright or usage requirements. I'm just sent out a couple of emails on this that I might wait on first, but that's essentially the approach I'm planning!

Unfortunately, although the SVGs converted using {rsvg} (with my workaround substituting the <mask> for a <clipPath>) render fine in browsers, they render without the clipping at all in ggplot2 :(

image

jimjam-slam commented 9 months ago

https://github.com/ropensci/rsvg/issues/40 is fixed, but I'm still having rendering problems as above—but going by https://github.com/ropensci/rsvg/issues/41 it looks like Paul is also making some updates to grImport2 (it looks like we're all getting hit by some upstream updates from librsvg at once). Will try to test the grImport2 update when I can!

jimjam-slam commented 1 month ago

Taking another swing at this with an updated grImport2, it looks like the circular clipping paths remain. However, I'm currently using grImport2 both for build-time processing and for rendering. If I bypass it entirely, I can use {ggsvg} to render hatScripts/circle-flags without any processing at all, and they appear to look correct:

svg_ae2 <- paste(readLines("svg/ae.svg"), collapse = "\n")
# Warning message:
# In readLines("svg/ae.svg") : incomplete final line found on 'svg/ae.svg'
grid::grid.draw(svg_to_rasterGrob(svg_ae2))
image

This means I could potentially re-write the package to just use ggsvg directly and skip {grImport2} (which I think appears to be where the problem is, rather than {rsvg}).