-
-
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
Render on demand #2336
Render on demand #2336
Changes from 10 commits
d2a6e63
8b51319
bd0f73d
9017d3a
fa2d179
f1b70d8
ce00493
781e718
845a700
61a3163
8d0bfd6
9ccc17b
63dc419
39c88a6
c5686c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -174,9 +174,17 @@ mutable struct GLVertexArray{T} | |
buffers::Dict{String,GLBuffer} | ||
indices::T | ||
context::GLContext | ||
requires_update::Observable{Bool} | ||
|
||
function GLVertexArray{T}(program, id, bufferlength, buffers, indices) where T | ||
new(program, id, bufferlength, buffers, indices, current_context()) | ||
va = new(program, id, bufferlength, buffers, indices, current_context(), true) | ||
for (name, buffer) in buffers | ||
on(buffer.requires_update) do _ # only triggers true anyway | ||
va.requires_update[] = true | ||
end | ||
end | ||
|
||
return va | ||
end | ||
end | ||
|
||
|
@@ -289,11 +297,14 @@ mutable struct RenderObject{Pre} | |
prerenderfunction::Pre | ||
postrenderfunction | ||
id::UInt32 | ||
requires_update::Bool | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should consistently use the same type for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see, some do need to be an observable..So maybe use Observable consistently? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For Otherwise I went with Bool since the structs are already mutable. I think that's a bit faster than a Refvalue in a mutable struct too... |
||
|
||
function RenderObject{Pre}( | ||
context, | ||
uniforms::Dict{Symbol,Any}, observables::Vector{Observable}, | ||
vertexarray::GLVertexArray, | ||
prerenderfunctions, postrenderfunctions | ||
prerenderfunctions, postrenderfunctions, | ||
track_updates = true | ||
) where Pre | ||
fxaa = to_value(pop!(uniforms, :fxaa, true)) | ||
RENDER_OBJECT_ID_COUNTER[] += one(UInt32) | ||
|
@@ -303,12 +314,46 @@ mutable struct RenderObject{Pre} | |
# But with this implementation, the fxaa flag can't be changed, | ||
# and since this is a UUID, it shouldn't matter | ||
id = pack_bool(RENDER_OBJECT_ID_COUNTER[], fxaa) | ||
new( | ||
robj = new( | ||
context, | ||
uniforms, observables, vertexarray, | ||
prerenderfunctions, postrenderfunctions, | ||
id | ||
id, true | ||
) | ||
|
||
if track_updates | ||
# gather update requests for polling in renderloop | ||
for uniform in values(uniforms) | ||
if uniform isa Observable | ||
on(uniform) do _ | ||
robj.requires_update = true | ||
end | ||
elseif uniform isa GPUArray | ||
on(uniform.requires_update) do _ | ||
robj.requires_update = true | ||
end | ||
end | ||
end | ||
on(_ -> robj.requires_update = true, vertexarray.requires_update) | ||
else | ||
# remove tracking from GPUArrays | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure why that's needed, could you elaborate the comment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ugh, classic case of not seing the whole picture in the github diff...sorry for the noise There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, it feels cleaner to me, to just let the GPUArrays track their updates, which should be very cheap, but then not connect them further if we don't track updates... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This? If all the buffers are also in uniforms it shouldn't be necessary. I added that early on when I was searching for things that need to be tracked
The way I added tracking it gets intialized for every renderloop. That can be a lot of extra useless callbacks and from my testing it can be noticeable (like 5-10% slower with fps_renderloop in what should be a bad case). I tried to restore performance again by removing tracking when it's not needed. That's what the code under the comment does. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I actually removed all the tracking in one go so I'm not sure how much the GPUArrays contribute. The test case I used also didn't trigger updates there, but in simple observables since that should have a higher ratio of tracking cost / total update cost and I wanted to know how bad it can get. I was planning to test having GPUArray updates wait for frames in the future, which would probably require something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. using BenchmarkTools, GLMakie
scene = Scene()
for _ in 1:100
heatmap!(scene, rand(16, 16))
end
display(GLMakie.Screen(render_on_demand = false), scene)
function update!(scene)
for p in scene.plots
notify(p[3])
end
end
@benchmark update!($scene) With cleanup
Without cleanup (the else branch commented out)
Without cleanup & single observer function
That's much more than I thought it would be... |
||
for uniform in values(uniforms) | ||
if uniform isa GPUArray | ||
foreach(off, uniform.requires_update.inputs) | ||
empty!(uniform.requires_update.inputs) | ||
end | ||
end | ||
for buffer in vertexarray.buffers | ||
if buffer isa GPUArray | ||
foreach(off, buffer.requires_update.inputs) | ||
empty!(buffer.requires_update.inputs) | ||
end | ||
end | ||
foreach(off, vertexarray.requires_update.inputs) | ||
empty!(vertexarray.requires_update.inputs) | ||
end | ||
|
||
return robj | ||
end | ||
end | ||
|
||
|
@@ -320,6 +365,11 @@ function RenderObject( | |
|
||
switch_context!(context) | ||
|
||
# This is a lazy workaround for disabling updates of `requires_update` when | ||
# not rendering on demand. A cleaner implementation should probably go | ||
# through @gen_defaults! and adjust constructors instead. | ||
track_updates = to_value(pop!(data, :track_updates, true)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The way it works atm is that GPUArrays always attach the update trackers when they get created. When the renderobject is created it either removes the tracking (i.e. observer functions) or connects the remaining uniforms based on what would usually be interpreted as a uniform. Seems quite hacky to me 🤷 A clean implementation wouldn't connect (and maybe also not define) the tracking observables in GPUArrays. I think with how things are organized atm There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In other words: please don't waste a second on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok I think the pr is ready then |
||
|
||
targets = get(data, :gl_convert_targets, Dict()) | ||
delete!(data, :gl_convert_targets) | ||
passthrough = Dict{Symbol,Any}() # we also save a few non opengl related values in data | ||
|
@@ -362,6 +412,7 @@ function RenderObject( | |
vertexarray, | ||
pre, | ||
post, | ||
track_updates | ||
) | ||
# automatically integrate object ID, will be discarded if shader doesn't use it | ||
robj[:objectid] = robj.id | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ function renderloop end | |
|
||
* `pause_renderloop = false`: creates a screen with paused renderlooop. Can be started with `GLMakie.start_renderloop!(screen)` or paused again with `GLMakie.pause_renderloop!(screen)`. | ||
* `vsync = false`: enables vsync for the window. | ||
* `render_on_demand = true`: renders the scene only if something has changed in it. | ||
* `framerate = 30.0`: sets the currently rendered frames per second. | ||
|
||
## GLFW window attributes | ||
|
@@ -40,6 +41,7 @@ mutable struct ScreenConfig | |
renderloop::Function | ||
pause_renderloop::Bool | ||
vsync::Bool | ||
render_on_demand::Bool | ||
framerate::Float64 | ||
|
||
# GLFW window attributes | ||
|
@@ -62,6 +64,7 @@ mutable struct ScreenConfig | |
renderloop::Union{Makie.Automatic, Function}, | ||
pause_renderloop::Bool, | ||
vsync::Bool, | ||
render_on_demand::Bool, | ||
framerate::Number, | ||
# GLFW window attributes | ||
float::Bool, | ||
|
@@ -82,6 +85,7 @@ mutable struct ScreenConfig | |
renderloop isa Makie.Automatic ? GLMakie.renderloop : renderloop, | ||
pause_renderloop, | ||
vsync, | ||
render_on_demand, | ||
framerate, | ||
# GLFW window attributes | ||
float, | ||
|
@@ -663,10 +667,45 @@ function fps_renderloop(screen::Screen) | |
end | ||
end | ||
|
||
|
||
function requires_update(screen::Screen) | ||
for (_, _, robj) in screen.renderlist | ||
visible = Bool(to_value(get(robj.uniforms, :visible, true))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we may want to use this PR to change the visible field into a well typed, mandatory field for RenderObject. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if I did what you had in mind but I moved it to a field. I also had the logic for re-rendering with visible wrong. I tried to set it up in a way that invisible renderobjects can't trigger updates, but the way I did it caused |
||
if visible && robj.requires_update | ||
return true | ||
end | ||
end | ||
return false | ||
end | ||
|
||
function on_demand_renderloop(screen::Screen) | ||
while isopen(screen) && !screen.stop_renderloop | ||
t = time_ns() | ||
time_per_frame = 1.0 / screen.config.framerate | ||
pollevents(screen) # GLFW poll | ||
|
||
if !screen.config.pause_renderloop && requires_update(screen) | ||
render_frame(screen) | ||
GLFW.SwapBuffers(to_native(screen)) | ||
end | ||
|
||
t_elapsed = (time_ns() - t) / 1e9 | ||
diff = time_per_frame - t_elapsed | ||
if diff > 0.001 # can't sleep less than 0.001 | ||
sleep(diff) | ||
else # if we don't sleep, we still need to yield explicitely to other tasks | ||
yield() | ||
end | ||
end | ||
end | ||
|
||
function renderloop(screen) | ||
isopen(screen) || error("Screen most be open to run renderloop!") | ||
try | ||
if screen.config.vsync | ||
if screen.config.render_on_demand | ||
GLFW.SwapInterval(0) | ||
on_demand_renderloop(screen) | ||
elseif screen.config.vsync | ||
GLFW.SwapInterval(1) | ||
vsynced_renderloop(screen) | ||
else | ||
|
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.
Why do we have this todo, and not just:
(I already have this locally, will push some changes like this later)
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.
I separated them so the update tracking could be removed again when using fps_renderloop