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

Divorce labels #38

Merged
merged 18 commits into from
Dec 15, 2021
Merged

Divorce labels #38

merged 18 commits into from
Dec 15, 2021

Conversation

teunbrand
Copy link
Collaborator

In brief this PR:

  • Divorces labelpathGrob() from textpathGrob().
  • Refactors some stuff to be more compact and thus easier to reuse in the labelpathGrob().
  • Tinkers with some small odds and ends here and there.

@AllanCameron
Copy link
Owner

Some nice changes in here Teun. I'll have a look through the code and figure out how it works. Do you want to put in some examples of labelpath to the readme, or even a vignette? Should we have matching geoms for labelpaths and textpaths?

@AllanCameron AllanCameron merged commit 1185bc6 into AllanCameron:main Dec 15, 2021
@teunbrand
Copy link
Collaborator Author

The labelpathGrob() is almost identical to textpathGrob() with the exception of the textbox construction and associated parameters. We can put in some examples in the readme, but I have the feeling that the readme is quite full already and maybe we should just publish a vignette with what every geom is capable of / when they might be useful. In an ideal scenario, the textpath/labelpath choice would be made on-the-fly depending on, for example, whether there is an explicit fill aesthetic or something of the sort (this logic possibly conflicts with geom_textsmooth(), which has a fill on its own).

@AllanCameron
Copy link
Owner

Yes, the readme is very long, but I think it's good to have lots of examples to entice the curious. Perhaps replacing rather than adding examples might be better, but obviously we need some work on the vignettes.

The geom_textsmooth doesn't actually have a fill aesthetic. The s.e. ribbon isn't created since the geom inherits from GeomTextpath, so the fill aesthetic remains available in all the textpath based geoms. However, I do wonder if changing the grob based on which aesthetics are present is a bit non-idiomatic and might surprise some users. I don't think there's a problem with just having a 1:1 mapping between labelpath geoms and textpath geoms. I think it would make the package feel more complete.

I suppose one thing we're going to be asked at some point is whether a geom_textsf is possible. You can see the potential in this SO question , where my answer now looks quite primitive if you think what our package could do. I have no idea how difficult a geom_textsf would be, though my guess is that it wouldn't be easy.

@teunbrand
Copy link
Collaborator Author

the readme is very long

Examples are nice, especially for a plotting package. I think we should aim for showing a wide range of applications and leave some of the details, such as the hjust options and the aspect ratio stability for vignettes. Now I think about it, perhaps we should just make an examples article for the website, that needn't be a vignette per se.

The s.e. ribbon isn't created

Should we create the SE ribbon in this case? Would it make any sense to give the option to let the user choose which path to label if we do include the ribbon (i.e. a min/mid/max option)?

whether a geom_textsf is possible.

I have 0 experience with {sf} so I have no clue how feasible this might be. However, ggplot2::stat_sf_coordinates() might help maybe?

@AllanCameron
Copy link
Owner

Should we create the SE ribbon in this case?

I'm not convinced we need a ribbon (though I am prepared to be convinced). The strongest argument I can think of in favour of the ribbon is that it would allow us to say that our geoms do all the same things that the native geoms do, but simply add curved text labels. That probably makes the package easier to use. The main problem I have with it is that in many cases the text will sit half inside, half outside the ribbon. Your suggestion that the min/mid/max lines could be chosen is a good one - it comes with some added code complexity, but at least on this occasion the code modifications could be done within the geom itself rather than further burdening poor old textpathGrob.

I have 0 experience with (sf)

I have very little. I think I have 23 sf tagged answers on SO, but in many of these the sf was incidental to the point of the question. From looking at the source code, it seems the function ggplot2:::sf_grob does the drawing for geom_sf. It is a long function (about 50 lines), but its job seems to be mostly data integrity checks and setting graphical defaults. It ultimately uses sf::st_as_grob to create its grobs. This function uses S3 dispatch to convert the various geometry object types to grobs using pretty simple code. I therefore think I can see a way of doing it by copy / modifying ggplot2:::sf_grob and writing an st_as_grob.LINESTRING_labelled S3 method that creates a textpath grob. However, I probably don't know enough about geospatial plots to know how useful this would actually be. It would certainly require a bit more research on my part.

I think we should aim for showing a wide range of applications and leave some of the details, such as the hjust options and the aspect ratio stability for vignettes

This seems sensible. All the iris density plots are probably a bit repetitive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants