EEVEE-Next: Horizon Scan: Use Spherical harmonics #118924

Merged
Clément Foucault merged 45 commits from fclem/blender:eevee-next-horizon-scan-refactor into main 2024-03-19 19:16:30 +01:00

This uses Spherical Harmonics to store the indirect lighting and
distant lighting visibility.

We can then reuse this information for each closure which divide
the cost of it by 2 or 3 in many cases, doing the scanning once.

The storage cost is higher than previous method, so we split the
resolution scaling to be independant of raytracing.

The spatial filtering has been split to its own pass for performance
reason. Upsampling now only uses 4 bilinearly interpolated samples
(instead of 9) using bilateral weights to avoid bleeding.

This also add a missing dot product (which soften the lighting
around corners) and fixes the blocky artifacts seen at lower
resolution.

1 rpp 1/16 rpp
Before
After
This uses Spherical Harmonics to store the indirect lighting and distant lighting visibility. We can then reuse this information for each closure which divide the cost of it by 2 or 3 in many cases, doing the scanning once. The storage cost is higher than previous method, so we split the resolution scaling to be independant of raytracing. The spatial filtering has been split to its own pass for performance reason. Upsampling now only uses 4 bilinearly interpolated samples (instead of 9) using bilateral weights to avoid bleeding. This also add a missing dot product (which soften the lighting around corners) and fixes the blocky artifacts seen at lower resolution. || 1 rpp | 1/16 rpp | | -------- | -------- | -------- | |Before|![](https://projects.blender.org/attachments/687337eb-c0e2-4b00-9970-310ccce0487a)| ![](https://projects.blender.org/attachments/47e96fbd-ee35-4d42-b46a-2654400335b9)| |After| ![](https://projects.blender.org/attachments/c291b904-64fb-4d9a-a5dc-f88361c185d9)| ![](https://projects.blender.org/attachments/6c310fe2-7328-4d04-9bdb-0f9990d69445)|
Clément Foucault added this to the 4.2 LTS milestone 2024-02-29 20:02:41 +01:00
Clément Foucault added the
Interest
EEVEE
Module
EEVEE & Viewport
labels 2024-02-29 20:02:41 +01:00
Clément Foucault added 15 commits 2024-02-29 20:02:50 +01:00
Clément Foucault added 2 commits 2024-03-01 19:41:52 +01:00
f614e912a2 Merge horizon scan denoising passes into one pass
Have a nice perf boost (43 > 47 fps) in complex scene.
Can likely be improved further as this remove the tiling
optimization
Clément Foucault added 3 commits 2024-03-09 21:23:32 +01:00
Clément Foucault added 2 commits 2024-03-11 15:26:52 +01:00
Clément Foucault added 4 commits 2024-03-12 16:24:00 +01:00
Author
Member

Current implementation make use of much more memory than previously.
Spherical harmonics need 8 times the storage previously used by the horizon scan. This has an impact on the spatial filtering efficiency which is now done at tracing resolution instead of running it at target resolution.

But this raises the need for a separate resolution parameter for the horizon scan which most of the time requires less resolution than the direct ray-tracing.

Current implementation make use of much more memory than previously. Spherical harmonics need 8 times the storage previously used by the horizon scan. This has an impact on the spatial filtering efficiency which is now done at tracing resolution instead of running it at target resolution. But this raises the need for a separate resolution parameter for the horizon scan which most of the time requires less resolution than the direct ray-tracing.
Clément Foucault added 5 commits 2024-03-12 19:18:38 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
25a49fef69
Split resolution scaling option for horizon scan
Clément Foucault requested review from Miguel Pozo 2024-03-12 19:32:39 +01:00
Clément Foucault requested review from Jeroen Bakker 2024-03-12 19:32:46 +01:00
Author
Member

Adding Miguel and Jeroen so they can test on their hardware if anything is much slower or broken.

Adding Miguel and Jeroen so they can test on their hardware if anything is much slower or broken.
Clément Foucault changed title from EEVEE-Next: Horizon Scan: Do tracing only once to EEVEE-Next: Horizon Scan: Use Spherical harmonics 2024-03-12 19:34:33 +01:00
First-time contributor

Could you add an option to add 1/8 RPP
I think isn't a bad idea for have a perfect balance between performance / quality

Could you add an option to add 1/8 RPP I think isn't a bad idea for have a perfect balance between performance / quality
Member

Just a quick test for now, but at least on this scene performs and looks better (at max resolution).
Quite promising, congrats!

Before After
imagen imagen
imagen imagen
Just a quick test for now, but at least on this scene performs and looks better (at max resolution). Quite promising, congrats! | Before | After | | --- | --- | | ![imagen](/attachments/8517691c-339e-4532-ac5d-8461c46a8d56) | ![imagen](/attachments/a788efba-d887-4023-8e3a-e000c4410598) | | ![imagen](/attachments/b8b99a5c-1f17-400e-a854-199c1adf70c0) | ![imagen](/attachments/61946f7f-6dcf-4f9c-b523-e6a31f6220ca) |
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/PR118924) when ready.
Member

This is just a test on metal. Details are much more natural. I couldn't find any differences when using the precision level slider. Is that expected? I also didn't see any visual differences when using a lower render resolution for the AO

Will still try to run it on iGPUs and do a quick scan of the code. So far so good!

This is just a test on metal. Details are much more natural. I couldn't find any differences when using the precision level slider. Is that expected? I also didn't see any visual differences when using a lower render resolution for the AO Will still try to run it on iGPUs and do a quick scan of the code. So far so good!
Member

I performed some performance measurements on different scenes. And got mixed results. For good comparison I merged main into this PR.

Scene Branch Renderer Resolution iGPU RX7700
rain main EEVEE 1.15 27.3
rain main EEVEE-Next 2.70 24.5
rain 118924 EEVEE-Next 1x 1.50 25.6
rain 118924 EEVEE-Next 4x 2.70 24.8
rain 118924 EEVEE-Next 16x 3.30 24.9
tree main EEVEE 0.9 15.5
tree main EEVEE-Next 0.2 14.8
tree 118924 EEVEE-Next 1x 0.22 14.6
tree 118924 EEVEE-Next 4x 0.23 14.8
tree 118924 EEVEE-Next 16x 0.23 14.7

rain = rain_restaurant.blend
tree = tree_creature.blend
display resolution = 4k
iGPU = RAPHAEL_MENDOCINO

I will look into the tree_creature performance in EEVEE-Next. I expected more uplift, but could be related to something else. Ok the performance bottleneck for the tree creature is shadowing. When disabling the shadowing I get (4.7 fps, 7.8 fps and 8.0 fps)

I performed some performance measurements on different scenes. And got mixed results. For good comparison I merged `main` into this PR. | Scene | Branch | Renderer | Resolution | iGPU | RX7700 | |-------|--------|------------|------------|------|--------| | rain | main | EEVEE | | 1.15 | 27.3 | | rain | main | EEVEE-Next | | 2.70 | 24.5 | | rain | 118924 | EEVEE-Next | 1x | 1.50 | 25.6 | | rain | 118924 | EEVEE-Next | 4x | 2.70 | 24.8 | | rain | 118924 | EEVEE-Next | 16x | 3.30 | 24.9 | | tree | main | EEVEE | | 0.9 | 15.5 | | tree | main | EEVEE-Next | | 0.2 | 14.8 | | tree | 118924 | EEVEE-Next | 1x | 0.22 | 14.6 | | tree | 118924 | EEVEE-Next | 4x | 0.23 | 14.8 | | tree | 118924 | EEVEE-Next | 16x | 0.23 | 14.7 | > rain = rain_restaurant.blend > tree = tree_creature.blend > display resolution = 4k > iGPU = RAPHAEL_MENDOCINO I will look into the tree_creature performance in EEVEE-Next. I expected more uplift, but could be related to something else. Ok the performance bottleneck for the tree creature is shadowing. When disabling the shadowing I get (4.7 fps, 7.8 fps and 8.0 fps)
Author
Member

If you don't get any noticeable uplift, then the bottleneck is somewhere else.

The performance drop in rain at full resolution might be explained by the bandwidth usage being larger in this case. That's why the default is 4x which is roughly the same bandwidth usage as before.

If you don't get any noticeable uplift, then the bottleneck is somewhere else. The performance drop in rain at full resolution might be explained by the bandwidth usage being larger in this case. That's why the default is 4x which is roughly the same bandwidth usage as before.
Member

Branch doesn't work on Metal.

image

Branch doesn't work on Metal. ![image](/attachments/518f50fd-6376-4ad8-8a2e-349388437a71)
791 KiB
Clément Foucault added 4 commits 2024-03-14 12:50:26 +01:00
Clément Foucault added 2 commits 2024-03-14 14:51:49 +01:00
Author
Member

The Metal issue should be fixed.

The Metal issue should be fixed.
Clément Foucault added 1 commit 2024-03-14 15:25:35 +01:00
Clément Foucault added 1 commit 2024-03-14 15:44:52 +01:00
e8cb5d3127 Use mean BxDF direction to evaluate the SH
Allows for a bit of reflection behavior for not max roughness materials
Jeroen Bakker reviewed 2024-03-18 13:04:31 +01:00
@ -68,1 +85,3 @@
ivec2 texel = (texel_fullres) / uniform_buf.raytrace.resolution_scale;
vec2 texel_size = 1.0 / vec2(textureSize(in_sh_0_tx, 0).xy);
ivec2 texel_fullres = texel * uniform_buf.raytrace.horizon_resolution_scale +
uniform_buf.raytrace.horizon_resolution_bias;
Member

Do we need to check the bounds?

if (any(greaterThanEqual(texel_fullres, extent))) {

Do we need to check the bounds? `if (any(greaterThanEqual(texel_fullres, extent))) {`
Author
Member

This one is necessary. The dispatch is in tiles that can process outside of the render target boundaries.

However, the background check is not needed since we already check for GBuffer header value.

This one is necessary. The dispatch is in tiles that can process outside of the render target boundaries. However, the background check is not needed since we already check for GBuffer header value.
fclem marked this conversation as resolved
@ -21,1 +22,4 @@
/* Avoid loading texels outside texture range. */
ivec2 extent = textureSize(gbuf_header_tx, 0).xy;
texel_fullres = min(texel_fullres, extent - 1);
Member

Do we need to add a boundry check? RAYTRACE_GROUP_SIZE = 8

Do we need to add a boundry check? `RAYTRACE_GROUP_SIZE = 8`
Author
Member

That is exactly what this line does. If the thread is outside the valid range, it will sample the edge pixels. This is needed because of the jitter in horizon_resolution_bias.

The imageStore should do the bound check for writing. However, it might be preferable to disable the threads that are outside the output bounds to leverage imageStoreFast. Will do that.

That is exactly what this line does. If the thread is outside the valid range, it will sample the edge pixels. This is needed because of the jitter in `horizon_resolution_bias`. The `imageStore` should do the bound check for writing. However, it might be preferable to disable the threads that are outside the output bounds to leverage `imageStoreFast`. Will do that.
fclem marked this conversation as resolved
Jeroen Bakker reviewed 2024-03-18 13:06:24 +01:00
@ -2995,6 +2995,12 @@ void blo_do_versions_400(FileData *fd, Library * /*lib*/, Main *bmain)
}
}
if (!MAIN_VERSION_FILE_ATLEAST(bmain, 402, 10)) {
Member

Add if (!DNA_struct_member_exists(fd->filesdna, for clarity.
See https://blender.chat/channel/blender-coders?msg=3aeDipEsYEtDJGwei for reference.

Add `if (!DNA_struct_member_exists(fd->filesdna,` for clarity. See https://blender.chat/channel/blender-coders?msg=3aeDipEsYEtDJGwei for reference.
fclem marked this conversation as resolved
Clément Foucault added 4 commits 2024-03-19 15:25:14 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
c4d22b6b5b
Add double versionning code
Author
Member

@blender-bot build

@blender-bot build
Jeroen Bakker approved these changes 2024-03-19 15:41:20 +01:00
Jeroen Bakker added this to the EEVEE & Viewport project 2024-03-19 15:41:29 +01:00
Miguel Pozo requested changes 2024-03-19 16:16:41 +01:00
Miguel Pozo left a comment
Member

I only took a quick look at the code, but I've tested the patch on my RTX GPU and on my cheap integrated AMD laptop and the quality and performance are better on both.

The only case when it's slower is when all materials are diffuse-only.
But optimizing for the common/worse case seems like a good compromise to make.

I only took a quick look at the code, but I've tested the patch on my RTX GPU and on my cheap integrated AMD laptop and the quality and performance are better on both. The only case when it's slower is when all materials are diffuse-only. But optimizing for the common/worse case seems like a good compromise to make.
@ -0,0 +10,4 @@
#pragma BLENDER_REQUIRE(eevee_lightprobe_eval_lib.glsl)
#pragma BLENDER_REQUIRE(eevee_closure_lib.glsl)
float bilateral_depth_weight(vec3 center_N, vec3 center_P, vec3 sample_P)
Member

There are some functions duplicated between the denoise and resolve shaders.
Couldn't these be moved to a lib file?

There are some functions duplicated between the denoise and resolve shaders. Couldn't these be moved to a lib file?
fclem marked this conversation as resolved
@ -1895,6 +1895,7 @@ typedef struct SceneEEVEE {
float gtao_quality;
float gtao_thickness;
float gtao_focus;
int gtao_resolution;
Member

Why are you using the gtao term for the DNA property?

Why are you using the `gtao` term for the DNA property?
Author
Member

To avoid discrepancy with existing code. If we are to rename it, we will rename all of them at once.

To avoid discrepancy with existing code. If we are to rename it, we will rename all of them at once.
Member

Oh... because the horizon scan is driven by the AOData from the AmbientOcclussion module.
But these new properties are added to RayTraceData directly.
I find that quite confusing, TBH.

Oh... because the horizon scan is driven by the `AOData` from the `AmbientOcclussion` module. But these new properties are added to `RayTraceData` directly. I find that quite confusing, TBH.
Author
Member

I do agree that this is confusing. But I still think this is best left for a future cleanup.

I do agree that this is confusing. But I still think this is best left for a future cleanup.
fclem marked this conversation as resolved
@ -8176,0 +8186,4 @@
RNA_def_property_ui_text(prop,
"Resolution",
"Control the quality of the horizon scan lighting "
"(lower size increase vram usage and quality)");
Member

Just a nitpick, but I would make clear that this also has a render time cost, not only a memory cost.
Something like (lower size increases quality at the cost of performance and vram).

Just a nitpick, but I would make clear that this also has a render time cost, not only a memory cost. Something like `(lower size increases quality at the cost of performance and vram)`.
Author
Member

I would argue that this is implicit from stating quality. For reference I copied Tile Size setting for volumetrics. Not sure this has ever been an issue there.

I would argue that this is implicit from stating `quality`. For reference I copied `Tile Size` setting for volumetrics. Not sure this has ever been an issue there.
Author
Member

I'll do a UI pass with the UX team later. I don't think this is blocker.

I'll do a UI pass with the UX team later. I don't think this is blocker.
fclem marked this conversation as resolved
Clément Foucault added 1 commit 2024-03-19 18:18:43 +01:00
Clément Foucault added 1 commit 2024-03-19 19:15:35 +01:00
09279ed42b Merge branch 'main' into eevee-next-horizon-scan-refactor
# Conflicts:
#	source/blender/draw/engines/eevee_next/shaders/eevee_spherical_harmonics_lib.glsl
Clément Foucault merged commit 23dce15f67 into main 2024-03-19 19:16:30 +01:00
Clément Foucault deleted branch eevee-next-horizon-scan-refactor 2024-03-19 19:16:33 +01: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
5 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#118924
No description provided.