Hey @bareya
Just a summary, what's the purpose of this? I am guessing it is to help out instancing (having the reference concept et-all) and I guess just a better way of syncing data?

I guess the only thing I'd suggest is to check the latest Cycles API changes in the latest Blender, there's a concept of setting ownership of data so that Hydra or whatever can take ownership and not Cycles itself (it's used for procedurals) to see if it's compatible with what you're doing (I guess it is).

Cheers!

Hey @boberfly

It's a draft, and this draft is a result of the work I have done next to Transformation Motion Blur PR (#148). I decided to separate those two PRs.

Just a summary, what's the purpose of this? I am guessing it is to help out instancing (having the reference concept et-all) and I guess just a better way of syncing data?

I mentioned all the problems with current design in our standups, but I will write them down for you as a list:

• Excessive locking - We should remove all direct access to the ccl::Scene from *::Sync methods and replace it by sub-classes that derive from HdBufferSource. *::Sync should spawn aforementioned sub-classes that are being collected by HdCyclesResourceRegistry and should be committed to the scene in one place. Direct access to ccl::Scene forces us to lock during the *::Sync calls. I am not sure if you remember, but I have done some profiling and during interactive Hydra session there is a lot of locking/unlocking happening just to check that PRim is not dirty, even if we don't commit any resources.

• Parallel object processing - *::Sync is being called from multiple threads, unfortunately we don't take advantage of it - scene locking. Deferring committing resources to HdCyclesResourceRegistry::CommitResources allows us to have an explicit control over parts can be processed in parallel.

• Committing resources through HdBufferSource - Currently we have a lot of code duplication. In most of the places code is copied just to create attributes. I recommend you to read code for curves and how curves handle arbitrary attribute creation. We just submit an instance of HdCyclesAttributeSource to HdCyclesResourceRegistry and attribute creation is handled independently of type of ccl::Geometry. Also, resolving an attribute happens in parallel. I recommend reading the code in HdCyclesResourceRegistry::_CommitResources and HdCyclesAttributeSource.

• Daisy chaining of HdBufferSource - Check HdBufferSource header file documentation. HdBufferSource::GetChainedBuffers comes super handy when it comes to chained computations such as normals creation for subdivision meshes. If we use chain of HdBufferSources we essentially build a dependency for computing attributes:

  Compute limit points -> Compute limit tangents -> Compute limit normals
  

  In current implementation we store limit u and v as fields(extra memory overhead) and when normals are requested, we compute them from aforementioned fields.

• Reduce memory - In many places we store data as fields for no reason. HdCyclesAttributeSource carries VtValue and the value is expired and removed right after HdCyclesAttributeSource instance is gone.

• Automatic binding/unbinding - HdInstance<> can automatically control lifespan of lights, materials and objects. Have a look into HdCyclesResourceRegistry committing code. Unbinding happens with help of unordered set, and I guess it comes down to linear complexity. Rather than being called by RPrims directly each time RPrim is removed by RenderIndex.

• Shared instances - I really recommend reading HdStMesh code. Topology, instances code can be shared between the objects. I really recommend reading about HdInstance and HdInstanceRegistry.

• Update only what is necessary - With explicit control of object creation/modification we can have centralized way of updating scene and session. Have a look into HdCyclesResourceRegistry committing code.

I guess the only thing I'd suggest is to check the latest Cycles API changes in the latest Blender, there's a concept of setting ownership of data so that Hydra or whatever can take ownership and not Cycles itself (it's used for procedurals) to see if it's compatible with what you're doing (I guess it is).

I will stick to current api for now, unless it becomes must have demand that comes from the production.

I hope that explains all the doubts you had about motivation for this PR.

Cheers!

Thank you for the clarity, I'm aware of your hard efforts addressing these things - it was just a little tricky to tell if this patch is directly related or something else/specific. Cheers!