EEVEE Next: Volumes #107176
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Blocks
#110653 EEVEE Next: Volumes: Lighting integration improvements
blender/blender
Reference: blender/blender#107176
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "pragma37/blender:pull-eevee-next-volumes"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Port of EEVEE unified volume rendering to EEVEE Next, using compute shaders.
Improvements:
Currently missing:
Main Task: #105672
First pass review. Still have a lot left.
@ -319,3 +319,3 @@
bl_label = "Volumetrics"
bl_options = {'DEFAULT_CLOSED'}
COMPAT_ENGINES = {'BLENDER_EEVEE'}
COMPAT_ENGINES = {'BLENDER_EEVEE', 'BLENDER_EEVEE_NEXT'}
Better duplicate the panel and mask unavailable options like shadows.
@ -0,0 +8,4 @@
class Instance;
class Volumes {
Volumes
could maybe renamed asVolumeModule
@ -191,3 +191,3 @@
# endif
vec3 g_orco;
/** WARNING: these are not attributes, these are global vars. */
Can we remove thoses? Seems like they were used for porting the old code but are unused now.
I think
g_orco
andobjectPosition
need to stay.I assumed the others were there for keeping compatibility with geometry-based rendering, even if they don't seem to be used.
But if you confirm it's not needed then they can be removed.
Yes, remove the one unused.
@ -116,3 +130,3 @@
Closure closure_eval(ClosureVolumeScatter volume_scatter)
{
/* TODO */
SELECT_CLOSURE(g_volume_scatter_data, g_volume_scatter_rand, volume_scatter);
Add a TODO. These do not need to use closure select and could just be combined.
@ -211,3 +227,3 @@
void attrib_load();
Closure nodetree_surface();
Closure nodetree_volume();
/* Closure nodetree_volume(); */
Why? In the future, we could have surface shaders evaluate the volume properties.
I don't know, it didn't feel right to add unused code to the generated shaders.
If we eventually need it, it should be easy to add it back.
@ -115,2 +116,4 @@
#endif
#ifdef MAT_TRANSPARENT
/* Volumes Integration */
This comment maybe introduce confusion. This could be confused with the actual integration which is another shader. This is the equivalent to the resolve step.
@ -0,0 +25,4 @@
vec3 ndc_cell = volume_to_ndc(vec3(uvs, 1e-5));
vec3 view_cell = get_view_space_from_depth(ndc_cell.xy, ndc_cell.z);
/* Ortho */
Code style. Use
bool is_persp
and useelse
clause for ortho.Done. But could we maybe add a
is_perpective
function tocommon_view_lib
?This is used in so many places that IMO it would make sense.
It would make sense. But this would be a separate cleanup.
@ -0,0 +60,4 @@
froxel_scattering = (froxel_scattering * (1.0f - froxel_transmittance)) /
max(vec3(1e-8), extinction);
/* accumulate and also take into account the transmittance from previous steps */
Comment style. This apply to the whole diff. Please fix existing comments when you copy them.
@ -0,0 +110,4 @@
return ld.color * ld.volume_power * power;
}
vec3 light_volume_light_vector(LightData ld, vec3 P)
Document this function. The name isn't clear by itself.
Actually, could this be merged into
eevee_light_eval_lib
?There's a
light_vector_get
there, but it doesn't handle area lights.Yes it can be moved. But this function returns the light vector to the closest point on the light shape and only for rects and ellipses as the other shapes use another method for area lighting. The function name and its documentation should reflect that.
@ -0,0 +4,4 @@
/* Based on Frosbite Unified Volumetric.
* https://www.ea.com/frostbite/news/physically-based-unified-volumetric-rendering-in-frostbite */
/* Step 4 : Apply final integration on top of the scene color. */
This shader should be part of the deferred pipeline shader. We shouldn't load the scene depth only for this pass.
This would have some implications though:
I can give it a try, but I would expect this approach would be slower?
Ok Leave it as is for now. It can be refactor after the deferred pipeline has matured.
@ -0,0 +63,4 @@
vec3 V = cameraVec(P);
vec2 phase = texelFetch(phase_tx, froxel, 0).rg;
float s_anisotropy = phase.x / max(1.0, phase.y);
This need a comment explaining why we divide by 2nd component.
To be honest I have no idea. Currently, only the first channel is ever written to.
It was like this in the current implementation so I left it there.
I assumed the second channel was intended to be used eventually.
It is used to store the number or phase that were written to it. This way we take the mean phase still using addive blending.
Oops! This should be solved now.
@ -0,0 +65,4 @@
vec2 phase = texelFetch(phase_tx, froxel, 0).rg;
float s_anisotropy = phase.x / max(1.0, phase.y);
/* Environment : Average color. */
This comment is obsolete.
@ -192,0 +204,4 @@
"eevee_light_data",
"eevee_shadow_data",
"eevee_sampling_data",
//"eevee_aov_out",
Remove the render passes they make no sense. in this context.
@ -0,0 +57,4 @@
.image(1, GPU_R11F_G11F_B10F, Qualifier::WRITE, ImageType::FLOAT_3D, "out_transmittance")
.do_static_compilation(true);
GPU_SHADER_CREATE_INFO(eevee_volume_resolve_common)
Doesnt seems to be necessary. Merge with eevee_volume_resolve.
@ -0,0 +65,4 @@
GPU_SHADER_CREATE_INFO(eevee_volume_resolve)
.additional_info("eevee_volume_resolve_common")
.fragment_out(0, Type::VEC4, "out_radiance", DualBlend::SRC_0)
This should also output to the volume renderpasses.
I think we should actually get final (f12) rendering working first before looking into this?
Ok but then add a TODO here.
For some reason, I can't mark conversations as solved, but all the ones I have not answered should be solved now.
set_jitter
f8bf15af0bI made it work on Metal.
I would say do not try to implement shadows in this patch as it is already quite complex as is.
There is some things that should be improved before landing it. It is mostly related to removing some old practice from the old engine.
@ -88,1 +88,4 @@
/* Volumes. */
#define VOLUME_GROUP_SIZE 4
#define VOLUME_2D_GROUP_SIZE 8
Rename to
VOLUME_INTEGRATION_GROUP_SIZE
.@ -426,0 +427,4 @@
/** \name Volumes
* \{ */
struct VolumesData {
This should be renamed to
VolumesInfoData
since this is not per object data.@ -0,0 +1,401 @@
/* SPDX-License-Identifier: GPL-2.0-or-later */
Remove plural form of Volume(s) in struct names and filenames.
@ -0,0 +3,4 @@
/** \file
* \ingroup draw_engine
*
* Volumetric effects rendering using frostbite approach.
Frostbite approach is too generic. Provide exact source title and like to the presentatino/ paper.
@ -0,0 +50,4 @@
namespace blender::eevee {
bool VolumeModule::GridAABB::init(Object *ob, const Camera &camera, const VolumesDataBuf &data)
Document functions that returns bool. Even more so when the function name is not explict.
Reminder that function documentation should be in header files.
@ -0,0 +52,4 @@
bool VolumeModule::GridAABB::init(Object *ob, const Camera &camera, const VolumesDataBuf &data)
{
auto to_global_grid_coords = [&](float3 wP) -> int3 {
This functino should be better documented.
@ -0,0 +83,4 @@
max = int3(INT32_MIN);
for (float3 corner : bbox.vec) {
corner = (float4x4(ob->object_to_world) * float4(corner, 1.0)).xyz();
Use
math::transform_point
@ -0,0 +118,4 @@
const Scene *scene_eval = inst_.scene;
const float3 viewport_size = DRW_viewport_size_get();
The viewport size should not be used as dimensions. Always use the render buffer size like light / shadow / motion blur do.
Also note that local use of DRW_context_state_get, DRW_viewport_size_get and the like are prohibited in Next engines.
@ -0,0 +189,4 @@
sync_world();
}
void VolumeModule::sync_world()
Move volume world to its own pipeline (eevee_pipeline.cc) for consistency.
@ -0,0 +217,4 @@
}
else {
/* If no world or volume material is present just clear the buffer. */
ps.shader_set(inst_.shaders.static_shader_get(VOLUME_CLEAR));
Use default world material for clearing instead of dedicated shader. The default output should be 100% transmission.
@ -0,0 +320,4 @@
float4 scatter = float4(0.0f);
float4 transmit = float4(1.0f);
dummy_scatter_tx_.ensure_3d(GPU_RGBA8, int3(1), GPU_TEXTURE_USAGE_SHADER_READ, scatter);
Better ensure them in the constructor. Also make sure their data is valid.
@ -0,0 +6,4 @@
* https://www.ea.com/frostbite/news/physically-based-unified-volumetric-rendering-in-frostbite */
/* Volume slice to view space depth. */
float volume_z_to_view_z(float z)
Move
view_z_to_volume_z
&volume_z_to_view_z
to shader shared file as the Bound box init use it.@ -0,0 +12,4 @@
namespace blender::draw {
bool volume_sub_pass(PassMain::Sub &ps, Scene *scene, Object *ob, GPUMaterial *gpu_material);
Document functions that returns bool. Even more so when the function name is not explict.
@ -0,0 +13,4 @@
namespace blender::draw {
bool volume_sub_pass(PassMain::Sub &ps, Scene *scene, Object *ob, GPUMaterial *gpu_material);
bool volume_sub_pass(PassSimple::Sub &ps, Scene *scene, Object *ob, GPUMaterial *gpu_material);
I think it is better to create the subpass and return a pointer (or nullptr if nothing to draw). It is clearer what is the inputs and outputs.
Still can`t mark conversations as resolved.
All the feedback that I have not answered should be addressed now.
@ -439,0 +473,4 @@
}
}
static inline float view_z_to_volume_z(
Rename to
volume_view_z_to_froxel_z
, fits module prefix.I think this would be misleading since the result is in the 0-1 range.
If you want to use the volume prefix here too, I think we should use a more accurate term.
Since this is pretty much
NDC
with a different distribution and range, maybe we could useNVC
and add a comment explaining this stands for “Normalized Volume Coordinates” and explain what they are?@ -0,0 +60,4 @@
const float4x4 &view_matrix = camera.data_get().viewmat;
const float4x4 &perspective_matrix = camera.data_get().winmat * view_matrix;
/** NOTE: Keep in sync with ndc_to_volume. */
We shouldn't need any of this with the shared GLSL/C++ code. Maybe it's a leftover comment?
The implementation is not fully shared since the way to retrieve the projection matrix and the coordinate scale is different.
I would say the projection matrix should be part of
VolumesInfoData
(as we will need one for TAA anyway) and maybe you can fold a bit of the math into it.This way you can share more of the code.
@ -0,0 +233,4 @@
return;
}
PassMain::Sub *ps = volume_sub_pass(
Code-style: Avoid
ps
as name. Use more descriptive name likeobject_volume_pass
.@ -0,0 +56,4 @@
/* Light Scattering. */
PassSimple scatter_ps_ = {"Volumes.Scatter"};
Framebuffer scatter_fb_;
Uneeded like the others framebuffers in this file.
@ -0,0 +8,4 @@
/* Volume slice to view space depth. */
float volume_z_to_view_z(float z)
{
float near = volumes_info_buf.depth_near;
If a file requires a specific resource (here
volumes_info_buf
), make it clear at the top the file. (seeeevee_light_eval_lib.glsl
for example)@ -0,0 +16,4 @@
return volume_z_to_view_z(near, far, distribution, is_persp, z);
}
float view_z_to_volume_z(float depth)
This one doesn't make much sense since it is only called once. Merge it into
ndc_to_volume
.Since this is the reverse of
volume_z_to_view_z
, I think leaving it as a separate function makes it easier to understand the code.@ -0,0 +35,4 @@
return coord;
}
vec3 ndc_to_volume(vec3 coord)
I would call it
volume_ndc_to_froxel
.@ -0,0 +44,4 @@
return coord;
}
float phase_function_isotropic()
volume_phase_function_isotropic
@ -0,0 +49,4 @@
return 1.0 / (4.0 * M_PI);
}
float phase_function(vec3 v, vec3 l, float g)
volume_phase_function_henyey_greenstein
@ -116,8 +118,9 @@ class Instance {
render_buffers(*this),
main_view(*this),
world(*this),
irradiance_cache(*this),
This triggers a warning about initialization order. Move it after
light_probes
.WIP: EEVEE Next: Volumesto EEVEE Next: VolumesThere is a bit of a design problem when things to too much effect centric instead of being data centric. In other words, the Objects & Worlds should call other modules to subscribe to them, not the other way around.
The issue manifests as some changes to class interfaces that shouldn't be there (exposing private method/members).
But from what I see it shouldn't be that hard to fix. It's just a matter of moving the code so that things are flowing the correct way.
I guess the issue came to be because the original port was done using the current EEVEE's codebase which is more design as effect centric.
As testing goes, it works great so far.
@ -230,6 +238,7 @@ void Instance::object_sync(Object *ob)
case OB_POINTCLOUD:
sync.sync_point_cloud(ob, ob_handle, res_handle, ob_ref);
Missing break here. I guess it was fine before because of the missing volume implementation.
@ -276,2 +271,4 @@
eMaterialGeometry geometry_type,
bool probe_capture = false);
private:
I don't really like moving these functions to public. I propose to add
MaterialPass volume
insideeevee::Material
. This way it is ready for multi material support when we add it.Then put the logic inside
sync_mesh
in thefor (auto i : material_array.gpu_materials.index_range()) {
for discarding the surface rendering.@ -110,0 +120,4 @@
world_ps_.state_set(DRW_STATE_WRITE_COLOR);
inst_.volume.bind_properties_buffers(world_ps_);
inst_.lights.bind_resources(&world_ps_);
inst_.shadows.bind_resources(&world_ps_);
Why do you need light and shadows here? the computation is not done at this stage.
@ -407,4 +434,2 @@
info.vertex_source_generated = vert_gen.str();
}
{
This is kind of a codestyle thing. But this block is supposed to the the frag gen. so just add
if (!is_compute)
here. Same for vertex.@ -393,0 +405,4 @@
if (do_vertex_attrib_load) {
vert_gen << global_vars.str() << attr_load.str();
}
else if (pipeline_type != MAT_PIPE_VOLUME) {
Having the default case be the compute one is a bit weird. Maybe have
do_compute_attrib_load = (pipeline_type == MAT_PIPE_VOLUME)
for clarity and test for this instead.@ -539,2 +557,2 @@
codegen_callback,
this);
return DRW_shader_from_world(
blender_world, nodetree, shader_uuid, is_volume, false, codegen_callback, this);
Deferred compilation should still be done for the volume shader as they are quite complex.
So the world volume should be deferred but the world itself don't, is that right?
Yes
@ -93,13 +93,17 @@ enum eShaderType {
SHADOW_TILEMAP_TAG_USAGE_SURFELS,
SHADOW_TILEMAP_TAG_USAGE_TRANSPARENT,
SUBSURFACE_EVAL,
What happened here? Why did it moved?
@ -442,0 +453,4 @@
float light_clamp;
packed_float3 inv_tex_size;
float shadow_steps;
int use_lights;
Use
bool1
for booleans in shared GLSL structs.@ -132,0 +133,4 @@
inst_.pipelines.forward.render_opaque(
render_view_new_, prepass_fb_, combined_fb_, rbufs.combined_tx);
inst_.volume.draw_resolve(render_view_new_);
You should call that from the
forward.render()
function instead of splitting it.But the volume resolve is not part of the forward pipeline at all?
I actually think we eventually will have to go in the opposite direction. The final ShadingView::render order should be something like this:
Otherwise, we have to compute some steps twice and forward opaque objects are going to be missing in things like SSR and SSGI.
The volume resolve is not that costly to require a separate pass except for its render pass output.
The deferred lighting and forward pass should sample the final volume textures. But that can be done in another patch.
And I agree there might be changes in the whole pipeline to avoid some double work and missing object.
@ -0,0 +206,4 @@
}
float3x3 world_matrix = float3x3(float4x4(ob->object_to_world));
float3 size = math::to_scale(world_matrix);
You can simplify to
math::to_scale(float4x4(ob->object_to_world))
. This does exactly the same thing without the intermediate copy.@ -0,0 +354,4 @@
DRW_stats_group_end();
}
void VolumeModule::draw_resolve(View &view)
The more I think of it the more I think this should be embeded called from the pipelines themselves.
We can commit this patch as is and refactor later.
@ -61,3 +60,4 @@
World(Instance &inst) : inst_(inst){};
~World();
::World *default_world_get();
The world should call the volume module. not the other way around.
@ -0,0 +1,16 @@
void main()
This file is used nowhere
@ -0,0 +64,4 @@
#endif
g_data = init_globals(wP);
#ifndef NO_ATTRIB_LOAD
This is a legacy flag that is no inside EEVEE-Next.
@ -108,3 +108,3 @@
BoundBox &bbox = *BKE_volume_boundbox_get(ref.object);
orco_add = (float3(bbox.vec[6]) + float3(bbox.vec[0])) * 0.5f; /* Center. */
orco_mul = float3(bbox.vec[6]) - float3(bbox.vec[0]); /* Size. */
orco_mul = (float3(bbox.vec[6]) - float3(bbox.vec[0])) * 0.5f; /* Size. */
Update the comment. It should be half size.
I also get the following errors on Metal (which are not metal specific):
The problem is that trying to force the volume pipeline into the geometry pipeline layout doesn't seem to work too well.
I mean, look at the recent
MaterialPass
commit.Now we have a
Material
class where either only thevolume
MaterialPass
is used, and everything else is garbage, or the other way around. And thematerial_sync
function is the same, two completely split branches because we're merging 2 separate paths, just because they're supposed to be conceptually the same. That's the opposite of being data centric.There may be room for improvements, but I don't really feel like the latest commits are going in that direction.
This is because
material_sync
not supposed to called twice. So you get all you materials passes for every pipeline for every material at once inmaterial_array_get
. This is what I suggested in refactoring the logic insync_mesh
.This is because we will have multiple material support and better mesh to volume support in the future. As well as volume surfaces.
To me this is going into the right direction.
Of course the growing
struct Material
is a concern, but this can be fixed in a way that doesn't change the codeflow.f07ba42ce7
f49a8fe437