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

v1.2 font handling and output goals #127

Closed
5 of 7 tasks
matthewstern opened this issue Mar 29, 2021 · 16 comments · Fixed by #128
Closed
5 of 7 tasks

v1.2 font handling and output goals #127

matthewstern opened this issue Mar 29, 2021 · 16 comments · Fixed by #128
Assignees
Labels
bug Something isn't working enhancement New feature or request
Milestone

Comments

@matthewstern
Copy link
Contributor

matthewstern commented Mar 29, 2021

This issue is meant to combine the related issues #91, #116, and #120, which I will close after posting this. There may be more useful details in each of those discussions, but I've done my best to capture the most useful links and findings below.

The situation:

  • our use of svg() does not export text (it converts text to shapes) and has not proven readable by Illustrator
  • recent developments in packages systemfonts, ragg, and svglite make cross-platform, consistent graphics rendering with custom fonts possible
  • our pkgdown website currently does not display Whitney fonts -- I believe this is due to implementation of ragg:agg_png() (and therefore systemfonts) within the pkgdown package.
  • due to the way we previously built fonts, we changed cmapplot_globals to a named list rather than an environment, although storing these sorts of variables in an environment seems to be the better practice. We may be able to return the environment structure with the new fonts construction (which may enable an easy fix to constant modifier function #117).

Definite projects for v1.2: (followed by possibly useful resources)

Possible projects:

Other resources (which may be out of date now that systemfonts and ragg exist):

@matthewstern matthewstern added bug Something isn't working enhancement New feature or request labels Mar 29, 2021
@matthewstern matthewstern added this to the v1.2 milestone Mar 29, 2021
@nmpeterson
Copy link
Contributor

Would reverting cmapplot_globals back to an environment have any unintended consequences related to the discussion in #67? I know @dlcomeaux was frustrated that cmapplot_globals could not be accessed by users who load the package via library() instead of load_all() when it was an environment, but I don't know if we now assume anywhere that a user can access it, or if that access would be beneficial over using an environment?

@dlcomeaux
Copy link
Collaborator

I do not believe that we currently rely on the assumption that a user can access it. The only case that I can think of where it would be relevant right now is colors - if a user wanted to use 'blackish' for anything that is not configured to use it by default. I know that in some of my personal implementations of cmapplot I would use additional colors, if we got them from Comms (i.e., if there were a specific light gray that we were supposed to use for fainter gridlines).

However, if shifting to an environment has other benefits, I think that it would make sense to do so. We could then potentially add a separate export for colors (and maybe font sizes?), as those are the ones where there could be value in the users having access?

@dlcomeaux
Copy link
Collaborator

However, the point I made in #117 stands - I do think that we should be careful about introducing the ability to monkey around with font sizes, if that is something that the Comms team feels strongly about otherwise.

@matthewstern
Copy link
Contributor Author

matthewstern commented Mar 29, 2021

Here's another resource on environments within packages that I just added to my original post above.

They write:

It seems we can properly access a package-wide variable from within a function of a package, but we’re not allowed to overwrite it (or create new variables at that level from within a function). Perhaps we could leverage environments to solve this problem. As it turns out, environments were likely a cleaner solution to our problem all along...

As long as this environment is created in one of our package’s .R files and not inside of a function, it will be accessible across our entire package. We can do all the things with this environment that we’re used to doing in our regular R environment (whether we knew we were using environments or not): create new variables (assign), modify existing variables (assign), remove variables (rm), retrieve data associated with a variable (get), list the variables in an environment (ls), etc.

Seems to me that shifting to an environment could allow for basic fetch and set functions could be added using get and assign. This would mirror how base R allows options() for setting global options and getOption() for inspecting them.

Maybe this is not relevant or important -- I am open to that, and to just leaving this all as a list. I do appreciate @dlcomeaux's point about being careful about giving users too much control (although I'm not sure I totally agree, as our users are few and in general know what they are doing). I included it here only because this font work opens an easy opportunity to make the conversion if we think there's reason to do it. But, if y'all don't think there's reason to, I'm definitely happy to be outvoted.

I agree with @dlcomeaux that if we ever program cmap colors more universally into the package (like, the whole palette, rather than just blackish), seems to me like that should become exported package data rather than a value in an environment.

@matthewstern
Copy link
Contributor Author

Also, as an FYI, ggplot2 uses an environment and set/get functions for global variables: https://github.com/tidyverse/ggplot2/search?q=ggplot_global

@sarahcmap
Copy link
Contributor

sarahcmap commented Apr 2, 2021

Thanks for putting all these links together @matthewstern. I see that ragg does not have a bmp export option - are we sticking with grDevices for this? Do folks use .bmp? I read it's not used much these days.

@nmpeterson
Copy link
Contributor

I'm personally of the opinion that PNG is the only raster format we should support/encourage. BMP files tend to be much larger, and JPG compression creates awful artifacts on graphics (it's great for photos, though).

@matthewstern
Copy link
Contributor Author

@sarahcmap, 100% don't need to maintain support for bmp, we just did that bc it was available and worked the same. To that end, my default here would be to continue to support png, tif, and jpg via ragg, but I can get with @nmpeterson 's perspective on this and support png only. Either is fine by me!

@nmpeterson
Copy link
Contributor

I don't have an issue with continuing to allow any raster formats supported by ragg, but I do think we should be explicitly recommending only PNG for use by CMAP staff

@nmpeterson nmpeterson mentioned this issue Apr 5, 2021
6 tasks
@matthewstern matthewstern linked a pull request Apr 5, 2021 that will close this issue
6 tasks
This was referenced May 4, 2021
@dlcomeaux
Copy link
Collaborator

I have been digging into this a bit this afternoon - I concur with @sarahcmap that everything except SVGs appears to be working properly. Specifically, while SVGs are exporting text as text, the bolding is not consistent with other exports - this is particularly noticeable for the title but it is also relevant for other text on the graph. Also, when you copy and paste that exported text, the font sizes and names are off in a software like Word. I haven't been able to get access to Illustrator today, so I'm not sure what the situation is there.

I haven't solved it, but I believe that I've identified part of the problem. The code underlying the SVGs appears to be doing two possibly related things that are causing issues:

  • All of the text is specified as font-family: Whitney rather than the defined fonts we've provided (with separate fonts for Book and Semibold, for example).
  • These different fonts are being expressed instead through variations in the weighting - this may or may not be standard practice, although from here, it looks like this is svglite's standard approach. However, the variations populating into our graphics appear to be too small to be readily distinguishable - the weights range from 325 on the low end to 375 on the high end. Per here, these weights seem off from the normal specifications. And, when I change the underlying weights to some new ones (300, 500, and 600), the weighting looks about right. It may not entirely solve the discrepancy, though, if there are other subtle differences between the base Whitney font and the various iterations we're using.

I exported the test fonts (using display_cmap_fonts()) and got the following as the raw image (this is a screenshot, GitHub wouldn't let me upload the SVG - I've pushed it to this branch as test_export.svg):
image

When I modified it to adjust the weighting, I got this (also pushed to this branch as text_export_modified.svg):
image

@nmpeterson
Copy link
Contributor

Just a note with the proper weights of the different Whitney variants:

  • Light: 300
  • Book: 400
  • Medium: 500
  • SemiBold: 600
  • Bold: 700
  • Black: 800

@dlcomeaux
Copy link
Collaborator

Yeah - I did try those, they don't seem to quite match up with the outputs from the other graphics. Here's what that looks like:
image

@dlcomeaux
Copy link
Collaborator

More generally, it appears that the fonts are not registering as "semibold" or "book" in systemfonts. I'm not sure what the reason for that would be, but if you look at the systemfonts::system_fonts() list, the weights for most of the Whitney fonts are blank. In the font registry, they are either normal or bold.

@nmpeterson
Copy link
Contributor

More generally, it appears that the fonts are not registering as "semibold" or "book" in systemfonts. I'm not sure what the reason for that would be, but if you look at the systemfonts::system_fonts() list, the weights for most of the Whitney fonts are blank. In the font registry, they are either normal or bold.

Bear in mind that, as of version 1.2, cmapplot does not use the fonts in systemfonts::system_fonts(). Rather, it uses the manually registered fonts in systemfonts::registry_fonts(). Since systemfonts::register_fonts(), which we use to perform that manual registration, does not provide a means to specify multiple weights within a font family, each weight has effectively been registered as its own family. So, rather than referencing "Whitney", we now have to reference "Whitney Book/Medium/Semibold" instead.

@matthewstern
Copy link
Contributor Author

Since systemfonts::register_fonts(), which we use to perform that manual registration, does not provide a means to specify multiple weights within a font family, each weight has effectively been registered as its own family.

The only addendum that I would make to this is that register_fonts actually does requires the identification of a "bold" face, so each of our three fonts has been assigned a bold weight. Whitney Semibold calls Whitney Black as bold, Medium calls Bold, and Book calls Semibold. It would be an interesting to know how svglite() would handle, say, a title string that was partially bolded via html.

So, rather than referencing "Whitney", we now have to reference "Whitney Book/Medium/Semibold" instead.

This is one of the reasons I find Daniel's svglite outputs so intriguing--they certainly appear to reference "Whitney" as opposed to any specific face, which strikes me as the primary challenge to overcome.

@nmpeterson
Copy link
Contributor

Yes, it's really bizarre. I wonder if it would produce the same output on a system that doesn't have Whitney already installed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants