Update compute_just() to match 'ggplot2' and fix bad justification with rotation

[aphalo]

This is a follow-up to #194. I made a pull request to 'ggplot2' fixing a bug in the handling of text justification for angles > 45 and < -45. The cause behind the bug was that the code now replaced ignored that the obvious interpretation of "inward" and "outward" refers to the plot while justification of the text is relative to the orientation of the text. The pull request has been now accepted and merged into the master branch of 'ggplot2'.

[slowkow]

I'd like to merge #196 if you think it works, but first could I ask how it is related to #195?

It seems like we should close #195, right?

[aphalo]

@slowkow I need to refresh my memory, please give me a couple of days. I will check this pull request (#196) against the code in ggplot2 and let you know if they still are consistent. Anyway, there were two different questions: 1) the bug in ggplot2 related to justification (#196) and 2) the renaming of the values returned by the position functions so that x and y columns return the nudged positions consistently with ggplot2's position functions, and use a different name (such as x_orig and y_orig for the original x and y positions from before nudging (#195). For 2) I had made an earlier pull request (#183) that I closed because I thought the code had to be improved and simplified, followed by the new fixed code in #195.
Five new position functions based on those that were part of #183 are now available in package ggpp (in CRAN and at GitHub, with online HTML documentation), but for them to work with ggrepel, the code in ggrepel needs to be updated to recognize the new naming convention. One way would be for ggrepel to adopt the new naming so that the position functions it defines become compatible with geoms from ggplot2 and other packages, while a less traumatic approach would be to simply make geom_text_repel() and geom_label_repel() aware of the naming used in ggpp and do on the fly a renaming of the columns returned by ggpp's position functions with no other change to ggrepel code.

[aphalo]

@slowkow Hi Kamil. I have been checking justification in ggrepel. There is a bug in geom_text_repel() but I do not yet know if it is in the new code in this pull request or something older. So, please do not merge this in yet. I will continue investigating and will soon share the code that demonstrate the problem.
In addition I see a different problem with justification with the position of the text within the boxes in geom_text_repel(). I see a similar, but not identical problem affecting ggplot2's geom_text().

[aphalo]

@slowkow Hi. I do not see any difference in behaviour between this pull request and ggrepel installed from CRAN, except for angle > 45 and angle < -45, which is expected. The problem with the bug in geom_text_repel() affects both the CRAN version and this branch. Somewhere in the code there is a place where hjust should be used but vjust is used instead. There is, I think another problem where the initial repulsion step could be in the wrong direction, but this could be an indirect result of the hjust bug. I will raise a separate issue for this bug with a reprex, and possibly a pull request if I can find the culprit line of code.

[aphalo]

@slowkow Hi. I have made quite some progress with the justification combined with rotation. I do not think the code is yet ready for release or merging as there are some borderline cases like multi-line rotated labels that are not put into the best place. Insights until now: 1) box.height and box.width were for the bounding box of the textGrob, while the code assumed they where the dimensions of the text label without rotation. I got much closer to the desired dimensions using stringWidth() and stringHeight(). 2) box.height and box.width were in the old code in "npc" (default) units, and when rotated changed their size. I converted string.width and string.height to "points" so that their size is independent of the x and y scales. 3) The use of cos() and sin() was in part wrong in the old code and I fixed this.

However, although the code works qualitatively well with different angle and size values and different values for hjust and vjust, in all cases labels are shifted a wrong distance, in most cases by little, but in a few cases like rotated multiline labels by a problematic amount. Zooming the output seems to also work correctly.

The problem in #171 could be related to this, but I am not yet sure. Using fill = NA helps with seeing were the segment really starts.

I pushed these code edits, but as I need to stop for today, I will added some reprex examples later in the week.

[aphalo]

@slowkow Hi. I do not see any difference in behaviour between this pull request and ggrepel installed from CRAN, except for angle > 45 and angle < -45, which is expected. The problem with the bug in geom_text_repel() affects both the CRAN version and this branch. Somewhere in the code there is a place where hjust should be used but vjust is used instead. There is, I think another problem where the initial repulsion step could be in the wrong direction, but this could be an indirect result of the hjust bug. I will raise a separate issue for this bug with a reprex, and possibly a pull request if I can find the culprit line of code.

This was a bad guess on my part, what was happening is that with rotation the height and width of the bounding box were swapped with respect to the height and width of the label string! Important

[slowkow]

Thank you so much for the update! When you let me know it is ready for merge, and I will try to run it on my system.

Do you think it might be a good idea to create a new vignette with all of your test cases (rotations, justifications) on a single page? Maybe this could facilitate catching issues? I like your examples.

[aphalo]

I think unit tests would be better (I can add some of the examples as tests using testthat). I think this code is now good to go. However, repulsion and segments are failing for some rotation angles. I will open a separate issue for these. Given these additional problems it could be safer for you to keep this code in a branch different from master/main until the other problems related to roation are fixed.

[aphalo]

@slowkow I have now added unit tests for justification with rotation and a warning about repulsion when rotation angles are not a multiple of 90 degrees. I think this pull request is ready to be merged. The plots in the ggrepel vignette Examples look o.k. to me when built using pull request #196. I am testing locally under Windows 10 and R 4.1.1.

I have also built the vignette of package 'ggpp' using this pull request and it also looks fine to me. I think the new "nudge functions" work very nicely together with the repulsive geoms. It is as if the nudging "encourages" the repulsion to be applied more consistently in a "good" direction.

The section on nudging of the 'ggpp' vignette has several examples.