[3.x] Fix low directional shadow quality on ultrawide aspect ratios

[Calinou]

This applies to both GLES3 and GLES2.

This closes #23603.

Testing project (3.x): test_light_atlas_bias.zip

Preview

7680×1440 (3-monitor setup with a 48:9 aspect ratio)

Before

After

In master (for reference)

[reduz]

Pancaking should be portable, but it will break compatibility, yet you need it to achieve full shadow stability.

[akien-mga]

servers/visual/visual_server_scene.cpp:1451:12: error: variable 'bias_scale' set but not used [-Werror=unused-but-set-variable]
 1451 |     real_t bias_scale = 1.0;
      |            ^~~~~~~~~~
servers/visual/visual_server_scene.cpp:1452:12: error: variable 'aspect_bias_scale' set but not used [-Werror=unused-but-set-variable]
 1452 |     real_t aspect_bias_scale = 1.0;
      |            ^~~~~~~~~~~~~~~~~

[akien-mga]

More errors in CI.

[Calinou]

Rebased and tested again, it works successfully.

Note that this PR improves shadow appearance even on a 16:9 aspect ratio:

Before

After

Peter-panning below the cube is lessened and the shadow is slightly sharper. Some more acne is visible in the distance, but this is likely fixable by increasing Bias Split Scale in the DirectionalLight properties.

[Calinou]

Rebased and tested again, it works successfully.

@akien-mga I wonder if we should merge this pull request for 3.4beta, and revert it if anything goes wrong during testing. The number of ultrawide monitor users is on the rise overall, so it would be good to have this fixed in 3.x eventually.

Performance seems to be roughly identical from my testing, with this PR being a tad slower (0.015 milliseconds per frame difference in the MRP, so not really anything meaningful).

[lawnjelly]

Does this PR change the dimensions of the shadow map? Or just the matrix used to render the shadows (sorry haven't stepped through in detail). If the latter it shouldn't worse case be any worse than a square. Maybe it is to deal with the aspect ratio on the x and y coordinate to choose the best orientation for the shadow matrix to make the best out of the available space.

It reminds me I mean to resurrect this PR #33340 at some point. It's really quite elegant and culls out a bunch more objects with directional lights. I only closed it because it was problematic with the shadow caching for omnis and spotlights, but I didn't realise at the time it was still very valid for directional lights which don't use caching.

[Calinou]

Does this PR change the dimensions of the shadow map? Or just the matrix used to render the shadows (sorry haven't stepped through in detail). If the latter it shouldn't worse case be any worse than a square.

As far as I know, it changes the matrix. The texture is still always a power-of-two square.

We could perhaps look into allowing non-power-of-two sizes to allow for more control over shadow quality, but I don't know if this is viable from a performance standpoint. I've seen many engines which can handle that without a performance penalty.

Edit: NPOT shadow sizes are being implemented in #54041 and #54042.

[Calinou]

I initially wanted to backport shadow normal offset bias/shadow pancaking to 3.x, but it's too difficult for me to handle this.
For reference, this is what reduces shadow acne/peter-panning immensely in master compared to 3.x (at little performance cost).

This pull request should be able to be merged as-is, as it still fixes a problem with the GLES3 and GLES2 renderers.

I'll leave some pointers here in case someone wants to do this work in a separate pull request:

• Shadow normal offset bias is relevant for all shadow types, while shadow pancaking is only used for DirectionalLight shadows.
• This can be implemented in both GLES3 and GLES2, but I recommend focusing on GLES3 first to make it less likely to run into limitations.
• 4ffc0d6 is the commit that originally implemented shadow normal offset bias and shadow pancaking. Only some of its changes are relevant for a 3.x backport, as that commit also includes unrelated changes.
• After picking changes from the above comment, I recommend manually picking changes from Fix directional shadow bias #51025 as well for DirectionalLight shadows, and then Improvements to SpotLight3D and OmniLight3D's shadows #51335 for OmniLight and SpotLights shadows.
• As pointed out by reduz above, this will break compatibility with existing 3D projects as shadow bias values will need to be adjusted. This change is surely worth the breakage though, given how much people run into shadow biasing issues in 3.x right now.
  • On the bright side, this will make 3D projects more ready for an upgrade to Godot 4.0 as well 🙂
  • It should be possible to write an editor script or plugin to automatically reset bias values on all lights within a project. The master branch's shadow algorithms make the default bias value viable in most cases, so you rarely need to tweak it.

[akien-mga]

As pointed out by reduz above, this will break compatibility with existing 3D projects as shadow bias values will need to be adjusted. This change is surely worth the breakage though, given how much people run into shadow biasing issues in 3.x right now.

Can this be made configurable with a project setting, so that we can offer the fix while preserving compatibility for existing projects? (We can have a project setting that's false by default but gets set to true in newly generated project.godot).

As a side note, if this were to be merged, maybe the s/float/real_t/ changes should be left out? AFAIK the 3.x branch doesn't properly support REAL_T_IS_DOUBLE so I don't think that changing this so late in the release cycle will have any benefit.

[Calinou]

Can this be made configurable with a project setting, so that we can offer the fix while preserving compatibility for existing projects? (We can have a project setting that's false by default but gets set to true in newly generated project.godot).

I was referring to shadow normal offset bias/shadow pancaking, which is not implemented in this PR. In its current state, this PR does not break compatibility with existing projects.

As a side note, if this were to be merged, maybe the s/float/real_t/ changes should be left out? AFAIK the 3.x branch doesn't properly support REAL_T_IS_DOUBLE so I don't think that changing this so late in the release cycle will have any benefit.

Sounds good, I'll look into that.

Edit: Done. I also rebased and made sure everything works as expected.

[parasyte]

FYI, I just tested this patch against the test project in #57549 (comment) and it unfortunately does not resolve that issue.

This PR does however notably improve shadow rendering quality. Here are some screenshots of the test project before and after applying this patch. Both screenshots show the "bad angle" in the test project (press space bar once after it loads).

Before:

After:

So yeah, the shadow edges are sharper (nice!) but the shadow still fades out completely toward the top of the screen.

---

Also, I had to rebase the commit on top of the latest 3.x branch, because this branch as-is does not load the FBX models in the test project correctly. It wasn't a difficult rebase, but there were a few conflicts to resolve.

[Calinou]

So yeah, the shadow edges are sharper (nice!) but the shadow still fades out completely toward the top of the screen.

This is expected if the DirectionalLight's Shadow Max Distance isn't high enough to cover the entire scene. 4.0 lets you tweak the fade transition distance – something I backported in #60246.

[parasyte]

That PR can be used to hide the fading, with some manually tuning. Another great improvement! What it does not address is the jarring transition between the shadow splits with just a 1-degree difference in camera rotation. Together, both patches do a decent job at masking the problem.

I don't know if there is a ticket specifically tracking the light volume inconsistencies with "optimized depth range" on directional lights (Maybe #57549 is now the canonical ticket for it?), but that is what the car test project is demonstrating.

[Calinou]

What it does not address is the jarring transition between the shadow splits with just a 1-degree difference in camera rotation.

Enabling Blend Splits in DirectionalLight helps hide those transitions, at the cost of performance and effective shadow resolution (as splits need to "cover" each other to perform blending). If you want a solution that has a lower performance cost and doesn't effective reduce shadow resolution, #60186 will be helpful here. It's already implemented in 4.0.beta.

godotengine/godot-proposals#4466 will also help improve general directional shadow quality, at a small cost to performance (as culling can't be as aggressive). That said, given the effective shadow resolution is increased a lot by this change, you could decrease shadow resolution and still have the same appearance as before (with likely the same performance, if not better). #54042 will allow for more granular adjustments to directional shadow resolution.

[parasyte]

This is not really the right place to take the discussion, but since you brought up all of these incremental changes, I should comment that I am happy to see any progress that improves shadow rendering quality in 3.x (performance is not a primary concern).

Directional shadows are not just bad in 3.x, they are unusable without all of these QoL improvements in large scenes. The existing tradeoff is made between having either a very low range distance with sharp shadows (i.e., high resolution), or extremely blurry shadows with longer range. There's no good middle ground with decent distance and decent sharpness.

For instance, "optimized" depth range is the best available option for maintaining sharp shadows in stable. But it also requires PSSM splits to get that sharpness at the cost of jarring transitions. Enabling blending on top of this creates a blurry mess, cancelling many of the reasons I would use PSSM in the first place.

And it's not that I actually want sharp edges on shadows, necessarily. In an ideal world I would have "soft shadows" available as shown in https://godotengine.org/article/vulkan-progress-report-7. But given the tools that are at my disposal, I prefer the shadows to make contact with the caster, at least reasonably convincingly (I have experimented with contact shadows ... uh, yeah, it's a thing that exists, I'll say that much).

I'll probably apply all three of these patches plus #54355 to 3.x and see how I get along with maintaining a branch. Ultimately, I would just update to 4.0 and call it a day. I'm at the mercy of the tech stack, though. My project is written in Rust, which has fairly good support on 3.x, but the GDExtension support for 4.0 is unstable and incomplete.

🤷

[Zireael07]

Hopefully this gets finally merged as I believe I saw another report of this today...

[Calinou]

Rebased and tested again, it works as expected.

[lawnjelly]

This entire block does nothing I think. radius_square is unused, instead radius is used later.

In fact maybe the entire block from if (aspect != 1.0) to radius_square *= ... has no side effects. 🤔

In 4.x, this equivalent code seems to write directly to radius, and doesn't seem to have a special case for aspect ratio outside 1.0.

[lawnjelly]

I've spent some time looking over this, I'm really not familiar with this part of the renderer but some observations:

• In 3.x especially, the function _light_instance_update_shadow() is huge, and hard to follow, and variable scope is unclear along with naming. This is evidenced by the difficulty reviewing the PR and also the radius_square bug above.
• I think it could benefit from being separated into 4 or more smaller functions (I've already done this in a test modification to try and work out what was going on), it is already much easier to read / debug. The functions in 4.x have already been made a lot smaller and more manageable.

[lawnjelly]

This doesn't seem anything like the bias scale from before (which was bias_scale = radius / first_radius). Maybe this is intentional, but wanted to check this was correct.

In master this value is shadow_texel_size, and bias_scale is z_max - z_min_cam.

[Calinou]

• I think it could benefit from being separated into 4 or more smaller functions (I've already done this in a test modification to try and work out what was going on)

Could you push your changes to a branch so I can take a look?