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

add linecaps (GLMakie, CairoMakie) #2536

Closed
wants to merge 54 commits into from
Closed

add linecaps (GLMakie, CairoMakie) #2536

wants to merge 54 commits into from

Conversation

ffreyer
Copy link
Collaborator

@ffreyer ffreyer commented Dec 30, 2022

Description

This pr aims to add linecaps to lines! and linesegments! in GLMakie.

Current prototype (second commit) should render caps for lines (triangular, rectangular or circular depending on the values in the shader.)

(Debug) Example for round line caps:

GLMakie.closeall(); display(GLMakie.Screen(), lines([1, 2, NaN, 4, 5], linewidth = 50))

Screenshot from 2022-12-30 10-51-45

TODO

  • (Fix this) Line caps and lines use different antialiasing algorithms which seem to give slightly different results: (this is true for triangle and circle too)
    image
  • Implementation for linesegments
  • add uniforms to adjust linecaps
  • add attributes for linecap
  • add per-vertex linewidth to lines! (closes allow for different line widths in lines #1541)
  • fix occasional bad triangle generation (with different linewidths the miter adjusted normals terminate triangles early sometimes, which results in a small kink in the line)
  • fix linestyle
  • add empty linecap (what Cairo calls butt, no extension past the start/end point of a line)
  • add linecap length and allow it to be negative to cut away from a line (This should help with keeping arrow markers and lines separate with arrows like Account for arrow head in arrow length #2472 suggests in 2D)
  • reuse line vertices for linecaps this isn't easily possible since different line caps use different uvs...
  • cleanup
  • update attribute passthrough in recipes
  • add example to lines/linesegments docs
  • update recipe docstrings
  • check blocks for overlapping lines
  • prototype update for 2d arrows

closes #1119, #2205

Also related: #871

Type of change

  • 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.

@@ -10,6 +10,7 @@ in vec4 f_color;
in vec2 f_uv;
in float f_thickness;
flat in uvec2 f_id;
flat in int f_type; // TODO bad for performance
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the comment in the scatter shader

uniform int shape; // shape is a uniform for now. Making them a in && using them for control flow is expected to kill performance

I assume we should find a way around this?

Copy link
Collaborator Author

@ffreyer ffreyer Dec 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did some benchmarking with

let 
    GLMakie.closeall()
    ps = [2 * rand(Point2f) .- 1 for _ in 1:2000]

    function myrenderloop(screen)
        yield()
        GLMakie.GLFW.SwapInterval(0)
        GLMakie.pollevents(screen)
        GLMakie.render_frame(screen)
        GLMakie.GLFW.SwapBuffers(GLMakie.to_native(screen))
        yield()

        @time for _ in 1:10_000
            GLMakie.pollevents(screen)
            GLMakie.render_frame(screen)
            GLMakie.GLFW.SwapBuffers(GLMakie.to_native(screen))
        end

        while GLMakie.isopen(screen) && !screen.stop_renderloop
            GLMakie.pollevents(screen)
            sleep(0.1)
            yield()
        end

        return
    end

    scene = Scene()
    linesegments!(scene, ps, linewidth = 10)
    screen = GLMakie.Screen(renderloop = myrenderloop)
    display(screen, scene)
end

I get the same ~5% CI got for this branch vs master.

I tried hacking together a two pass version which does the old lines in the first pass and caps in the second pass. This way we can swap out the flat in int f_type for uniform int linecap and uniform int render_pass. This ends up being about 10% slower than master though, doing both passes. Doing just the line pass is 3-4% slower.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's ok for linecaps! Can we optionally disable it and get the 5% back for high perf use cases? We could also think about having a different recipe for that though 🤷 I think the default should make good lines, and performance should be the special case!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reducing the number of options in the fragment shader (5e206a9) helped a bit ... I think. I'm never sure with gpu benchmarking. When I tested it I got a bunch of benchmarks that were ~2% slower than master and then a bunch up to 5% slower 🤷

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran them again without background processes and I'm getting ~2% now (812ff3f)

@MakieBot
Copy link
Collaborator

MakieBot commented Dec 30, 2022

Compile Times benchmark

Note, that these numbers may fluctuate on the CI servers, so take them with a grain of salt. All benchmark results are based on the mean time and negative percent mean faster than the base branch. Note, that GLMakie + WGLMakie run on an emulated GPU, so the runtime benchmark is much slower. Results are from running:

using_time = @ctime using Backend
# Compile time
create_time = @ctime fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @ctime Makie.colorbuffer(display(fig))
# Runtime
create_time = @benchmark fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @benchmark Makie.colorbuffer(display(fig))
using create display create display
GLMakie 28.71s (28.59, 29.16) 0.21+- 16.43s (16.22, 16.96) 0.25+- 15.28s (15.13, 15.71) 0.20+- 9.50ms (9.37, 9.57) 0.07+- 119.97ms (118.67, 121.79) 1.15+-
master 29.00s (28.70, 29.28) 0.23+- 16.31s (16.09, 16.65) 0.19+- 15.18s (15.07, 15.41) 0.15+- 9.29ms (9.19, 9.37) 0.07+- 112.24ms (110.95, 113.13) 0.88+-
evaluation -1.01%, -0.29s faster ✓ (-1.30d, 0.03p, 0.22std) +0.74%, 0.12s invariant (0.56d, 0.32p, 0.22std) +0.65%, 0.1s invariant (0.56d, 0.32p, 0.17std) +2.17%, 0.21ms slower X (2.89d, 0.00p, 0.07std) +6.44%, 7.73ms slower❌ (7.55d, 0.00p, 1.01std)
CairoMakie 25.32s (25.21, 25.60) 0.14+- 16.59s (16.36, 17.01) 0.24+- 2.65s (2.60, 2.72) 0.04+- 9.30ms (9.14, 9.49) 0.14+- 4.17ms (4.10, 4.28) 0.07+-
master 25.62s (25.41, 25.88) 0.16+- 16.50s (16.20, 16.83) 0.22+- 2.56s (2.50, 2.60) 0.04+- 9.04ms (8.96, 9.14) 0.06+- 4.10ms (3.92, 4.19) 0.09+-
evaluation -1.19%, -0.3s faster ✓ (-2.00d, 0.00p, 0.15std) +0.51%, 0.08s invariant (0.38d, 0.50p, 0.23std) +3.42%, 0.09s slower X (2.31d, 0.00p, 0.04std) +2.87%, 0.27ms slower X (2.47d, 0.00p, 0.10std) +1.87%, 0.08ms invariant (0.98d, 0.09p, 0.08std)
WGLMakie 36.40s (36.24, 36.61) 0.12+- 19.96s (19.78, 20.30) 0.18+- 21.24s (20.83, 21.71) 0.27+- 12.74ms (12.23, 13.22) 0.41+- 1.71s (1.65, 1.77) 0.05+-
master 36.76s (36.58, 37.03) 0.16+- 19.88s (19.73, 20.08) 0.11+- 21.13s (20.91, 21.51) 0.22+- 12.75ms (12.29, 13.16) 0.31+- 1.73s (1.66, 1.80) 0.05+-
evaluation -0.97%, -0.35s faster ✓ (-2.46d, 0.00p, 0.14std) +0.39%, 0.08s invariant (0.51d, 0.36p, 0.15std) +0.54%, 0.11s invariant (0.47d, 0.40p, 0.24std) -0.06%, -0.01ms invariant (-0.02d, 0.97p, 0.36std) -1.24%, -0.02s invariant (-0.44d, 0.42p, 0.05std)

@@ -269,7 +270,7 @@ function draw_atomic(screen::Screen, scene::Scene, @nospecialize(x::Lines))
data[:pattern] = ls
else
linewidth = gl_attributes[:thickness]
data[:pattern] = ls .* (to_value(linewidth) * 0.25)
data[:pattern] = ls * _mean(to_value(linewidth))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took this as somewhat of a shortcut for now, because linestyle needs some cleanup anyway.

Example with variable linewidth:

GLMakie.closeall()
widths = [57.37044620677474, 27.847264587380923, 56.663525479541775, 47.302310563997985, 56.36164168328704, 43.014590186214285, 58.832167079498525, 27.208798997411307, 17.12575358453462, 45.95171757911098, 26.771344098755126, 35.85649823581235, 53.08232474276624, 27.217846025535966, 43.98443255191129, 37.389819316502695, 18.07005596531466, 59.569472267387056, 50.919270815270814, 36.74914882840677, 53.08834081120422, 45.44455789568474, 40.147375157549135, 22.166337026901253, 50.53203686576642, 15.238811525483367, 12.780634172815944, 12.980713770063876, 28.175981476856755, 37.542112019663485]
ys = [0.3672275443344566, 0.13769475910008522, 0.22445059716611304, 0.5883310148108587, 0.7893622240334566, 0.3129809823712394, 0.953606981656488, 0.2770419071274035, 0.3048558242752848, 0.6782285082882588, 0.5447836889282002, 0.19905515156197973, 0.19770277191975638, 0.537590947032379, 0.28027667604204765, 0.8287252657638743, 0.9336781225747905, 0.1371941303349402, 0.5973028266375866, 0.3561025405662749, 0.43500848119792546, 0.36410392908891054, 0.9117308440489152, 0.8008057243055526, 0.2349037873519133, 0.21294938231466431, 0.8334978136186971, 0.5097696792342837, 0.21488349759400982, 0.3434422610208452]
fig = Figure()
ax = Axis(fig[1, 1])
# p = linesegments!(ax, ys, linewidth = widths./10, linecap = 4, linecap_length = widths./10, color = :red)
# p = linesegments!(ax, ys, linewidth = widths./10, linecap = 4, linecap_length = widths./10, linestyle = :dot)
p = lines!(ax, ys, linewidth = widths./10, linecap = 0, linecap_length = widths, color = :red)
p = lines!(ax, ys, linewidth = widths./10, linecap = 0, linecap_length = widths, linestyle = :dash)
screen = display(GLMakie.Screen(), fig)

produces
Screenshot from 2022-12-31 17-07-54
Screenshot from 2022-12-31 17-08-04

and pixel perfect results with constant linewidth (tested explicitly for linewidth = 50 on a straight line with lines and linesegments looks the same).

There is however some artifacting with :dash that got more frequent but isn't new. From master:

Screenshot from 2022-12-31 17-19-36

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, I also changed the scaling with linewidth in lines! (not here but in the shader) which makes :dash shorter notably at large linestyles. It's now 3lw on, 3lw off like the pattern requests. master has weird scaling, maybe quadratic? For linewidth 50, it seems to be about 40.1lw on, 40.1lw off. For linewidth 10 about 10.5lw on 10.5lw off

lines([0, 0], linewidth = 50, linestyle = :dash)

Screenshot from 2022-12-31 17-26-52

@ffreyer
Copy link
Collaborator Author

ffreyer commented Dec 31, 2022

Refimg test I added for linecaps:

Screenshot from 2022-12-31 18-55-16

This shows (nothing, :square, :round), default for lines, then (nothing, :square, :round) for linesegments. New default is :square like in PyPlot.jl

@ffreyer ffreyer changed the title add linecaps to GLMakie add linecaps (GLMakie, CairoMakie) Dec 31, 2022
@ffreyer ffreyer marked this pull request as ready for review December 31, 2022 21:58
@github-actions
Copy link
Contributor

Missing reference images

Found 1 new images without existing references.
Upload new reference images before merging this PR.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 1, 2023

Missing reference images

Found 1 new images without existing references.
Upload new reference images before merging this PR.

@github-actions
Copy link
Contributor

Missing reference images

Found 1 new images without existing references.
Upload new reference images before merging this PR.

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jan 18, 2023

https://discourse.julialang.org/t/y-axis-in-cairomakie-jl-not-vertical/93165 made me realize that a pixel offset attribute for lines is probably less niche than I first thought, so I'm reworking linecap_length into a more general length_offset.

@jkrumbiegel This would allow you to nudge the start and endpoints of a line around by some number of pixel. So for an axis you could make ticks start 1 frame thickness later, and shorten/lengthen some of the frame lines by one thickness to avoid overlap. Or for 2d arrows you could use this to stop the line before the marker.

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jan 18, 2023

I added length_offset for lines and linesegments across the board with this refimg:
Screenshot from 2023-01-18 19-17-05
WGLMakie support is there but the refimg test doesn't match because WGLMakie can't handle linecaps and doesn't have line joins

@github-actions
Copy link
Contributor

Missing reference images

Found 2 new images without existing references.
Upload new reference images before merging this PR.

@github-actions
Copy link
Contributor

Missing reference images

Found 2 new images without existing references.
Upload new reference images before merging this PR.

@github-actions
Copy link
Contributor

Missing reference images

Found 2 new images without existing references.
Upload new reference images before merging this PR.

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jan 24, 2023

GLMakie display: +10.76%, 19.38ms slower ❌ (4.54d, 0.00p, 4.17std)

This seems to be mostly display time, not render time. Running render-N-frames check returns (1.7% min, 8.5% max, 1.8% mean, 1.3% median) more time spend on rendering here. (I'm now running 30 timings at 1k frames and filter spikes > 20% mean.) I guess this spike in display time isn't really that surprising with me adding another buffer to upload.

Code ```julia let for _ in 1:30 GLMakie.closeall() ps = [2 * rand(Point2f) .- 1 for _ in 1:2000]
    function myrenderloop(screen)
        yield()
        GLMakie.GLFW.SwapInterval(0)
        GLMakie.pollevents(screen)
        GLMakie.render_frame(screen)
        GLMakie.GLFW.SwapBuffers(GLMakie.to_native(screen))
        yield()

        @time for _ in 1:1_000
            GLMakie.pollevents(screen)
            GLMakie.render_frame(screen)
            GLMakie.GLFW.SwapBuffers(GLMakie.to_native(screen))
        end

        while GLMakie.isopen(screen) && !screen.stop_renderloop
            GLMakie.pollevents(screen)
            sleep(0.1)
            yield()
        end

        return
    end

    scene = Scene()
    linesegments!(scene, ps, linewidth = 10, linecap = nothing)
    screen = GLMakie.Screen(renderloop = myrenderloop)
    display(screen, scene)
    GC.gc()
    sleep(1)
end

end

using Statistics

function remove_spikes(ts, threshold = 0.2)
m = mean(ts)
return filter(t -> abs(t - m) < threshold * m, ts)
end

</details>

@github-actions
Copy link
Contributor

Missing reference images

Found 2 new images without existing references.
Upload new reference images before merging this PR.

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jan 30, 2023

In Cairo linecaps are applied to dashes as well. It's set up to take space away from the "off" part of dashes. I.e. (with pattern being the vector of float we generate from a linestyle)

  cap              cap      cap
 |---|------------|---|    |---|------
     ^            ^            ^
 pattern[1]   pattern[2]   pattern[3]

I've adjusted the linestyle lengths so that caps take space from the "on" part of a dash/dot instead of the "off" part. So it's not:

      cap      cap              cap
     |---|----|---|            |---|--
     ^            ^            ^
 pattern[1]   pattern[2]   pattern[3]

@github-actions
Copy link
Contributor

Missing reference images

Found 2 new images without existing references.
Upload new reference images before merging this PR.

@ffreyer ffreyer mentioned this pull request Feb 10, 2023
7 tasks
@ffreyer ffreyer marked this pull request as draft February 25, 2023 12:51
@ffreyer
Copy link
Collaborator Author

ffreyer commented Feb 25, 2023

This will interfere heavily with #2666 and I think it's better to redo this than #2666 so I'm making this a draft for now

@SimonDanisch
Copy link
Member

What do we do with this pr after #2666 got merged?

@ffreyer
Copy link
Collaborator Author

ffreyer commented Mar 31, 2023

Probably kill it and restart it. Most of this would need to be redone either way.

I've also been thinking it would be good to match Cairo linecaps in GLMakie. They put them on each dash/dot, this pr only did lin start/end.

@SimonDanisch
Copy link
Member

Ok, makes sense! I'll close it then ;)

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.

allow for different line widths in lines Linesegments - Missing corners in segment drawing
3 participants