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

Fix bezier marker stroke #2961

Merged
merged 12 commits into from
Jul 3, 2023
Merged

Fix bezier marker stroke #2961

merged 12 commits into from
Jul 3, 2023

Conversation

ffreyer
Copy link
Collaborator

@ffreyer ffreyer commented May 16, 2023

Description

Without the ClosePath stroke outlines are incomplete in CairoMakie.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

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.

@MakieBot
Copy link
Collaborator

MakieBot commented May 16, 2023

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 13.46s (13.31, 13.64) 0.11+- 1.31s (1.28, 1.34) 0.02+- 939.27ms (915.48, 987.84) 23.58+- 13.45ms (13.33, 13.55) 0.08+- 104.60ms (103.27, 106.23) 1.10+-
master 13.45s (13.27, 13.58) 0.12+- 1.32s (1.27, 1.36) 0.03+- 940.31ms (889.57, 959.17) 25.13+- 13.44ms (13.18, 13.65) 0.15+- 105.09ms (102.48, 108.04) 2.01+-
evaluation +0.12%, 0.02s invariant (0.14d, 0.80p, 0.11std) -0.54%, -0.01s invariant (-0.26d, 0.64p, 0.03std) -0.11%, -1.04ms invariant (-0.04d, 0.94p, 24.36std) +0.04%, 0.0ms invariant (0.04d, 0.94p, 0.12std) -0.46%, -0.48ms invariant (-0.30d, 0.59p, 1.56std)
CairoMakie 13.75s (13.56, 14.16) 0.21+- 1.64s (1.60, 1.68) 0.03+- 351.29ms (340.52, 373.00) 12.27+- 16.16ms (15.98, 16.36) 0.13+- 8.56ms (8.40, 8.81) 0.13+-
master 14.05s (13.78, 14.65) 0.29+- 1.64s (1.56, 1.74) 0.06+- 400.83ms (375.99, 465.56) 31.06+- 16.46ms (16.09, 16.96) 0.34+- 8.59ms (8.39, 8.71) 0.12+-
evaluation -2.19%, -0.3s faster ✓ (-1.19d, 0.05p, 0.25std) -0.11%, -0.0s invariant (-0.04d, 0.95p, 0.05std) -14.10%, -49.54ms faster✅ (-2.10d, 0.00p, 21.67std) -1.86%, -0.3ms invariant (-1.18d, 0.06p, 0.23std) -0.37%, -0.03ms invariant (-0.25d, 0.65p, 0.13std)
WGLMakie 14.10s (13.85, 14.31) 0.14+- 1.39s (1.32, 1.46) 0.05+- 12.74s (12.56, 12.91) 0.15+- 16.87ms (15.39, 22.50) 2.53+- 946.35ms (902.22, 1076.44) 59.46+-
master 14.25s (14.00, 14.45) 0.16+- 1.44s (1.40, 1.48) 0.03+- 12.75s (12.51, 13.08) 0.18+- 16.08ms (14.41, 17.75) 1.32+- 916.47ms (874.04, 936.49) 21.68+-
evaluation -1.02%, -0.14s invariant (-0.96d, 0.10p, 0.15std) -3.69%, -0.05s faster ✓ (-1.27d, 0.04p, 0.04std) -0.05%, -0.01s invariant (-0.04d, 0.94p, 0.17std) +4.63%, 0.78ms invariant (0.39d, 0.49p, 1.93std) +3.16%, 29.88ms invariant (0.67d, 0.25p, 40.57std)

@ffreyer
Copy link
Collaborator Author

ffreyer commented May 16, 2023

I added another refimg that includes stroke. It seems like the signed distance field derived for GLMakie is also kind of messed up. Looks like the what's happening with 2D scaling in #2950... Are we introducing that along the way? (Also the sizes differ)

CairoMakie:
Screenshot from 2023-05-16 19-49-29

GLMakie:
Screenshot from 2023-05-16 19-49-36

@jkrumbiegel
Copy link
Member

Huh didn't the sizes match at one point in the past?

@ffreyer
Copy link
Collaborator Author

ffreyer commented May 16, 2023

I don't think we checked with stroke. I think I've seen that with other stroked markers though. Or maybe lines.

On the bad stroke in GLMakie - this is happening because the sdf is generated with a different aspect ratio here

path_unit_square = fit_to_unit_square(path_replaced, false)

which results in different width and height scaling here
path_width = widths(Makie.bbox(path))

@ffreyer
Copy link
Collaborator Author

ffreyer commented May 17, 2023

This will need a new texture atlas. Should I just increment this?

const SERIALIZATION_FORMAT_VERSION = "v1"

@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 May 17, 2023

With e86aee6 (Circle scatter to mark center and debug boxes turned on in distance_shape.frag)

Screenshot from 2023-05-17 19-16-53

With 245fea7:

Screenshot from 2023-05-17 20-38-11

@github-actions
Copy link
Contributor

Missing reference images

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

@jkrumbiegel
Copy link
Member

Should I see a difference between them? :)

@ffreyer
Copy link
Collaborator Author

ffreyer commented May 18, 2023

GLMakie master: vline has blurry edge and hline has shrunken stroke/glow
patch 1: fix scaling of stroke/glow/AA area
patch 2: reduce size of quad (gray background in image, also size is texture atlas)

@jkrumbiegel
Copy link
Member

ahh now I see it, nice!

@SimonDanisch
Copy link
Member

This will need a new texture atlas. Should I just increment this?

Yes, I think that should be enough!

@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

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.

@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

Missing reference images

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

@SimonDanisch SimonDanisch merged commit a82e8a5 into master Jul 3, 2023
14 checks passed
@SimonDanisch SimonDanisch deleted the ff/fix_bezier_stroke branch July 3, 2023 08:51
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

4 participants