EEVEE-Next: Irradiance Cache: Initial Implementation #108639

Merged
Clément Foucault merged 86 commits from fclem/blender:eevee-next-irradiance-cache into main 2023-06-23 08:39:52 +02:00

This is a full rewrite of the irradiance volume baking.
The baking is much faster and doesn't scale linearly with the number
of irradiance samples in the volumes.

Ref #105643

This is a full rewrite of the irradiance volume baking. The baking is much faster and doesn't scale linearly with the number of irradiance samples in the volumes. Ref #105643
Clément Foucault added 68 commits 2023-06-06 00:59:40 +02:00
Spawning surfels uses a new shader pipeline (called `capture`) to avoid
the complexity of modifying the deferred or forward pipeline.
The shaders are light to compile and should only be compiled on bake.

This is very WIP: the surfel projection box is hardcoded and the 3
projections are done every frame. The surfels are placed in a regular
grid because of the raster pipeline used to spawn them.

Note: While this is really fast, I am not sure this will scale well for
highly detailed lighting conditions. In the case where the surfel density
is too low, the aliasing might create really poor result.
This removes the complexity of resource sharing as it isn't even leveraged
yet. So keep the code as simple as possible for now.
This allow probe update and deletion to trigger an auto bake.
This should allow to keep both version working during the period where
both eevee versions are available.
Was missing manager sync, a buffer read and copy-on-write tagging.
This fixes the light leaking issue caused by the surfel lists.
The fix consists in clumping more surfel together when creating the lists
then rewire the coplanar surfels to more valid surfels up and down the
same list.
This is still broken as the light seems to never converge.
Now it follows proper conventional notation.

# Conflicts:
#	source/blender/draw/engines/eevee_next/shaders/eevee_surfel_ray_comp.glsl
Light bouncing needs a feedback mechanism. We cannot reuse the same
radiance for scattering light and accumulating it.

This splits the final accumulation to `radiance` and bouncing light into
`incomming_light` and `outgoing_light`.
The framebuffer default size was only set during the first bind. This
is because the `dirty_attachments_ tag` wasn't set and thus the
framebuffer size was never passed down to the GL.

Split to `default_size_set()` to not affect other code paths that use
`size_set()`.
This adds a separate pass to project the neighbor surfels to the final
irradiance sample points.
Waiting for the commit to be in master.
This make the baking step output local space spherical harmonics which
need to be rotated back to world before being evaluated.
In practice we rotate the evaluated direction.
# Conflicts:
#	source/blender/blenkernel/intern/lightprobe.cc
#	source/blender/draw/engines/eevee_next/shaders/eevee_debug_surfels_frag.glsl
#	source/blender/editors/render/render_shading.cc
#	source/blender/makesdna/DNA_lightprobe_types.h
This approach allow streaming of visible grids and
has a controlled memory budget.
This remove the per irradiance grid approach. The idea is to do the light
transport only once for each visibility collection and store the
irradiance for all irradiance grid using this visibility mask.

This removes the need for the lightprobe near/far clip parameters.
This doesn't implement the world irradiance extraction process,
but only reserve the storage for it.
This avoid triggering the asserts in place in the Metal
backend. However it does not work yet.
# Conflicts:
#	source/blender/blenloader/intern/versioning_400.cc
# Conflicts:
#	source/blender/blenloader/intern/versioning_400.cc
This mode allows much faster convergence.
Also fixes a lot of small issues with surfel list projections
such as aliasing producing banding in SH volumes.
- Cleanup unused options
- Add global surfel density option
- Increase default samples
# Conflicts:
#	source/blender/draw/engines/eevee_next/eevee_defines.hh
#	source/blender/draw/engines/eevee_next/shaders/eevee_deferred_light_frag.glsl
#	source/blender/draw/intern/draw_manager.c
... to work with selection and all objects.
The reordering was making the grid upload broken
This is needed now that more denser irradiance volumes
are possible
EEVEE-Next: Irradiance Bake: Move baking props to irradiance volumes
Some checks failed
buildbot/vexp-code-patch-coordinator Build done.
6627263d40
Clément Foucault requested review from Jeroen Bakker 2023-06-06 01:12:15 +02:00
Clément Foucault changed title from WIP: EEVEE-Next: Irradiance Cache: Initial Implementation to EEVEE-Next: Irradiance Cache: Initial Implementation 2023-06-06 01:12:51 +02:00
Member

I tried a test run on Mac Studio M1. Currently Eevee-next only renders black. When looking at individual buffers the light buffer is black. Also with default blender cube or an emissive material. Lights are on in the viewport. I will do a code review and check other systems.

I tried a test run on Mac Studio M1. Currently Eevee-next only renders black. When looking at individual buffers the light buffer is black. Also with default blender cube or an emissive material. Lights are on in the viewport. I will do a code review and check other systems.
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR108639) when ready.
Jeroen Bakker reviewed 2023-06-06 08:39:56 +02:00
@ -595,6 +617,27 @@ class RENDER_PT_eevee_indirect_lighting_display(RenderButtonsPanel, Panel):
row.prop(props, "gi_irradiance_display_size", text="Irradiance Size")
row.prop(props, "gi_show_irradiance", text="", toggle=True)
class RENDER_PT_eevee_next_indirect_lighting_display(RenderButtonsPanel, Panel):
Member

Code format: Add empty line

Code format: Add empty line
fclem marked this conversation as resolved
Jeroen Bakker reviewed 2023-06-06 08:41:23 +02:00
@ -219,3 +219,3 @@
if (region->regiontype == RGN_TYPE_TOOL_HEADER) {
if (((sl->spacetype == SPACE_IMAGE) && hide_image_tool_header) ||
sl->spacetype == SPACE_SEQ) {
sl->spacetype == SPACE_SEQ)
Member

Unneeded change Make sure your clang-format points to the one in blender-git/lib

Unneeded change Make sure your clang-format points to the one in blender-git/lib
fclem marked this conversation as resolved
Jeroen Bakker requested changes 2023-06-06 09:40:39 +02:00
Jeroen Bakker left a comment
Member

I went over the code I think it would require some technical documentation to understand. Although it is a complex feature it seems that the architecture of eevee-next works quite good here.

What is important to fix before landing:

I went over the code I think it would require some technical documentation to understand. Although it is a complex feature it seems that the architecture of eevee-next works quite good here. What is important to fix before landing: - code style formatting and other small feedback - validate if it runs on other M1 hardware as expected. (My system could be an exception, which we can investigate afterwards). - check issue mentioned on blender chat. - Fix linux compilation error see https://projects.blender.org/blender/blender/pulls/108304/files#diff-9bc01120b7e0649e1a663cd1e54ed5c51ac35b80
@ -0,0 +27,4 @@
* Create the job description.
* This is called for async (modal) bake operator.
* The actual work will be done by `EEVEE_NEXT_lightbake_job()`.
* IMPORTANT: Must run on the main thread because of potential GPUContext creation.
Member

Guard with an assert?

Guard with an assert?
Author
Member

It is already guarded in LightBake class.

It is already guarded in LightBake class.
fclem marked this conversation as resolved
@ -0,0 +42,4 @@
* Allocate dependency graph and job description (EEVEE_NEXT_LightBake).
* Dependency graph evaluation does *not* happen here. It is delayed until
* `EEVEE_NEXT_lightbake_job` runs.
* IMPORTANT: Must run on the main thread because of potential GPUContext creation.
Member

guard with an assert

guard with an assert
Author
Member

It is already guarded in LightBake class.

It is already guarded in `LightBake` class.
fclem marked this conversation as resolved
@ -0,0 +42,4 @@
int grid_index;
};
struct ReflectionCube : public LightProbe {};
Member

Code style (clang-format)

Code style (clang-format)
fclem marked this conversation as resolved
@ -0,0 +62,4 @@
radiance_transfer(receiver, radiance, L);
}
void radiance_transfer_sky(inout Surfel receiver, vec3 sky_L)
Member

Personally I would use the term background. sky is a specific background.

Personally I would use the term background. sky is a specific background.
Author
Member

I will use world as it fits better the terminology used in the rest of EEVEE-Next.

I will use world as it fits better the terminology used in the rest of EEVEE-Next.
fclem marked this conversation as resolved
@ -1037,3 +1053,2 @@
*/
class Image {
};
class Image {};
Member

clang format

clang format
fclem marked this conversation as resolved
@ -1172,3 +1172,3 @@
case OB_SOLID:
if (U.experimental.enable_workbench_next &&
STREQ(engine_type->idname, "BLENDER_WORKBENCH_NEXT")) {
STREQ(engine_type->idname, "BLENDER_WORKBENCH_NEXT"))
Member

clang format

clang format
fclem marked this conversation as resolved
Member

I tried a test run on Mac Studio M1. Currently Eevee-next only renders black.

  • validate if it runs on other M1 hardware as expected. (My system could be an exception, which we can investigate afterwards).

I experience the same issue with the M1 Pro. All objects are black with the exception of during and after the baking process where incorrect indirect lighting is applied to the area contained within the Irradiance Volume.

System Information
Operating system: macOS-13.4-arm64-arm-64bit 64 Bits
Graphics card: M1 Pro (Using the Metal Backend)

> I tried a test run on Mac Studio M1. Currently Eevee-next only renders black. > - validate if it runs on other M1 hardware as expected. (My system could be an exception, which we can investigate afterwards). I experience the same issue with the M1 Pro. All objects are black with the exception of during and after the baking process where incorrect indirect lighting is applied to the area contained within the Irradiance Volume. **System Information** Operating system: macOS-13.4-arm64-arm-64bit 64 Bits Graphics card: M1 Pro (Using the Metal Backend)
First-time contributor

Unable to bake in windows due to the error i post a fix in blender.chat :

" @fclem For the fix when blender crash when you try to bake the irradiance in windows with the error message i'll give you UP the fixe are this : in file blender\source\blender\draw\engines\eevee_next\shaders\eevee_surfel_ray_comp.glsl Line 46 i put the inout before Surfel sender and he give this code line :

New :
void radiance_transfer_surfel(inout Surfel receiver, inout Surfel sender)

Past (Crash on windows) are
void radiance_transfer_surfel(inout Surfel receiver, Surfel sender)

Same in C:\blender-git\blender\source\blender\draw\engines\eevee_next\shaders\eevee_lightprobe_irradiance_ray_comp.glsl line 27 i put the inout before Surfel surfel and he give this code line :

New : void irradiance_capture(inout Surfel surfel, vec3 P, inout SphericalHarmonicL1 sh)

Past (Crash on windows) are : void irradiance_capture(Surfel surfel, vec3 P, inout SphericalHarmonicL1 sh)

Now when you try to bake irradiance cache in EEVEE NEXT, blender not crash anymore in Windows. Works perfect and density of surfels working too.

"


After the fixe the bake works correctly but give wrong results (all stay black, or with normals color or sometime white color randomly)

  • Default scene with no baking irradiance
    image

  • Default scene with irradiance baking (In irradiance option and Render > Indirect lightning) Both give a black scene
    image

  • Default scene with baking irradiance (Debug mode 3 - Surfels Normals / Density of Surfels : 1.0)
    image

  • Default scene with baking irradiance (Debug mode 3 - Surfels Normals / Density of Surfels : 0.5)
    image

  • Sometimes surfels ovelap, and are not correctly placed when you have 2 plan one over the other.
    image

Compiled from machine :
Windows 11 x64 - Home édition 22H2 / Build 22621.1702
GPU : RTX 2060 6 GO (Lenovo)
CPU : INTEL I5 9300H
PC MODEL : LENOVO LEGION RTX 15INCH Y540

And taked from this branch : https://projects.blender.org/fclem/blender/src/branch/eevee-next-irradiance-cache At 6PM (18H french hours / 06/06/2023)

Unable to bake in windows due to the error i post a fix in blender.chat : " @fclem For the fix when blender crash when you try to bake the irradiance in windows with the error message i'll give you UP the fixe are this : in file `blender\source\blender\draw\engines\eevee_next\shaders\eevee_surfel_ray_comp.glsl ` Line `46` i put the `inout` before `Surfel sender` and he give this code line : New : `void radiance_transfer_surfel(inout Surfel receiver, inout Surfel sender)` Past (Crash on windows) are `void radiance_transfer_surfel(inout Surfel receiver, Surfel sender)` Same in `C:\blender-git\blender\source\blender\draw\engines\eevee_next\shaders\eevee_lightprobe_irradiance_ray_comp.glsl` line `27` i put the `inout` before `Surfel surfel` and he give this code line : New : `void irradiance_capture(inout Surfel surfel, vec3 P, inout SphericalHarmonicL1 sh)` Past (Crash on windows) are : `void irradiance_capture(Surfel surfel, vec3 P, inout SphericalHarmonicL1 sh)` Now when you try to bake irradiance cache in EEVEE NEXT, blender not crash anymore in Windows. Works perfect and density of surfels working too. " ------- After the fixe the bake works correctly but give wrong results (all stay black, or with normals color or sometime white color randomly) - Default scene with no baking irradiance ![image](/attachments/18095955-cb0e-4485-bf81-5c65bf408ce9) - Default scene with irradiance baking (In irradiance option and Render > Indirect lightning) Both give a black scene ![image](/attachments/3f52cb07-49bb-42c6-89bc-cbe929806e4d) - Default scene with baking irradiance (Debug mode 3 - Surfels Normals / Density of Surfels : 1.0) ![image](/attachments/5773e3ba-1053-47b8-90d6-0eab72f8b9ca) - Default scene with baking irradiance (Debug mode 3 - Surfels Normals / Density of Surfels : 0.5) ![image](/attachments/84a358f1-22c0-4920-87c8-bad192852521) - Sometimes surfels ovelap, and are not correctly placed when you have 2 plan one over the other. ![image](/attachments/5dcdb90a-d0d6-4f9b-8c46-870944c2494f) Compiled from machine : Windows 11 x64 - Home édition 22H2 / Build 22621.1702 GPU : RTX 2060 6 GO (Lenovo) CPU : INTEL I5 9300H PC MODEL : LENOVO LEGION RTX 15INCH Y540 And taked from this branch : https://projects.blender.org/fclem/blender/src/branch/eevee-next-irradiance-cache At 6PM (18H french hours / 06/06/2023)
1.1 MiB
1013 KiB
1018 KiB
1.0 MiB
1021 KiB
First-time contributor

Confirming the same issue as Jeroen and Alaska. All objects render pure black regardless of light type, material changes, etc.

Mac Studio
M1 Ultra
macOS 13.3.1

Confirming the same issue as Jeroen and Alaska. All objects render pure black regardless of light type, material changes, etc. Mac Studio M1 Ultra macOS 13.3.1
Clément Foucault added 2 commits 2023-06-06 23:54:18 +02:00
Clément Foucault added 1 commit 2023-06-06 23:54:44 +02:00
Author
Member

@dodo-2

Sometimes surfels ovelap, and are not correctly placed when you have 2 plan one over the other

This is expected. The surfel representation is not precise enough to detect these cases. The only workaround for now is to increase surfel density. But the indirect lighting usually doesn't require that much precision.

After the fixe the bake works correctly but give wrong results
This need to be investigated. I will try to reproduce on an NVidia GPU.

@dodo-2 > Sometimes surfels ovelap, and are not correctly placed when you have 2 plan one over the other This is expected. The surfel representation is not precise enough to detect these cases. The only workaround for now is to increase surfel density. But the indirect lighting usually doesn't require that much precision. > After the fixe the bake works correctly but give wrong results This need to be investigated. I will try to reproduce on an NVidia GPU.
First-time contributor

I saw on the EA videos and the PDF, that the surfels spread randomly and their density increased or decreased depending on the distance from the camera and not aligned in a "Grid" form like a Matrix.
Are you going to reproduce these 2 functions (Random placement and density by distance)?

PS : this error come sometimes idk why
image

I saw on the EA videos and the PDF, that the surfels spread randomly and their density increased or decreased depending on the distance from the camera and not aligned in a "Grid" form like a Matrix. Are you going to reproduce these 2 functions (Random placement and density by distance)? PS : this error come sometimes idk why ![image](/attachments/488d6dfd-71ee-458b-bebe-7b1617d5ed69)
956 KiB
Author
Member

I saw on the EA videos and the PDF, that the surfels spread randomly and their density increased or decreased depending on the distance from the camera and not aligned in a "Grid" form like a Matrix.
Are you going to reproduce these 2 functions (Random placement and density by distance)?

Pseudo Random placement can be added and help some cases of aliasing. But the EA video you are surely referencing is not what we implemented at all.

PS : this error come sometimes idk why

Yes, the baking seems currently broken on some platform. We are investigating.

> I saw on the EA videos and the PDF, that the surfels spread randomly and their density increased or decreased depending on the distance from the camera and not aligned in a "Grid" form like a Matrix. > Are you going to reproduce these 2 functions (Random placement and density by distance)? Pseudo Random placement can be added and help some cases of aliasing. But the EA video you are surely referencing is not what we implemented at all. > PS : this error come sometimes idk why Yes, the baking seems currently broken on some platform. We are investigating.
Clément Foucault added 2 commits 2023-06-09 20:41:16 +02:00
Clément Foucault added 1 commit 2023-06-21 12:48:51 +02:00
Clément Foucault added 1 commit 2023-06-21 13:37:58 +02:00
# Conflicts:
#	source/blender/blenloader/intern/versioning_400.cc
#	source/blender/draw/CMakeLists.txt
#	source/blender/draw/engines/eevee_next/eevee_defines.hh
#	source/blender/draw/engines/eevee_next/eevee_shader.cc
#	source/blender/draw/engines/eevee_next/eevee_shader.hh
#	source/blender/draw/engines/eevee_next/eevee_shader_shared.hh
#	source/blender/draw/engines/eevee_next/shaders/infos/eevee_hiz_info.hh
Author
Member

The main issue seems to be solved for NVidia and MacOS.

However, on Metal backend, there are remaining warnings that I think should be tackled as separate issue.

WARN (gpu.debug.metal): source/blender/gpu/metal/mtl_context.mm:505 ensure_begin_render_pass: Framebuffer validation failed, falling back to default framebuffer
WARN (gpu.debug.metal): source/blender/gpu/metal/mtl_context.mm:505 ensure_begin_render_pass: Framebuffer validation failed, falling back to default framebuffer
WARN (gpu.debug.metal): source/blender/gpu/metal/mtl_context.mm:505 ensure_begin_render_pass: Framebuffer validation failed, falling back to default framebuffer
WARN (gpu.debug.metal): source/blender/gpu/metal/mtl_texture.mm:604 update_sub: gpu::MTLTexture::update_sub supplied bpp is 12 bytes (3 components per pixel), but backing texture bpp is 8 bytes (4 components per pixel) (TODO(Metal): Channel Conversion needed) (w: 4, h: 4, d: 12)
WARN (gpu.debug.metal): source/blender/gpu/metal/mtl_texture.mm:604 update_sub: gpu::MTLTexture::update_sub supplied bpp is 12 bytes (3 components per pixel), but backing texture bpp is 8 bytes (4 components per pixel) (TODO(Metal): Channel Conversion needed) (w: 4, h: 4, d: 12)
WARN (gpu.debug.metal): source/blender/gpu/metal/mtl_texture.mm:604 update_sub: gpu::MTLTexture::update_sub supplied bpp is 12 bytes (3 components per pixel), but backing texture bpp is 8 bytes (4 components per pixel) (TODO(Metal): Channel Conversion needed) (w: 4, h: 4, d: 12)
WARN (gpu.debug.metal): source/blender/gpu/metal/mtl_texture.mm:604 update_sub: gpu::MTLTexture::update_sub supplied bpp is 12 bytes (3 components per pixel), but backing texture bpp is 8 bytes (4 components per pixel) (TODO(Metal): Channel Conversion needed) (w: 4, h: 4, d: 12)
The main issue seems to be solved for NVidia and MacOS. However, on Metal backend, there are remaining warnings that I think should be tackled as separate issue. ``` WARN (gpu.debug.metal): source/blender/gpu/metal/mtl_context.mm:505 ensure_begin_render_pass: Framebuffer validation failed, falling back to default framebuffer WARN (gpu.debug.metal): source/blender/gpu/metal/mtl_context.mm:505 ensure_begin_render_pass: Framebuffer validation failed, falling back to default framebuffer WARN (gpu.debug.metal): source/blender/gpu/metal/mtl_context.mm:505 ensure_begin_render_pass: Framebuffer validation failed, falling back to default framebuffer WARN (gpu.debug.metal): source/blender/gpu/metal/mtl_texture.mm:604 update_sub: gpu::MTLTexture::update_sub supplied bpp is 12 bytes (3 components per pixel), but backing texture bpp is 8 bytes (4 components per pixel) (TODO(Metal): Channel Conversion needed) (w: 4, h: 4, d: 12) WARN (gpu.debug.metal): source/blender/gpu/metal/mtl_texture.mm:604 update_sub: gpu::MTLTexture::update_sub supplied bpp is 12 bytes (3 components per pixel), but backing texture bpp is 8 bytes (4 components per pixel) (TODO(Metal): Channel Conversion needed) (w: 4, h: 4, d: 12) WARN (gpu.debug.metal): source/blender/gpu/metal/mtl_texture.mm:604 update_sub: gpu::MTLTexture::update_sub supplied bpp is 12 bytes (3 components per pixel), but backing texture bpp is 8 bytes (4 components per pixel) (TODO(Metal): Channel Conversion needed) (w: 4, h: 4, d: 12) WARN (gpu.debug.metal): source/blender/gpu/metal/mtl_texture.mm:604 update_sub: gpu::MTLTexture::update_sub supplied bpp is 12 bytes (3 components per pixel), but backing texture bpp is 8 bytes (4 components per pixel) (TODO(Metal): Channel Conversion needed) (w: 4, h: 4, d: 12)
Jeroen Bakker reviewed 2023-06-22 08:29:59 +02:00
@ -201,3 +202,3 @@
}
void blo_do_versions_400(FileData * /*fd*/, Library * /*lib*/, Main *bmain)
void blo_do_versions_400(FileData * fd, Library * /*lib*/, Main *bmain)
Member

run make format

run `make format`
fclem marked this conversation as resolved
Member

@fclem

can you add 118e637e0f to this branch to fix Linux linking issues. And run make format :-) that would solve most of my issues. The other code-style was adding asserts to guard bad usage and a rename of a function (nitpicks).

One thing I noticed is that you need to add a irradiance grid to the scene in order to render anything. Is that expected? If so I would add it to the commit description.

Also I found that the next configuration didn't work:

  • Mesa Intel A770: stalls during compilation. The Mesa Intel A770 driver has many issues with OpenGL.
  • Metal (M1 Ultra): we discussed yesterday that that isn't an issue for this branch, but for the backend and should be solved separately.

For the rest seems fine for initial land in main.

@fclem can you add https://projects.blender.org/Jeroen-Bakker/blender/commit/118e637e0f4eeb6c05b1688be34a5259b3bc1ff2 to this branch to fix Linux linking issues. And run `make format` :-) that would solve most of my issues. The other code-style was adding asserts to guard bad usage and a rename of a function (nitpicks). One thing I noticed is that you need to add a irradiance grid to the scene in order to render anything. Is that expected? If so I would add it to the commit description. Also I found that the next configuration didn't work: - Mesa Intel A770: stalls during compilation. The Mesa Intel A770 driver has many issues with OpenGL. - Metal (M1 Ultra): we discussed yesterday that that isn't an issue for this branch, but for the backend and should be solved separately. For the rest seems fine for initial land in main.
Clément Foucault added 3 commits 2023-06-22 12:05:57 +02:00
Author
Member

@blender-bot build

@blender-bot build
Hans Goudey reviewed 2023-06-22 18:23:44 +02:00
@ -0,0 +301,4 @@
WM_JOB_TYPE_LIGHT_BAKE);
LightBake *bake = new LightBake(
bmain, view_layer, scene, original_probes, true, frame, delay_ms);
Member

std::move(original_probes)

`std::move(original_probes)`
fclem marked this conversation as resolved
@ -0,0 +316,4 @@
return wm_job;
}
void *EEVEE_NEXT_lightbake_job_data_alloc(struct Main *bmain,
Member

These struct keywords are unnecessary in C++

These `struct` keywords are unnecessary in C++
fclem marked this conversation as resolved
@ -0,0 +322,4 @@
blender::Vector<struct Object *> original_probes,
int frame)
{
LightBake *bake = new LightBake(bmain, view_layer, scene, original_probes, false, frame);
Member

std::move(original_probes)

`std::move(original_probes)`
fclem marked this conversation as resolved
@ -0,0 +17,4 @@
struct wmJob;
/** Opaque type hiding eevee::LightBake. */
typedef struct EEVEE_NEXT_LightBake EEVEE_NEXT_LightBake;
Member

typedef -> using

`typedef` -> `using`
Author
Member

I'm not sure about what you are suggesting. EEVEE_NEXT_LightBake is never defined and is just here to hide eevee::LightBake. Are you suggesting using EEVEE_NEXT_LightBake = struct EEVEE_NEXT_LightBake;?

I'm not sure about what you are suggesting. `EEVEE_NEXT_LightBake` is never defined and is just here to hide `eevee::LightBake`. Are you suggesting `using EEVEE_NEXT_LightBake = struct EEVEE_NEXT_LightBake;`?
Member

Yeah, using EEVEE_NEXT_LightBake = EEVEE_NEXT_LightBake
That's making less sense right now, not sure why this using/typdef is necessary in C++

Yeah, `using EEVEE_NEXT_LightBake = EEVEE_NEXT_LightBake` That's making less sense right now, not sure why this using/typdef is necessary in C++
fclem marked this conversation as resolved
Clément Foucault added 2 commits 2023-06-22 18:38:40 +02:00
Address Review
Some checks failed
buildbot/vexp-code-patch-coordinator Build done.
d0a456db2d
Author
Member

@blender-bot build linux

@blender-bot build linux
Hans Goudey reviewed 2023-06-22 22:43:06 +02:00
Hans Goudey left a comment
Member

Was just reading through this out of curiosity, but left some comments as I went.

Was just reading through this out of curiosity, but left some comments as I went.
@ -451,1 +485,4 @@
void Instance::light_bake_irradiance(
Object &probe,
std::function<void()> context_enable,
Member

Looks like these could use FunctionRef instead of std::function? That's a bit clearer since these aren't stored more permanently.

Looks like these could use `FunctionRef` instead of `std::function`? That's a bit clearer since these aren't stored more permanently.
fclem marked this conversation as resolved
@ -34,1 +90,4 @@
return {};
}
Vector<IrradianceBrickPacked> allocated;
allocated.resize(brick_len);
Member

These can be combined into one line: Vector<IrradianceBrickPacked> allocated(brick_len);

These can be combined into one line: `Vector<IrradianceBrickPacked> allocated(brick_len);`
fclem marked this conversation as resolved
@ -64,0 +389,4 @@
display_grids_ps_.bind_texture("irradiance_c_tx", &irradiance_c_tx);
display_grids_ps_.bind_texture("irradiance_d_tx", &irradiance_d_tx);
int sample_count = static_cast<int>(BKE_lightprobe_grid_cache_frame_sample_count(cache));
Member

Functional cast for arithmetic types:
const int sample_count = int(BKE_lightprobe_grid_cache_frame_sample_count(cache));

Functional cast for arithmetic types: `const int sample_count = int(BKE_lightprobe_grid_cache_frame_sample_count(cache));`
fclem marked this conversation as resolved
@ -0,0 +68,4 @@
std::mutex result_mutex_;
public:
LightBake(struct Main *bmain,
Member

struct Main -> Main, etc.

Same below, and in other files.

`struct Main` -> `Main`, etc. Same below, and in other files.
fclem marked this conversation as resolved
@ -0,0 +330,4 @@
void EEVEE_NEXT_lightbake_job_data_free(void *job_data)
{
delete reinterpret_cast<LightBake *>(job_data);
Member

reinterpret_cast -> static_cast when casting from void *

`reinterpret_cast` -> `static_cast` when casting from `void *`
fclem marked this conversation as resolved
@ -0,0 +52,4 @@
void LightProbeModule::sync_probe(const Object *ob, ObjectHandle &handle)
{
const ::LightProbe *light_probe = (const ::LightProbe *)ob->data;
Member

static_cast

`static_cast`
fclem marked this conversation as resolved
@ -0,0 +109,4 @@
}
}
#if 0 /* TODO make this work with new per object light cache. */
Member

Is the comment meant to be on a separate line from #if 0?

Is the comment meant to be on a separate line from `#if 0`?
@ -0,0 +3,4 @@
* Virtual shadowmapping: Usage tagging
*
* Shadow pages are only allocated if they are visible.
* This pass iterates the surfels buffer and tag all tiles that are needed for light shadowing as
Member

tag -> tags

`tag` -> `tags`
@ -213,0 +291,4 @@
/* -------------------------------------------------------------------- */
/** \name Load/Store
*
* This section define the compression scheme of spherical harmonic data.
Member

define -> defines

`define` -> `defines`
@ -21,0 +140,4 @@
.uniform_buf(IRRADIANCE_GRID_BUF_SLOT,
"IrradianceGridData",
"grids_infos_buf[IRRADIANCE_GRID_MAX]")
/* NOTE: Use uint instead of IrradianceBrickPacked because Metal need to know the exact type.*/
Member

need -> needs

`need` -> `needs`
fclem marked this conversation as resolved
@ -1760,0 +1790,4 @@
"ALL",
0,
"All Light Probes",
"Delete all light probes baked lighting data"},
Member

probes -> probes' (possessive and plural (ends with an s)). Same for selected

`probes` -> `probes'` (possessive and plural (ends with an s)). Same for selected
fclem marked this conversation as resolved
@ -1760,0 +1800,4 @@
"ACTIVE",
0,
"Active Only",
"Only delete the active light probe baked lighting data"},
Member

probe -> probe's

`probe` -> `probe's`
fclem marked this conversation as resolved
@ -158,1 +158,4 @@
prop = RNA_def_property(srna, "grid_bake_samples", PROP_INT, PROP_NONE);
RNA_def_property_ui_text(
prop, "Bake Samples", "Number of rays direction to evaluate when baking");
Member

rays direction -> ray directions I think?

`rays direction` -> `ray directions` I think?
Clément Foucault added 3 commits 2023-06-22 22:57:12 +02:00
Author
Member

@blender-bot build linux

@blender-bot build linux
Clément Foucault added 2 commits 2023-06-23 00:01:18 +02:00
Member

Only blender organization members with write access can start builds. See documentation for details.

Only blender organization members with write access can start builds. See [documentation](https://projects.blender.org/infrastructure/blender-bot/src/branch/main/README.md) for details.
Jeroen Bakker approved these changes 2023-06-23 07:59:03 +02:00
Clément Foucault added 1 commit 2023-06-23 08:31:25 +02:00
Clément Foucault merged commit ddd88c00b4 into main 2023-06-23 08:39:52 +02:00
Clément Foucault deleted branch eevee-next-irradiance-cache 2023-06-23 08:39:54 +02:00
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
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
7 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#108639
No description provided.