Skip to content

Commit

Permalink
Cycles Renderer : Remove the session reset code
Browse files Browse the repository at this point in the history
This was an attempt to deal with a fundamental mismatch between the Cycles and Renderer APIs : you can't create a `ccl::session` without already having `ccl::SessionParams` (threads, shading system, device etc), but the values for those will arrive via `Renderer::option()` _after_ construction of the renderer. And unfortunately the Renderer constructor is the most natural place  to construct the `ccl::Session`.

Our strategy was to create a default session in the renderer constructor, and then store options into `m_sessionParams`, and then in `reset()` attempt to replace the original session with a new one constructed from `m_sessionParams`. This was made tricky by the fact that the session also owns the scene, and we needed to transplant all the old scene objects into the new scene. This almost worked, but had several flaws :

- On Windows, we were getting deadlocks due to deleting the scene lock while holding it. This could have been fixed separately while keeping the reset concept alive - see GafferHQ#5101.
- The Cycles objects we transplanted had pointers to the old scene, and this would trigger assertions when freeing them in debug builds. I suspect they were also the cause of the crash exposed by `InteractiveCyclesRenderTest.testShadingModes` - with the reset code removed, the test now passes.
- More generally, it was working against the Cycles API, and could be broken in any number of ways by future Cycles changes.

Just removing the `reset()` code isn't the end of the story though : it means that the options for devices, threads etc are now completely non-functioning.  This results in test failures for `RendererTest.testCustomAOV` and `RendererTest.testOSLInSVMShadingSystem`. We'll deal with that in future commits : the purpose of this commit is to document why the old approach wasn't working, and clear the decks a little in preparation for a new approach.
  • Loading branch information
johnhaddon committed Feb 26, 2024
1 parent 961d2a9 commit f8d3cb5
Showing 1 changed file with 7 additions and 75 deletions.
82 changes: 7 additions & 75 deletions src/GafferCycles/IECoreCyclesPreview/Renderer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -542,9 +542,8 @@ class ShaderCache : public IECore::RefCounted
{
}

void update( ccl::Scene *scene, NodesCreated &shaders )
void update( NodesCreated &shaders )
{
m_scene = scene;
updateShaders( shaders );
}

Expand Down Expand Up @@ -1499,9 +1498,8 @@ class InstanceCache : public IECore::RefCounted
{
}

void update( ccl::Scene *scene, NodesCreated &object, NodesCreated &geometry )
void update( NodesCreated &object, NodesCreated &geometry )
{
m_scene = scene;
updateObjects( object );
updateGeometry( geometry );
}
Expand Down Expand Up @@ -1796,9 +1794,8 @@ class LightCache : public IECore::RefCounted
{
}

void update( ccl::Scene *scene, NodesCreated &nodes )
void update( NodesCreated &nodes )
{
m_scene = scene;
updateLights( nodes );
}

Expand Down Expand Up @@ -3166,17 +3163,7 @@ class CyclesRenderer final : public IECoreScenePreview::Renderer
m_sessionParams.device = firstCPUDevice();
}

if( m_session )
{
// A trick to retain the same pointer when re-creating a session.
m_session->~Session();
new ( m_session ) ccl::Session( m_sessionParams, m_sceneParams );
}
else
{
m_session = new ccl::Session( m_sessionParams, m_sceneParams );
}

m_session = new ccl::Session( m_sessionParams, m_sceneParams );
m_session->progress.set_update_callback( function_bind( &CyclesRenderer::progress, this ) );

m_scene = m_session->scene;
Expand Down Expand Up @@ -3205,9 +3192,9 @@ class CyclesRenderer final : public IECoreScenePreview::Renderer
// Add every shader each time, less issues
m_shaderCache->nodesCreated( m_shadersCreated );

m_lightCache->update( m_scene, m_lightsCreated );
m_instanceCache->update( m_scene, m_objectsCreated, m_geometryCreated );
m_shaderCache->update( m_scene, m_shadersCreated );
m_lightCache->update( m_lightsCreated );
m_instanceCache->update( m_objectsCreated, m_geometryCreated );
m_shaderCache->update( m_shadersCreated );
}

void updateOptions()
Expand Down Expand Up @@ -3266,18 +3253,6 @@ class CyclesRenderer final : public IECoreScenePreview::Renderer
//film->tag_update( m_scene );
integrator->tag_update( m_scene, ccl::Integrator::UPDATE_ALL );
}

// If anything changes in scene or session, we reset.
if( m_scene->params.modified( m_sceneParams ) ||
m_session->params.modified( m_sessionParams )
)
{
// Flag it true here so that we never mutex unlock a different scene pointer due to the reset
if( m_renderState != RENDERSTATE_RENDERING )
{
reset();
}
}
}

void updateOutputs()
Expand Down Expand Up @@ -3501,49 +3476,6 @@ class CyclesRenderer final : public IECoreScenePreview::Renderer
m_outputsChanged = false;
}

void reset()
{
m_session->cancel();
m_renderState = RENDERSTATE_READY;
// This is so cycles doesn't delete the objects that Gaffer manages.
m_scene->objects.clear();
m_scene->geometry.clear();
m_shaderCache->flushTextures();
m_scene->shaders.resize( m_shaderCache->numDefaultShaders() );
m_scene->lights.clear();

const ccl::Integrator integratorCopy = *m_scene->integrator;
const ccl::Background backgroundCopy = *m_scene->background;
const ccl::Film filmCopy = *m_scene->film;

init();

// Re-apply the settings for these.
for( const ccl::SocketType &socketType : m_scene->integrator->type->inputs )
{
m_scene->integrator->copy_value(socketType, integratorCopy, *integratorCopy.type->find_input( socketType.name ) );
}
for( const ccl::SocketType &socketType : m_scene->background->type->inputs )
{
m_scene->background->copy_value(socketType, backgroundCopy, *backgroundCopy.type->find_input( socketType.name ) );
}
for( const ccl::SocketType &socketType : m_scene->film->type->inputs )
{
m_scene->film->copy_value(socketType, filmCopy, *filmCopy.type->find_input( socketType.name ) );
}

m_scene->background->set_shader( m_scene->default_background );

m_scene->shader_manager->tag_update( m_scene, ccl::ShaderManager::UPDATE_ALL );
m_scene->integrator->tag_update( m_scene, ccl::Integrator::UPDATE_ALL );
m_scene->background->tag_update( m_scene );

m_session->stats.mem_peak = m_session->stats.mem_used;
// Make sure the instance cache points to the right scene.
updateSceneObjects( true );
m_scene->geometry_manager->tag_update( m_scene, ccl::GeometryManager::UPDATE_ALL );
}

void updateCamera()
{
// Check that the camera we want to use exists,
Expand Down

0 comments on commit f8d3cb5

Please sign in to comment.