Objects: Eevee and workbench rendering of new Volume, Hair, PointCloud
ClosedPublic
Actions

Authored by Brecht Van Lommel (brecht) on Feb 27 2020, 7:23 PM.

Details

Reviewers

Clément Foucault (fclem)

Maniphest Tasks

T73201: New volume object type

Commits

rBfd53b72871e0: Objects: Eevee and workbench rendering of new Volume, Hair, PointCloud

Summary

This is the code review that follows on to D6945 and D6952, see those for
details on the overall status and how to test.

Only the volume draw code should be considered ready. Hair plugs into the
existing hair rendering code and is fairly straightforward. The pointcloud
drawing is a hack using overlays rather than Eevee / workbench.

The most tricky part in this code is the case where each volume grid has
a different transform, which requires an additional matrix in the shader and
non-trivial logic in Eevee volume drawing. In the common case were all the
transforms match we don't use the additional per-grid matrix in the shader.

There is some room for optimization and simplification here. The orco
transform for meshes could be changed to a matrix (but where to store the
matrix since there exists no DRW_shgroup_uniform_mat4_copy?). Even if the
transforms are different, it is most likely only a translation difference
so we could avoid doing the full transform in that case.

Diff Detail

Repository

rB Blender

Build Status

Buildable 7150
Build 7150: arc lint + arc unit

Event Timeline

Brecht Van Lommel (brecht) created this revision.Feb 27 2020, 7:23 PM

Harbormaster completed remote builds in B6825: Diff 22147.Feb 27 2020, 7:23 PM

Brecht Van Lommel (brecht) added a parent revision: D6945: Objects: add Volume object type, and prototypes for Hair and PointCloud.Feb 27 2020, 7:24 PM

Brecht Van Lommel (brecht) added a parent revision: D6951: Smoke: put density/color in separate textures, fixes for workbench shader.

Brecht Van Lommel (brecht) edited parent revisions, added: D6952: Eevee: internal support for arbitrary number of volume grids; removed: D6951: Smoke: put density/color in separate textures, fixes for workbench shader.

Just arc patch D6955 should get you all the revisions to test this patch automatically, or you can use the new-object-types branch which also has the Cycles rendering part.

Rebase on master, add points drawing for volumes.

Harbormaster completed remote builds in B7150: Diff 22773.Mar 13 2020, 12:49 PM

Brecht Van Lommel (brecht) added a child revision: D7141: Objects: add infrastructure for hair, pointcloud, volume modifiers.Mar 13 2020, 12:55 PM

Honestly it's mostly style nitpick. It's just about codeflow inside eevee's code then I'll accept.

If you can take care of the transform simplification before merging, even better. But not a showstopper.

Pointcloud handling seems a bit strange to me currently but I get that's WIP so it's fine.

source/blender/draw/engines/eevee/eevee_materials.c
1954	A bit of a style issue: I would like the branching for other object type to be done inside `EEVEE_cache_populate`. Call `EEVEE_volumes_cache_object_add` dirrectly there.
source/blender/draw/engines/eevee/eevee_volumes.c
399	function name should be `eevee_volume_object_grid_init` or something similar. `cache_init` is reserved for the initialization of the cache before iteration over objects.
455	Use the `DRW_shgroup_uniform_*_copy` variants as much as you can. It is faster and cleaner: `DRW_shgroup_uniform_vec3_copy(grp, "volumeOrcoLoc", (float[3]){0.5f, 0.5f, 0.5f});`
459	Style: Use LISTBASE_FOREACH macro. Can also change other for loops in this file.
470	style nitpick: I prefer to have one call to `DRW_shgroup_uniform_texture` and the texture defined by a ternary expression just above.
476	If I understand, the transform is only a change of AABB inside the object space. Why not pass only 2 * vec3 and do a simple MADD in the shader? Also make it mandatory. I don't really think this adds so much overhead and would make the code simpler. Optimization and code cleanup can be done after the patch is being merged.
536	If `eevee_volume_object_cache_init` become simple enough after the suggested changes, don't separate it to another function to keep codeflow clearer.
597	Y u no love my ternary operator? :sad: I get that `white` is a bad constant name (maybe `volume_color_default` is better) but the `use_constant_color` bool is self documenting. Anyway, nitpicking.
source/blender/draw/engines/overlay/overlay_wireframe.c
152	I would put that below the `/* Fast path for duplis. */` and setup the fast path like for meshes before returning. I don't know if instances are to be supported in the future.
source/blender/draw/engines/workbench/shaders/workbench_volume_frag.glsl
139	Seems to have be a bit of code duplication here. Wouldn't hurt to unify more by having both idef branches define `emission` `shadows` `density`. Shader compiler should optimize the constants (at least it wouldn't be worse than the worst case).
source/blender/draw/engines/workbench/shaders/workbench_volume_vert.glsl
10	dont put ifdefs around uniform declarations. They are optimized out. As in : we never had problem with any drivers with unused uniforms ;).
source/blender/draw/engines/workbench/workbench_engine.c
354	Redundant comment?
source/blender/draw/engines/workbench/workbench_volume.c
247	Culling should automatically works with object boundbox. Of course this means the rendering should only draw inside the object bbox. if it's not, I can look up why.
source/blender/draw/intern/draw_cache.h
225	Should use `GPU_texture_get_mipmap_size()` to get the same. Unless there is a purpose for caching I don't see the point of having it here?
228	Would prefer to have only the AABB madd factors stored. (unless i'm wrong and AABB is not enough) This could be done after merge. Not really a problem.

This revision now requires changes to proceed.Mar 17 2020, 12:39 AM

Thanks for the review!

source/blender/draw/engines/eevee/eevee_volumes.c
476	It's not an AABB in general. The transform can include rotation, even if that's not the case for most .vdb files (I haven't actually come across any that I didn't generate myself). I considered assuming all grids have the same rotation and moving that to the object matrix, so this code becomes simpler. But at this point it's hard to know if that will cause problems for users. I plan to get some feedback about this to see if it's really worth supporting, or if we can simplify it to AABBs.
597	I was doing some refactoring to share code but that didn't end up happening. I'll change it back.
source/blender/draw/engines/workbench/shaders/workbench_volume_frag.glsl
139	Right, I duplicated this because the formulas for smoke density/color were wrong, but I fixed those in master now so it can be unified again.

Clément Foucault (fclem) added inline comments.Mar 17 2020, 1:39 PM

source/blender/draw/engines/workbench/shaders/workbench_volume_frag.glsl
132	Unrelated to this patch but this pow should be replaced by the texture using srgb texture format. This might change the look since the interpolation is going to happen in linear space.

Brecht Van Lommel (brecht) marked 14 inline comments as done.Mar 17 2020, 1:50 PM

Brecht Van Lommel (brecht) added inline comments.

source/blender/draw/engines/eevee/eevee_volumes.c
476	The code could also be simplified by always using a matrix. Though I found this a bit tricky due to there being no `mat4_copy`, it's not clear where to store the matrix then.
536	I don't see how it can become significantly simpler. I've now moved the mesh part into its own function as well, I think it's a bit more clear this way.
source/blender/draw/engines/overlay/overlay_wireframe.c
152	The issue is that `OVERLAY_extra_loose_points` packs the color in the matrix, which doesn't work with instancing. I guess mesh loose points and edges drawing has the same issue though, so I'll move the code below and perhaps both can be solved together.
source/blender/draw/intern/draw_cache.h
228	As mentioned before, AABB is not enough in general.