CyclesRenderer : Prep for refactoring the session reset code #5696
Conversation
And document their unfortunate reason for existence.
This is all dead code because we don't define `WITH_CYCLES_TEXTURE_CACHE`, and the current Cycles release doesn't include the feature. We can reintroduce this in future if Cycles _does_ add the feature, but until then the less code we have to maintain, the better.
It was unclear what the "source of truth" was here - the nodes in the `ccl::Scene` or the member data. It looks like the member data was always just being used to store a copy of the scene data so that we could restore it after doing a scene reset. We don't need member data for that - we can just use temporary variables in `reset()` itself. The less member state we have to track, the better.
The default in Cycles is now 1.0.
As the new test (courtesy of Alex via GafferHQ#5101) shows, we don't need to delete OSL shaders when in SVM mode to avoid Cycles crashes (but perhaps this was not always the case?). This avoids a linear-in-the-number-of-shaders check on every render, and simplifies some logic. The outcome is now no longer dependent on whether the SVM/OSL mismatch occurs before or after rendering has started : in both cases we now just give Cycles what the user asked for, and let it ignore shaders it can't handle. We could really do with a warning when there are OSL shaders in an SVM render, but that will be easier to achieve without expensive checks after a bit more refactoring. Look out for it in an upcoming commit.
This doesn't need to be member data, and it's easier to reason about the code if there is less persistent state to track.
There was a problem hiding this comment.
Thanks John! This prep all looks good to me, there's a little bit of cruft to clean up in Changes.md, but otherwise this looks good to merge in advance of the session reset PR.
The only slightly contentious thing is the silent omission of OSL shaders in SVM mode, but from the sounds of it you have a plan for that.
Changes.md
Outdated
| - CyclesOptions : | ||
| - Removed `useFrameAsSeed` plug. The frame is now automatically used as the seed if `seed` is not set. | ||
| - Removed `cryptomatteAccurate`. This feature is no longer present in Cycles. | ||
| - Removed all texture cache options. These had never been exposed in the UI because this never became an offical Cycles feature.>>>>>>> bb7bdf0cb (CyclesOptions, CyclesRenderer : Remove `useFrameAsSeed`) |
There was a problem hiding this comment.
Little bit of merge detritus at the end here.
There was a problem hiding this comment.
Thanks for spotting that - I've squashed in a fix and merged.
| @@ -1003,6 +1003,39 @@ if env["ARNOLD_ROOT"] : | |||
| # Definitions for the libraries we wish to build | |||
| ############################################################################################### | |||
|
|
|||
| # When Cycles is built, it uses several preprocessor variables that enable and | |||
There was a problem hiding this comment.
Indeed this was real frustration - I do have a patch for them to merge that fills out a proper config header via CMake, but until then this is the best we can do...
Changes.md
Outdated
| - CyclesOptions : | ||
| - Removed `useFrameAsSeed` plug. The frame is now automatically used as the seed if `seed` is not set. | ||
| - Removed `cryptomatteAccurate`. This feature is no longer present in Cycles. | ||
| - Removed all texture cache options. These had never been exposed in the UI because this never became an offical Cycles feature.>>>>>>> bb7bdf0cb (CyclesOptions, CyclesRenderer : Remove `useFrameAsSeed`) |
There was a problem hiding this comment.
A shame they never got that in, I was using this one with special Blackbird builds but a good call to just remove it until they merge something in later...
| @@ -3800,6 +3655,10 @@ class CyclesRenderer final : public IECoreScenePreview::Renderer | |||
| AttributesCachePtr m_attributesCache; | |||
|
|
|||
| // Nodes created to update to Cycles | |||
| /// \todo I don't see why these need to be state on the Renderer. | |||
There was a problem hiding this comment.
Yeah I'm curious to see how you approach this one, I put this here because it was a way to store newly-created nodes to hand off in a deferred single-threaded way under 1 lock and something done after any resets that needed to be made. It could live in the caches probably too as members, or we do scoped scene locks like the hydra delegate does and use the newer create_node calls that came after in later Cycles versions: https://github.com/blender/cycles/blob/main/src/scene/scene.h#L203
There was a problem hiding this comment.
Thanks for the info Alex. Initially I think I'll probably just squirrel these away inside the caches to help separate things out a bit, but longer term I definitely want to explore doing it on the fly with each thread taking the scene lock it needs it.
This didn't map to an actual Cycles parameter, and made for a more complex mechanism than the one we use for Arnold. Behaviour now matches Arnold, including using the correct default value for the frame. We don't expose the sampling seed for 3Delight yet, but this "use explicit seed or fall back to frame" behaviour will map very well to it in future. This might even make a good candidate for promoting to a standard option.
This feature was removed in the Cycles X timeframe - see : - https://www.mail-archive.com/[email protected]/msg149216.html - https://projects.blender.org/blender/blender/issues/87837 Curiously, the `CRYPT_ACCURATE` enum value still exists in the Cycles codebase, but looking at the source I can't see anything using it.
johnhaddon
force-pushed
the
cyclesPrep
branch
from
a5b4284
to
c8845a8
Compare
This simplifies the CyclesRenderer internals in preparation for refactoring the session reset code that has been causing so many problems. I've removed a bunch of redundant options and also some member variables that weren't necessary and which were making it harder to track the internal data dependencies. I'd like to get this reviewed separately so that a future session-reset-refactor PR can be focussed entirely on that topic, and not on this peripheral tidying.