Cycles: Implement persistent storage for all scene data
Needs ReviewPublic

Authored by Lukas Stockner (lukasstockner97) on Apr 9 2017, 5:04 PM.

Details

Summary

Currently Persistent Data only keeps images in memory, this patch extends it to the full Scene and BlenderSync.

But, for preview animation renders, syncing can easily take more time than the rendering itself. When people start to use viewport rendering and screenshot every frame just to avoid expensive resyncs, something has to be fixed...

One hack that I'm not really happy about in this patch is the "update ID". The problem there is that for viewport rendering, the depsgraph tags updated objects and Cycles uses that info for resyncing. But, with persistent data for regular rendering, another way for detecting updates is needed. Therefore, a counter is added that's incremented with every update - by keeping the previous counter in memory, Cycles can check whether an object needs to be updated.

For quick preview animations containing only small changes between frames (like only having camera animation), this can easily save 50% of the render time or more.

Diff Detail

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

Nice, this is a feature I've always wantend, as evidenced by the optimistic persistent_data property name. Also for future network rendering this should be nice, where scene export can more easily become the bottleneck even for final frames.

The update ID thing may be unavoidable, I can't immediately think of a better solution. Support for this in the API has been requested before, though in most cases it was possible to track this outside Blender using is_updated, but for rendering that's harder. Note that is_updated also exists for datablock lists instead of just individual datablocks, to detect removal of datablocks of a certain type. I think we'd need an update_id for that as well?

If we go this way, we might as well use this instead of is_updated in Cycles I think. We can then also stop tracking updates when the viewport render is paused, or when the timeout to restart the render has not passed yet.

intern/cycles/render/nodes.cpp
272

{ on new line.

476

{ on new line.

1400

{ one new line.

source/blender/makesdna/DNA_ID.h
134

Should be unsigned short at least I think, negative id's would be a bit strange even if they might work ok.

I wonder if 65536 updates and wrap around might be possible in practice, probably very unlikely to only check after so many updates?

source/blender/makesrna/intern/rna_ID.c
980

One danger here is that users might write update_id > last_update_id instead of update_id != last_update_id. So perhaps a note about wrap around is needed.

source/blender/makesdna/DNA_ID.h
134

Also a question here is what should happen with undo and file loading. Maybe it should just be reset to 0.

It might be tempting to keep this value and then someone might use it for something crazy like version control, but that would not be reliable under undo/redo and wrap around.

While i agree this is a useful feature, the changes in ID and depsgraph i'm quite skeptical about. Such an update detection is something we are changing now in 2.8 branch, together with ownership of data and depsgraph. There is unlikely to be a need for update_id since depsgraph will more like per-render-engine. If we expose anything to API now, it'll be either extra legacy crap to support in the new design or it'll be yet another point of scripts failure. Either of this two things i am not looking forward.

Taking into account the fact, that update detection is rather weak (it exposes whole can of warms of intermediate render errors users will start encountering), perhaps it's not a bad idea to hold this off for until we get all the depsgraph, engines and tagging figured out first.

intern/cycles/blender/blender_sync.cpp
137

Would be using const qualifier where possible.

There is also now seems to be a difference between is_updated and is_updated_data(). Is that intentional?

intern/cycles/render/shader.cpp
517

Please use explicit foo != NULL for pointers comparison. Same applies to cases below.

source/blender/makesdna/DNA_ID.h
134

We might have exactly 65536 updates and then you'll be screwed. Also note that it's not 65536 real updates, those are 65536 tags, which isn't so much unlikely to happen between render attempts.

Hi Lukas,
this patch saves huge amount of preprocessing times, but seem to only work in simple scenes. The BMW benchmark uses it perfectly, but rendering the Viktor scene twice does the whole preprocessing a second time. As it's a really useful patch, an update would be really awesome :)