EEVEE-Next: Jittered Shadow Transparency #119480

Merged

Miguel Pozo merged 12 commits from pragma37/blender:pull-eevee-transparent-shadows into main

2024-03-20 15:56:08 +01:00

Miguel Pozo commented

2024-03-14 17:57:46 +01:00

Member

Smooth transparent shadows by jittering their opacity threshold every sample.
Always enabled on final renders, optionally enabled in the viewport with scene.eevee.shadow_jittered_transparency.

Smooth transparent shadows by jittering their opacity threshold every sample. Always enabled on final renders, optionally enabled in the viewport with `scene.eevee.shadow_jittered_transparency`.

Miguel Pozo added the

EEVEE & Viewport

labels 2024-03-14 17:57:46 +01:00

Miguel Pozo added 1 commit 2024-03-14 17:57:57 +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-coordinator Build done. Details

7ae9d002be Jittered Shadow Transparency

Miguel Pozo added this to the EEVEE & Viewport project 2024-03-14 17:58:01 +01:00

Miguel Pozo commented

2024-03-14 17:59:03 +01:00

Author

Member

@blender-bot build +gpu

@blender-bot build +gpu

Miguel Pozo requested review from Clément Foucault 2024-03-14 17:59:16 +01:00

Clément Foucault reviewed 2024-03-14 18:03:15 +01:00

source/blender/draw/engines/eevee_next/shaders/eevee_surf_shadow_frag.glsl Outdated

						
				@ -35,0 +37,4 @@

				      uniform_buf.pipeline.alpha_hash_scale, noise_offset, g_data.P);

				#  else

				  /* A fully random threshold yields cleaner results than the hashed alpha. */

				  float random_threshold = hash_vec3_to_float(g_data.P * (1.0 + noise_offset));

Clément Foucault commented

2024-03-14 18:03:15 +01:00

Member

Isn't reducing uniform_buf.pipeline.alpha_hash_scale clean enough?

Isn't reducing `uniform_buf.pipeline.alpha_hash_scale` clean enough?

Clément Foucault commented

2024-03-14 19:47:42 +01:00

Member

Split this change to its own PR. We have to evaluate all aspect of it. The transparency_hashed_alpha_threshold is engineered to be temporally stable which plays nice with TAA.

Split this change to its own PR. We have to evaluate all aspect of it. The `transparency_hashed_alpha_threshold` is engineered to be temporally stable which plays nice with TAA.

Miguel Pozo commented

2024-03-15 16:51:07 +01:00

Author

Member

Done: #119526

fclem marked this conversation as resolved

Miguel Pozo commented

2024-03-14 18:16:17 +01:00

Author

Member

Isn't reducing uniform_buf.pipeline.alpha_hash_scale clean enough?

Nope!

0.01	0.001 🙀

Replying here since I can't attach images on the review comment.

> Isn't reducing `uniform_buf.pipeline.alpha_hash_scale` clean enough? Nope! | 0.01 | 0.001 🙀 | | --- | --- | | ![imagen](/attachments/15e40908-6d60-491c-a396-17c476887e33) | ![imagen](/attachments/3065721b-1281-4c64-80f2-6cd558be681e) | Replying here since I can't attach images on the review comment.

643 KiB

641 KiB

Miguel Pozo added 1 commit 2024-03-15 16:37:30 +01:00

d82101578c Remove transparency threshold improvements

Miguel Pozo referenced this pull request

2024-03-15 16:47:58 +01:00

EEVEE-Next: Fix transparent shadows convergence #119526

Miguel Pozo added a new dependency 2024-03-15 16:49:23 +01:00

#119526 EEVEE-Next: Fix transparent shadows convergence

Clément Foucault requested changes 2024-03-18 15:20:50 +01:00

Dismissed

source/blender/draw/engines/eevee_next/eevee_shadow.cc Outdated

						
				@ -909,2 +913,3 @@

				  const bool is_initialized = shadow_ob.resource_handle.raw != 0;

				  if ((handle.recalc != 0 || !is_initialized) && is_shadow_caster) {

				  if (is_shadow_caster &&

				      (handle.recalc || !is_initialized || (has_transparent_shadows && jittered_transparency_)))

Clément Foucault commented

2024-03-18 15:10:10 +01:00

Member

I don't understand how this can work for render. Object sync only happens once per motion blur step. So was this tested in viewport, viewport render and F12 render?

The correct way to handle this for render, is to add a separate list of casters_updated (maybe named transparent_casters_updated_) that is processed once per sample (it must not produce infinite update inside the ShadowModule::set_view do...while loop).

I would also populate this transparent_casters_updated_ out of this handle.recalc case since it doesn't mean the object has moved, so only one bound need to be transformed. You can then process transparent_casters_updated_ only in render or if jittered_transparency_ is enabled.

I don't understand how this can work for render. Object sync only happens once per motion blur step. So was this tested in viewport, viewport render and F12 render? The correct way to handle this for render, is to add a separate list of `casters_updated` (maybe named `transparent_casters_updated_`) that is processed once per sample (it must not produce infinite update inside the `ShadowModule::set_view` `do...while` loop). I would also populate this `transparent_casters_updated_` out of this `handle.recalc` case since it doesn't mean the object has moved, so only one bound need to be transformed. You can then process `transparent_casters_updated_` only in render or if `jittered_transparency_` is enabled.

Miguel Pozo commented

2024-03-18 17:56:02 +01:00

Author

Member

As discussed on chat, there was an issue in ShadowModule that caused shadow-maps to be re-rendered every sample for image renders.
I fixed that issue, and then implemented a separate jittered_transparent_caster_update_ps_ that runs every sample.

As discussed on chat, there was an issue in `ShadowModule` that caused shadow-maps to be re-rendered every sample for image renders. I fixed that issue, and then implemented a separate `jittered_transparent_caster_update_ps_` that runs every sample.

Miguel Pozo added 3 commits 2024-03-18 17:53:32 +01:00

2ec359b420 Fix shadows being re-rendered every sample

3b5534ef43 Default initialize

101a4da554 Add a separate jittered_transparent_caster_update_ps_

Miguel Pozo added 1 commit 2024-03-18 18:01:02 +01:00

d5da364c06 Run jittered_transparent_caster_update_ps_ only once per sample

Miguel Pozo added 1 commit 2024-03-18 18:12:03 +01:00

556939393e Fix past_casters_updated_ and cleanup

Miguel Pozo force-pushed pull-eevee-transparent-shadows from 27f071fcbc to 556939393e

2024-03-18 20:24:50 +01:00

Compare

Miguel Pozo commented

2024-03-18 20:36:53 +01:00

Author

Member

The force push doesn't contain any change.
I just reverted the last 3 commits, since I've pushed them here by error.

The force push doesn't contain any change. I just reverted the last 3 commits, since I've pushed them here by error.

Miguel Pozo requested review from Clément Foucault 2024-03-18 20:37:06 +01:00

Clément Foucault approved these changes 2024-03-19 11:00:40 +01:00

Dismissed

Clément Foucault requested changes 2024-03-19 11:10:00 +01:00

Dismissed

source/blender/makesrna/intern/rna_scene.cc Outdated

						
				@ -8027,6 +8027,14 @@ static void rna_def_scene_eevee(BlenderRNA *brna)

				      prop, "Tracing Method", "Select the tracing method used to find scene-ray intersections");

				  RNA_def_property_update(prop, NC_SCENE | ND_RENDER_OPTIONS, nullptr);

				  prop = RNA_def_property(srna, "shadow_jittered_transparency", PROP_BOOLEAN, PROP_NONE);

Clément Foucault commented

2024-03-19 11:02:02 +01:00

Member

This is unclear that this is a viewport only setting. I'm wondering what would be better. Also python properties should follow the pythonic naming rule (prefix use). So I suggest use_shadow_jittered_transparency_viewport.

But I wonder if we really need this property. EEVEE legacy didn't refresh the transparent shadows in the viewport at all if soft shadows were disabled.

Also I am now thinking that this is uneeded for things like foliage and other transparent materials that use a very sharp transparency. So would that be a material property instead? That feels overkill.

This is unclear that this is a viewport only setting. I'm wondering what would be better. Also python properties should follow the pythonic naming rule (prefix `use`). So I suggest `use_shadow_jittered_transparency_viewport`. But I wonder if we really need this property. EEVEE legacy didn't refresh the transparent shadows in the viewport at all if soft shadows were disabled. Also I am now thinking that this is uneeded for things like foliage and other transparent materials that use a very sharp transparency. So would that be a material property instead? That feels overkill.

Miguel Pozo commented

2024-03-19 17:55:14 +01:00

Author

Member

But I wonder if we really need this property. EEVEE legacy didn't refresh the transparent shadows in the viewport at all if soft shadows were disabled.

I think it's needed, otherwise we're pushing users to use an even more expensive setting (that in EEVEE-Next is supposed to be per-light), just to get ok quality transparent shadows.

Also I am now thinking that this is uneeded for things like foliage and other transparent materials that use a very sharp transparency. So would that be a material property instead? That feels overkill.

That's a good point, but this is also an issue for final renders.

> But I wonder if we really need this property. EEVEE legacy didn't refresh the transparent shadows in the viewport at all if soft shadows were disabled. I think it's needed, otherwise we're pushing users to use an even more expensive setting (that in EEVEE-Next is supposed to be per-light), just to get ok quality transparent shadows. > Also I am now thinking that this is uneeded for things like foliage and other transparent materials that use a very sharp transparency. So would that be a material property instead? That feels overkill. That's a good point, but this is also an issue for final renders.

Clément Foucault commented

2024-03-19 18:08:36 +01:00

Member

I think it's needed, otherwise we're pushing users to use an even more expensive setting (that in EEVEE-Next is supposed to be per-light), just to get ok quality transparent shadows.

I think you are right. And there will be the possibility of setting jittered shadows per light later on anyway.

But I would say that then the setting should be the same for jittered soft shadows and stochastic transparency. So use_shadow_jittered_transparency_viewport would simply become use_shadow_jittered_viewport.

> I think it's needed, otherwise we're pushing users to use an even more expensive setting (that in EEVEE-Next is supposed to be per-light), just to get ok quality transparent shadows. I think you are right. And there will be the possibility of setting jittered shadows per light later on anyway. But I would say that then the setting should be the same for jittered soft shadows and stochastic transparency. So `use_shadow_jittered_transparency_viewport` would simply become `use_shadow_jittered_viewport`.

Miguel Pozo commented

2024-03-19 20:02:18 +01:00

Author

Member

I've updated the property names so we don't break compat, but I kept the UI label the same.
We can change it once jittered soft shadows land.

I've updated the property names so we don't break compat, but I kept the UI label the same. We can change it once jittered soft shadows land.

fclem marked this conversation as resolved

source/blender/makesrna/intern/rna_scene.cc Outdated

						
				@ -8030,0 +8031,4 @@

				  RNA_def_property_boolean_sdna(prop, nullptr, "flag", SCE_EEVEE_SHADOW_JITTERED_TRANSPARENCY);

				  RNA_def_property_ui_text(prop,

				                           "Jittered Shadow Transparency",

				                           "Enable jitterd transparent shadows on the viewport. (Expensive, "

Clément Foucault commented

2024-03-19 11:01:19 +01:00

Member

Typo jittered

Typo `jittered`

pragma37 marked this conversation as resolved

Miguel Pozo added 2 commits 2024-03-19 17:46:27 +01:00

a90e32ffe8 Fix typo

b01e8daa2e Add viewport suffix and clarification

Miguel Pozo added 2 commits 2024-03-19 20:00:29 +01:00

10556af0e9 Rename

1e677b992e Merge branch 'main' into pull-eevee-transparent-shadows

Miguel Pozo added 1 commit 2024-03-19 20:07:12 +01:00

b360d0ef36 Revert threshold changes merge

Clément Foucault approved these changes 2024-03-19 21:27:21 +01:00

Miguel Pozo merged commit 881fd2dbd5 into main

2024-03-20 15:56:08 +01:00

Miguel Pozo referenced this issue from a commit

2024-03-20 15:56:09 +01:00

EEVEE-Next: Jittered Shadow Transparency

Miguel Pozo deleted branch pull-eevee-transparent-shadows

2024-03-20 15:56:11 +01:00

Miguel Pozo referenced this issue from a commit

2024-03-20 16:05:16 +01:00

EEVEE-Next: Fix transparent shadows convergence

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

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.

Blocks

#119526 EEVEE-Next: Fix transparent shadows convergence

blender/blender

Reference: blender/blender#119480

No description provided.