-
Notifications
You must be signed in to change notification settings - Fork 2k
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
fix "inward" and "outward" hjust and vjust in geom_text() with angle > 45 #4447
Conversation
In geom_text() when abs(angle) %% 90 > 45 justification applied based on the orientation of the text labels is done along x axis for vjust and along y axis for hjust. For "outward" and "inward" to behave correctly, the position tested against 0.5 needs to change from x to y and vice versa based on the value of angle for each row in data.
abs(angle) > 45 should work in most cases, handling correctly angles up to -135 and +135 degrees. For angles < -135 and > +135, the "inward" behaves as "outward" and vice versa but still with respect to the correct axis middle point. Using `%% 90` in the previous commit was wrong.
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.
Thanks! I think I would prefer if all the angle logic was contained in compute_just()
rather than getting repeated...
so, compute_just()
would be:
compute_just <- function(just, x, y, angle) {
...
}
@thomasp85 I think angle is relevant only to |
The cleanest way to encapsulate all the tests within compute_just() is to pass `data` as an argument to this function. This is not the suggested approach, but I think it is better. Performance impact may need to be tested.
In its new version compute_just() handles more situations than earlier so the number of test cases needed in larger.
Cater with case when "inward" and/or "outward" are used together with "right", "center", "middle" or "left", as only "inward" and "outward" should be responsive to "angle". Edit and add test cases.
This commit reverts some of the edits in the three previous commits. Set signature of compute_just() as suggested by thomasp85. As angle is set in default aes to zero, we can assume angle will be always available: remove corresponding test. Set default arguments for new parameters of compute_just() so that geom_label() does not need edits and so that existing test cases remain valid. Add new test cases.
@thomasp85 When revising the code at first and also in the original pull request I failed to realize that angle is set to zero in default_aes. Once I realized this I could simplify the code and easily implement the change you suggested. I also added a couple of test cases. At one point I edited geom_label() but I later reverted all these changes. |
Thanks! Can I get you to add a bulletpoint to the NEWS file? |
The problem described in tidyverse#4169 has changed but is not solved yet.
@thomasp85 I added the bullet point but this created a conflict. Anyway you had linked to #4169 so I tested against the reprex there once I noticed this earlier issue and I am afraid the problem is not solved by my pull request yet. I will have a look at my pull request as this seems to be a problem due to negative angles more negative than -45 degrees that I haven't properly considered as a special case. |
Don't worry about the conflict - that is common with the news file and can be fixed just before merging. Thanks for looking into the reprex👍 |
Add the logic needed to handle correctly the larger angles. The previous code worked correctly only for angle in -135...135. This also solves tidyverse#4169.
Resolve conflict in NEWs.md
I updated the code to handle correctly angles in -360...+360. To handle angles outside this range I could add I think that now this is ready. I renamed some variables to enhance readability of the code. library(ggplot2)
my.cars <- mtcars[c(TRUE, FALSE, FALSE, FALSE), ]
p <- ggplot(my.cars, aes(wt, mpg, label = rownames(my.cars))) +
geom_point(colour = "red")
p + geom_text(hjust = "outward") p + geom_text(hjust = "outward", angle = 30) p + geom_text(hjust = "outward", angle = 70) p + geom_text(hjust = "outward", angle = 90) p + geom_text(hjust = "outward", angle = 150) p + geom_text(hjust = "outward", angle = 220) p + geom_text(hjust = "outward", angle = 320) p + geom_text(hjust = "outward", angle = -30) p + geom_text(hjust = "outward", angle = -70) p + geom_text(hjust = "outward", angle = -90) p + geom_text(hjust = "outward", angle = -150) p + geom_text(hjust = "outward", angle = -220) p + geom_text(hjust = "outward", angle = -320) # Issue #4169
print(ggplot(data.frame(x = c(0, 1, 2))) +
geom_text(aes(
x = x,
y = 1,
label = paste0("Annotation", x),
angle = x * 60 - 90,
hjust = "outward"
)) +
coord_polar() +
scale_x_continuous(limits = c(0, 3))
) Created on 2021-05-04 by the reprex package (v2.0.0) |
I think you should add |
Add a modulo 360 to force all angles to the -360 to +360 degrees. Add three test cases: a) for negative angles, 2) for angle > 135 degrees and 3) for angle > 360.
@thomasp85 In addition to your request I added three new test cases. I think they will not be a burden as they directly test |
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.
Almost there - sorry for not catching these two small issues earlier
R/geom-label.R
Outdated
@@ -67,6 +67,7 @@ GeomLabel <- ggproto("GeomLabel", Geom, | |||
} | |||
|
|||
data <- coord$transform(data, panel_params) | |||
|
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.
Can you avoid touching files and functions that you don't change
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.
Sorry, this was an accident. Left from from en edit that I reverted during development. Will fix in a minute.
R/geom-text.r
Outdated
bottom = 0, middle = 0.5, top = 1)[just]) | ||
bottom = 0, middle = 0.5, top = 1)[just]) |
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.
Same as other comment - don't touch code format in unrelated code
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.
Will also fix.
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.
Thank you for all your work on this!
Thank you for helping me get the pull request in good shape! |
Fix #4169
In
geom_text()
whenabs(angle) > 45
justification applied based on the orientation of the text labels is done along the x axis forvjust
and along the y axis forhjust
. For"outward"
and"inward"
to behave correctly, the value tested against 0.5 needs to change from x to y and vice versa based on the value ofangle
in each row indata
.This pull request adds code to
draw_panel
inGeomText
to handle"outward"
and"inward"
as special cases.Reprex with 'ggplot2' 3.3.3
Created on 2021-04-24 by the reprex package (v2.0.0)
Session info
Similar reprex, adding one example, after applying this pull request to current 'master' branch from GitHub.
Created on 2021-04-25 by the reprex package (v2.0.0)
Session info
This may change the rendering of some old plots, but the old behavior is in my view wrong, and this pull request a bug fix.