Implement polar axis

[asinghvi17]

Description

This PR implements a PolarAxis layoutable which should be able to serve as a template for a more general NonlinearAxis layoutable.

rs, θs = LinRange(0, 1, 100), LinRange(0, 2π, 100)
field = [exp(r) + cos(θ) for r in rs, θ in θs];

fig = Figure()
po = PolarAxis(fig[1, 1])
surface!(po.scene, rs, θs, field; shading = false, colormap = :RdBu)
autolimits!(po)

record(fig, "test.mp4", LinRange(0, 2*pi, 600); framerate=60) do i
    po.θ_0[] = i
end

polar_rotation_1.mp4

To-dos

• Angular tick labels
• Radial tick labels
• Tighter bounding box around the plot
• Figure out how to enable plot!(po, ...)
• Fix stack overflow error when MakieLayout.can_be_current_axis(::PolarAxis) = true
• Better update semantics, hook up zooming
• Poly drawn around the plot to simulate clipping

Type of change

Delete options that do not apply:

• New feature (non-breaking change which adds functionality)

Checklist

• Added an entry in NEWS.md (for new features and breaking changes)
• Added or changed relevant sections in the documentation
• Added unit tests for new algorithms, conversion methods, etc.
• Added reference image tests for new plotting functions, recipes, visual options, etc.

Fixes #309, #521

[MakieBot]

Compile Times benchmark

Note, that these numbers may fluctuate on the CI servers, so take them with a grain of salt.

using time

master:  9.11 < 9.21 > 9.31, 0.06+-
pr:      9.11 < 9.18 > 9.30, 0.06+-
speedup: 0.99 < 1.00 > 1.01, 0.01+-
median:  -0.35% => invariant

This PR does not change the using time.

ttfp time

master   25.00 < 25.19 > 25.71, 0.26+-
pr       25.04 < 25.23 > 25.51, 0.19+-
speedup: 0.99 < 1.00 > 1.01, 0.01+-
median:  +0.18% => invariant

This PR does not change the ttfp time.

[asinghvi17]

Just fixed the bounding box issue - it was an issue in my transformation method for rects.

Need to hook up protrusions, enable plotting, and the axis should be ready to go!

[adigitoleo]

Haven't had a lot of time to look at it yet unfortunately, but I'm wondering how best to handle integration with existing methods that modify xgridvisible/ygridvisible or other x<parameter>/y<parameter>. I've noticed that this implementation uses inherit in some places but not everywhere. I think I remember that e.g. clearing the grid with one of the builtin methods isn't supported yet. I guess we'll need to check to what extent r and θ can be made semantically equivalent to x and y in the API, or where we need dedicated methods that handle exotic axes separately.

[ffreyer]

I'm not sure if it makes much of a difference but I think (val,) is preferable over Ref(val) in these cases because it doesn't allocate.

Also project(scene, apply_transform(f, point)) (or the lower level variants) should be used here and above. The variants that specify input and output space have some extra branching and matrix multiplications in them

[SimonDanisch]

I think this should really be a map(points) do p

[asinghvi17]

Is there a difference between that and what's done here, aside from code cleanliness? Basically, I didn't want to call camera and apply_transform in an inner loop so did this instead.

Also, AFAIK since the transform_func is an Observable{Any}, having apply_transform in an inner loop takes a lot of time compared to having it outside.

[SimonDanisch]

having apply_transform in an inner loop

You currently already do apply_transform(f, points), no? So it might as well already live outside the map/broadcast.
Also, I think symbols, types and numbers don't need a Ref, since they're already treated as scalar by broadcast.
I'd also prefer convert(Vector{Point{N, T}}, ...) since it's less complex, has a clearly defined return type and also doesn't copy if project already returns the correct type.

[ffreyer]

It's probably better to plot text first, and then use normal bounding boxes to update things. Otherwise changes and optimizations there might get left behind here. (I think right now it's more or less the same amount of work. plot first just needs to guard against infinite loops.)

[asinghvi17]

I'll definitely look further into the structure of text then. I was under the impression that everything got squashed into one GlyphCollection but could definitely be wrong.

[asinghvi17]

Also, if the GlyphCollection changes when position changes, then it's harder to lift off it, since I only care about the text extents of each segment - not their positions (at least for now). If we want a 100% tight bbox then it can be tried, but I would definitely like to do that later.

[ffreyer]

Oh, you're doing bboxes per string in a vector with that. If you're doing boundbox(p) they'll get combined, but the glyph collections should be available per string.

GlyphCollection changes when position changes

glyph collections shouldn't trigger on plot.position anymore. If they are we should clean that up, because there is no reason to mix them until the final rendering code.

[asinghvi17]

Thanks for the detailed review @ffreyer! I'll probably have that last comment wrapped up tomorrow or so. It seems like the bounding boxes can be obtained by:

ttp = (po::PolarAxis).blockscene.plots[end-2]
Makie.boundingbox(ttp.plots[1].plots[1][1][][1], Makie.to_rotation(ttp.plots[1].plots[1].rotation[]))

although this is pretty fragile. The bboxes do seem to be independent of position, though.

@adigitoleo, about your comment - now that theming is hooked up, I will create some setters for e.g. rticks, θticks which handle both the ticks and their labels (size, color, alignment, etc). I'll also implement hide[/x/y]decorations methods for the polar axis.

[jkrumbiegel]

I'm not the biggest fan of θticks just because there's not a single unicode symbol in the rest of our api. And many people probably don't know immediately what it means anyway.

[juliohm]

Any recent update regarding this PR? This is the only recipe that is missing in order to migrate all my plots to Makie.jl

[asinghvi17]

Kind of lost track on this, if you've tested it are there any issues with it currently @juliohm? (beyond those already mentioned)

[TacHawkes]

Is this PR still alive? I would love to have polar axes in Makie...

[asinghvi17]

ah, I guess this kind of lost steam for a bit. I'll have to take a look at the code again.

[brendanjohnharris]

What needs to happen to get this PR moving? Unit tests and docs?
I've tested the basic functionality on the most recent version of Makie, it works fine after a few tweaks.
Happy to help however I can, to me polar plots are an essential part of a plotting package. Really surprised they've been missing from Makie for so long...

[jkrumbiegel]

Really surprised they've been missing from Makie for so long

They're just tricky to implement correctly given specific rendering requirements they have. For example, non-rectangular clipping masks, transformation-dependent rendering of primitives, handling of arbitrary axis types in certain code paths and more.

[sethaxen]

+1 for this. Currently needing polar axes and using TikZ. Would be nice to use Makie.

[ffreyer]

I've been playing around with this pr a bit, so I have some more thoughts:

I agree with Julius that $\theta$ should go. It's kind of annoying to type, might be confusing to some and also not that readable imo. Maybe we can just use angle/angular_something instead?

I'm wondering if it would be better to move grid lines and text to the transform aware scene. That would get rid off all the manual projections. For grid lines that should be no problem, but for text it might be a bit difficult to figure out the correct positioning. Though I think it's possible to derive the needed things from character boundingboxes (which should be mostly static), the scene area and limits.

Note that you can (up to maybe unavoidable equal length problems) also specify text alignment freely as a vector:

θtick_align[] = map(θtickpos) do p
    s, c = sincos(p[2])
    Point2f(0.5 - 0.5c, 0.5 - 0.5s)
end
...
text!(..., align = θtick_align)

How should zooming work for this? Should translating work? I guess like Axis is a static rectangle with varying ticks, this should be a static circle with varying ticks? And probably only zooming, no translation?

[asinghvi17]

How should zooming work for this? Should translating work? I guess like Axis is a static rectangle with varying ticks, this should be a static circle with varying ticks? And probably only zooming, no translation?

Well you could potentially have a circle segment between some theta and some r.

θ should go

We could go \theta -> t or so, I wouldn't mind that but do want to keep the semantic meaning.

[adigitoleo]

How should zooming work for this? Should translating work?

Just to provide a point of comparison, not necessarily saying we need to do the same, but matplotlib seems to only support zooming the way you suggest, with a static circular axis. See these docs and this screen capture:

polar_mpl.mp4

[ffreyer]

I'm wondering if it would be better to move grid lines and text to the transform aware scene. That would get rid off all the manual projections. For grid lines that should be no problem, but for text it might be a bit difficult to figure out the correct positioning. Though I think it's possible to derive the needed things from character boundingboxes (which should be mostly static), the scene area and limits.

Still need to implement tight tick spacing but I think it'll work.

Note that you can (up to maybe unavoidable equal length problems) also specify text alignment freely as a vector:

θtick_align[] = map(θtickpos) do p
    s, c = sincos(p[2])
    Point2f(0.5 - 0.5c, 0.5 - 0.5s)
end
...
text!(..., align = θtick_align)

Also figured out why this wasn't working. The alignment is pretty clean now, though the padding is still hard coded. Here's a screenshot with awkward tick steps and debug colors for clipping:

I can push my changes here, but I changed quite a lot so maybe I should just start a continuation pr instead? Otherwise it might get hard/confusing to work with this one if my approach doesn't work/isn't good.

How should zooming work for this? Should translating work?

Just to provide a point of comparison, not necessarily saying we need to do the same, but matplotlib seems to only support zooming the way you suggest, with a static circular axis. See these docs and this screen capture:

Yea I think that's probably the default we should go for too.

[SimonDanisch]

Merged in #2990