EEVEE-Next: Jittered Soft Shadows #119753

Open
Miguel Pozo wants to merge 46 commits from pragma37/blender:pull-eevee-jittered-shoft-shadows into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Member

Jittered Soft Shadows support.
Improves soft shadow quality at the cost of re-rendering shadow maps every sample.
Disabled by default in the viewport unless enabled in the Scene settings.

Tracing-only Jitter-only Jitter+Over-blur
imagen imagen imagen

Tracing-only is the method used by default in EEVEE-Next.
Jitter-only is the method used by EEVEE-Legacy Soft Shadows.
Jitter+Over-blur combines both.

Jittered Soft Shadows support. Improves soft shadow quality at the cost of re-rendering shadow maps every sample. Disabled by default in the viewport unless enabled in the Scene settings. | Tracing-only | Jitter-only | Jitter+Over-blur | | --- | --- | --- | | ![imagen](/attachments/e5ca6120-0666-4e86-b6e0-3d7512587b86) | ![imagen](/attachments/a72631aa-14f8-4e10-a748-848fc4bd4ab2) | ![imagen](/attachments/07c5de65-61d2-48e7-b78c-9c3cbdcaf844) | Tracing-only is the method used by default in EEVEE-Next. Jitter-only is the method used by EEVEE-Legacy Soft Shadows. Jitter+Over-blur combines both.
Miguel Pozo added the
Interest
EEVEE
Module
EEVEE & Viewport
labels 2024-03-21 17:36:03 +01:00
Miguel Pozo added 6 commits 2024-03-21 17:36:09 +01:00
Miguel Pozo added 1 commit 2024-03-21 20:18:20 +01:00
Miguel Pozo force-pushed pull-eevee-jittered-shoft-shadows from a29db7a750 to 6df6039725 2024-03-22 23:31:46 +01:00 Compare
Miguel Pozo added 3 commits 2024-04-04 20:57:11 +02:00
Miguel Pozo added 4 commits 2024-04-05 19:59:04 +02:00
Miguel Pozo added 2 commits 2024-04-11 01:35:38 +02:00
Miguel Pozo added 2 commits 2024-04-11 16:19:25 +02: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-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
fc9d6e0d97
Update UI tooltips
Author
Member

@blender-bot build +gpu

@blender-bot build +gpu
Miguel Pozo changed title from WIP: EEVEE-Next: Jittered Soft Shadows to EEVEE-Next: Jittered Soft Shadows 2024-04-11 16:50:04 +02:00
Miguel Pozo requested review from Clément Foucault 2024-04-11 16:50:20 +02:00
Clément Foucault requested changes 2024-04-14 20:42:35 +02:00
Clément Foucault left a comment
Member

Results looks good. Just have to fix the directional and do some cleanup.

Results looks good. Just have to fix the directional and do some cleanup.
@ -117,6 +117,10 @@ class DATA_PT_EEVEE_light(DataButtonsPanel, Panel):
col.separator()
col.prop(light, "use_shadow", text="Cast Shadow")
col.prop(light, "use_shadow_jittering", text="Shadow Jittering")

You don't need to give a text to properties which alread have the same name.

You don't need to give a `text` to properties which alread have the same name.
pragma37 marked this conversation as resolved
@ -94,1 +97,3 @@
la->shadow_trace_distance);
la->sun_angle * softness,
la->shadow_trace_distance,
this->sun.radius * la->shadow_softness_factor);

Why use sun.radius here? it is Clampped to a min and thus will always produce some flicker if jitter is enabled. Prefer computing the disk size again from the angle la->sun_angle * la->shadow_softness_factor when jittering the position (code is less hard to follow).

I feel the same way as for punctual. Just pass sun_angle, jitter amount and tracing amount.

Why use sun.radius here? it is Clampped to a min and thus will always produce some flicker if jitter is enabled. Prefer computing the disk size again from the angle `la->sun_angle * la->shadow_softness_factor` when jittering the position (code is less hard to follow). I feel the same way as for punctual. Just pass `sun_angle`, jitter amount and tracing amount.
pragma37 marked this conversation as resolved
@ -113,3 +118,3 @@
radius,
this->local.influence_radius_max,
la->shadow_softness_factor,
softness,

This becomes way too hard to keep in mind all these different versions of radius. It's better to just pass light radius, the jitter amount and the tracing amount. The clamping for the shadow projection near/far clip should be inside compute_projection_boundaries.

This becomes way too hard to keep in mind all these different versions of radius. It's better to just pass light radius, the jitter amount and the tracing amount. The clamping for the shadow projection near/far clip should be inside `compute_projection_boundaries`.
Author
Member

It's better to just pass light radius

The problem is, what radius? The original or the one set by the Light module?
The compute_projection_boundaries receives both.

I've cleaned the code a bit. We can clarify the naming, but keeping all the separate parameters around and having the softness/jittering logic scattered over the module is going to make things way more confusing, IMO.

> It's better to just pass light radius The problem is, what radius? The original or the one set by the Light module? The `compute_projection_boundaries` receives both. I've cleaned the code a bit. We can clarify the naming, but keeping all the separate parameters around and having the softness/jittering logic scattered over the module is going to make things way more confusing, IMO.

I'm fine with the new code that passes the light data struct. But you should not copy the value of do_jittering here.

So, as I previously suggested, keep the bl_light->shadow_softness_factor, bl_light->shadow_jitter_overblur and bl_light->radius inside ShadowPunctual and do the clamping compute_projection_boundaries. Currently you only moved the problem down a function.

And yes, I would use radius_jitter and radius_raytrace for clarity. I suggest to have them as getters (i.e: shadow_punctual.radius_jitter_get(bool do_jitter)).

I'm fine with the new code that passes the light data struct. But you should not copy the value of `do_jittering` here. So, as I previously suggested, keep the `bl_light->shadow_softness_factor`, `bl_light->shadow_jitter_overblur` and `bl_light->radius` inside `ShadowPunctual` and do the clamping `compute_projection_boundaries`. Currently you only moved the problem down a function. And yes, I would use `radius_jitter` and `radius_raytrace` for clarity. I suggest to have them as getters (i.e: `shadow_punctual.radius_jitter_get(bool do_jitter)`).
pragma37 marked this conversation as resolved
@ -344,1 +349,3 @@
if (handle.recalc != 0 || !light.initialized) {
::Light *bl_light = static_cast<::Light *>(ob->data);
light.do_jittering = inst_.shadows.do_jittering() && bl_light->mode & LA_SHADOW_JITTER;

Set do_jittering in light.sync. inst_.shadows.do_jittering is checked in sync_jitter() and early out anyway.

Set `do_jittering` in `light.sync`. `inst_.shadows.do_jittering` is checked in `sync_jitter()` and early out anyway.
Author
Member

I've added this back for the disablement on playback/navigation.
It's redundant for sure, but the code becomes a bit messy otherwise.

I've added this back for the disablement on playback/navigation. It's redundant for sure, but the code becomes a bit messy otherwise.
@ -349,4 +386,3 @@
float4x4 obmat_tmp = light.object_mat;
/* Clear embedded custom data. */
obmat_tmp[0][3] = obmat_tmp[1][3] = obmat_tmp[2][3] = 0.0f;

This can be removed. And you can use light.object_mat directly as it is junk free now!.

This can be removed. And you can use `light.object_mat` directly as it is junk free now!.
pragma37 marked this conversation as resolved
@ -687,0 +723,4 @@
float3 jitter = object_mat_.x_axis() * r_disk.x + object_mat_.y_axis() * r_disk.y;
object_mat_[2] += float4(jitter, 0.0f);
object_mat_ = math::orthogonalize(object_mat_, math::Axis::Z);
light.object_mat = object_mat_;

I am confused, you are jittering the light matrix that is use for shading? This is wrong as it changes the shading and doesn't even change the shadow projection.

Although the shadowing looks correct. So you should be able to generate the tilemaps with this matrix and only use this matrix for shadowing (have to find trick to pack it inside the LightData, might be able to pack a quaternion in there).

I am confused, you are jittering the light matrix that is use for shading? This is wrong as it changes the shading and doesn't even change the shadow projection. Although the shadowing looks correct. So you should be able to generate the tilemaps with this matrix and only use this matrix for shadowing (have to find trick to pack it inside the LightData, might be able to pack a quaternion in there).
Author
Member

Yes, I mentioned it at the last week meeting, and you seemed to agree (?).
Maybe I didn't explain it properly.

I don't understand what you mean by not changing the shadow projection.

The shading is jittered as well, which is not ideal, but I think the final image should converge to the same result anyway.

Yes, I mentioned it at the last week meeting, and you seemed to agree (?). Maybe I didn't explain it properly. I don't understand what you mean by not changing the shadow projection. The shading is jittered as well, which is not ideal, but I think the final image should converge to the same result anyway.

Yes, I mentioned it at the last week meeting, and you seemed to agree (?).

Rotating the matrix to render and sample the shadow map is the right approach, but using it for shading is not. I thought you mentioned this because you looked up what legacy EEVEE was doing (which is effectively what I just mentioned).

I don't understand what you mean by not changing the shadow projection.

I mean the tilemap.viewmat is not changed, which is the viewmat the shadow is rendered with. So you effectively only jitter the tracing / projection direction (I think, not sure) but the shadow maps are still render from the initial position of the light.

The shading is jittered as well, which is not ideal, but I think the final image should converge to the same result anyway.

No, it doesn't converge to the same result. Make a sunlight with 20° angle and look up the difference on a smooth reflective surface (i.e: metal).

> Yes, I mentioned it at the last week meeting, and you seemed to agree (?). Rotating the matrix to render and sample the shadow map is the right approach, but using it for shading is not. I thought you mentioned this because you looked up what legacy EEVEE was doing (which is effectively what I just mentioned). > I don't understand what you mean by not changing the shadow projection. I mean the `tilemap.viewmat` is not changed, which is the `viewmat` the shadow is rendered with. So you effectively only jitter the tracing / projection direction (I think, not sure) but the shadow maps are still render from the initial position of the light. > The shading is jittered as well, which is not ideal, but I think the final image should converge to the same result anyway. No, it doesn't converge to the same result. Make a sunlight with 20° angle and look up the difference on a smooth reflective surface (i.e: metal).
Author
Member

I mean the tilemap.viewmat is not changed, which is the viewmat the shadow is rendered with. So you effectively only jitter the tracing / projection direction (I think, not sure) but the shadow maps are still render from the initial position of the light.

It's changed. This is changing object_mat_ just before calling the tilemaps_distribution, which is what those functions pass to tilemap->sync_orthographic.

I don't understand why the position of a directional light would need to be changed. They're not supposed to have a position at all?

> I mean the `tilemap.viewmat` is not changed, which is the `viewmat` the shadow is rendered with. So you effectively only jitter the tracing / projection direction (I think, not sure) but the shadow maps are still render from the initial position of the light. It's changed. This is changing `object_mat_` just before calling the `tilemaps_distribution`, which is what those functions pass to `tilemap->sync_orthographic`. I don't understand why the position of a directional light would need to be changed. They're not supposed to have a position at all?

It's changed. This is changing object_mat_ just before calling the tilemaps_distribution, which is what those functions pass to tilemap->sync_orthographic.

Ok, my bad on this.

I don't understand why the position of a directional light would need to be changed. They're not supposed to have a position at all?

Sorry, I mean the initial light direction. But whatever, I was wrong.

> It's changed. This is changing object_mat_ just before calling the tilemaps_distribution, which is what those functions pass to tilemap->sync_orthographic. Ok, my bad on this. > I don't understand why the position of a directional light would need to be changed. They're not supposed to have a position at all? Sorry, I mean the initial light direction. But whatever, I was wrong.
Author
Member

I've made the split of jitter rotation into its own quaternion, but it's still WIP.

I've made the split of jitter rotation into its own quaternion, but it's still WIP.
pragma37 marked this conversation as resolved
@ -90,6 +90,8 @@ void shadow_tag_usage_tilemap_punctual(
}
vec3 lP = light_world_to_local(light, P - light._position);
lP -= light_local_data_get(light).shadow_projection_shift;

This is invalid to shift here. The visibility computation for early out should use the non-jittered position.

This is invalid to shift here. The visibility computation for early out should use the non-jittered position.
pragma37 marked this conversation as resolved
@ -35,2 +33,2 @@
light.do_not_access_directly._pad0_reserved = intBitsToFloat(clipmap_base_offset.x);
light.do_not_access_directly._pad1_reserved = intBitsToFloat(clipmap_base_offset.y);
// light.do_not_access_directly.shadow_scale = intBitsToFloat(0);
// light.do_not_access_directly.shadow_projection_shift = intBitsToFloat(0);

This needs to be fixed.

This needs to be fixed.
Author
Member

I've updated the code, but I'm not sure if it's right.
The test is disabled and fails even in main if I enable it back.

I've updated the code, but I'm not sure if it's right. The test is disabled and fails even in main if I enable it back.
@ -562,0 +560,4 @@
}
else {
lP = light_world_to_local(light, P - light._position) -
light_local_data_get(light).shadow_projection_shift;

This is very shady, I'm pretty sure it make things explode to jitter the shading point position (it can move it behind the light). And if doesn't, it is still super confusing. What is the reason to move this here?

Also if you change it here why not in shadow_sample()?

This is very shady, I'm pretty sure it make things explode to jitter the shading point position (it can move it behind the light). And if doesn't, it is still super confusing. What is the reason to move this here? Also if you change it here why not in `shadow_sample()`?
Author
Member

How is this shady? This is just computing the shadow map local space.
I moved it here because I found way more confusing the way it was.
Also, doesn't that mess some computations in shadow_ray_generate_punctual, like the direction?

Also if you change it here why not in shadow_sample()?

I forgot, but that's needed regardless, isn't it?

How is this shady? This is just computing the shadow map local space. I moved it here because I found way more confusing the way it was. Also, doesn't that mess some computations in `shadow_ray_generate_punctual`, like the `direction`? > Also if you change it here why not in shadow_sample()? I forgot, but that's needed regardless, isn't it?

I forgot, but that's needed regardless, isn't it?

Yes. Both need to be changed.

How is this shady?

Because lP refers to object local position which in this case, is the light.

Also, doesn't that mess some computations in shadow_ray_generate_punctual, like the direction?

Yes you are right. The existing code is also quite confusing if not wrong. That said, it isn't a reason to move this out of the existing branch.

> I forgot, but that's needed regardless, isn't it? Yes. Both need to be changed. > How is this shady? Because `lP` refers to object local position which in this case, is the light. > Also, doesn't that mess some computations in shadow_ray_generate_punctual, like the direction? Yes you are right. The existing code is also quite confusing if not wrong. That said, it isn't a reason to move this out of the existing branch.

The existing code is also quite confusing if not wrong

The existing code made sense because it was only used for area light shadow shift.
projection_origin is the point in light object space where the shadow is recorded at. So after the shadow scale mix, you need to move the point to shadow space for local_ray_start/end. Here the local_ denotes the shadow space. But I agree it could be better named.

> The existing code is also quite confusing if not wrong The existing code made sense because it was only used for area light shadow shift. `projection_origin` is the point in light object space where the shadow is recorded at. So after the shadow scale mix, you need to move the point to shadow space for `local_ray_start/end`. Here the `local_` denotes the shadow space. But I agree it could be better named.
@ -81,3 +81,2 @@
float shadow_resolution_scale;
float _pad0;
float shadow_jitter_overblur;

We should give is a small default. 0.1 looks fine.

We should give is a small default. `0.1` looks fine.
pragma37 marked this conversation as resolved
Miguel Pozo added 7 commits 2024-04-15 19:26:27 +02:00
Miguel Pozo added 1 commit 2024-04-15 19:33:20 +02:00
Miguel Pozo added 1 commit 2024-04-15 20:22:49 +02:00
Miguel Pozo added 3 commits 2024-04-17 16:41:09 +02:00
Miguel Pozo added 2 commits 2024-04-17 21:32:03 +02:00
Miguel Pozo changed title from EEVEE-Next: Jittered Soft Shadows to WIP: EEVEE-Next: Jittered Soft Shadows 2024-04-17 21:33:02 +02:00

Sphere Lights

This is how it looks like when a sphere light (no soft falloff) crosses the light plane.
Capture d’écran 2024-04-17 à 21.46.01.png

So, I agree that the ray end should be chosen around the shadow jittered position. However, the ray should be clipped to the light sphere to avoid stopping the ray outside the sphere.

Area Lights

Currently the ray direction is chosen on the light shape at shadow position. This shifts the shadow angle when the shading point is close to the light and not centered.
Capture d’écran 2024-04-17 à 22.47.02.png

To fix this, the shadow position needs to be closer to the light shape (and adjust the clipping plane). The right distance should be the diagonal of the area sampled for the raytracing rays' end.

Default adjustment

After more testing 0.2 seems to be better for the default viewport sample count (16).

Viewport Stability

Disable jittering during transform and viewport rotation (see volume code for example).

#### Sphere Lights This is how it looks like when a sphere light (no soft falloff) crosses the light plane. ![Capture d’écran 2024-04-17 à 21.46.01.png](/attachments/f651da4e-879d-43fc-bb3b-40de6e289f98) So, I agree that the ray end should be chosen around the shadow jittered position. However, the ray should be clipped to the light sphere to avoid stopping the ray outside the sphere. #### Area Lights Currently the ray direction is chosen on the light shape at shadow position. This shifts the shadow angle when the shading point is close to the light and not centered. ![Capture d’écran 2024-04-17 à 22.47.02.png](/attachments/018f239e-d036-46b8-885b-c507a619c51f) To fix this, the shadow position needs to be closer to the light shape (and adjust the clipping plane). The right distance should be the diagonal of the area sampled for the raytracing rays' end. #### Default adjustment After more testing 0.2 seems to be better for the default viewport sample count (16). #### Viewport Stability Disable jittering during transform and viewport rotation (see volume code for example).
Miguel Pozo force-pushed pull-eevee-jittered-shoft-shadows from 8b07f361e6 to 064b4c429f 2024-04-18 17:54:05 +02:00 Compare
Author
Member

This is how it looks like when a sphere light (no soft falloff) crosses the light plane.
Capture d’écran 2024-04-17 à 21.46.01.png

I think this has more to do with the backfacing shadow bias issue than with the jitter itself.
Still, it converges to the (I think) correct radius:

Jittered Bulb Size Overlay Tracing
imagen imagen imagen
> This is how it looks like when a sphere light (no soft falloff) crosses the light plane. > ![Capture d’écran 2024-04-17 à 21.46.01.png](/attachments/f651da4e-879d-43fc-bb3b-40de6e289f98) I think this has more to do with the backfacing shadow bias issue than with the jitter itself. Still, it converges to the (I think) correct radius: | Jittered | Bulb Size Overlay | Tracing | | --- | --- | --- | | ![imagen](/attachments/b03e2fa8-e809-4031-b273-3762f3185426) | ![imagen](/attachments/9d1db1ed-9a91-4e6c-8abd-20954c478ab0) | ![imagen](/attachments/6e39f3dd-1bcb-4f8e-9868-e8eb4d71b6dc) |
Miguel Pozo added 1 commit 2024-04-19 17:08:30 +02:00
Miguel Pozo added 1 commit 2024-04-19 17:34:18 +02:00
Miguel Pozo added 1 commit 2024-04-19 17:58:17 +02:00
Miguel Pozo added 2 commits 2024-04-19 19:48:14 +02:00
Miguel Pozo changed title from WIP: EEVEE-Next: Jittered Soft Shadows to EEVEE-Next: Jittered Soft Shadows 2024-04-19 20:44:35 +02:00
Clément Foucault requested changes 2024-04-19 23:37:28 +02:00
Clément Foucault left a comment
Member

When moving the view or an object the shadow is not centered at the origin of the light and not using the full spread. Which lead me to think that the random sampling is not fully disabled in these case.

When moving the view or an object the shadow is not centered at the origin of the light and not using the full spread. Which lead me to think that the random sampling is not fully disabled in these case.
@ -153,6 +153,10 @@ class DATA_PT_EEVEE_light_shadow(DataButtonsPanel, Panel):
layout.active = context.scene.eevee.use_shadows and light.use_shadow
col = layout.column()
col.prop(light, "use_shadow_jittering")

Use similar layout as new camera jitter UI:

        col = layout.column(align=False, heading="Jitter Camera")
        row = col.row(align=True)
        sub = row.row(align=True)
        sub.prop(props, "use_bokeh_jittered", text="")
        sub = sub.row(align=True)
        sub.active = props.use_bokeh_jittered
        sub.prop(props, "bokeh_overblur")
Use similar layout as new camera jitter UI: ``` col = layout.column(align=False, heading="Jitter Camera") row = col.row(align=True) sub = row.row(align=True) sub.prop(props, "use_bokeh_jittered", text="") sub = sub.row(align=True) sub.active = props.use_bokeh_jittered sub.prop(props, "bokeh_overblur") ```
pragma37 marked this conversation as resolved
@ -780,2 +780,4 @@
col.prop(props, "shadow_normal_bias", text="Normal Bias")
col = layout.column()
col.prop(props, "use_shadow_jittered_viewport", text="Jittered Shadows (Viewport)")

This was moved to Sampling > Viewport

This was moved to `Sampling > Viewport`
pragma37 marked this conversation as resolved
@ -341,2 +328,4 @@
Light &light = light_map_.lookup_or_add_default(handle.object_key);
light.used = true;
::Light *bl_light = static_cast<::Light *>(ob->data);

unused var

unused var
Author
Member

It was at some point, but not since I added the toggle on viewport playback/navigation.

It was at some point, but not since I added the toggle on viewport playback/navigation.
pragma37 marked this conversation as resolved
@ -737,3 +787,2 @@
jittered_transparency_ = !inst_.is_viewport() ||
scene.eevee.flag & SCE_EEVEE_SHADOW_JITTERED_VIEWPORT;
do_jittering_ = !inst_.is_viewport() || (scene.eevee.flag & SCE_EEVEE_SHADOW_JITTERED_VIEWPORT &&

Style: Always add parenthesis between binary operators and logical operators (i.e: (a & b) && c. This avoids dumb reviewers to verify operator precedence and improves readability.

Style: Always add parenthesis between binary operators and logical operators (i.e: `(a & b) && c`. This avoids dumb reviewers to verify operator precedence and improves readability.
Author
Member

Done, but I personally tend to have more trouble parsing nested parenthesis than operator precedence.

Done, but I personally tend to have more trouble parsing nested parenthesis than operator precedence.
pragma37 marked this conversation as resolved
@ -261,3 +262,3 @@
shadow_P,
Ng,
lv.L,
L,

At this point, just pass stack.cl[0].shadow_offset to shadow_eval. It will be needed to fix the SSS shadow anyway.

Also prefer calling shadow_dir instead of L. L is reserved for direction towards the light (except in small function / sampling context where it can just refer to outgoing ray direction). But I don't think it is worth changing all existing usage of it right now.

At this point, just pass `stack.cl[0].shadow_offset` to `shadow_eval`. It will be needed to fix the SSS shadow anyway. Also prefer calling `shadow_dir` instead of `L`. `L` is reserved for direction towards the light (except in small function / sampling context where it can just refer to outgoing ray direction). But I don't think it is worth changing all existing usage of it right now.
pragma37 marked this conversation as resolved
@ -17,0 +28,4 @@
}
}
vec3 light_local_to_shadow_local(LightData light, vec3 lP, bool is_directional)

Use const bool is_directional for all three functions.

Use `const bool is_directional` for all three functions.
Author
Member

Done, but why?

Done, but why?
@ -53,2 +55,4 @@
float local_min = FLT_MAX;
float local_max = -FLT_MAX;
vec3 L = shadow_vector_get(light, true, vec3(0));

Negate and rename shadow_forward_z_axis.

Negate and rename `shadow_forward_z_axis`.
pragma37 marked this conversation as resolved
@ -55,6 +55,26 @@ Quaternion Quaternion_identity()
return Quaternion(1, 0, 0, 0);
}
Quaternion as_quaternion(vec4 quat)

Move this function next to as_vec4 as it is a conversion function.

Move this function next to `as_vec4` as it is a conversion function.
pragma37 marked this conversation as resolved
@ -58,0 +60,4 @@
return Quaternion(quat.x, quat.y, quat.z, quat.w);
}
Quaternion invert(Quaternion quat)

Use same names as in BLI_math_quaternion.hh. This is conjugate and not the inverse. Also move to the /** \name Rotation Functions block. The top of the file is for type definitions.

Use same names as in `BLI_math_quaternion.hh`. This is `conjugate` and not the inverse. Also move to the `/** \name Rotation Functions` block. The top of the file is for type definitions.
Author
Member

It's the inverse if we assume we only use unit quaternion.
The same happens with the rotate function, it assumes a unit quaternion.

I think it's a fair assumption (at least in the GPU code), but if it's not then it's a problem for both functions.

It's the inverse if we assume we only use unit quaternion. The same happens with the `rotate` function, it assumes a unit quaternion. I think it's a fair assumption (at least in the GPU code), but if it's not then it's a problem for both functions.
@ -58,0 +62,4 @@
Quaternion invert(Quaternion quat)
{
quat.x *= -1.0;

For code readability, do not use multiply operator to flip signs in general.
You can also use Quaternion(-quat.x, -quat.y, -quat.z, quat.w);.

For code readability, do not use multiply operator to flip signs in general. You can also use `Quaternion(-quat.x, -quat.y, -quat.z, quat.w);`.
pragma37 marked this conversation as resolved
@ -58,0 +68,4 @@
return quat;
}
vec3 rotate(Quaternion quat, vec3 vec)

This should be transform_point. Also move to the /** \name Rotation Functions block. The top of the file is for type definitions.

Note that since a quaternion is only a rotation from the origin, there is no transform_direction as it does exactly the same thing.

A reference to the source of this algorithm should be added for future user of the code to verify exactness. I found this one https://fgiesen.wordpress.com/2019/02/09/rotating-a-single-vector-using-a-quaternion/

Add tests with same numbers as the CPU rotation API test for compatibility check.

This should be `transform_point`. Also move to the `/** \name Rotation Functions` block. The top of the file is for type definitions. Note that since a quaternion is only a rotation from the origin, there is no `transform_direction` as it does exactly the same thing. A reference to the source of this algorithm should be added for future user of the code to verify exactness. I found this one https://fgiesen.wordpress.com/2019/02/09/rotating-a-single-vector-using-a-quaternion/ Add tests with same numbers as the CPU rotation API test for compatibility check.
@ -325,0 +331,4 @@
RNA_def_property_override_flag(prop, PROPOVERRIDE_OVERRIDABLE_LIBRARY);
RNA_def_property_update(prop, 0, "rna_Light_update");
prop = RNA_def_property(srna, "shadow_jitter_overblur", PROP_FLOAT, PROP_FACTOR);

Should be PROP_PERCENTAGE.

Should be `PROP_PERCENTAGE`.
Author
Member

Done, but I can't say I like it.

Done, but I can't say I like it.
pragma37 marked this conversation as resolved
@ -325,0 +332,4 @@
RNA_def_property_update(prop, 0, "rna_Light_update");
prop = RNA_def_property(srna, "shadow_jitter_overblur", PROP_FLOAT, PROP_FACTOR);
RNA_def_property_range(prop, 0.0f, 1.0);

Nitpick: It might be copied from somewhere else, but 1.0 is missing a f.

Nitpick: It might be copied from somewhere else, but `1.0` is missing a `f`.
pragma37 marked this conversation as resolved
@ -325,0 +333,4 @@
prop = RNA_def_property(srna, "shadow_jitter_overblur", PROP_FLOAT, PROP_FACTOR);
RNA_def_property_range(prop, 0.0f, 1.0);
RNA_def_property_ui_range(prop, 0.0f, 1.0f, 0.1f, 2);

UI range (soft range) should be similar to the DOF overblur. 20% soft max sounds good.

UI range (soft range) should be similar to the DOF overblur. `20%` soft max sounds good.
pragma37 marked this conversation as resolved

I think this has more to do with the backfacing shadow bias issue than with the jitter itself.

I am not talking about the artifact themselves they will be present one way or the other when the light crosses the plane. I'm talking about the shadow cut at the center because we clip the ray at the light radius at the shadow position.

The result of this make the light bleed (no shadowing) to anything between a radius or its size and a radius of its double size. This is showing 2 samples but it averages out overtime.
Capture d’écran 2024-04-20 à 00.06.03.png

First issue is that the light_sphere_disk_radius is broken if the distance is less than radius.
return sphere_radius * inversesqrt(max(1e-8, 1.0 - square(sphere_radius / distance_to_sphere))); fixes it. Second problem now, is that the shadows are broken inside the sphere 😅. Might as well collapse to a ray at the shadow map origin in that case which will produce no shadowing inside the sphere. But doing so at the shadow jittered location will exhibit the inital problem I mentionned at the start of this comment. So we need to early out in this radius somehow.

Another issue now is that casters from inside the sphere can cast shadow outside the sphere. I don't have a good efficient solution for this. But if we resort to adding a discard in the shadow shader to solve #119334 we could discard when inside the sphere.
Capture d’écran 2024-04-20 à 00.52.08.png

> I think this has more to do with the backfacing shadow bias issue than with the jitter itself. I am not talking about the artifact themselves they will be present one way or the other when the light crosses the plane. I'm talking about the shadow cut at the center because we clip the ray at the light radius at the shadow position. The result of this make the light bleed (no shadowing) to anything between a radius or its size and a radius of its double size. This is showing 2 samples but it averages out overtime. ![Capture d’écran 2024-04-20 à 00.06.03.png](/attachments/0d69a191-bca8-4d38-881c-1825f5f3b224) First issue is that the `light_sphere_disk_radius` is broken if the distance is less than radius. `return sphere_radius * inversesqrt(max(1e-8, 1.0 - square(sphere_radius / distance_to_sphere)));` fixes it. Second problem now, is that the shadows are broken inside the sphere 😅. Might as well collapse to a ray at the shadow map origin in that case which will produce no shadowing inside the sphere. But doing so at the shadow jittered location will exhibit the inital problem I mentionned at the start of this comment. So we need to early out in this radius somehow. Another issue now is that casters from inside the sphere can cast shadow outside the sphere. I don't have a good efficient solution for this. But if we resort to adding a discard in the shadow shader to solve #119334 we could discard when inside the sphere. ![Capture d’écran 2024-04-20 à 00.52.08.png](/attachments/561ba33d-2126-45e4-a901-7433bbbcbd99)
Miguel Pozo added 6 commits 2024-04-22 15:49:07 +02:00
Author
Member

When moving the view or an object the shadow is not centered at the origin of the light and not using the full spread. Which lead me to think that the random sampling is not fully disabled in these case.

I can't repro this.

I've added your suggested fix for light_sphere_disk_radius and disabled shadows inside the light radius.
I've also inverted the bias for backfaces, which helps quite a bit with lights intersecting geometry.

It's still not perfect but it's already much better than traced shadows
There are also some light intersection issues that are not shadow-related.
I don't think fully solving light-geometry intersections should be within the scope of this PR.

> When moving the view or an object the shadow is not centered at the origin of the light and not using the full spread. Which lead me to think that the random sampling is not fully disabled in these case. I can't repro this. I've added your suggested fix for `light_sphere_disk_radius` and disabled shadows inside the light radius. I've also inverted the bias for backfaces, which helps quite a bit with lights intersecting geometry. It's still not perfect but it's already much better than traced shadows There are also some light intersection issues that are not shadow-related. I don't think fully solving light-geometry intersections should be within the scope of this PR.
Miguel Pozo added 3 commits 2024-04-23 17:32:41 +02:00
This pull request has changes conflicting with the target branch.
  • source/blender/blenkernel/BKE_blender_version.h
  • source/blender/draw/engines/eevee_next/eevee_light.cc
  • source/blender/draw/engines/eevee_next/eevee_shadow.cc
  • source/blender/draw/engines/eevee_next/shaders/eevee_shadow_lib.glsl
  • source/blender/draw/engines/eevee_next/shaders/eevee_shadow_tag_usage_lib.glsl
  • source/blender/draw/engines/eevee_next/shaders/eevee_shadow_tilemap_bounds_comp.glsl
  • source/blender/draw/engines/eevee_next/shaders/eevee_shadow_tracing_lib.glsl
  • source/blender/draw/tests/eevee_test.cc

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u pull-eevee-jittered-shoft-shadows:pragma37-pull-eevee-jittered-shoft-shadows
git checkout pragma37-pull-eevee-jittered-shoft-shadows
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 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.

Dependencies

No dependencies set.

Reference: blender/blender#119753
No description provided.