From 5d20c892dedce7bc7486acbd72fbd35da69e413e Mon Sep 17 00:00:00 2001 From: skal Date: Wed, 20 May 2026 22:44:44 +0200 Subject: fix: code review cleanup — bugs, dead code, factorization (-167 lines) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bugs: - B1: fix dead tempo debug (prev_tempo captured after assignment) - B2: fix ReloadAssetsFromFile leak for disk-loaded assets; simplify DropAsset - B3: fix get_free_pool_slot leak (unregister synth + free data on reuse) - B4: volatile -> std::atomic with acquire/release in miniaudio_backend, synth - B5: fix unaligned reads in scene_loader (memcpy-based read_f32/read_u32) - B6: fix shader module + BGL + pipeline layout leaks in gpu.cc, pipeline_builder Dead code: - D1: remove unused particle_defs.h - D3: remove create_post_process_pipeline_simple (zero callers) - D4: remove empty gpu_draw() - D5: remove write-only Hybrid3D::initialized_ - D6: remove legacy pending buffer path in audio.cc Factorization: - F1: Effect::run_fullscreen_pass() replaces boilerplate in 5 effects - F2: particle_common.wgsl snippet, #include in 3 WGSL shaders - F3: gpu_create_shader_module() helper, used in 3 call sites - F5: get_world_aabb() shared between bvh.cc and physics.cc - F6: samples_to_seconds() replaces 6 inline expressions - F7: gpu_create_linear/nearest_sampler use SamplerCache; add nearest() preset 37/37 tests passing. handoff(Claude): code review batch — all items verified, no regressions. --- src/gpu/effect.cc | 25 +++++++++++++++ src/gpu/effect.h | 7 +++++ src/gpu/gpu.cc | 71 ++++++++++++++++++------------------------ src/gpu/gpu.h | 7 +++-- src/gpu/gpu_headless.cc | 9 ------ src/gpu/gpu_stub.cc | 9 ------ src/gpu/pipeline_builder.cc | 11 +++---- src/gpu/post_process_helper.cc | 34 +++++--------------- src/gpu/post_process_helper.h | 5 --- src/gpu/sampler_cache.h | 4 +++ src/gpu/wgsl_effect.cc | 17 ++-------- 11 files changed, 85 insertions(+), 114 deletions(-) (limited to 'src/gpu') diff --git a/src/gpu/effect.cc b/src/gpu/effect.cc index 1257090..2e93a11 100644 --- a/src/gpu/effect.cc +++ b/src/gpu/effect.cc @@ -2,6 +2,7 @@ #include "gpu/effect.h" #include "gpu/gpu.h" +#include "gpu/sampler_cache.h" #include "gpu/sequence.h" #include "util/fatal_error.h" @@ -72,6 +73,30 @@ std::string Effect::find_downstream_output( return ""; } +void Effect::run_fullscreen_pass(WGPUCommandEncoder encoder, + WGPURenderPipeline pipeline, + WGPUBindGroup bind_group, + WGPUTextureView output_view, + WGPULoadOp load_op) { + HEADLESS_RETURN_IF_NULL(encoder); + + WGPURenderPassColorAttachment color_attachment = {}; + gpu_init_color_attachment(color_attachment, output_view); + color_attachment.loadOp = load_op; + + WGPURenderPassDescriptor pass_desc = {}; + pass_desc.colorAttachmentCount = 1; + pass_desc.colorAttachments = &color_attachment; + + WGPURenderPassEncoder pass = + wgpuCommandEncoderBeginRenderPass(encoder, &pass_desc); + wgpuRenderPassEncoderSetPipeline(pass, pipeline); + wgpuRenderPassEncoderSetBindGroup(pass, 0, bind_group, 0, nullptr); + wgpuRenderPassEncoderDraw(pass, 3, 1, 0, 0); + wgpuRenderPassEncoderEnd(pass); + wgpuRenderPassEncoderRelease(pass); +} + void Effect::create_linear_sampler() { sampler_.set(gpu_create_linear_sampler(ctx_.device)); } diff --git a/src/gpu/effect.h b/src/gpu/effect.h index 70ba9be..47dd3c2 100644 --- a/src/gpu/effect.h +++ b/src/gpu/effect.h @@ -79,6 +79,13 @@ class Effect { // consumer). Returns "" if no such effect exists or it has no outputs. std::string find_downstream_output(const std::vector& dag) const; + // Helper: Run a fullscreen triangle pass (pipeline + bind_group → output) + static void run_fullscreen_pass(WGPUCommandEncoder encoder, + WGPURenderPipeline pipeline, + WGPUBindGroup bind_group, + WGPUTextureView output_view, + WGPULoadOp load_op = WGPULoadOp_Clear); + // Helper: Create linear sampler (call in subclass constructor if needed) void create_linear_sampler(); diff --git a/src/gpu/gpu.cc b/src/gpu/gpu.cc index 0be5afe..f5f1515 100644 --- a/src/gpu/gpu.cc +++ b/src/gpu/gpu.cc @@ -5,6 +5,7 @@ #include "gpu.h" #include "effects/shaders.h" #include "generated/timeline.h" +#include "gpu/sampler_cache.h" #include "gpu/shader_composer.h" #include "platform/platform.h" @@ -124,26 +125,28 @@ WGPUTextureView gpu_create_texture_view_2d(WGPUTexture texture, return wgpuTextureCreateView(texture, &view_desc); } +WGPUShaderModule gpu_create_shader_module(WGPUDevice device, + const char* wgsl_code, + const char* label) { + WGPUShaderSourceWGSL wgsl_src = {}; + wgsl_src.chain.sType = WGPUSType_ShaderSourceWGSL; + wgsl_src.code = str_view(wgsl_code); + WGPUShaderModuleDescriptor desc = {}; + desc.label = label_view(label); + desc.nextInChain = &wgsl_src.chain; + return wgpuDeviceCreateShaderModule(device, &desc); +} + WGPUSampler gpu_create_linear_sampler(WGPUDevice device) { - WGPUSamplerDescriptor desc = {}; - desc.addressModeU = WGPUAddressMode_ClampToEdge; - desc.addressModeV = WGPUAddressMode_ClampToEdge; - desc.addressModeW = WGPUAddressMode_ClampToEdge; - desc.magFilter = WGPUFilterMode_Linear; - desc.minFilter = WGPUFilterMode_Linear; - desc.mipmapFilter = WGPUMipmapFilterMode_Nearest; - desc.maxAnisotropy = 1; - return wgpuDeviceCreateSampler(device, &desc); + WGPUSampler s = SamplerCache::Get().get_or_create(device, SamplerCache::clamp()); + if (s) wgpuSamplerAddRef(s); // Caller owns a reference (for RAII wrappers) + return s; } WGPUSampler gpu_create_nearest_sampler(WGPUDevice device) { - WGPUSamplerDescriptor desc = {}; - desc.addressModeU = WGPUAddressMode_ClampToEdge; - desc.addressModeV = WGPUAddressMode_ClampToEdge; - desc.magFilter = WGPUFilterMode_Nearest; - desc.minFilter = WGPUFilterMode_Nearest; - desc.maxAnisotropy = 1; - return wgpuDeviceCreateSampler(device, &desc); + WGPUSampler s = SamplerCache::Get().get_or_create(device, SamplerCache::nearest()); + if (s) wgpuSamplerAddRef(s); // Caller owns a reference (for RAII wrappers) + return s; } TextureWithView gpu_create_dummy_scene_texture(WGPUDevice device) { @@ -167,15 +170,8 @@ RenderPass gpu_create_render_pass(WGPUDevice device, WGPUTextureFormat format, // Compose shader to resolve #include directives std::string composed_shader = ShaderComposer::Get().Compose({}, shader_code); - // Create Shader Module - WGPUShaderSourceWGSL wgsl_src = {}; - wgsl_src.chain.sType = WGPUSType_ShaderSourceWGSL; - wgsl_src.code = str_view(composed_shader.c_str()); - WGPUShaderModuleDescriptor shader_desc = {}; - shader_desc.label = label_view("render_shader"); - shader_desc.nextInChain = &wgsl_src.chain; WGPUShaderModule shader_module = - wgpuDeviceCreateShaderModule(device, &shader_desc); + gpu_create_shader_module(device, composed_shader.c_str(), "render_shader"); // Create Bind Group Layout & Bind Group std::vector bgl_entries; @@ -257,6 +253,10 @@ RenderPass gpu_create_render_pass(WGPUDevice device, WGPUTextureFormat format, pass.pipeline = wgpuDeviceCreateRenderPipeline(device, &pipeline_desc); + wgpuShaderModuleRelease(shader_module); + wgpuPipelineLayoutRelease(pipeline_layout); + wgpuBindGroupLayoutRelease(bind_group_layout); + return pass; } @@ -268,14 +268,8 @@ ComputePass gpu_create_compute_pass(WGPUDevice device, const char* shader_code, // Compose shader to resolve #include directives std::string composed_shader = ShaderComposer::Get().Compose({}, shader_code); - WGPUShaderSourceWGSL wgsl_src = {}; - wgsl_src.chain.sType = WGPUSType_ShaderSourceWGSL; - wgsl_src.code = str_view(composed_shader.c_str()); - WGPUShaderModuleDescriptor shader_desc = {}; - shader_desc.label = label_view("compute_shader"); - shader_desc.nextInChain = &wgsl_src.chain; WGPUShaderModule shader_module = - wgpuDeviceCreateShaderModule(device, &shader_desc); + gpu_create_shader_module(device, composed_shader.c_str(), "compute_shader"); std::vector bgl_entries; std::vector bg_entries; @@ -319,6 +313,11 @@ ComputePass gpu_create_compute_pass(WGPUDevice device, const char* shader_code, pipeline_desc.compute.entryPoint = str_view("main"); pass.pipeline = wgpuDeviceCreateComputePipeline(device, &pipeline_desc); + + wgpuShaderModuleRelease(shader_module); + wgpuPipelineLayoutRelease(pipeline_layout); + wgpuBindGroupLayoutRelease(bind_group_layout); + return pass; } @@ -439,16 +438,6 @@ void gpu_init(PlatformState* platform_state) { } -void gpu_draw(float audio_peak, float aspect_ratio, float time, float beat_time, - float beat_phase) { - // Rendering is driven externally via the sequence pipeline - (void)audio_peak; - (void)aspect_ratio; - (void)time; - (void)beat_time; - (void)beat_phase; -} - void gpu_resize(int width, int height) { if (width <= 0 || height <= 0) return; diff --git a/src/gpu/gpu.h b/src/gpu/gpu.h index bbced41..c5d0123 100644 --- a/src/gpu/gpu.h +++ b/src/gpu/gpu.h @@ -34,8 +34,6 @@ struct RenderPass { }; void gpu_init(PlatformState* platform_state); -void gpu_draw(float audio_peak, float aspect_ratio, float time, float beat_time, - float beat_phase); void gpu_resize(int width, int height); void gpu_shutdown(); @@ -92,6 +90,11 @@ gpu_create_render_pass(WGPUDevice device, const char* shader_code, ResourceBinding* bindings, int num_bindings); +// Create a shader module from WGSL source code +WGPUShaderModule gpu_create_shader_module(WGPUDevice device, + const char* wgsl_code, + const char* label = "shader"); + // Common sampler configurations WGPUSampler gpu_create_linear_sampler(WGPUDevice device); WGPUSampler gpu_create_nearest_sampler(WGPUDevice device); diff --git a/src/gpu/gpu_headless.cc b/src/gpu/gpu_headless.cc index 547ca18..c60da00 100644 --- a/src/gpu/gpu_headless.cc +++ b/src/gpu/gpu_headless.cc @@ -48,15 +48,6 @@ void gpu_init(PlatformState* platform_state) { } } -void gpu_draw(float audio_peak, float aspect_ratio, float time, float beat_time, - float beat_phase) { - (void)audio_peak; - (void)aspect_ratio; - (void)time; - (void)beat_time; - (void)beat_phase; -} - void gpu_resize(int width, int height) { (void)width; (void)height; diff --git a/src/gpu/gpu_stub.cc b/src/gpu/gpu_stub.cc index d889666..246c3a6 100644 --- a/src/gpu/gpu_stub.cc +++ b/src/gpu/gpu_stub.cc @@ -41,15 +41,6 @@ void gpu_init(PlatformState* platform_state) { (void)platform_state; } -void gpu_draw(float audio_peak, float aspect_ratio, float time, float beat_time, - float beat_phase) { - (void)audio_peak; - (void)aspect_ratio; - (void)time; - (void)beat_time; - (void)beat_phase; -} - void gpu_resize(int width, int height) { (void)width; (void)height; diff --git a/src/gpu/pipeline_builder.cc b/src/gpu/pipeline_builder.cc index 2d9ec07..acd2ae9 100644 --- a/src/gpu/pipeline_builder.cc +++ b/src/gpu/pipeline_builder.cc @@ -1,5 +1,6 @@ // WGPU render pipeline builder - implementation #include "gpu/pipeline_builder.h" +#include "gpu/gpu.h" #include "util/fatal_error.h" RenderPipelineBuilder::RenderPipelineBuilder(WGPUDevice device) @@ -15,12 +16,8 @@ RenderPipelineBuilder& RenderPipelineBuilder::shader(const char* wgsl, shader_text_ = compose ? ShaderComposer::Get().Compose({}, wgsl) : wgsl; if (device_ == nullptr) return *this; - WGPUShaderSourceWGSL wgsl_src{}; - wgsl_src.chain.sType = WGPUSType_ShaderSourceWGSL; - wgsl_src.code = str_view(shader_text_.c_str()); - WGPUShaderModuleDescriptor shader_desc{}; - shader_desc.nextInChain = &wgsl_src.chain; - shader_module_ = wgpuDeviceCreateShaderModule(device_, &shader_desc); + shader_module_ = + gpu_create_shader_module(device_, shader_text_.c_str(), "pipeline_shader"); desc_.vertex.module = shader_module_; desc_.vertex.entryPoint = str_view("vs_main"); return *this; @@ -90,5 +87,7 @@ WGPURenderPipeline RenderPipelineBuilder::build() { WGPURenderPipeline pipeline = wgpuDeviceCreateRenderPipeline(device_, &desc_); wgpuPipelineLayoutRelease(layout); + if (shader_module_) + wgpuShaderModuleRelease(shader_module_); return pipeline; } diff --git a/src/gpu/post_process_helper.cc b/src/gpu/post_process_helper.cc index 79fda20..871a238 100644 --- a/src/gpu/post_process_helper.cc +++ b/src/gpu/post_process_helper.cc @@ -39,33 +39,13 @@ WGPURenderPipeline create_post_process_pipeline(WGPUDevice device, return pipeline; } -// Helper to create a simple post-processing pipeline (no effect params) -WGPURenderPipeline -create_post_process_pipeline_simple(WGPUDevice device, WGPUTextureFormat format, - const char* shader_code) { - // Headless mode: skip pipeline creation (compiled out in STRIP_ALL) - HEADLESS_RETURN_VAL_IF_NULL(device, nullptr); - - WGPUBindGroupLayout bgl = - BindGroupLayoutBuilder() - .sampler(PP_BINDING_SAMPLER, WGPUShaderStage_Fragment) - .texture(PP_BINDING_TEXTURE, WGPUShaderStage_Fragment) - .uniform(PP_BINDING_UNIFORMS, - WGPUShaderStage_Vertex | WGPUShaderStage_Fragment) - .build(device); - - const std::string composed_shader = - ShaderComposer::Get().Compose({}, shader_code); - - WGPURenderPipeline pipeline = RenderPipelineBuilder(device) - .shader(composed_shader.c_str()) - .bind_group_layout(bgl) - .format(format) - .build(); - - wgpuBindGroupLayoutRelease(bgl); - return pipeline; -} +// NOTE: create_post_process_pipeline_simple() was removed (zero callers). +// If a 3-binding pipeline is needed in the future, add a `bool use_effect_params` +// parameter to create_post_process_pipeline() instead. +// Example: +// WGPURenderPipeline p = create_post_process_pipeline(device, format, code); +// // Then pass {nullptr, 0} as effect_params to pp_update_bind_group — +// // the dummy buffer fallback handles it automatically. void gpu_upload_mat4(WGPUQueue queue, WGPUBuffer buffer, size_t offset, const mat4& m) { diff --git a/src/gpu/post_process_helper.h b/src/gpu/post_process_helper.h index 178a4d5..8f2bd21 100644 --- a/src/gpu/post_process_helper.h +++ b/src/gpu/post_process_helper.h @@ -17,11 +17,6 @@ WGPURenderPipeline create_post_process_pipeline(WGPUDevice device, WGPUTextureFormat format, const char* shader_code); -// No effect params, 3 bindings only -WGPURenderPipeline create_post_process_pipeline_simple(WGPUDevice device, - WGPUTextureFormat format, - const char* shader_code); - void pp_update_bind_group(WGPUDevice device, WGPURenderPipeline pipeline, WGPUBindGroup* bind_group, WGPUTextureView input_view, GpuBuffer uniforms, GpuBuffer effect_params); diff --git a/src/gpu/sampler_cache.h b/src/gpu/sampler_cache.h index 0a2afa0..562db29 100644 --- a/src/gpu/sampler_cache.h +++ b/src/gpu/sampler_cache.h @@ -51,4 +51,8 @@ class SamplerCache { return {WGPUAddressMode_ClampToEdge, WGPUAddressMode_ClampToEdge, WGPUFilterMode_Linear, WGPUFilterMode_Linear, 1}; } + static SamplerSpec nearest() { + return {WGPUAddressMode_ClampToEdge, WGPUAddressMode_ClampToEdge, + WGPUFilterMode_Nearest, WGPUFilterMode_Nearest, 1}; + } }; diff --git a/src/gpu/wgsl_effect.cc b/src/gpu/wgsl_effect.cc index 0ce4730..1cb0ecb 100644 --- a/src/gpu/wgsl_effect.cc +++ b/src/gpu/wgsl_effect.cc @@ -36,19 +36,6 @@ void WgslEffect::render(WGPUCommandEncoder encoder, input_view, uniforms_buffer_.get(), params_buffer_.get()); - WGPURenderPassColorAttachment color_attachment = {}; - gpu_init_color_attachment(color_attachment, output_view); - color_attachment.loadOp = load_op_; - - WGPURenderPassDescriptor pass_desc = {}; - pass_desc.colorAttachmentCount = 1; - pass_desc.colorAttachments = &color_attachment; - - WGPURenderPassEncoder pass = - wgpuCommandEncoderBeginRenderPass(encoder, &pass_desc); - wgpuRenderPassEncoderSetPipeline(pass, pipeline_.get()); - wgpuRenderPassEncoderSetBindGroup(pass, 0, bind_group_.get(), 0, nullptr); - wgpuRenderPassEncoderDraw(pass, 3, 1, 0, 0); - wgpuRenderPassEncoderEnd(pass); - wgpuRenderPassEncoderRelease(pass); + run_fullscreen_pass(encoder, pipeline_.get(), bind_group_.get(), output_view, + load_op_); } -- cgit v1.2.3