From fc5c6ab374bb593ece6fe02a213fe9c1d1d953eb Mon Sep 17 00:00:00 2001 From: Lukas Stockner Date: Mon, 29 Sep 2025 03:12:18 +0200 Subject: [PATCH] Fix #146223: Cycles: Wrong output with multi-device OSL rendering We used to set shader->osl_surface_ref during shader compilation and then pushed it into the shared vectors. This worked as long as everything was serial - but after the multithreading change, we a) compile everything and then b) build the shared vectors since just pushing into them from multiple threads would not work. However, if there are multiple devices, then each shader will be compiled multiple times - so in the end, shader->osl_surface_ref etc. will be set to the last device's value. Then, we end up pushing that value into every device's vectors, which breaks for the earler devices. The fix is simple - just preallocate the vectors and pass the correct index into the compilation function. This way, each thread can safely store its result and we can get rid of shader->osl_surface_ref entirely. Note that while multiple shaders are compiled in parallel, the loop over devices for a given shader is serial, so there's no concern of conflicts over other shader internals. Pull Request: https://projects.blender.org/blender/blender/pulls/146617 --- intern/cycles/scene/osl.cpp | 57 +++++++++++++----------------------- intern/cycles/scene/osl.h | 2 +- intern/cycles/scene/shader.h | 17 ----------- 3 files changed, 21 insertions(+), 55 deletions(-) diff --git a/intern/cycles/scene/osl.cpp b/intern/cycles/scene/osl.cpp index fde9b0285da..c180508c967 100644 --- a/intern/cycles/scene/osl.cpp +++ b/intern/cycles/scene/osl.cpp @@ -661,22 +661,31 @@ void OSLShaderManager::device_update_specific(Device *device, og->displacement_state.clear(); og->bump_state.clear(); og->background_state.reset(); + + /* Allocate space for the shader groups. Needs to be done here so the multithreaded + * compilation can write to it safely. */ + og->surface_state.resize(scene->shaders.size()); + og->volume_state.resize(scene->shaders.size()); + og->displacement_state.resize(scene->shaders.size()); + og->bump_state.resize(scene->shaders.size()); }); /* create shaders */ Shader *background_shader = scene->background->get_shader(scene); - /* compile each shader to OSL shader groups */ + /* Compile each shader to OSL shader groups. */ TaskPool task_pool; - for (Shader *shader : scene->shaders) { - assert(shader->graph); + for (int shader_id = 0; shader_id < scene->shaders.size(); shader_id++) { + + auto compile = [scene, shader_id, background_shader](Device *sub_device, OSLGlobals *og) { + Shader *shader = scene->shaders[shader_id]; + assert(shader->graph); - auto compile = [scene, shader, background_shader](Device *sub_device, OSLGlobals *) { OSL::ShadingSystem *ss = scene->osl_manager->get_shading_system(sub_device); OSLCompiler compiler(ss, scene); compiler.background = (shader == background_shader); - compiler.compile(shader); + compiler.compile(og, shader_id, shader); }; task_pool.push([device, compile] { OSLManager::foreach_osl_device(device, compile); }); @@ -687,21 +696,8 @@ void OSLShaderManager::device_update_specific(Device *device, return; } - /* collect shader groups from all shaders */ + /* Tag updates after shader compilation is done. */ for (Shader *shader : scene->shaders) { - OSLManager::OSLManager::foreach_osl_device( - device, [shader, background_shader](Device *, OSLGlobals *og) { - /* push state to array for lookup */ - og->surface_state.push_back(shader->osl_surface_ref); - og->volume_state.push_back(shader->osl_volume_ref); - og->displacement_state.push_back(shader->osl_displacement_ref); - og->bump_state.push_back(shader->osl_surface_bump_ref); - - if (shader == background_shader) { - og->background_state = shader->osl_surface_ref; - } - }); - if (shader->emission_sampling != EMISSION_SAMPLING_NONE) { scene->light_manager->tag_update(scene, LightManager::SHADER_COMPILED); } @@ -1540,7 +1536,7 @@ OSL::ShaderGroupRef OSLCompiler::compile_type(Shader *shader, ShaderGraph *graph return std::move(current_group); } -void OSLCompiler::compile(Shader *shader) +void OSLCompiler::compile(OSLGlobals *og, int shader_id, Shader *shader) { if (shader->is_modified()) { ShaderGraph *graph = shader->graph.get(); @@ -1550,34 +1546,21 @@ void OSLCompiler::compile(Shader *shader) /* generate surface shader */ if (shader->reference_count() && shader->has_surface) { - shader->osl_surface_ref = compile_type(shader, graph, SHADER_TYPE_SURFACE); + og->surface_state[shader_id] = compile_type(shader, graph, SHADER_TYPE_SURFACE); if (has_bump) { - shader->osl_surface_bump_ref = compile_type(shader, graph, SHADER_TYPE_BUMP); + og->bump_state[shader_id] = compile_type(shader, graph, SHADER_TYPE_BUMP); } - else { - shader->osl_surface_bump_ref = OSL::ShaderGroupRef(); - } - } - else { - shader->osl_surface_ref = OSL::ShaderGroupRef(); - shader->osl_surface_bump_ref = OSL::ShaderGroupRef(); } /* generate volume shader */ if (shader->reference_count() && shader->has_volume) { - shader->osl_volume_ref = compile_type(shader, graph, SHADER_TYPE_VOLUME); - } - else { - shader->osl_volume_ref = OSL::ShaderGroupRef(); + og->volume_state[shader_id] = compile_type(shader, graph, SHADER_TYPE_VOLUME); } /* generate displacement shader */ if (shader->reference_count() && shader->has_displacement) { - shader->osl_displacement_ref = compile_type(shader, graph, SHADER_TYPE_DISPLACEMENT); - } - else { - shader->osl_displacement_ref = OSL::ShaderGroupRef(); + og->displacement_state[shader_id] = compile_type(shader, graph, SHADER_TYPE_DISPLACEMENT); } /* Estimate emission for MIS. */ diff --git a/intern/cycles/scene/osl.h b/intern/cycles/scene/osl.h index 5eb431e7dd3..7b13fa440f8 100644 --- a/intern/cycles/scene/osl.h +++ b/intern/cycles/scene/osl.h @@ -153,7 +153,7 @@ class OSLCompiler { #ifdef WITH_OSL OSLCompiler(OSL::ShadingSystem *ss, Scene *scene); #endif - void compile(Shader *shader); + void compile(OSLGlobals *og, int shader_id, Shader *shader); void add(ShaderNode *node, const char *name, bool isfilepath = false); diff --git a/intern/cycles/scene/shader.h b/intern/cycles/scene/shader.h index f14ddaa4c00..9036e757421 100644 --- a/intern/cycles/scene/shader.h +++ b/intern/cycles/scene/shader.h @@ -4,15 +4,6 @@ #pragma once -#ifdef WITH_OSL -# include /* Needed before `sdlexec.h` for `int32_t` with GCC 15.1. */ -/* So no context pollution happens from indirectly included windows.h */ -# ifdef _WIN32 -# include "util/windows.h" -# endif -# include -#endif - #include "kernel/types.h" #include "scene/attribute.h" @@ -128,14 +119,6 @@ class Shader : public Node { /* determined before compiling */ uint id; -#ifdef WITH_OSL - /* osl shading state references */ - OSL::ShaderGroupRef osl_surface_ref; - OSL::ShaderGroupRef osl_surface_bump_ref; - OSL::ShaderGroupRef osl_volume_ref; - OSL::ShaderGroupRef osl_displacement_ref; -#endif - Shader(); /* Estimate emission of this shader based on the shader graph. This works only in very simple