EEVEE-Next: Jittered Soft Shadows #119753
No reviewers
Labels
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#119753
Loading…
Reference in New Issue
No description provided.
Delete Branch "pragma37/blender:pull-eevee-jittered-shoft-shadows"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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 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.
a29db7a750
to6df6039725
@blender-bot build +gpu
WIP: EEVEE-Next: Jittered Soft Shadowsto EEVEE-Next: Jittered Soft ShadowsResults 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.@ -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.@ -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
.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
andbl_light->radius
insideShadowPunctual
and do the clampingcompute_projection_boundaries
. Currently you only moved the problem down a function.And yes, I would use
radius_jitter
andradius_raytrace
for clarity. I suggest to have them as getters (i.e:shadow_punctual.radius_jitter_get(bool do_jitter)
).@ -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
inlight.sync
.inst_.shadows.do_jittering
is checked insync_jitter()
and early out anyway.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!.@ -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).
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.
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 mean the
tilemap.viewmat
is not changed, which is theviewmat
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.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).
It's changed. This is changing
object_mat_
just before calling thetilemaps_distribution
, which is what those functions pass totilemap->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?
Ok, my bad on this.
Sorry, I mean the initial light direction. But whatever, I was wrong.
I've made the split of jitter rotation into its own quaternion, but it's still WIP.
@ -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.
@ -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.
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()
?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 thedirection
?I forgot, but that's needed regardless, isn't it?
Yes. Both need to be changed.
Because
lP
refers to object local position which in this case, is the light.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 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 forlocal_ray_start/end
. Here thelocal_
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.EEVEE-Next: Jittered Soft Shadowsto WIP: EEVEE-Next: Jittered Soft ShadowsSphere Lights
This is how it looks like when a sphere light (no soft falloff) crosses the light plane.
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.
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).
8b07f361e6
to064b4c429f
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:
WIP: EEVEE-Next: Jittered Soft Shadowsto EEVEE-Next: Jittered Soft ShadowsWhen 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:
@ -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
@ -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
It was at some point, but not since I added the toggle on viewport playback/navigation.
@ -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.Done, but I personally tend to have more trouble parsing nested parenthesis than operator precedence.
@ -261,3 +262,3 @@
shadow_P,
Ng,
lv.L,
L,
At this point, just pass
stack.cl[0].shadow_offset
toshadow_eval
. It will be needed to fix the SSS shadow anyway.Also prefer calling
shadow_dir
instead ofL
.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.@ -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.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
.@ -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.@ -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 isconjugate
and not the inverse. Also move to the/** \name Rotation Functions
block. The top of the file is for type definitions.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);
.@ -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.
@ -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
.Done, but I can't say I like it.
@ -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 af
.@ -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.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.
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.
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.
Checkout
From your project repository, check out a new branch and test the changes.