-
-
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: ff/safe-context
Are you sure you want to change the base?
Conversation
Benchmark ResultsSHA: f3980abb7c72e144fdfd047df2c661dea60a8770 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 |
- reuse buffers when regenerating gl render pipeline - switch Connection from Dict to Vector - inline is_compatible and promote_type - cache pipeline
doesn't fix the issue but maybe helps?
d9019d0
to
6992b74
Compare
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