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

Change text signature to positions, text = text #2020

Merged
merged 33 commits into from Jun 17, 2022
Merged

Conversation

jkrumbiegel
Copy link
Collaborator

@jkrumbiegel jkrumbiegel commented Jun 4, 2022

Description

Fixes #1468, #1138

The text function has an unusual signature compared to the other plotting functions, it takes the string(s) as its positional arg and the positions as a keyword arg. That means positions have to be passed in a specific way (usually as Point or Vector{Point}) because the conversion pipeline that usually applies for PointBased does not apply to attributes. This additionally causes problems for AlgebraOfGraphics, as the expected mapping(x, y, text = text) doesn't work, instead one has to construct a vector of points manually.

Another side effect is that a text plot initialized with a String cannot be changed to a LaTeXString later, as the signature of Text is fixed to String at that point and any subsequent LaTeXString will be converted to "$content$".

This PR changes the default args for text such that it's called with text(positions, text = strings). It has the conversion trait PointBased now so all expected signatures for passing x and y work. All the old signatures still exist and reroute to that new definition. The last Text in the chain is still the one receiving a vector of GlyphCollections, so the backends hopefully don't need to change much (GLMakie has some problem with gl_convert right now).

TODO

  • Add new signature with PointBased conversion trait
  • Reroute old signatures
  • Check that all Blocks use the new format internally (only Axis so far)
  • Rewrite docs
  • Check that interactive updating still works
  • Line elements from MathTeXEngine are currently broken because they were implemented with a separate LineSegments plot object and it's not clear how that should work when switching back and forth between String and LaTeXString args. An option could be to render lines as stretched line chars instead, which would mean that no separate LineSegments shenanigans would be needed. (cc @Kolaru)

Type of change

Delete options that do not apply:

  • New feature (non-breaking change which adds functionality) It should hopefully be non-breaking as the old signatures reroute.

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.

@Kolaru
Copy link
Contributor

Kolaru commented Jun 5, 2022

  • An option could be to render lines as stretched line chars instead, which would mean that no separate LineSegments shenanigans would be needed. (cc @Kolaru)

Wouldn't a string that mix stretched and unstreched characters cause the same problem (i.e. mixing stuff that were not planned to be mixed)? If not I can think about how to do that properly.

@ffreyer
Copy link
Collaborator

ffreyer commented Jun 5, 2022

We have scale, rotation and translation as part of the per character interface so stretching is fine. The bad part is that it's currently using two plot types.

Iirc unicode also has a section of characters that are undefined and meant to be overwritten for stuff like this. I think with how GLMakie works we can even reproduce lines one to one as a character.

@jkrumbiegel
Copy link
Collaborator Author

The way it works right now is that a string including stylistic attributes is translated to a GlyphCollection and that is then rendered. So it's in principle fine to put a long underscore character in there instead of the separate line elements you're currently using. That would just simplify the implementation but I can imagine that the long underscore could look blurry at the edges in glmakie, due to the bitmap

@ffreyer
Copy link
Collaborator

ffreyer commented Jun 6, 2022

Iirc unicode also has a section of characters that are undefined and meant to be overwritten for stuff like this. I think with how GLMakie works we can even reproduce lines one to one as a character.

I played around a bit with this now. The characters I was thinking of are private use character: https://www.unicode.org/faq/private_use.html. I guess it's not generally safe to use them like this, but we can (and should anyway) restrict ourselves to a font that doesn't use them.

My basic idea is to manually create signed distance fields for vline and hline and insert them into the texture atlas linked against characters in the private use section. During latex layouting we can then replace VLine and HLine with the corresponding characters, set up the metrics and be more or less done with it on the GLMakie side.

Without thinking carefully if all my scales make sense I get this:

scene = Scene()
text!(scene, L"\frac{1}{2}", textsize = 200, align = (:center, :center))

Screenshot from 2022-06-06 20-54-48

The bit of fuzz on the left is texture bleed I believe. I think that just needs a pixel of padding to be fixed. The size of the sdf in the atlas is (2 * GLYPH_PADDING[] + 1)^2, so as little as possible.

@jkrumbiegel
Copy link
Collaborator Author

I tried out just using the underscore character for this, calculating how to stretch it so that it can replace the HLine. Works pretty well and would remove the need for LineSegments. The implementation is very ad-hoc right now because I just wanted to get an example quickly.

grafik

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

@MakieOrg MakieOrg deleted a comment from MakieBot Jun 17, 2022
@github-actions
Copy link
Contributor

Missing reference images

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

@MakieBot
Copy link
Collaborator

MakieBot commented Jun 17, 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 master. 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.82s (13.49, 14.37) 0.37+- 30.69s (28.74, 32.28) 1.39+- 24.79s (22.17, 27.67) 1.84+- 32.73ms (32.17, 33.79) 0.57+- 115.01ms (112.48, 121.44) 3.03+-
master 13.87s (13.10, 14.68) 0.55+- 30.37s (28.75, 31.31) 0.81+- 26.02s (24.88, 27.83) 1.09+- 29.85ms (29.05, 31.07) 0.68+- 107.43ms (105.39, 113.26) 2.65+-
evaluation -0.34%, -0.05s invariant (-0.10d, 0.85p, 0.46std) +1.06%, 0.33s invariant (0.29d, 0.60p, 1.10std) -4.96%, -1.23s invariant (-0.81d, 0.16p, 1.47std) +8.79%, 2.88ms worse❌ (4.58d, 0.00p, 0.63std) +6.59%, 7.58ms worse❌ (2.66d, 0.00p, 2.84std)
CairoMakie 8.62s (8.60, 8.66) 0.02+- 22.79s (22.71, 22.85) 0.05+- 3.42s (3.41, 3.43) 0.01+- 17.80ms (17.69, 17.95) 0.09+- 25.20ms (25.05, 25.34) 0.10+-
master 8.70s (8.68, 8.72) 0.02+- 22.17s (22.08, 22.26) 0.05+- 3.50s (3.49, 3.52) 0.01+- 16.28ms (16.20, 16.45) 0.10+- 25.12ms (25.08, 25.22) 0.05+-
evaluation -0.94%, -0.08s invariant (-4.08d, 0.00p, 0.02std) +2.72%, 0.62s invariant (12.41d, 0.00p, 0.05std) -2.18%, -0.07s invariant (-6.42d, 0.00p, 0.01std) +8.56%, 1.52ms worse❌ (16.33d, 0.00p, 0.09std) +0.30%, 0.08ms invariant (1.00d, 0.09p, 0.07std)
WGLMakie 10.54s (10.44, 10.67) 0.09+- 31.42s (30.99, 32.17) 0.45+- 60.93s (59.77, 63.67) 1.40+- 22.39ms (21.35, 23.45) 0.67+- 1.65s (1.62, 1.67) 0.02+-
master 10.69s (10.57, 10.91) 0.11+- 31.34s (30.82, 32.31) 0.49+- 58.11s (57.15, 59.81) 0.87+- 19.58ms (19.00, 19.95) 0.33+- 1.63s (1.61, 1.65) 0.01+-
evaluation -1.37%, -0.14s invariant (-1.44d, 0.02p, 0.10std) +0.26%, 0.08s invariant (0.17d, 0.75p, 0.47std) +4.63%, 2.82s invariant (2.42d, 0.00p, 1.14std) +12.52%, 2.8ms worse❌ (5.33d, 0.00p, 0.50std) +0.98%, 0.02s invariant (0.98d, 0.09p, 0.02std)

@github-actions
Copy link
Contributor

Missing reference images

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

@jkrumbiegel jkrumbiegel merged commit ce1a809 into master Jun 17, 2022
@jkrumbiegel jkrumbiegel deleted the jk/text-refactor branch June 17, 2022 13:15
@ffreyer
Copy link
Collaborator

ffreyer commented Jun 17, 2022

Was there a problem with using underscores as lines?

@jkrumbiegel
Copy link
Collaborator Author

Yes they didn't render well in GLMakie at small sizes..

@ffreyer ffreyer mentioned this pull request Jul 18, 2022
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.

axis.xlabel or axis.ylabel with LaTeX strings not working
5 participants