Details
Diff Detail
- Repository
- rB Blender
- Branch
- denoisingsplit2
- Build Status
Buildable 627 Build 627: arc lint + arc unit
Event Timeline
Update the host code to reflect changes in the other parts (also fixes a couple of OpenCL bugs).
Looks generally fine, mostly minor comments.
| intern/cycles/blender/blender_session.cpp | ||
|---|---|---|
| 429–435 | Could this be computed in DenoisingTask::init_from_devicetask? So that the code can be shared with non-Blender integrations. | |
| intern/cycles/device/device.h | ||
| 277 | mem_address_alignment() seems like a more clear name, not sure what offsets have to do with it. I'm not sure what the require alignment is on CUDA, and on older CPUs SSE needs alignment as well. I suggest to use 16 as the default value here instead of 1. | |
| 278–284 | Maybe rename to mem_alloc_sub_ptr and mem_free_sub_ptr. | |
| intern/cycles/device/device_memory.h | ||
| 178 | Would rename to memory_elements_size. | |
| 323 | Can you add a comment explaining what this class is? Perhaps device_sub_ptr would be a better name, following OpenCL terminology. | |
| intern/cycles/device/device_multi.cpp | ||
| 302 | Style: Device *sub_device, RenderTile *tiles. | |
| 308–311 | It took me a while to understand what's going on here, this comment would have been more clear for me: /* If the tile was rendered on another device, copy its memory to * to the current device now, for the duration of the denoising task. * Note that this temporarily modifies the RenderBuffers and calls * the device, so this function is not thread safe. */ | |
| 313 | Could this allocate a new device_vector<float> mem rather than referencing the global tiles[i].buffers->buffer and doing the original_ptr swapping? Not that this solves the thread safety entirely, but seems a bit unnecessary to access the global buffers here. | |
| 333 | Similar comment as above, not sure this access to global buffers is needed. | |
| intern/cycles/device/device_task.h | ||
| 68–69 | Same comment here, might as well use map/unmap terminology so it's consistent. | |
| intern/cycles/device/opencl/opencl_base.cpp | ||
| 613 | Why 4? | |
| intern/cycles/device/opencl/opencl_mega.cpp | ||
| 154 | { on new line. | |
| intern/cycles/device/opencl/opencl_split.cpp | ||
| 157 | { on new line. | |
| intern/cycles/device/opencl/opencl_util.cpp | ||
| 1076 | Would rename to mem_address_alignment. | |
| intern/cycles/render/buffers.cpp | ||
| 161–163 | Move this after if(!buffer.device_pointer) check. | |
| intern/cycles/render/session.cpp | ||
| 504 | Rename to map_neighbor_tiles / unmap_neighbor_tiles? | |
| intern/cycles/render/session.h | ||
| 225 | Maybe rename to render_tiles. | |
| intern/cycles/render/tile.cpp | ||
| 362 | Maybe rename to finish_tile? Seems a bit more clear to me. | |
| 384 | Would prefer to split this and similar code in this function over two lines. | |
| intern/cycles/render/tile.h | ||
| 39 | Would just name it State since it's already in the Tile namespace. | |
| 72 | This seems to serve no purpose, always set to NULL? | |
| intern/cycles/util/util_types.h | ||
| 309 ↗ | (On Diff #8687) | This is not used it seems, and there's align_up anyway. |
Addressed all review points, except for the separate device_vector.
It would definitely be nicer, but doing so would allocate the host memory again and the memory content would have to be copied onto the host (unless we change the device_vector to allow two vectors sharing the same host memory).
So, I'd love a clean way to get rid of the original_ptr trickery that doesn't allocate useless memory, but I don't really see one...
I don't think it's a problem to allocate some temporary memory on the host. Ideally we'd support direct memory copies between devices without the host eventually as well. I don't insist on doing that now though, the code works as it is.
Anyway, to me it seems we should commit this for 2.79. No need to mark it as an experimental feature I think, certainly the denoising results can be improved further over time, but I guess there aren't really problematic known bugs / gotchas. If denoising produces poor results, users can always render with more samples.
Some more comments on the UI:
- Could we avoid having the Cycles denoising properties in Blender DNA if D2444 is applied first? Would be nice if we could keep this part of the Cycles addon only.
- Can we remap the Strength and Feature Strength from -4..4 to 0..1 with default 0.5? The numbers don't really correspond to anything the user needs to know about, so might as well use range 0..1 then.
- Here's some tweaks for the Denoising panel:
