Skip to content

Conversation

@ffreyer
Copy link
Collaborator

@ffreyer ffreyer commented Mar 19, 2025

Description

Prototype for #4874

This adds textlabel!() which draws some text input and renders an automatically resized shape behind it.

f,a,p = textlabel(Point2f(0), "Boxed", background_color = :lightblue, background_strokewidth = 0)

p = textlabel!(Point2f(1, 0), "Ellipse", align = (:center, :center), fontsize = 30,
    shape = Circle(Point2f(0), sqrt(2f0)), shape_limits = Rect2f(-1,-1,2,2),
    background_color = :lightgray, background_strokecolor = :black, background_strokewidth = 2)
f

grafik

TODO:

  • simplify tooltip by reusing textlabel
  • clean up reactive stuff
  • process multiple glyphcollection and multiple text plots
  • consider stroke & glow in boundingbox
  • check render order across backends & verify that there is no z-fighting
  • probably rename marker to background or something else?
  • tests
  • docs
  • argument cleanup
  • try staggering position z's to fix overlap order in GL backends
  • attribute cleanup

Questions:

  • Should this work in 3D with markerspace = :data?
    • Not in the first iteration
  • Should this work with per-plot (and per-glyphcollection?) backgrounds
    • Refactored to not use a text plot as an input
  • Should the reference rect be statically sized? I.e. should 0..1 or -1..1 correspond to limits of the boundingbox by default (like BezierPath markers), or should the boundingbox of the background be scaled to match?
    • Yes. Currently using 0..1 x 0..1 so the calculated translation and scale can be interpreted as origin and size

Type of change

Delete options that do not apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • Added an entry in CHANGELOG.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.

@github-project-automation github-project-automation bot moved this to Work in progress in PR review Mar 19, 2025
@MakieBot
Copy link
Collaborator

MakieBot commented Mar 19, 2025

Benchmark Results

SHA: 6036fe0f7dfb2e629864b05eb6ba71892ac73210

Warning

These results are subject to substantial noise because GitHub's CI runs on shared machines that are not ideally suited for benchmarking.

GLMakie
CairoMakie
WGLMakie

@jkrumbiegel
Copy link
Member

The most common shape will be rounded rect, which you can't stretch without getting skewed corners. So meshscatter won't work unless we have a screen space aware rounded rect shader on top of a rectangle or something.

@ffreyer
Copy link
Collaborator Author

ffreyer commented Mar 19, 2025

Ugh that's true. So the low tech solution is poly then, probably with a

struct RoundedRect{N, T} <: GeometryPrimitive{N, T}
    origin::Point{N, T}
    widths::Vec{N, T}
    cornerradius::T
end

in GeometryBasics so actually have something to pass.


I've been thinking about extending scatter with some "complex" shapes before, i.e. with sdfs defined in shaders like with Rect and Circle. We could pass up to 4 parameters through uv_offset_width without adding more uniforms (does that even matter much?). RoundedRect just needs a corner radius as far as I can tell, so it would fit the bill. I assume this would end up being higher quality and better performance, so maybe it's worth spending time on for this?

Iirc I originally thought about either for 2D arrows or tooltips. 2D arrows should be doable with 2 parameters - head length and tail width. Having this would again remove a lines(egments) plot, fix overlap issues for semi-transparent arrows and probably simplify the whole scaling business. Tooltips are more complicated with the little triangle, but I think we still only need a side, a position fraction and triangle size.

For CairoMakie this could effectively just lower to BezierPaths. All the data for the shape generation needs to be there anyway.

This could also be generalized drastically by adding a render pipeline step that accumulates sdfs instead of immediately turning them into colors. We would probably want #4689 for this though, and compatibility with CairoMakie is questionable...

Comment on lines 136 to 141
# FXAA generates artifacts if:
# mesh has fxaa = true and text/lines has false
# and the luma/brightness difference between text_color/strokecolor and background_color is low enough

# The defaults use fxaa = false to remove artifacting and use an opaque
# stroke to hide the pixelated border.
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'm so annoyed by this...

Luma/brightness dependence
Screenshot 2025-03-20 161733

With how fxaa works atm I'm pretty sure there is no way to avoid these issues:

parent = Scene(backgroundcolor = :blue, size = (120, 310))

# No fxaa involved for reference
scene = Scene(parent, viewport = Rect2f(10, 250, 100, 50), backgroundcolor = :lightgray, clear = true)
text!(scene, "Test 1", fontsize = 30, align = (:center, :center))
scene

verts = Makie.roundedrectvertices(Rect2f(-0.9,-0.9,1.8,1.8), 0.5, 10)
m = Makie.poly_convert(verts)

# default fxaa settings create a fuzzy font (and lines, scatter)
scene = Scene(parent, viewport = Rect2f(10, 190, 100, 50))
mesh!(scene, m, color = :lightgray, shading = NoShading, fxaa = true)
text!(scene, "Test 2", fontsize = 30, align = (:center, :center), fxaa = false)

# no fxaa fixes fuzziness but remove aliasing on mesh
scene = Scene(parent, viewport = Rect2f(10, 130, 100, 50))
mesh!(scene, m, color = :lightgray, shading = NoShading, fxaa = false)
text!(scene, "Test 3", fontsize = 30, align = (:center, :center), fxaa = false)

# fxaa = true reduces text (scatter, lines) quality
scene = Scene(parent, viewport = Rect2f(10, 70, 100, 50))
mesh!(scene, m, color = :lightgray, shading = NoShading, fxaa = true)
text!(scene, "Test 4", fontsize = 30, align = (:center, :center), fxaa = true)

# hiding aliased border doesn't work with transparency
scene = Scene(parent, viewport = Rect2f(10, 10, 100, 50))
mesh!(scene, m, color = (:lightgray, 0.5), shading = NoShading, fxaa = false)
lines!(scene, [verts..., verts[1]], color = (:lightgray, 0.5), fxaa = false, linewidth = 4)
text!(scene, "Test 5", fontsize = 30, align = (:center, :center), fxaa = false)

parent

image

This could be fixed by using scatter or by reworking the render pipeline. Iirc I tried adjusting the pipeline in #4689 and had some test failures with it.

Copy link
Member

@SimonDanisch SimonDanisch left a comment

Choose a reason for hiding this comment

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

Attributes

  • I'd say one primitive should own color/strokecolor/etc...Maybe easiest tofollow CSS:

    • strokecolor is for the stroke around it
    • color means text_color
    • background_color (already correct)
    • background_linestyle -> linestyle (border_style?)
    • pad -> padding
    • etc... I could also just do a renaming in the end
  • We should expand e.g. padding=2 -> (2, 2, 2, 2)

  • Should we follow text and have it textlabel(positions; text=strings) ?

Styling

Would be nice if this can look nice by default. Personally, I'd go for white bgcolor, rounded edges and some padding by default. But can also leave this up for a final review

Features

This has to work for 3d axes with draw_on_top, since that's the main use case I see for this and one reason why it's hard for a user to implement something like this.

Bugs

begin 
    f, ax, pl = scatter(rand(Point3f, 10), axis=(; type=Axis3))
    pos = Point3f(0.5)
    textlabel!(ax, pos, "MatrixSpace", pad=(10, 10, 10, 10), draw_on_top=false, cornerradius=2)
    f
end

The stroke seems to be cut off:
image

@aplavin
Copy link
Contributor

aplavin commented Mar 21, 2025

I will be so happy to remove/deprecate the textwithbox recipe from MakieExtra when this lands! :)
For me, the main (and the only) usecase is drawing a semi-transparent box behind the text to make it legible on different backgrounds:
textwithbox!((50, 50), text="Some text here", poly=(;color=(:white, 0.5)))
image
This would be textlabel!((50, 50), text="Some text here", backgroundcolor=(:white, 0.5)) with this PR, right?

Btw, in textwithbox the poly argument is directly forwarded to a poly! call, supporting any poly attribute automatically. I wonder if this approach could be an option here as well?

@ffreyer
Copy link
Collaborator Author

ffreyer commented Mar 22, 2025

Attributes

* I'd say one primitive should own color/strokecolor/etc...Maybe easiest tofollow CSS:
  
  * strokecolor is for the stroke around it
  * color means text_color
  * background_color (already correct)
  * background_linestyle -> linestyle  (border_style?)
  * pad -> padding
  * etc... I could also just do a renaming in the end

Yea that's probably easier than going back and forth.

* We should expand e.g. padding=2 -> (2, 2, 2, 2)

Done

Features

This has to work for 3d axes with draw_on_top, since that's the main use case I see for this and one reason why it's hard for a user to implement something like this.

Done by pre-transforming.

Bugs

begin 
    f, ax, pl = scatter(rand(Point3f, 10), axis=(; type=Axis3))
    pos = Point3f(0.5)
    textlabel!(ax, pos, "MatrixSpace", pad=(10, 10, 10, 10), draw_on_top=false, cornerradius=2)
    f
end

The stroke seems to be cut off

Because clip planes are also inherited if space != :data. Fixed that by not using automatic for now. Maybe we should handle that like transformation, i.e. only inherit them if parent and child space match?

This would be textlabel!((50, 50), text="Some text here", backgroundcolor=(:white, 0.5)) with this PR, right?

Right now it would be textlabel!((50, 50), "Some text here", background_color=(:white, 0.5)) but the syntax will probably change a bit.

Btw, in textwithbox the poly argument is directly forwarded to a poly! call, supporting any poly attribute automatically. I wonder if this approach could be an option here as well?

I'm not a fan of that. I think generally a recipe adds more context to what passthrough attributes mean which is good to add to their names. E.g. background_color is pretty clear on its own while poly = (color = ..., ) requires you to understand that the background is treated as a poly. There also tend to be attributes that the recipe should handle internally which may seem settable like this.

@ffreyer ffreyer moved this from Work in progress to Ready to review in PR review Apr 7, 2025
@ffreyer
Copy link
Collaborator Author

ffreyer commented Apr 7, 2025

This is failing the Tooltip test due to text stroke and glow being included in bbox calculations. That's fine.

It's also failing two WGLMakie tests due to timeouts, which I don't understand. Maybe related to these errors?

[4350:0405/160808.508450:ERROR:gles2_cmd_decoder_passthrough.cc(1082)] [GroupMarkerNotSet(crbug.com/242999)!:A0A02200CC2D0000]Automatic fallback to software WebGL has been deprecated. Please use the --enable-unsafe-swiftshader flag to opt in to lower security guarantees for trusted content.

@ffreyer ffreyer changed the title Text background recipe textlabel recipe Apr 8, 2025
@ffreyer ffreyer mentioned this pull request Apr 11, 2025
3 tasks
points_transformed = apply_transform(transform_func, points)
function poly_convert(polygon::AbstractVector{<:VecTypes{N, T}}, transform_func=identity) where {N, T}
points2d = convert(Vector{Point2{float_type(T)}}, polygon)
points_transformed = apply_transform(transform_func, points2d)
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to limit the info going in to transform_func to 2D?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Iirc earcut_triangulate errored with 3D points

Copy link
Member

@asinghvi17 asinghvi17 Apr 11, 2025

Choose a reason for hiding this comment

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

shouldn't we force 2D after transformation then, not before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason this is in the pr is to fix overlap in W/GLMakie. I'm effectively setting z values so that things are ordered as:

poly points 1
text 1
poly points 2 
text 2
...

where all the poly points are in the same poly plot. If the z values are dropped in poly!() the elements will no longer be interleaved and you'll have the issue Julius showed #4879 (comment) on every backend again.

The change I made basically just allows z values in points to be used for z ordering. That's what I need for this pr. The other option would be to copy and paste edited parts of poly. I can do that too if that's the preferred solution.

@jkrumbiegel jkrumbiegel mentioned this pull request Apr 15, 2025
@ffreyer ffreyer mentioned this pull request Apr 15, 2025
23 tasks
Copy link
Member

@SimonDanisch SimonDanisch left a comment

Choose a reason for hiding this comment

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

Ready to merge, whenever all the problems from the last review are addressed.

@ffreyer ffreyer moved this from Ready to review to Ready to merge in PR review Apr 16, 2025
@ffreyer
Copy link
Collaborator Author

ffreyer commented Apr 16, 2025

This would be ready to merge if WGLMakie wouldn't fail to render two tests. I have no idea why those are timing out, but they do so in local CI runs for me too. They however don't if I run the code manually and display them in vscode or in firefox.

Note that missing refimages do not show up as failing CI. Check refimages/ci run

This was referenced Apr 23, 2025
@ffreyer ffreyer closed this May 5, 2025
@github-project-automation github-project-automation bot moved this from Ready to merge to Closed in PR review May 5, 2025
@ffreyer ffreyer reopened this May 5, 2025
@ffreyer ffreyer merged commit e6e0608 into master May 5, 2025
37 of 43 checks passed
@ffreyer ffreyer deleted the ff/text-background branch May 5, 2025 13:37
@github-project-automation github-project-automation bot moved this from Closed to Merged in PR review May 5, 2025
@jkrumbiegel
Copy link
Member

The docs page for this hasn't actually been included in the makedocs call

@ffreyer ffreyer mentioned this pull request May 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Merged

Development

Successfully merging this pull request may close these issues.

8 participants