Skip to content
This repository has been archived by the owner on Jul 13, 2021. It is now read-only.

improve linestyles, add "-.-." and (:dash, :loose) variants #593

Merged
merged 12 commits into from Jan 12, 2021
Merged

improve linestyles, add "-.-." and (:dash, :loose) variants #593

merged 12 commits into from Jan 12, 2021

Conversation

greimel
Copy link
Contributor

@greimel greimel commented Jan 7, 2021

This improves CairoMakie. Adjusting the other two backends is still todo.

Before:

image

After:

image

using CairoMakie

fig, ax, plt = lines(1:2, 2:3, linestyle=:dash)
lines!(ax, 1:2, 3:4, linestyle=:dashdot)
lines!(ax, 1:2, 4:5, linestyle=:dot)
lines!(ax, 1:2, 5:6, linestyle=:dashdotdot)

lines!(ax, 2:3, 2:3, linestyle=:dash, linewidth=5)
lines!(ax, 2:3, 3:4, linestyle=:dashdot, linewidth=5)
lines!(ax, 2:3, 4:5, linestyle=:dot, linewidth=5)
lines!(ax, 2:3, 5:6, linestyle=:dashdotdot, linewidth=5)

fig

fixes MakieOrg/Makie.jl#807

src/conversions.jl Outdated Show resolved Hide resolved
@jkrumbiegel
Copy link
Member

ah also as a side note, you can leave out the ax parameter in the mutating functions if you plot into the last created axis :)

@greimel
Copy link
Contributor Author

greimel commented Jan 7, 2021

Now, it is more flexible. Not sure if part of this would better fit in PlotUtils or so?

fig, ax, plt = lines(1:2, 2:3, linestyle="-....", linewidth=2)
lines!(1:2, 3:4, linestyle=(".", :dense), linewidth=2)
lines!(1:2, 4:5, linestyle=("-", :loose), linewidth=2)
lines!(1:2, 5:6, linestyle=(:dash, :dense), linewidth=2)
lines!(1:2, 6:7, linestyle=(:dash, 0.0), linewidth=2)
fig

image

@greimel
Copy link
Contributor Author

greimel commented Jan 8, 2021

This is ready from my side.

CairoMakie works without changes.
GLMakie needs JuliaPlots/GLMakie.jl#157.
WGLMakie doesn't support linestyle, so no action needed.

I am looking forward to your review.

src/conversions.jl Outdated Show resolved Hide resolved
src/conversions.jl Outdated Show resolved Hide resolved
@greimel
Copy link
Contributor Author

greimel commented Jan 8, 2021

tests pass again :-)

@jkrumbiegel
Copy link
Member

jkrumbiegel commented Jan 8, 2021

nice, I think this is good to go then. What do you think @SimonDanisch

@greimel
Copy link
Contributor Author

greimel commented Jan 8, 2021

it's technically breaking because it messes up patterns in GLMakie. Compat in GLMakie should only be bumped after the companion PR is merged there.

@jkrumbiegel
Copy link
Member

I don't quite understand why glmakie's behavior is different in the first place, doesn't the abstractplotting api specify someplace that we're using linewidth-dependent dash sizes? so that's what glmakie should have done anyway.

@SimonDanisch
Copy link
Member

I'm pretty sure GLMakie was intended to have linewidth scaled patterns... Not sure what's going on there, it's some pretty gnarly code.

@greimel
Copy link
Contributor Author

greimel commented Jan 8, 2021

you are right. I was wrong. That's GLMakie before this PR.

[image deleted]

So, what this PR does is changing the patterns in GLMakie. So, the dashing is more stretched.

EDIT: staring longer at this, I am not sure if the dashes are perfectly scaled.

new picture:

image

both are dashed, but one looks more like dotted with large linewidth.

using GLMakie
lines(1:2, 1:2, linestyle=:dash, linewidth=1)
lines!(1:2, 3:4, linestyle=:dash, linewidth=20)

@greimel
Copy link
Contributor Author

greimel commented Jan 8, 2021

with this PR, but released GLMakie

image

not too bad

@SimonDanisch
Copy link
Member

So GLMakie actually doesn't need to change?

@jkrumbiegel
Copy link
Member

How is CairoMakie compared to that? It looks large

@greimel
Copy link
Contributor Author

greimel commented Jan 8, 2021

As you wish. The open PR makes the looks more consistent. I'll post a picture in a sec.

@jkrumbiegel
Copy link
Member

jkrumbiegel commented Jan 8, 2021

Yeah I saw that you added a scaling factor there, but it's not good practice to add scaling factors by eye-balling :) There should be a correct version no?

But as the AbstractPlotting side seems correct to me, I'd argue that GLMakie is buggy and adding this PR here is not breaking in that sense

@greimel
Copy link
Contributor Author

greimel commented Jan 8, 2021

CairoMakie:

lines-Cairo

GLMakie:

lines-GL

(both with the current PR)

@greimel
Copy link
Contributor Author

greimel commented Jan 8, 2021

Yeah I saw that you added a scaling factor there, but it's not good practice to add scaling factors by eye-balling :) There should be a correct version no?

I agree, see my comment: JuliaPlots/GLMakie.jl#157 (comment)

EDIT: I saw now that my comment over there missed some words to make sense. Now fixed, sorry.

@SimonDanisch
Copy link
Member

Besides the weird artifact:
image
GLMakie seems to look more correct? (Sorry for asking seemingly rhetorical questions, but I'm just dropping in and haven't read through everything)

@greimel
Copy link
Contributor Author

greimel commented Jan 8, 2021

What does look more correct to you? If you mean the dash length, that's a matter of taste. The CairoMakie output approximately matches the pre-PR state of GLMakie.

@jkrumbiegel
Copy link
Member

jkrumbiegel commented Jan 8, 2021

I don't think GLMakie is better, the dash pattern should be 3 times the linewidth, that is what you see in the CairoMakie example. In the glmakie the small dashes are much too long I'd say, and the big dashes less so but still.

But overall I think GLMakie is not so bad that we can't merge this and fix it more comprehensively later?

@SimonDanisch
Copy link
Member

Ah I see.. The pattern looked visually more similar, but yeah you're right.

@greimel
Copy link
Contributor Author

greimel commented Jan 8, 2021

@SimonDanisch compare tikZ line styles as given here: MakieOrg/Makie.jl#807 (comment)

@jkrumbiegel
Copy link
Member

jkrumbiegel commented Jan 8, 2021

Looking at this again, I think the multipliers we have here just don't look that good, even if cairomakie displays them correctly. In the tikz example the 3pt that we've translated into 3x multiplier is not a multiplier, and 3x is actually quite short for a dash as we can see above. So we should look at changing those multipliers to something more visually pleasing I guess? We should still stick to multipliers, as that makes conceptually more sense

@greimel
Copy link
Contributor Author

greimel commented Jan 8, 2021

In my opinion, they look visually pleasing for linewidth > 2. Also linewidth = 2 looks nicer than linewidth = 1.

Here some patterns with linewidth = 2.

lines-Cairo

I would rather make the default linewidth == 2.

see also the plot in #593 (comment) where linewidth was 2 as well (probably @jkrumbiegel didn't realize and that's why he didn't complain before ;-))

@jkrumbiegel
Copy link
Member

huh yeah that looks better, maybe for small linewidths the fixed gaps actually look better, and only once the linewidth gets large enough it starts looking cramped. well I guess it's ok then :) we can also just go with it for a while and if people complain too much we make adjustments

@jkrumbiegel jkrumbiegel changed the title Fix linestyle improve linestyles, add "-.-." and (:dash, :loose) variants Jan 12, 2021
@jkrumbiegel jkrumbiegel merged commit 7cf3b09 into JuliaPlots:master Jan 12, 2021
@greimel greimel deleted the linestyle branch January 12, 2021 14:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve linestyles (including a fix for broken line styles in CairoMakie)
3 participants