[Soc-2018-dev] Weekly Report #11 - Further Development for Cycles' Volume Rendering

[Brecht Van Lommel]

Hi,

That's fine, thanks for addressing my long list of comments.

What do you mean by mass import, selecting multiple VDB files in the file
selector and importing them all? Sounds good to me.

For the deformation motion blur option, we could perhaps have a separate
option for volumes, that might be easier for users to understand.

I'll find some time to test the GPU rendering stuff. Overall the changes
you made look good at first glance, thanks!

Brecht.

On Thu, Aug 2, 2018, 18:54 Geraldine Chua <chua.gsk at gmail.com> wrote:

> Hi Brecht,
>
> Sorry for the late reply, as I've been working on fixing as much of the
> stuff you've mentioned as possible.
>
> * File > Import of bunny_cloud.vdb still shows wrong results in the
>> viewport, I think you figure out that this was due to the grids starting at
>> a negative index or something like that. Is there a fix for this?
>>
>
> Oh, I had already fixed this in Cycles but forgot to add the fix to
> intern/openvdb. If you tried to render bunny_cloud.vdb, it would be
> actually positioned normally. I have patched it up now. (trying to view
> bunny_cloud in the viewport crashes Blender on my computer so I didn't
> notice it's wrong transform XD)
>
> * For the UI, when using OpenVDB import for smoke I suggest to just remove
>> all the buttons except for the file path and clipping. The "OpenVDB Import"
>> can be a hidden setting that is enabled when using File > Import, and there
>> is no need to be able to switch the smoke type, divisions, .. when it's all
>> coming from OpenVDB. It's all a bit of a hack but we might as well hide it
>> a bit more.
>>
>
> My current way of hiding all other smoke options is really hacky (too many
> nots...), so maybe there's a better way of doing it. Incidentally, would it
> be acceptable to implement mass import of VDB files for animation purposes?
> It basically reuses a lot of the VDB pointcache code, and would make the
> importer more immediately viable for users.
>
> * In MeshManager::device_update_attributes(), add (mesh->motion_steps > 1)
>> to the test to only request velocity if motion blur is enabled.
>> * Once this is done, the "Deformation" setting in the object "Motion
>> Blur" panel should allow disabling of volume motion blur. The tooltip of
>> that setting can then be adjusted to explain that.
>>
>
> So the way I interpreted this is, if "Deformation" is unchecked, Volume
> motion blur is disabled? That's how I implemented it but it seems a little
> unintuitive. Doesn't that option refer to dynamic mesh motion blur?
>
> * Please remove device_memory_openvdb.h and related code to use OpenVDB
>> accessors in the kernel, we won't merge this into master.
>>
>
> :( At least it was an interesting exercise to see if the VDB accessors
> worked well in the kernel.
>
> * Rename TILE_SIZE, LAST_TILE_WIDTH_SHIFT to all have a TEX_ prefix, to
>> avoid potential confusion about what these are used for.
>>
>
> For this, I created a new struct SparseTextureInfo, so now we're not
> passing 7 different variables in the kernel -.-
>
> I'm still not entirely satisfied with the variable names in
> SparseTextureInfo though. If anyone has suggestions, I am very open to
> changing them D:
>
> * On the Blender side, rename filter_openvdb to filter_volume,
>> FILE_TYPE_OPENVDB to FILE_TYPE_VOLUME, is_openvdb to use_volume_file, and
>> openvdb_filepath to volume_filepath. Not so important now, but a bit more
>> future proof in case we support more file types in the future.
>>
>
> I'm guessing that that the stuff in editors/io is fine since it
> specifically handles OpenVDB files so I didn't change those but I have
> updated everything else.
>
> * In shader_eval_volume(), move setting sd->P_v = volume_get_position(kg,
>> sd); out of #ifdef __OBJECT_MOTION__. We still need this even when
>> compiling without motion blur support.
>> * In ShaderData, don't put P_v at the very end of the struct, maybe put
>> it after the ray differentials. The closures need to be the last member
>> since that array has a dynamic size.
>> * ImageManager::name_from_grid_type() can be a static function rather
>> than a class member function. Also rename "default" to "dense" in there.
>> * For OpenVDB, I prefer to get rid of the dependency on openvdb_capi.h
>> and intern/openvdb_reader.h. These mostly exists because other Blender code
>> is C only, but for Cycles it's already using the OpenVDB headers itself, so
>> we might as well use them everywhere. At least as far as I can tell the
>> code would not become much more complicated, and it's easier to make it
>> work for Cycles integration into other software then.
>> * If you have the time, it would be nice to add support for loading
>> boolean and double OpenVDB grids as well. These would be converted to
>> floats, and I guess most handled by templating.
>>
>
> Done with all of these :)
>
> * Build error, CUDA and OpenCL device code needs to be updated to use the
>> same code as CPU device. Add grid_info size directly in
>> device_memory::memory_size() so the code is not duplicated.
>>     intern/cycles/device/device_cuda.cpp:1024:25: error: ‘void*’ is not a
>> pointer-to-object type
>>         size += mem.grid_info->memory_size();
>> * The CUDA code in compute_sparse_coordinates() I guess is unfinished,
>> since *ix is being used without initialized and the result of modff() is
>> not used, etc. I can help finishing this code if needed.
>>
>
> The reason grid_info's memory use doesn't directly get added to
> memory_size() is because I believe the function gets used for memory
> allocation, freeing, etc. so I didn't want to mess with it. In order to
> avoid having allocation errors, grid_info memory check is now logged
> separately.
>
> I've been holding back on fixing OpenCL until the CPU implementation was
> mostly finalized and I've had to completely rewrite the CUDA sparse grid
> generator since the old version worked on some incorrect assumptions and
> was generally very bad. I've uploaded my rewrite and in theory it should
> now sample properly (when I run the algorithm on CPU it samples fine), but
> unfortunately, I can't really test it out since my desktop's graphics card
> is broken... I would really be thankful for your help :)
>
> My main concern is how CUDA and OpenCL load textures in the kernel. For
> CPU rendering, it seems to be fine to not directly allocate the
> device_memory grid_info data and instead store a pointer to the data in the
> volume texture. This approach may not work out for CUDA/OpenCL. If it
> doesn't, I think the offsets device_memory will need to be allocated as an
> actual image.
>
> So right now, the main hurdles left are merging with 2.8(?) and making
> sure that sparse grids can be rendered on OpenCL/CUDA, unless there are any
> more issues with the current state of the branch.
>
> Best regards,
> Geraldine
>
>
> On Mon, Jul 30, 2018 at 12:20 AM, Brecht Van Lommel <
> brechtvanlommel at gmail.com> wrote:
>
>> Great work! I committed Stefan's patch now.
>>
>> So overall loading VDB files and rendering them as sparse grids seems to
>> be working well, which is what I hoped for from this project. Here's my
>> notes from reviewing the code:
>>
>> * Build error, CUDA and OpenCL device code needs to be updated to use the
>> same code as CPU device. Add grid_info size directly in
>> device_memory::memory_size() so the code is not duplicated.
>>     intern/cycles/device/device_cuda.cpp:1024:25: error: ‘void*’ is not a
>> pointer-to-object type
>>         size += mem.grid_info->memory_size();
>> * Building with OSL seems to be broken due to some conflict with our
>> foreach() macro and OpenVDB headers. I'll fix this in master as part of
>> making C++11 a minimum requirement, then we don't need a custom foreach
>> macro anymore and the conflict will be solved.
>> * Please remove device_memory_openvdb.h and related code to use OpenVDB
>> accessors in the kernel, we won't merge this into master.
>> * File > Import of bunny_cloud.vdb still shows wrong results in the
>> viewport, I think you figure out that this was due to the grids starting at
>> a negative index or something like that. Is there a fix for this?
>> * For the UI, when using OpenVDB import for smoke I suggest to just
>> remove all the buttons except for the file path and clipping. The "OpenVDB
>> Import" can be a hidden setting that is enabled when using File > Import,
>> and there is no need to be able to switch the smoke type, divisions, ..
>> when it's all coming from OpenVDB. It's all a bit of a hack but we might as
>> well hide it a bit more.
>> * In shader_eval_volume(), move setting sd->P_v =
>> volume_get_position(kg, sd); out of #ifdef __OBJECT_MOTION__. We still
>> need this even when compiling without motion blur support.
>> * In ShaderData, don't put P_v at the very end of the struct, maybe put
>> it after the ray differentials. The closures need to be the last member
>> since that array has a dynamic size.
>> * Rename TILE_SIZE, LAST_TILE_WIDTH_SHIFT to all have a TEX_ prefix, to
>> avoid potential confusion about what these are used for.
>> * The CUDA code in compute_sparse_coordinates() I guess is unfinished,
>> since *ix is being used without initialized and the result of modff() is
>> not used, etc. I can help finishing this code if needed.
>> * ImageManager::name_from_grid_type() can be a static function rather
>> than a class member function. Also rename "default" to "dense" in there.
>> * In MeshManager::device_update_attributes(), add (mesh->motion_steps >
>> 1) to the test to only request velocity if motion blur is enabled.
>> * Once this is done, the "Deformation" setting in the object "Motion
>> Blur" panel should allow disabling of volume motion blur. The tooltip of
>> that setting can then be adjusted to explain that.
>> * For OpenVDB, I prefer to get rid of the dependency on openvdb_capi.h
>> and intern/openvdb_reader.h. These mostly exists because other Blender
>> code is C only, but for Cycles it's already using the OpenVDB headers
>> itself, so we might as well use them everywhere. At least as far as I can
>> tell the code would not become much more complicated, and it's easier to
>> make it work for Cycles integration into other software then.
>> * On the Blender side, rename filter_openvdb to filter_volume,
>> FILE_TYPE_OPENVDB to FILE_TYPE_VOLUME, is_openvdb to use_volume_file,
>> and openvdb_filepath to volume_filepath. Not so important now, but a bit
>> more future proof in case we support more file types in the future.
>> * If you have the time, it would be nice to add support for loading
>> boolean and double OpenVDB grids as well. These would be converted to
>> floats, and I guess most handled by templating.
>>
>>
>> On Sat, Jul 28, 2018 at 6:03 PM Geraldine Chua <chua.gsk at gmail.com>
>> wrote:
>>
>>> This week was spent mostly reviewing and testing everything done in the
>>> past 10 weeks, and making revisions for better optimizations.
>>>
>>> Some changes:
>>>
>>>    - For motion blur, advected position is now cached in ShaderData and
>>>    only one sample is done (originally 3 samples were taken due to some
>>>    confusion over what was causing lack of blur). As an aside, normalized
>>>    position is also now cached, so there may be some modest speed-ups now in
>>>    rendering volumes even without motion blur.
>>>    - Sparse grids are now directly loaded into device memory rather
>>>    than loaded into a vector and then memcpy to device memory (used to be
>>>    based on how scaling images works).
>>>    - OpenVDBs are directly loaded as sparse grids (originally they were
>>>    loaded first as dense grids to allow some post-processing and then
>>>    converted to sparse grids).
>>>    - External VDBs may have background values that are not 0.0. In
>>>    theory, that means voxel values should be shifted to account for a
>>>    background value of 0.0 in Cycles, which is now done to all imported grids,
>>>    though whether this effect is required or not depends on the VDB imported.
>>>    - Changed how VDB values are iterated through from using accessors
>>>    to iterating through leaf nodes and acessing their buffers, greatly
>>>    decreasing iteration time (thanks to suggestion from Brecht!)
>>>
>>>
>>> Unrelated to my tasks, but the volume mesh builder has significant
>>> slowdown with very large volumes (e.g. a single 577x572x438 density grid
>>> takes ~25 seconds to build a mesh for), which is probably only somewhat
>>> obvious now when testing volumes from external VDBs. Stefan posted a patch
>>> that looks to fix this issue (https://developer.blender.org/P756).
>>>
>>> *To-dos next week*
>>>
>>> Next week should be mostly wrapping up, minor revisions, and merging. I
>>> will await code review to see if any new changes need to be made.
>>> --
>>> Soc-2018-dev mailing list
>>> Soc-2018-dev at blender.org
>>> https://lists.blender.org/mailman/listinfo/soc-2018-dev
>>>
>>
>> --
>> Soc-2018-dev mailing list
>> Soc-2018-dev at blender.org
>> https://lists.blender.org/mailman/listinfo/soc-2018-dev
>>
>>
> --
> Soc-2018-dev mailing list
> Soc-2018-dev at blender.org
> https://lists.blender.org/mailman/listinfo/soc-2018-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.blender.org/pipermail/soc-2018-dev/attachments/20180803/596b8e7f/attachment-0001.html>