Page MenuHome

Fix T71123: OptiX error in Cycles viewport when adding HDRI
ClosedPublic

Authored by Patrick Mours (pmoursnv) on Oct 28 2019, 4:01 PM.

Details

Summary

Adding an environment texture to the background causes a SHADER_EVAL_BACKGROUND launch. The OptiX implementation only creates the pipeline for that launch when kernels were loaded with the "use_background_light" feature and that feature is only set if the background light is enabled. But during synchronization with Cycles the "is_enabled" flag is never updated again after it was initially set. It is set to "false" via "LightManager::disable_ineffective_light" while no background texture node existed, but never set to "true" again after one was added. As such kernels were not reloaded with the required feature and the launch failed.

This change sets the "is_enabled" flag back to its default value of "true" when lights are synced. It is then later updated in "LightManager::disable_ineffective_light" if necessary as before. But now it retains the correct value after a background node was added to the scene.

Diff Detail

Repository
rB Blender
Branch
cycles_viewport_hdri_fix (branched from master)
Build Status
Buildable 5485
Build 5485: arc lint + arc unit

Event Timeline

That is a good catch!

I feel like the fix belongs to light.cpp: is not blender-integration specific, but specific to optimization happening in light.cpp.
It is almost like a missing loop in there to enable background:

1diff --git a/intern/cycles/render/light.cpp b/intern/cycles/render/light.cpp
2index 8c7a21da561..dc3f7c8f8ac 100644
3--- a/intern/cycles/render/light.cpp
4+++ b/intern/cycles/render/light.cpp
5@@ -221,13 +221,11 @@ void LightManager::disable_ineffective_light(Scene *scene)
6 */
7 Shader *shader = (scene->background->shader) ? scene->background->shader :
8 scene->default_background;
9- bool disable_mis = !(has_portal || shader->has_surface_spatial_varying);
10- if (disable_mis) {
11- VLOG(1) << "Background MIS has been disabled.\n";
12- foreach (Light *light, scene->lights) {
13- if (light->type == LIGHT_BACKGROUND) {
14- light->is_enabled = false;
15- }
16+ const bool disable_mis = !(has_portal || shader->has_surface_spatial_varying);
17+ VLOG_IF(1, disable_mis) << "Background MIS has been disabled.\n";
18+ foreach (Light *light, scene->lights) {
19+ if (light->type == LIGHT_BACKGROUND) {
20+ light->is_enabled = !disable_mis;
21 }
22 }
23 }

Otherwise every integration will need to have the fix on their side.

Good point. That along doesn't fix it though, because kernels are not reloaded between the calls to disable_ineffective_light and device_update_background (which is where the launch happens) in the light manager update routine. So the light needs to be enabled before the update already.

I think having both the light.cpp code + the blender_object.cpp code makes sense. Considering that the light is first created in blender_object.cpp via light_map.sync (which sets is_enabled to true in the constructor), it seems right for it to set is_enabled again during an update there too.

Fixed background light update in light manager too

@Sergey Sharybin (sergey) Should this still go into 2.81? Seems like a small fix without much potential for breaking stuff. But Bcon3 is almost over.

Is still good for 2.81 branch. Please go ahead!

This revision is now accepted and ready to land.Nov 4 2019, 6:03 PM