add shadowtext functionality to ggrepel #142
Conversation
|
Thank you for the pull request. This could be a nice new feature! It looks like the shadowtext Artistic 2.0 license is compatible with GPL, so we can use the code as you've done. I agree that we should not add Do you prefer Request 1: Instead, I would like to ask that you please add the new aesthetics to the table below. For example, Lines 135 to 153 in f99274d Eventually, I might reorganize a bit and try to clarify that some of these are parameters for Request 2: Could I also ask you to add a new example to Request 3: Could you please update DESCRIPTION to add yourself as a contributor? Also add a new item to NEWS.md. I'll review the code and make some comments. |
| if (!is.unit(x)) | ||
| x <- unit(x, default.units) | ||
| if (!is.unit(y)) | ||
| y <- unit(y, default.units) |
There was a problem hiding this comment.
Could we move this outside the loop?
|
Hey @slowkow, Sorry for the slow response, I got struck down by a virus for a couple of days just after I made this pull request. I hope that's correlation and not causation. I see that you already did all of the work for me! Thanks a lot!
I'm not a big fan of
I think this is an issue with ggplot2 in general. I think your vignette is extensive enough that anyone interested in using ggrepel should be able to find their way, especially since you provide so many examples. I made a few more changes to my fork:
Would you like me to make another PR? Kind regards, |
|
Thanks, Robrecht! I like this new feature and I hope our users will enjoy it, too. Sorry for merging without waiting for you. I added your code comment in b194b72 so we can remember where it came from. You are now a contributor 🙂 I'd be happy to accept future pull requests if you have any other ideas! Here's the new documentation: Lines 127 to 177 in 9ec17e3 |
|
I don't think it is necessary to prevent dependency of |
|
Hi Guangchuang, thanks for the comment. Thanks for writing shadowtextGrob() in the shadowtext package. I think it's great, and Robrecht and I tailored it to match our needs for ggrepel. It's nice to have the freedom to consider other features I might like to add to it that ggrepel users might enjoy. What I love about open source software development, is that when we use permissive software licenses like General Public License (ggrepel, ggplot2, R) and Artistic License (shadowtext), this gives all of us the freedom to contribute to an enormous collection of work we all create and share. ggrepel also has code copied from ggplot2, shown below. ggwordcloud also started by using and modifying my repel_boxes.cpp code from the ggrepel package into a new file called wordcloud_boxes.cpp. I'm very impressed, and happy to see that my small contribution could be reused in a way that I couldn't have done myself. Thanks again for sharing! 🐮 Kamil Example 1# Parse takes a vector of n lines and returns m expressions.
# See https://github.com/tidyverse/ggplot2/issues/2864 for discussion.
#
# parse(text = c("alpha", "", "gamma"))
# #> expression(alpha, gamma)
#
# parse_safe(text = c("alpha", "", "gamma"))
# #> expression(alpha, NA, gamma)
#
parse_safe <- function(text) {
stopifnot(is.character(text))
out <- vector("expression", length(text))
for (i in seq_along(text)) {
expr <- parse(text = text[[i]])
out[[i]] <- if (length(expr) == 0) NA else expr[[1]]
}
out
}Lines 52 to 69 in 72bdd55 Example 2#' Nudge points a fixed distance
#'
#' `position_nudge` is generally useful for adjusting the position of
#' items on discrete scales by a small amount. Nudging is built in to
#' [geom_text()] because it's so useful for moving labels a small
#' distance from what they're labelling.
#'
#' @family position adjustments
#' @param x,y Amount of vertical and horizontal distance to move.
#' @export
#' @examples
#' df <- data.frame(
#' x = c(1,3,2,5),
#' y = c("a","c","d","c")
#' )
#'
#' ggplot(df, aes(x, y)) +
#' geom_point() +
#' geom_text(aes(label = y))
#'
#' ggplot(df, aes(x, y)) +
#' geom_point() +
#' geom_text(aes(label = y), position = position_nudge(y = -0.1))
#'
#' # Or, in brief
#' ggplot(df, aes(x, y)) +
#' geom_point() +
#' geom_text(aes(label = y), nudge_y = -0.1)
position_nudge <- function(x = 0, y = 0) {
ggproto(NULL, PositionNudge,
x = x,
y = y
)
}
#' @rdname ggplot2-ggproto
#' @format NULL
#' @usage NULL
#' @export
PositionNudge <- ggproto("PositionNudge", Position,
x = 0,
y = 0,
setup_params = function(self, data) {
list(x = self$x, y = self$y)
},
compute_layer = function(self, data, params, layout) {
# transform only the dimensions for which non-zero nudging is requested
if (any(params$x != 0)) {
if (any(params$y != 0)) {
transform_position(data, function(x) x + params$x, function(y) y + params$y)
} else {
transform_position(data, function(x) x + params$x, NULL)
}
} else if (any(params$y != 0)) {
transform_position(data, NULL, function(y) y + params$y)
} else {
data # if both x and y are 0 we don't need to transform
}
}
)Lines 1 to 65 in 72bdd55 |
|
Hello Guangchuang, I agree with Kamil's statements but wish to clarify the decisions I made in making this PR. I apologise for having reused your code without your consent, but this is exactly the beauty of open-source software. I considered proposing to add shadowtext as a dependency to ggrepel. However, simply using Since shadowtext is mentioned in the documentation, if people use the shadowtext functionality in ggrepel, they will undoubtedly learn about using shadowtext the package when they need it in a non-repel scenario :) I hope this helps you understand the reasoning behind my actions, even if you do not agree with them per se. If you have any further questions or comments, I'll be happy to answer them. Kind regards, |
|
I didn't say you are doing it in the wrong way. I am quite happy to see the The modification is quite simple, You can access the And you can also ask me to return a Taking another issue that you are considering to change the parameter names, IMO such things should be happened in the Don't get me wrong. I just wondering whether there is a better way to do it. |
|
Thank you Kamil for In the beginning, I had some problems installing from Github but then I downloaded and compiled locally it works fine. A trivial note: the example code on your example webpage is a little bit confusing @slowkow since there is no And the webpage says |
|
You're right! The 0.9.0 version has not yet been released, it is only available through GitHub right now. When time allows, I will upload the next version to CRAN. And the |
|
This increases the graphic size (pdf) subtaintially (from ~1Mb to 15Mb) in my case with 49 legends. Is there a way to control this? |
|
@kklot Please consider opening a new issue and providing a reprex. |
Hello @slowkow!
This PR is related to issue #103, adding 'halos' to ggrepel. I looked at the shadowtext implementation and found that adding it to ggrepel would not be that difficult. At first, I simply reused the
shadowtext::shadowtextGrob()function, but that would add an extra dependency toggrepel. SinceshadowtextGrob()is relatively simple, I instead copypasted it and changed it a little bit to make it work better with ggrepel.The timings are in line with what is to be expected:
Code for timings
The only issue I still have is, where do I document this functionality? Since it's an aesthetic, it's not a parameter I can document with
@param bg.colourand@param bg.r. Where do you think this documentation should end up?Alternatively, I could create a separate function,
geom_shadowtext_repel(). It could use the same underlying implementation, just with different default aesthetics (bg.colour = NA, bg.r = .1forgeom_text_repel()andbg.colour = "white", bg.r = .1forgeom_shadowtext_repel).What are your thoughts on this?
Kind regards,
Robrecht