<div dir="ltr"><div>Great work! I committed Stefan's patch now.</div><div><br></div><div>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:<br></div><div><br></div><div>* Build error, CUDA and OpenCL device code needs to be updated to use the same code as CPU device. Add <span style="font-family:monospace,monospace">grid_info</span> size directly in <span style="font-family:monospace,monospace">device_memory::memory_size()</span> so the code is not duplicated.<br><span style="font-family:monospace,monospace">    intern/cycles/device/device_cuda.cpp:1024:25: error: ‘void*’ is not a pointer-to-object type<br>        size += mem.grid_info->memory_size();</span></div><div>* Building with OSL seems to be broken due to some conflict with our <span style="font-family:monospace,monospace">foreach()</span> 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.<span style="font-family:monospace,monospace"><br></span></div><div>* Please remove device_memory_openvdb.h and related code to use OpenVDB accessors in the kernel, we won't merge this into master.</div><div>* 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?<br></div><div>* 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.<br></div><div>* In <span style="font-family:monospace,monospace">shader_eval_volume()</span>, move setting <span style="font-family:monospace,monospace">sd->P_v = volume_get_position(kg, sd);</span> out of <span style="font-family:monospace,monospace">#ifdef __OBJECT_MOTION__</span>. We still need this even when compiling without motion blur support.</div><div>* In <span style="font-family:monospace,monospace">ShaderData</span>, don't put <span style="font-family:monospace,monospace">P_v</span> 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.</div><div>* Rename <span style="font-family:monospace,monospace">TILE_SIZE, LAST_TILE_WIDTH_SHIFT</span> to all have a <span style="font-family:monospace,monospace">TEX_</span> prefix, to avoid potential confusion about what these are used for.<br></div><div>* The CUDA code in <span style="font-family:monospace,monospace">compute_sparse_coordinates()</span> I guess is unfinished, since <span style="font-family:monospace,monospace">*ix</span> is being used without initialized and the result of <span style="font-family:monospace,monospace">modff()</span> is not used, etc. I can help finishing this code if needed.</div><div>* <span style="font-family:monospace,monospace">ImageManager::name_from_grid_type()</span> can be a static function rather than a class member function. Also rename "default" to "dense" in there.</div><div>* In <span style="font-family:monospace,monospace">MeshManager::device_update_attributes()</span>, add <span style="font-family:monospace,monospace">(mesh->motion_steps > 1)</span> to the test to only request velocity if motion blur is enabled.</div><div>* 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.<br></div><div>* For OpenVDB, I prefer to get rid of the dependency on <span style="font-family:monospace,monospace">openvdb_capi.h </span>and<span style="font-family:monospace,monospace"> intern/openvdb_reader.h. </span>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.</div><div>* On the Blender side, rename <span style="font-family:monospace,monospace">filter_openvdb</span> to <span style="font-family:monospace,monospace">filter_volume</span>, <span style="font-family:monospace,monospace">FILE_TYPE_OPENVDB</span> to <span style="font-family:monospace,monospace">FILE_TYPE_VOLUME</span>, <span style="font-family:monospace,monospace">is_openvdb</span> to <span style="font-family:monospace,monospace">use_volume_file</span>, and <span style="font-family:monospace,monospace">openvdb_filepath</span> to <span style="font-family:monospace,monospace">volume_filepath</span>. Not so important now, but a bit more future proof in case we support more file types in the future.</div><div>* 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.<br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr">On Sat, Jul 28, 2018 at 6:03 PM Geraldine Chua <<a href="mailto:chua.gsk@gmail.com" target="_blank">chua.gsk@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">This week was spent mostly reviewing and testing everything done in the past 10 weeks, and making revisions for better optimizations.<br><br>Some changes:<br><ul><li>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.</li><li>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).</li><li>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).</li><li>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.</li><li>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!)</li></ul><br>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 (<a href="https://developer.blender.org/P756" target="_blank">https://developer.blender.org/P756</a>).<br><br><b>To-dos next week</b><br><br>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.<br></div>
-- <br>
Soc-2018-dev mailing list<br>
<a href="mailto:Soc-2018-dev@blender.org" target="_blank">Soc-2018-dev@blender.org</a><br>
<a href="https://lists.blender.org/mailman/listinfo/soc-2018-dev" rel="noreferrer" target="_blank">https://lists.blender.org/mailman/listinfo/soc-2018-dev</a><br>
</blockquote></div>