Skip to content
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

Draft
wants to merge 70 commits into
base: ff/safe-context
Choose a base branch
from

Conversation

ffreyer
Copy link
Collaborator

@ffreyer ffreyer commented Dec 24, 2024

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 in render_frame.

TODO/Plans/Changes:

  • extract OpenGL parts of GLFramebuffer and move them to GLAbstraction
  • rework PostProcessor type to better represent a renderloop task (generic abstract type with specialized implementations, try using dispatched function)
  • add task for rendering
  • add task for z sorting
  • maybe add task for setup!()
  • switch to one GLFramebuffer per task (as needed) instead of one global framebuffer
  • if needed, implement GLFence for synching between framebuffers
  • add a FramebufferFactory which holds onto all attachments, adds new ones as required and distributes them to postprocessors
  • create and connect abstract RenderPipeline prototype
  • Makie render pipeline: add optional requirements to BufferFormat (e.g. clamping, filter, mipmap, multisample?)
  • Makie render pipeline: cleanup is_compatible_with() (only used symmetrical variant)
  • cleanup old GLAbstraction.Framebuffer
  • derive fragment_output from gl-render pipeline (i.e. base shader outputs off the render pipeline)
    • do it more specifically for render stages that align with a render object
  • reconsider/cleanup:
    • frame setup!()
    • fxaa/ssao render stage
    • handling of colorbuffer, depthbuffer, picking (i.e. hardcoded names?)
  • more directly integrate Makie Renderpipeline with Screen()
  • improve usability of Makie render pipeline
  • fix test error related to ... faulty context? fixed by Make cleanup of OpenGL objects explicit in GLMakie #4699
  • add tests:
    • Makie Pipeline
    • GLMakie rendering with different pipelines
  • add docs
  • maybe graph visualization/widget for setting up pipelines
  • update precompiles to maybe improve benchmarks
  • try replacing (dims, type) with Vec{dims, type} as a simplification use type enum to avoid runtime dispatch
  • connect all texture attributes
  • consider using ShaderAbstractions.Sampler don't want CPU data for framebuffer attachments

Note: 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:

  • 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)

... or maybe just internal rework for now?

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.

@MakieBot
Copy link
Collaborator

MakieBot commented Dec 24, 2024

Benchmark Results

SHA: f3980abb7c72e144fdfd047df2c661dea60a8770

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

@ffreyer
Copy link
Collaborator Author

ffreyer commented Dec 29, 2024

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)

which also randomly gives up on itself every so often. (These are errors. Need to wait on task to see them.) When trying to hunt down incorrect context with

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

ppu = screen.px_per_unit[]
a = viewport(scene)[]
glViewport(round.(Int, ppu .* minimum(a))..., round.(Int, ppu .* widths(a))...)
render(elem)

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".

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jan 1, 2025

CI seems to finish consistently now but I can still reproduce failures locally. Usually takes 5000+ spammed windows

ffreyer added a commit that referenced this pull request Jan 2, 2025
ffreyer added a commit that referenced this pull request Jan 2, 2025
@ffreyer ffreyer force-pushed the ff/render_pipeline_master branch from d9019d0 to 6992b74 Compare January 4, 2025 01:18
@ffreyer ffreyer changed the base branch from master to ff/safe-context January 4, 2025 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Work in progress
Development

Successfully merging this pull request may close these issues.

2 participants