Port webgpu_app to render graph - #251
Conversation
487d552 to
e341435
Compare
|
Failing pipeline are unrelated unit tests. Review will be for Gerald :) |
a8cac59 to
36aab61
Compare
split the width, height and depht/layer count in TextureDesc into stand alone fields reduced code dup. added a missing unit test added common case factories for TextureDesc
ada13c6 to
6ca2849
Compare
|
ready to merge upstream. |
|
Qt Code Review Report Recommendation: Request changes All current CI checks pass. I did not submit a GitHub review or modify the checkout. Lint findings [L-001] Release builds accept a null allocator File: webgpu/base/RenderGraph.cpp:909 Rule: LCY-4 Finding: start_recording() uses Q_ASSERT(allocator) as its only null guard, then immediately dereferences it. The guard disappears in release builds. Mitigation: Make null handling part of the runtime contract or express non-nullness through the API. The deterministic scan produced 78 raw diagnostics. The remaining hits were primarily formatting/style rules or unchanged-line matches and are not proposed as review comments. Deep analysis findings [D-001] Spontaneous profiling callback has data races File: webgpu/base/RenderGraph.cpp:2921 Category: Thread Safety Confidence: 97/100 Finding: WGPUCallbackMode_AllowSpontaneous may execute on an arbitrary thread, but the callback reads and writes ordinary GpuProfiler and Slot fields also accessed by the render/UI thread. Trace: The callback changes results and slot->pending; execute(), free_slot(), sample_history(), and RenderGraphPanel access the same fields without synchronization. Mitigation: Use AllowProcessEvents with the existing main-thread event pump, or synchronize publication and slot reuse. [D-002] Pending callback can outlive its allocator File: webgpu/base/RenderGraph.cpp:2922 Category: Ownership & Lifecycle Confidence: 91/100 Finding: Callback userdata points inside GraphAllocator, but destroy_allocator() neither cancels nor drains pending mappings before deleting it. Trace: The callback dereferences both GpuProfiler* and Slot* after asynchronous completion. Mitigation: Drain/cancel pending callbacks before teardown or put callback state in independently owned, reference-counted storage. [D-003] GPU profiler resources are leaked File: webgpu/base/RenderGraph.cpp:508 Category: Ownership & Lifecycle Confidence: 98/100 Finding: Profiling creates a query set, resolve buffer, and three readback buffers, but none are released. Trace: destroy_allocator() tears down the resource pools and arenas only; no corresponding query-set or buffer releases exist. Mitigation: Add explicit GpuProfiler teardown after pending operations are settled. [D-004] Integer overflow bypasses buffer bounds validation File: webgpu/base/RenderGraph.cpp:2180 Category: Error Handling Confidence: 95/100 Finding: bufOffset + bufSize > bufferSize can wrap, allowing an invalid range through compilation. Trace: Public buffer ranges accept uint64_t; execution later forwards the unchecked range to WebGPU. Mitigation: Check offset > bufferSize || size > bufferSize - offset. Add overflow-boundary tests. [D-005] Texture subresource indices are silently truncated File: webgpu/base/RenderGraph.cpp:3067 Category: Error Handling Confidence: 96/100 Finding: Public mip/layer fields are uint32_t, but accesses store them as uint8_t/uint16_t before validation. Trace: Mip 256 or layer 65536 becomes zero, so validation can accept and execute a different subresource. Mitigation: Preserve uint32_t values or reject unrepresentable inputs before conversion. [D-006] RenderGraph GPU timing UI can never collect data File: apps/webgpu_app/App.cpp:245 Category: Performance & Quality Confidence: 100/100 Finding: The only RenderGraph::execute() call hardcodes profiling to false. Trace: No queries are resolved, pendingSlot remains unset, and collect_gpu_timings() immediately returns. The panel’s timing history therefore stays empty. Mitigation: Feed an actual profiling request from the panel, gated on timestamp-query support. [D-007] RenderGraph panel becomes inactive on idle frames File: apps/webgpu_app/ui/RenderGraphPanel.cpp:2739 Category: Performance & Quality Confidence: 98/100 Finding: The panel clears g_render_graph every GUI frame, while App only publishes a graph when the scene redraws. Trace: On a static scene, the next UI frame has no replacement and displays “Render graph inactive.” Mitigation: Retain a durable diagnostic snapshot, or keep the last graph until begin_frame() invalidates it. [D-008] Surface acquisition failure leaves CPU timing unbalanced File: apps/webgpu_app/App.cpp:182 Category: Error Handling Confidence: 88/100 Finding: The PR moved m_cputimer->start() before surface acquisition, but the failure branch returns without calling stop(). Mitigation: Start after successful acquisition or use scoped cleanup. [D-009] std::format lacks its required include File: webgpu/engine/overlay/OverlayRenderer.cpp:106 Category: API & C++ Correctness Confidence: 99/100 Finding: The new code uses std::format without including , relying on build-order or transitive-header behavior. Mitigation: Include directly. [D-010] Resolve accesses display as unknown File: apps/webgpu_app/ui/RenderGraphPanel.cpp:57 Category: API & C++ Correctness Confidence: 96/100 Finding: rg_access_name() omits AccessType::ResolveAttachment, so resolve accesses appear as "?". Mitigation: Add the missing case and preferably keep the enum switch exhaustive. Investigation targets (human verification needed) [I-001] Allocator ownership depends on manual teardown File: apps/webgpu_app/RenderingContext.h:81 Category: Ownership & Lifecycle Confidence: 74/100 Finding: RenderingContext owns a raw GraphAllocator* without destructor or repeated-initialization protection. How to verify: Confirm that every native/Wasm lifecycle calls destroy() exactly once and initialize() cannot repeat. Prefer RAII ownership. [I-002] Profiler resource creation is unchecked File: webgpu/base/RenderGraph.cpp:508 Category: Error Handling Confidence: 72/100 Finding: Initialization is marked complete without checking query-set or buffer creation results. How to verify: Confirm failure-object semantics for every supported WebGPU implementation and disable profiling cleanly when creation fails. [I-003] Headers depend on transitive declarations File: webgpu/engine/Window.h:57 Category: API & C++ Correctness Confidence: 72/100 Finding: TextureHandle currently reaches Window.h through unrelated renderer headers. How to verify: Run standalone-header compilation; add a direct include or forward declaration if independence is required. Summary Category Lint Deep Investigate Total Ownership & Lifecycle 1 2 1 4 The checkout remains clean: ## main...origin/main. |
GeraldKimmersdorfer
left a comment
GeraldKimmersdorfer
left a comment
There was a problem hiding this comment.
As I already mentioned this is an amazing tool. It's very low-level such that really rolling it out to all the features of webigeo including the forks that are currently in development will take a while, but I'm sure we'll get there.
I didnt really do a thorough review on code level, but more on a feature level. My immediate suggestions before we go on with the merge:
- When the RenderGraph Window opens please make sure that it already fills a good portion of the screen. When I opened it I couldnt find it at first.
- Is it possible that when we don't "repaint each frame" that the Panel shows the last render graph configuration instead of "Render Graph inactivate" (i know this destroys immediate mode paradigm - please only implement when easily possible) OR another suggestions which would make that actually a non-issue: Is it possible to have the GuiPipeline also be run via the RenderGraph? Such that most of the time we would only see the easy BlitGraph with the backbuffertexture and when we do a full repaint we see the whole graph?
- As pointed out in a comment, would it be possible to either remove all of the profiling code or at least activate it such that we can use the functionality? As we discussed this code will at some point be removed by a more general profiler thats why I lean more towards the deletion of all this code.
- Deactivating VSync seems to be broken now. At least it always seems to cap at 60FPS for me. Please have a look at that.
- Please add your copyright to all the files you touch. I know its cumbersome but it also is helpful in giving a first glimpse on who worked on the various files.
- I also wrote it as a comment: It's obviously not your job but maybe you could remove the inline shader code that I added a long time ago for the GuiPipeline in the app
- As commented: I think the BindGroups for CloudRenderer are now constructed in the hot loop? Please have a look at that
More long term refactors that I think are good ideas, but out of scope for the pr (will add in my backlog or as github issues eventually):
- Make the Panel code more lean by reusing the ImNodes library. (Which requires to add certain functionality to the ImNodes code though)
- Write a general Profiler which the RenderGraph can hook into (already partly done on my branch)
| @@ -0,0 +1,2821 @@ | |||
| /***************************************************************************** | |||
| * Mostly written by Opus 4.8 | |||
There was a problem hiding this comment.
I'd say its still your copyright, but that's up to you. Dont know how it works with Coding Agents but they are no legal entity. :/
There was a problem hiding this comment.
I'd say, leave opus out of it. and it's definitely your copyright (as well). I don't think it's useful to put opus 4.8 there, and fable, and opus 5, and 5.6 sol, and 5.6 terra and clang format 21 etc. there. you are responsible for the code. if you don't want to own it, don't push it!
There was a problem hiding this comment.
oh this seems useful thx
| GuiPipelineUBO initial { glm::vec2(1280.0f, 1024.0f) }; | ||
| m_blit_ubo->write(m_queue, &initial); | ||
|
|
||
| const char preprocessed_code[] = R"( |
There was a problem hiding this comment.
I only did this inline because at the first version of webigeo the App didnt have access to the shader manager. Now with the shared Manager I'd prefer to have it as standalone shader file. Would it be okay for you to do that? (Otherwise you can leave it for me)
There was a problem hiding this comment.
Only brainstorming here, but I think we should modify imnodes in a way such that it allows zoom and then use this library here instead? No need to reinvent the wheel I guess. At the same time ImNodes doesnt have a lot of functionalities that you coded here (like building groups). Anyway thats only a suggestion for further down the line. Of no importance right now.
| namespace webgpu_app { | ||
|
|
||
| //TODO: move | ||
| webgpu::rg::RenderGraph* g_render_graph = nullptr; |
There was a problem hiding this comment.
I mean this place is as good as any, but maybe it would make sense to be inside the Context-Object?
| } | ||
|
|
||
| // per-pass GPU us over time, one line per pass. sampled in draw(), so it keeps running on any tab. | ||
| static void rg_draw_timings(RenderGraphStorage& s) |
There was a problem hiding this comment.
The profiler that you have implemented is quite cool, but as we already discussed will be obsolete once we will write a more general Profiler-Implementation somewhere down the line. For the time being I'd propose two paths:
- Delete all of your profiling related code
- Activate the profiler such that we can use it within webigeo and we'll remove it once it becomes obsolete.
| }, | ||
| [this, lo_color, lo_depth, gbuffer_depth, shared_config_bind_group](webgpu::rg::PassContext& c) { | ||
|
|
||
| webgpu::raii::BindGroup bind_group(c.device, *m_render_clouds_layout, |
There was a problem hiding this comment.
We used to only construct the bindgroups on resize. With this - if im not mistaken - we do construct two of them each frame? Is that negligible? Since they are static anyway could we still have them only reconstructed on size_change?
3 participants
Ports the webgpu app onto the render graph added in #250. The frame is now
built by declaring resources and passes each frame instead of the previous hand-ordered
rendering.
Changes
webgpu/engine/Windowbuilds the graph each frame and drivesexecute().atmosphere, clouds, track, and the height-lines, screen-space-snow, texture, and
tile-debug overlays, plus
OverlayRendererand the sharedOverlayinterface.RenderGraphPanelis a debug UI panel for inspecting the compiled graph (passes,resources, ordering, culling), plumbed in through
ImGuiManagerandRenderingContext.App.cppandapps/webgpu_app/CMakeLists.txtregister the panel.webgpu/base/gpu_utilsused by the integration.Files
App.cpp,ImGuiManager.{h,cpp},RenderingContext.{h,cpp},ui/RenderGraphPanel.{h,cpp},CMakeLists.txtWindow.{h,cpp}, the atmosphere/cloud/track renderers, the overlays andOverlayRenderer,webgpu/base/gpu_utils.{h,cpp}