Remove all global attributes from TextureAtlas implementation #2498
Conversation
…kie.jl into sd/texatlas-refactor
Compile Times benchmarkNote, 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))
|
|
Ok, I finally fixed the issue with JuliaLang/julia#47184 (review) by, drumroll, changing using GLMakie
atlas = Makie.get_texture_atlas();
p = Makie.render_path(Makie.to_spritemarker(:utriangle));
heatmap(Makie.sdistancefield(p, 5, 12), colorrange=(-40, 40)) # will be a flat image when not working
open("not-faulty.txt", "w") do io
code_native(io, Makie.sdistancefield, Tuple{Matrix{UInt8}, Int, Int}, debuginfo=:none)
endShould also Work with older makie versions, I suppose! |
|
Anyone against changing the arrow marker in |
|
This seems to be a real change: |
|
I remember counting pixels for a few sizes, probably with rect marker. I guess we could check the exact sub-matrix for a marker in the atlas (width, height, values at a given pixelsize and font), and the relevant stuff GLMakie etc produces (uvs, quad scale, position)? |
src/utilities/texture_atlas.jl
Outdated
| # We use float max here to avoid texture bleed. See #2096 | ||
| fill(Float16(0.5PIXELSIZE_IN_ATLAS[] + GLYPH_PADDING[]), initial_size...), | ||
| fill(Float16(0.5pix_per_glyph + glyph_padding), resolution, resolution), |
There was a problem hiding this comment.
Seems like I didn't update this when I switched away from floatmax.
The fill value there is the maximum distance from the center of a glyph. With stroke- and glowwidth the background can become relevant during rendering and floatmax to some value can become a harsh cut-off. With this intermediate value it's less harsh (though still there). Basically this is closer to the values you'd get with more padding, which gives you softer transitions with large glow/strokewidths.
src/utilities/texture_atlas.jl
Outdated
| uv_width = Vec(lbrt[3] - lbrt[1], lbrt[4] - lbrt[2]) | ||
| full_pixel_size_in_atlas = uv_width .* Vec2f(size(ta.data) .- 1) | ||
| return full_pixel_size_in_atlas ./ Makie.PIXELSIZE_IN_ATLAS[] | ||
| full_pixel_size_in_atlas = uv_width .* Vec2f(size(atlas) .- 1) |
There was a problem hiding this comment.
This should be size(atlas) now as well, I think. Same with other divisions or multiplications by size(atlas) - 1).
src/utilities/texture_atlas.jl
Outdated
| # 0 based | ||
| idx_left_bottom = minimum(uv_pixel) | ||
| idx_right_top = maximum(uv_pixel) | ||
|
|
||
| # transform to normalized texture coordinates | ||
| # -1 for indexing offset | ||
| uv_left_bottom_pad = (idx_left_bottom) ./ tex_size | ||
| uv_right_top_pad = (idx_right_top .- 1) ./ tex_size |
There was a problem hiding this comment.
Oh and this seems to work better 0.75 instead of 1, but I don't understand why. Might be specific to the example we were looking at?
There was a problem hiding this comment.
Could the #define ANTIALIAS_RADIUS 0.8 be relevant in that context?
There was a problem hiding this comment.
0.8 works better though it doesn't seem to be sensitive to what ANTIALIAS_RADIUS is set to...
There was a problem hiding this comment.
I currently have it set to 0 in sprites.geom and distance_shape.frag, testing with p = BezierPath([MoveTo(0, 0), LineTo(0.5, 0.5), LineTo(0.5, -0.5), LineTo(-0.5, -0.5)]) to throw in something else.
There was a problem hiding this comment.
Is 0.8 also better in WGLMakie? I only checked for GLMakie
If you can narrow it down to the behavior of a particular function that would help. code_llvm will sadly lie to you and not give you the version that went into the pkgimage |
…kie.jl into sd/texatlas-refactor
Missing reference imagesFound 1 new images without existing references. |
Missing reference imagesFound 1 new images without existing references. |
Missing reference imagesFound 1 new images without existing references. |
|
To explain the changes I made: The ta = Makie.get_texture_atlas()
fig, ax, p = image(0..1, 0..1, ta.data, interpolate = false)
uv = Makie.glyph_uv_width!(ta, 'L', Makie.defaultfont())
x0, y0, x1, y1 = uv
r = Rect2f(x0, y0, x1-x0, y1-y0)
lines!(ax, r, color = :red)
fig
I also The second problem here is with the downsampling in For Bezierpaths we can avoid this problem by matching the generated image size to
|
Missing reference imagesFound 1 new images without existing references. |
Missing reference imagesFound 1 new images without existing references. |
|
Simon noticed that glow didn't work correctly anymore: Afaik when opengl samples near the edge of a pixel it'll interpolate between that pixel and the neighboring pixel. So when we set the uv's for a glyph tight to the outer edges in the texture atlas, it'll consider the pixels around the glyph as well. (I forgot about this yesterday...) For reference, the uv Rect of the scatter marker now looks like this: I guess this should also fix the line artifacts that are sometimes visible with glow and make the background value entirely irrelevant as we never interpolate to it!? |
|
Ok, I think this is finally ready! Thanks a ton @ffreyer! WGLMakie looks a bit broken, but I think that has been the case before and should get fixed in #2428. function TextureAtlas(; resolution=2048, pix_per_glyph=64, glyph_padding=12, downsample=5)
return TextureAtlas(
RectanglePacker(Rect2{Int32}(0, 0, resolution, resolution)),
Dict{UInt32, Int}(),
# We use the maximum distance of a glyph as a background to reduce texture bleed. See #2096
fill(Float16(0.5pix_per_glyph + glyph_padding), resolution, resolution),
Vec4f[],
pix_per_glyph,
glyph_padding,
downsample,
Function[]
)
endand should return data == |
…rg#2498) * uff * remove globals from texture atlas code * fix remaining problems + download from releases * update test * fix vectorized arguments * using stable hashes * fix test * better packing + clean ups * fix problems with native code caching * small offset fix * small fixes * add alignment test * change name * fix scaling issues? * match Bezier path render size to downsample * update tests with glyph alignment * update streamplot arrow sizes * switch to pixel centered uv's Co-authored-by: ffreyer <frederic481994@hotmail.de>
That our texture atlas relied on globals for setting e.g. the resolution/glyph_padding/glyph_per_pixel, was always kind of insane, considering we're allowing backends to use different texture atlases (e.g. for smaller sizes in WGLMakie).
I suspected that this also causes #2480, but that turned out to be false so far :( although
.does work now, but the marker is still rendered incorrectly! So looks like something goes wrong in FreeTypeAbstraction in the new Julia version or with caching binary.I've also started using our own serialization format which may remove some invalidations making it easier to compile getting the texture atlas (lets see what the benchmarks say). But what it definitely helps with is, that we can now download the serialized format for all Julia versions and never need to cache fonts again :) I hope this will make a positive dent in the CI runtimes.
Todo: