-
-
Notifications
You must be signed in to change notification settings - Fork 317
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
Rework postprocessor handling in GLMakie #4689
base: master
Are you sure you want to change the base?
Conversation
Benchmark ResultsSHA: 44cef5fc77c2f0b5f1549b9df4a00de46381d74f Warning These results are subject to substantial noise because GitHub's CI runs on shared machines that are not ideally suited for benchmarking. |
The test failure is very rarely reproducible by spam opening and closing windows. Maybe once every 5-10min with using GLMakie, Colors
function make_scene()
scene = Scene()
color = map(tick -> HSV(tick.count % 360, 0.8, 0.7), events(scene).tick)
scatter!(scene, (0,0), color = color, markersize = 100)
display(GLMakie.Screen(render_on_demand = false, framerate = 10_000), scene)
end
running = Ref(true)
running[] = false
task = begin
running[] = true
@async while running[]
try
if rand() < 0.1
GLMakie.closeall()
make_scene()
else
make_scene()
end
yield()
# sleep(0.1)
catch e
GLMakie.closeall()
rethrow(e)
end
end
end
wait(task)
is_current_context(window) = GLFW.GetCurrentContext().handle == window.handle
# in some function
@assert is_current_context(screen.glscreen) "Context incorrect" I ended up in this block of code Makie.jl/GLMakie/src/postprocessing.jl Lines 168 to 171 in 5eeb34d
which is exactly the same as before, including all the code it calls (afaik). I started that block with an assert and then a switch_context!() so the context should be lost somewhere in there. Or maybe assert itself is causing task switches and I was just chasing after that?The latest commit still fails (without asserts, just running my test for ~10min) and I have not been able to reproduce it on master. After adding a bunch more context switches I'm now getting a program linking error which simply reports "Program Link Failed for unknown reason". |
CI seems to finish consistently now but I can still reproduce failures locally. Usually takes 5000+ spammed windows |
GLMakie/test/glmakie_refimages.jl
Outdated
@reference_test "Custom stage in render pipeline" begin | ||
GLMakie.closeall() | ||
|
||
function GLMakie.construct(::Val{:Tint}, screen, framebuffer, inputs, parent) | ||
frag_shader = """ | ||
{{GLSL_VERSION}} | ||
|
||
in vec2 frag_uv; | ||
out vec4 fragment_color; | ||
|
||
uniform sampler2D color_buffer; // \$(name of input)_buffer | ||
uniform mat3 color_transform; // from Stage attributes | ||
|
||
void main(void) { | ||
vec4 c = texture(color_buffer, frag_uv).rgba; | ||
fragment_color = vec4(color_transform * c.rgb, c.a); | ||
// fragment_color = vec4(1,0,0, c.a); | ||
} | ||
""" | ||
|
||
shader = GLMakie.LazyShader( | ||
screen.shader_cache, | ||
GLMakie.loadshader("postprocessing/fullscreen.vert"), | ||
GLMakie.ShaderSource(frag_shader, :frag) | ||
) | ||
|
||
inputs[:color_transform] = parent.attributes[:color_transform] | ||
|
||
robj = GLMakie.RenderObject(inputs, shader, | ||
GLMakie.PostprocessPrerender(), nothing, screen.glscreen | ||
) | ||
robj.postrenderfunction = () -> GLMakie.draw_fullscreen(robj.vertexarray.id) | ||
|
||
return GLMakie.RenderPass{:Tint}(framebuffer, robj) | ||
end | ||
|
||
function GLMakie.run_step(screen, glscene, step::GLMakie.RenderPass{:Tint}) | ||
# Blend transparent onto opaque | ||
wh = size(step.framebuffer) | ||
GLMakie.set_draw_buffers(step.framebuffer) | ||
GLMakie.glViewport(0, 0, wh[1], wh[2]) | ||
GLMakie.GLAbstraction.render(step.robj) | ||
return | ||
end | ||
|
||
|
||
begin | ||
# Pipeline matches test_pipeline_2D up to color_tint | ||
pipeline = Makie.Pipeline() | ||
|
||
render1 = push!(pipeline, Makie.RenderStage(transparency = false, fxaa = true)) | ||
render2 = push!(pipeline, Makie.TransparentRenderStage()) | ||
oit = push!(pipeline, Makie.OITStage()) | ||
fxaa = push!(pipeline, Makie.FXAAStage(filter_in_shader = false)) | ||
render3 = push!(pipeline, Makie.RenderStage(transparency = false, fxaa = false)) | ||
color_tint = push!(pipeline, Makie.Stage(:Tint, | ||
inputs = [:color => Makie.BufferFormat()], # defaults to 4x N0f8, i.e. 32Bit color | ||
outputs = [:color => Makie.BufferFormat()], | ||
color_transform = Observable(Makie.Mat3f( | ||
# sepia filter | ||
0.393, 0.349, 0.272, | ||
0.769, 0.686, 0.534, | ||
0.189, 0.168, 0.131 | ||
)) | ||
)) | ||
display_stage = push!(pipeline, Makie.DisplayStage()) | ||
|
||
connect!(pipeline, render1, fxaa) | ||
connect!(pipeline, render1, display_stage, :objectid) | ||
connect!(pipeline, render2, oit) | ||
connect!(pipeline, render2, display_stage, :objectid) | ||
connect!(pipeline, oit, fxaa, :color) | ||
connect!(pipeline, fxaa, color_tint, :color) | ||
connect!(pipeline, render3, color_tint, :color) | ||
connect!(pipeline, render3, display_stage, :objectid) | ||
connect!(pipeline, color_tint, display_stage, :color) | ||
end | ||
|
||
|
||
GLMakie.activate!(render_pipeline = pipeline) | ||
cow = load(Makie.assetpath("cow.png")) | ||
f,a,p = image(rotr90(cow)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7cd5b44
to
4d106cf
Compare
* pull tweaks from #4689 * pull context validation from #4689 * handle texture atlas explicitly + fix errors * explicitly clean up shaders & programs * handle GLBuffer, GLVertexArray, Postprocessors * handle Texture explicitly * fix SSAO error * test cleanup * add some sanity checks * fix tests * cleanup finalizers, free(), unsafe_free() * skip GLFW initialized assert for CI * update changelog * fix freeing of unused uniforms & cleanup unused pattern_sections * count and check failed OpenGL cleanup * try to handle GLFW CI failure * try fix CI * require context for Texture and GLBuffer in most cases * fix incorrect uniform cleanup * CI please * fix typo * try handling GLFW init errors differently * maybe just destroy the window if the context dies? * fix typos * maybe also cleanup dead screens when trying to reopen? * tweak require_context to maybe catch error earlier * switch before requiring * avoid requiring context for get_texture and thus robj cleanup * treat scene finalizer * Treat scene finalizer in texture atlas too * add missing passthrough * move object deletion test to end * warn on dead context * avoid the last few current_context() calls * fix scatter indices * put finalizers behind debug constant * don't skip observable cleanup in free * cleanup require_context * fix typo * fix test errors * clean up * fix errors * fix freeing of GLBuffer Observables * cleanup unsafe_free * cleanup called_from_finalizer & add comments to implied functions * remove try .. catch in free * slightly safer + better verify_free * some small cleanups * need to switch context * catch GC bugs early and try destroy! outside * switch context, since those can be called from anywhere * switch instead of require --------- Co-authored-by: SimonDanisch <[email protected]>
Description
This pr aims to rework and streamline postprocesser handling. I would like to implement a system similar to Nodes in Blender or Unreal Blueprints to handle the render pipeline. I.e. describe the various operations in
render_frame()
(z sorting, rendering plots, running postprocessors) as Nodes with connectable inputs and outputs. These nodes and edges should be free of OpenGL, i.e. an abstraction of what the backend is doing. Resolving them should produce a set of necessary buffers for data transfer and a pipeline of backend tasks to run inrender_frame
.TODO/Plans/Changes:
maybe add task forsetup!()
is_compatible_with()
(only used symmetrical variant)try replacing (dims, type) withuse type enum to avoid runtime dispatchVec{dims, type}
as a simplificationconsider using ShaderAbstractions.Samplerdon't want CPU data for framebuffer attachmentsNote: I started with this on #4150 and figured these changes can be done independently and are probably also useful independently. There is still some code related to that pr in here (e.g. glscene mentions) and I will probably make some decisions with that pr in mind.
Type of change
Delete options that do not apply:
... or maybe just internal rework for now?
Checklist