EEVEE Next: Volumes #107176

Merged
Miguel Pozo merged 126 commits from pragma37/blender:pull-eevee-next-volumes into main 2023-08-04 16:47:22 +02:00
Member

Port of EEVEE unified volume rendering to EEVEE Next, using compute shaders.

Improvements:

  • Skip empty volume outside object bounds. (Large performance increase)

Currently missing:

  • Shadows and irradiance integration.
  • Grid-space TAA.

Main Task: #105672

Port of EEVEE unified volume rendering to EEVEE Next, using compute shaders. Improvements: - Skip empty volume outside object bounds. (Large performance increase) Currently missing: - Shadows and irradiance integration. - Grid-space TAA. Main Task: #105672
Miguel Pozo added 60 commits 2023-04-20 16:04:43 +02:00
bcc6789520 Revert "Use PassMain"
This reverts commit 073764391eb4aa8cb868946d37d6f2f716cb30d6.
Miguel Pozo added this to the EEVEE & Viewport project 2023-04-20 16:05:09 +02:00
Miguel Pozo requested review from Clément Foucault 2023-04-20 16:05:30 +02:00
Miguel Pozo added the
Module
EEVEE & Viewport
label 2023-04-20 16:06:01 +02:00
Miguel Pozo added 1 commit 2023-04-20 18:49:29 +02:00
Clément Foucault requested changes 2023-04-23 23:36:40 +02:00
Clément Foucault left a comment
Member

First pass review. Still have a lot left.

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.

Better duplicate the panel and mask unavailable options like shadows.
fclem marked this conversation as resolved
@ -0,0 +8,4 @@
class Instance;
class Volumes {
  • All members need to be documented.
  • The top of this header should contain an overall idea of the technique used.
  • Volumes could maybe renamed as VolumeModule
- All members need to be documented. - The top of this header should contain an overall idea of the technique used. - `Volumes` could maybe renamed as `VolumeModule`
fclem marked this conversation as resolved
@ -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.

Can we remove thoses? Seems like they were used for porting the old code but are unused now.
Author
Member

I think g_orco and objectPosition 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.

I think `g_orco` and `objectPosition` 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.

Yes, remove the one unused.
pragma37 marked this conversation as resolved
@ -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.

Add a TODO. These do not need to use closure select and could just be combined.
fclem marked this conversation as resolved
@ -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.

Why? In the future, we could have surface shaders evaluate the volume properties.
Author
Member

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.

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.
fclem marked this conversation as resolved
@ -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.

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.
fclem marked this conversation as resolved
@ -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 use else clause for ortho.

Code style. Use `bool is_persp` and use `else` clause for ortho.
Author
Member

Done. But could we maybe add a is_perpective function to common_view_lib?
This is used in so many places that IMO it would make sense.

Done. But could we maybe add a `is_perpective` function to `common_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.

It would make sense. But this would be a separate cleanup.
fclem marked this conversation as resolved
@ -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.

Comment style. This apply to the whole diff. Please fix existing comments when you copy them.
fclem marked this conversation as resolved
@ -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.

Document this function. The name isn't clear by itself.
Author
Member

Actually, could this be merged into eevee_light_eval_lib?
There's a light_vector_get there, but it doesn't handle area lights.

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.

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.
pragma37 marked this conversation as resolved
@ -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 shader should be part of the deferred pipeline shader. We shouldn't load the scene depth only for this pass.
Author
Member

This would have some implications though:

  • The deferred shader uses a stencil mask (I guess it's for the background?), so this would conflict with the world volume rendering.
  • The opaque forward pass would need to perform the volume resolve as well.

I can give it a try, but I would expect this approach would be slower?

This would have some implications though: - The deferred shader uses a stencil mask (I guess it's for the background?), so this would conflict with the world volume rendering. - The opaque forward pass would need to perform the volume resolve as well. 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.

Ok Leave it as is for now. It can be refactor after the deferred pipeline has matured.
fclem marked this conversation as resolved
@ -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.

This need a comment explaining why we divide by 2nd component.
Author
Member

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.

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.

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.
Author
Member

Oops! This should be solved now.

Oops! This should be solved now.
fclem marked this conversation as resolved
@ -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.

This comment is obsolete.
fclem marked this conversation as resolved
@ -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.

Remove the render passes they make no sense. in this context.
fclem marked this conversation as resolved
@ -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.

Doesnt seems to be necessary. Merge with eevee_volume_resolve.
pragma37 marked this conversation as resolved
@ -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.

This should also output to the volume renderpasses.
Author
Member

I think we should actually get final (f12) rendering working first before looking into this?

I think we should actually get final (f12) rendering working first before looking into this?

Ok but then add a TODO here.

Ok but then add a TODO here.
pragma37 marked this conversation as resolved
Miguel Pozo added 7 commits 2023-04-24 16:13:51 +02:00
Author
Member

For some reason, I can't mark conversations as solved, but all the ones I have not answered should be solved now.

For some reason, I can't mark conversations as solved, but all the ones I have not answered should be solved now.
Clément Foucault added 3 commits 2023-05-08 19:22:01 +02:00
Clément Foucault added 2 commits 2023-05-08 19:31:06 +02:00
Clément Foucault added 2 commits 2023-05-09 19:59:04 +02:00
Clément Foucault added 1 commit 2023-05-10 14:29:35 +02:00
Clément Foucault requested changes 2023-05-10 17:56:54 +02:00
Clément Foucault left a comment
Member

I 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.

I 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.

Rename to `VOLUME_INTEGRATION_GROUP_SIZE`.
fclem marked this conversation as resolved
@ -426,0 +427,4 @@
/** \name Volumes
* \{ */
struct VolumesData {

This should be renamed to VolumesInfoData since this is not per object data.

This should be renamed to `VolumesInfoData` since this is not per object data.
pragma37 marked this conversation as resolved
@ -0,0 +1,401 @@
/* SPDX-License-Identifier: GPL-2.0-or-later */

Remove plural form of Volume(s) in struct names and filenames.

Remove plural form of Volume(s) in struct names and filenames.
pragma37 marked this conversation as resolved
@ -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.

Frostbite approach is too generic. Provide exact source title and like to the presentatino/ paper.
pragma37 marked this conversation as resolved
@ -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.

Document functions that returns bool. Even more so when the function name is not explict. Reminder that function documentation should be in header files.
pragma37 marked this conversation as resolved
@ -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.

This functino should be better documented.
pragma37 marked this conversation as resolved
@ -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

Use `math::transform_point`
pragma37 marked this conversation as resolved
@ -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.

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.
pragma37 marked this conversation as resolved
@ -0,0 +189,4 @@
sync_world();
}
void VolumeModule::sync_world()

Move volume world to its own pipeline (eevee_pipeline.cc) for consistency.

Move volume world to its own pipeline (eevee_pipeline.cc) for consistency.
pragma37 marked this conversation as resolved
@ -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.

Use default world material for clearing instead of dedicated shader. The default output should be 100% transmission.
pragma37 marked this conversation as resolved
@ -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.

Better ensure them in the constructor. Also make sure their data is valid.
pragma37 marked this conversation as resolved
@ -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.

Move `view_z_to_volume_z` & `volume_z_to_view_z` to shader shared file as the Bound box init use it.
pragma37 marked this conversation as resolved
@ -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.

Document functions that returns bool. Even more so when the function name is not explict.
pragma37 marked this conversation as resolved
@ -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.

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.
pragma37 marked this conversation as resolved
Miguel Pozo added 4 commits 2023-05-11 21:12:15 +02:00
Miguel Pozo added 1 commit 2023-05-12 12:47:55 +02:00
Miguel Pozo added 1 commit 2023-05-12 12:50:15 +02:00
Miguel Pozo added 1 commit 2023-05-12 13:10:57 +02:00
Author
Member

Still can`t mark conversations as resolved.
All the feedback that I have not answered should be addressed now.

Still can`t mark conversations as resolved. All the feedback that I have not answered should be addressed now.
Miguel Pozo added 7 commits 2023-05-16 17:39:35 +02:00
Miguel Pozo added 2 commits 2023-06-01 16:18:27 +02:00
Miguel Pozo added 1 commit 2023-06-27 16:12:22 +02:00
Miguel Pozo added 1 commit 2023-06-27 16:36:00 +02:00
Clément Foucault requested changes 2023-06-27 18:45:26 +02:00
@ -439,0 +473,4 @@
}
}
static inline float view_z_to_volume_z(

Rename to volume_view_z_to_froxel_z, fits module prefix.

Rename to `volume_view_z_to_froxel_z`, fits module prefix.
Author
Member

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 use NVC and add a comment explaining this stands for “Normalized Volume Coordinates” and explain what they are?

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 use `NVC` and add a comment explaining this stands for “Normalized Volume Coordinates” and explain what they are?
fclem marked this conversation as resolved
@ -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?

We shouldn't need any of this with the shared GLSL/C++ code. Maybe it's a leftover comment?
Author
Member

The implementation is not fully shared since the way to retrieve the projection matrix and the coordinate scale is different.

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.

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.
pragma37 marked this conversation as resolved
@ -0,0 +233,4 @@
return;
}
PassMain::Sub *ps = volume_sub_pass(

Code-style: Avoid ps as name. Use more descriptive name like object_volume_pass.

Code-style: Avoid `ps` as name. Use more descriptive name like `object_volume_pass`.
pragma37 marked this conversation as resolved
@ -0,0 +56,4 @@
/* Light Scattering. */
PassSimple scatter_ps_ = {"Volumes.Scatter"};
Framebuffer scatter_fb_;

Uneeded like the others framebuffers in this file.

Uneeded like the others framebuffers in this file.
pragma37 marked this conversation as resolved
@ -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. (see eevee_light_eval_lib.glsl for example)

If a file requires a specific resource (here `volumes_info_buf`), make it clear at the top the file. (see `eevee_light_eval_lib.glsl` for example)
pragma37 marked this conversation as resolved
@ -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.

This one doesn't make much sense since it is only called once. Merge it into `ndc_to_volume`.
Author
Member

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.

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.
fclem marked this conversation as resolved
@ -0,0 +35,4 @@
return coord;
}
vec3 ndc_to_volume(vec3 coord)

I would call it volume_ndc_to_froxel.

I would call it `volume_ndc_to_froxel`.
fclem marked this conversation as resolved
@ -0,0 +44,4 @@
return coord;
}
float phase_function_isotropic()

volume_phase_function_isotropic

`volume_phase_function_isotropic`
pragma37 marked this conversation as resolved
@ -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

`volume_phase_function_henyey_greenstein`
pragma37 marked this conversation as resolved
Miguel Pozo added 1 commit 2023-06-29 18:37:56 +02:00
Clément Foucault requested changes 2023-06-30 14:10:01 +02:00
@ -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.

This triggers a warning about initialization order. Move it after `light_probes`.
pragma37 marked this conversation as resolved
Miguel Pozo added 7 commits 2023-07-03 15:24:54 +02:00
Miguel Pozo added 4 commits 2023-07-17 20:30:23 +02:00
Miguel Pozo changed title from WIP: EEVEE Next: Volumes to EEVEE Next: Volumes 2023-07-17 20:31:44 +02:00
Miguel Pozo added 1 commit 2023-07-20 19:45:25 +02:00
Miguel Pozo added 2 commits 2023-07-25 18:31:14 +02:00
Miguel Pozo added 3 commits 2023-07-31 13:06:36 +02:00
Miguel Pozo added a new dependency 2023-07-31 15:42:26 +02:00
Clément Foucault requested changes 2023-07-31 17:53:39 +02:00
Clément Foucault left a comment
Member

There 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.

There 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.

Missing break here. I guess it was fine before because of the missing volume implementation.
pragma37 marked this conversation as resolved
@ -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 inside eevee::Material. This way it is ready for multi material support when we add it.

Then put the logic inside sync_mesh in the for (auto i : material_array.gpu_materials.index_range()) { for discarding the surface rendering.

I don't really like moving these functions to public. I propose to add `MaterialPass volume` inside `eevee::Material`. This way it is ready for multi material support when we add it. Then put the logic inside `sync_mesh` in the `for (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.

Why do you need light and shadows here? the computation is not done at this stage.
pragma37 marked this conversation as resolved
@ -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.

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.
pragma37 marked this conversation as resolved
@ -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.

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.
pragma37 marked this conversation as resolved
@ -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.

Deferred compilation should still be done for the volume shader as they are quite complex.
Author
Member

So the world volume should be deferred but the world itself don't, is that right?

So the world volume should be deferred but the world itself don't, is that right?
pragma37 marked this conversation as resolved
@ -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?

What happened here? Why did it moved?
pragma37 marked this conversation as resolved
@ -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.

Use `bool1` for booleans in shared GLSL structs.
pragma37 marked this conversation as resolved
@ -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.

You should call that from the `forward.render()` function instead of splitting it.
Author
Member

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:

  • Deferred & Forward prepass
  • HiZ
  • Volume properties
  • Lights
  • Shadows
  • Volume compute
  • Forward opaque shading
  • Deferred pass and shading
  • Volume resolve
  • Forward transparent shading

Otherwise, we have to compute some steps twice and forward opaque objects are going to be missing in things like SSR and SSGI.

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: - Deferred & Forward prepass - HiZ - Volume properties - Lights - Shadows - Volume compute - Forward opaque shading - Deferred pass and shading - Volume resolve - Forward transparent shading Otherwise, we have to compute some steps twice and forward opaque objects are going to be missing in things like SSR and SSGI.

But the volume resolve is not part of the forward pipeline at all?
As much as the raytracing module is not part of the deferred pipeline. It just gets called by it.

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.

> But the volume resolve is not part of the forward pipeline at all? As much as the raytracing module is not part of the deferred pipeline. It just gets called by it. 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.

You can simplify to `math::to_scale(float4x4(ob->object_to_world))`. This does exactly the same thing without the intermediate copy.
pragma37 marked this conversation as resolved
@ -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.

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.

The world should call the volume module. not the other way around.
pragma37 marked this conversation as resolved
@ -0,0 +1,16 @@
void main()

This file is used nowhere

This file is used nowhere
pragma37 marked this conversation as resolved
@ -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.

This is a legacy flag that is no inside EEVEE-Next.
pragma37 marked this conversation as resolved
@ -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.

Update the comment. It should be half size.
pragma37 marked this conversation as resolved

I also get the following errors on Metal (which are not metal specific):

WARN (gpu.debug.metal): source/blender/gpu/metal/mtl_context.mm:1412 ensure_buffer_bindings: [UBO] Shader 'WOWorld' expected UBO 'drw_volume' to be bound at buffer location: 9 (buffer[[5]]) -- but nothing was bound -- binding dummy buffer
ERROR (gpu.debug.metal): source/blender/gpu/metal/mtl_context.mm:1477 ensure_buffer_bindings: [SSBO] Shader 'WOWorld' expected SSBO 'sampling_buf' to be bound at SSBO location: 6 (buffer[[8]]) -- but nothing was bound.
I also get the following errors on Metal (which are not metal specific): ``` WARN (gpu.debug.metal): source/blender/gpu/metal/mtl_context.mm:1412 ensure_buffer_bindings: [UBO] Shader 'WOWorld' expected UBO 'drw_volume' to be bound at buffer location: 9 (buffer[[5]]) -- but nothing was bound -- binding dummy buffer ERROR (gpu.debug.metal): source/blender/gpu/metal/mtl_context.mm:1477 ensure_buffer_bindings: [SSBO] Shader 'WOWorld' expected SSBO 'sampling_buf' to be bound at SSBO location: 6 (buffer[[8]]) -- but nothing was bound. ```
Miguel Pozo added 5 commits 2023-08-01 20:21:42 +02:00
Miguel Pozo added 2 commits 2023-08-02 18:39:11 +02:00
Author
Member

There is a bit of a design problem when things to too much effect centric instead of being data centric.

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 the volume MaterialPass is used, and everything else is garbage, or the other way around. And the material_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.

> There is a bit of a design problem when things to too much effect centric instead of being data centric. 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 the `volume` `MaterialPass` is used, and everything else is garbage, or the other way around. And the `material_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.
Miguel Pozo added 1 commit 2023-08-02 19:02:10 +02:00

Now we have a Material class where either only the volume MaterialPass is used, and everything else is garbage, or the other way around.

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 in material_array_get. This is what I suggested in refactoring the logic in sync_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.

> Now we have a Material class where either only the volume MaterialPass is used, and everything else is garbage, or the other way around. 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 in `material_array_get`. This is what I suggested in refactoring the logic in `sync_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.
Miguel Pozo added 6 commits 2023-08-04 16:32:40 +02:00
Clément Foucault approved these changes 2023-08-04 16:44:27 +02:00
Miguel Pozo merged commit ff470f3f2e into main 2023-08-04 16:47:22 +02:00
Miguel Pozo referenced this issue from a commit 2023-08-04 16:47:22 +02:00
Miguel Pozo deleted branch pull-eevee-next-volumes 2023-08-04 16:47:25 +02:00
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
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
EEVEE & Viewport
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
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
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
EEVEE & Viewport
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
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
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 Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Reference: blender/blender#107176
No description provided.