Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cycles : VDB volumes refactor #5373

Draft
wants to merge 1 commit into
base: 1.4_maintenance
Choose a base branch
from

Conversation

boberfly
Copy link
Collaborator

@boberfly boberfly commented Jul 2, 2023

Generally describe what this PR will do, and why it is needed

  • Refactored the VDB loading/rendering code for Cycles
  • It now defers voxel grid creation into the scene-lock
  • Shader modification now requires an expensive object rebuild, but now it reliably will update in a live render (this has been addressed)
  • The unit-test I can't figure out how to load in a vdb, need help on that one

Related issues

Dependencies

Not hard-dependencies, but I did apply this based on these two, so I am not sure how it'll go without them.

Breaking changes

  • GeometryAlgo now doesn't have a scene argument for the convert functions, as these were only for volumes and now scene is only needed in convertVoxelGrids

Checklist

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have tested my change(s) in the test suite, and added new test cases where necessary.
  • My code follows the Gaffer project's prevailing coding style and conventions.

TODO

  • I think we can do half-precision on vdb's, I'll add it to this when I figure that out. - Should be done now
  • Unit-test needs fixing.

@boberfly boberfly requested a review from johnhaddon July 2, 2023 13:26
@boberfly boberfly force-pushed the fixes/cycles_volume_update branch from bd29b32 to 7027247 Compare July 2, 2023 23:47
@boberfly
Copy link
Collaborator Author

boberfly commented Jul 3, 2023

Just a note for myself - after a few shader tweaks and/or attributes changes, there is a segfault here, or more specifically in Cycles itself:
https://github.com/GafferHQ/gaffer/blob/main/src/GafferCycles/IECoreCyclesPreview/Renderer.cpp#L826

When any old shaders assigned get de-referenced. Something to figure out and I'm not too sure yet why that is...

@boberfly boberfly force-pushed the fixes/cycles_volume_update branch from 7027247 to 27de35d Compare July 3, 2023 00:15
@boberfly
Copy link
Collaborator Author

Some more investigations:
https://projects.blender.org/blender/cycles/pulls/6

With this MR pull, I can remove including the shader hash + expensive rebuild of a shader per-shader update. My hunch why we run into some of these shader issues vs Blender is Blender tends to keep the same ccl::Shader pointer but rather updates the contents by changing the graph inside.

@johnhaddon I think I saw in GafferArnold that you have a mechanism to update already created shaders, is that something that we could do there also as an alternative approach? I wasn't sure how to determine when a shader is being updated vs created as you'd normally just get a shader and if the hash doesn't match, it requires a new one created.

@boberfly boberfly force-pushed the fixes/cycles_volume_update branch 2 times, most recently from 6094ed2 to 50f2674 Compare July 30, 2023 13:50
@boberfly boberfly changed the base branch from main to 1.3_maintenance July 30, 2023 13:50
@boberfly boberfly force-pushed the fixes/cycles_volume_update branch 2 times, most recently from ee12b6f to 98d2960 Compare July 30, 2023 14:15
@johnhaddon
Copy link
Member

I think I saw in GafferArnold that you have a mechanism to update already created shaders, is that something that we could do there also as an alternative approach?

Sorry for the ludicrous delay in replying to this. Yes, we do have something like that in IECoreArnold - ShaderNetworkAlgo::update(). We use it to update the network for light shaders, while keeping the terminal shader intact (because that is the light itself, and needs to remain stable for light linking). This is possible for lights because they always have their own unique shader network, rather than an "instanced" one - they don't go through the ShaderCache at all.

@boberfly
Copy link
Collaborator Author

boberfly commented Oct 1, 2024

Just adding here that the custom cycles patch I use to update shaders that's not a broken link:
https://projects.blender.org/boberfly/cycles/src/branch/volume_shader

Just to add also, this looks pretty exciting and a much-needed revamp of the volume integrator in cycles:
https://projects.blender.org/blender/blender/pulls/128389

@boberfly boberfly changed the base branch from 1.3_maintenance to 1.4_maintenance October 1, 2024 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants