teunbrand / ggh4x

ggplot extension: options for tailored facets, multiple colourscales and miscellaneous
https://teunbrand.github.io/ggh4x/
Other
541 stars 33 forks source link

Fix flaw in tick length assignment #20

Closed teunbrand closed 4 years ago

teunbrand commented 4 years ago

While calculating ticks, we assign the theme element axis.tick.length twice for the same grob. This flaw exist in ggplot2 too, but I'm not keen on bothering these good folks since I've flooded their issues page enough with silly issues.

Specifically here:

https://github.com/teunbrand/ggh4x/blob/6079af285c05cdf21d25ca9da547465460475cd4/R/guide_axis_utils.R#L59-L60

param$pos is an npc unit. param$tick_dir is a unitless scalar (-1 or 1) and length is the tick length in whatever units the user defined in the theme. This means that pos before updating becomes a compound unit of unit([-1/1], "npc") + user_unit.

Later, we again assign the tick length to the grob size:

https://github.com/teunbrand/ggh4x/blob/6079af285c05cdf21d25ca9da547465460475cd4/R/guide_axis_utils.R#L72-L73

One of these should be enough. Since they are the same, this is fine in theory but becomes problematic for https://github.com/teunbrand/elementalist/issues/3. Since we've refactored the guide drawing already here, it would be best to provide a vannila guide_axis2() that omits this redundancy and fixes the issue over at elementalist.

Also, we make the same mistake in guide_axis_minor.R. This doesn't make any sense, as we're already defining the minor ticks relative to the major ticks, so we could just keep everything in the grob in npc coordinate space and assign the absolute length later for the grob as a whole.

Nothing apparently changes if we substitute the first unit assignment with just simple npc units. When guides in ggplot2 are being refactored, likely because of switching to the ggproto system, I'll try to sneak the idea in that they should omit 1 of the 2 unit assignments (preferably the one in the grob).

teunbrand commented 4 years ago

Tick lengths are now refactored and correct