Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stepped update of directional shadow maps #76291

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BastiaanOlij
Copy link
Contributor

@BastiaanOlij BastiaanOlij commented Apr 20, 2023

This PR adds a setting to each viewport that allows us to update direction shadow maps over the course of either 2 or 4 frames if PSSM 4 is used.

We have the following two options.

  1. Two step updates in the following sequence:
  • 1 + 2 + 3
  • 1 + 2 + 4

The 1st and second cascades are always updated while the 3rd and 4th cascades alternate between frames.

  1. Four step updates in the following sequence:
  • 1 + 2
  • 1 + 3
  • 1 + 2
  • 1 + 4

The 1st cascade is thus always updated, the 2nd cascade is updated every other frame, and cascades 3 and 4 are only updated every 4 frames.

Note that if this feature is enabled, we create a shadow map for each viewport instead of reusing the save shadow map as we need to keep results of previous frames.
For this reason we're also keeping track of the shadow map cascade information for each directional light instance per viewport. We need to have access to data from the previous frame and can't reuse the same state.

Future improvement
This implementation currently has one known drawback. When the light is coming from behind the camera, objects moving away from the light will exhibit self shadowing in the frames where a cascade is used that hasn't been updated.
Our current plan to fix this is to only render shadows for static object for those cascades that are not rendered every frame. We're still debating if we'll do a separate dynamic render pass (likely) to remove any visual effects.

This however requires us to implement godotengine/godot-proposals#4635 first, so we will do this in a follow up PR.

For now it is up to the developer to use the cascade mode that works for their specific scene.


@jcostello
Copy link
Contributor

Awesome. When its finish do you mind to post some benchmarks?

@BastiaanOlij
Copy link
Contributor Author

Awesome. When its finish do you mind to post some benchmarks?

Defo need to do some benchmarks and see if this is worth it. Pretty sure once it works @Calinou has a few scenes we can throw against it.

@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented Apr 24, 2023

Ok, so the shadow map being cleared didn't come from the region logic not working, but seems to originate from this code in _pre_opaque_render:

RD::get_singleton()->draw_list_begin(light_storage->direction_shadow_get_fb(), RD::INITIAL_ACTION_DROP, RD::FINAL_ACTION_DISCARD, RD::INITIAL_ACTION_CLEAR, RD::FINAL_ACTION_CONTINUE);
RD::get_singleton()->draw_list_end();

This doesn't make a whole lot of sense as we create a draw list and actually draw our shadowmaps later on.
The only thing I can think of is that this is some round about way to make sure the map is cleared at least once. If that is the case there are better ways to do this and this should be moved into update_directional_shadow_atlas and only run once.

That we have a single directional shadow map that is (re)used for all viewports does break our approach, I need to add some extra code that if our step system is used, we do not create/use this shadow map but we create one for the viewport.

@Calinou
Copy link
Member

Calinou commented Apr 24, 2023

That we have a single directional shadow map that is (re)used for all viewports does break our approach, I need to add some extra code that if our step system is used, we do not create/use this shadow map but we create one for the viewport.

Won't this result in decreased performance as soon as you have two viewports? There's no way to disable directional shadows for a specific viewport right now. A lot of people rely on a second viewport to render a first-person viewmodel (for better or worse), so this path needs to be made as fast as possible until we have a proper render layer system (so that you wouldn't need a second viewport anymore).

I'm a bit surprised we can't time-slice the directional shadow map that is rendered across all viewports.

@clayjohn
Copy link
Member

That we have a single directional shadow map that is (re)used for all viewports does break our approach, I need to add some extra code that if our step system is used, we do not create/use this shadow map but we create one for the viewport.

Won't this result in decreased performance as soon as you have two viewports? There's no way to disable directional shadows for a specific viewport right now. A lot of people rely on a second viewport to render a first-person viewmodel (for better or worse), so this path needs to be made as fast as possible until we have a proper render layer system (so that you wouldn't need a second viewport anymore).

I'm a bit surprised we can't time-slice the directional shadow map that is rendered across all viewports.

Right now the shadow maps aren't shared, the texture is. So the process we follow is:
Viewport 1:
-- Clear entire shadow atlas
-- Render all 4 splits
-- Render scene
Viewport 2:
-- Clear entire shadow atlas
-- Render all 4 splits
-- Render scene

So even if you turn time slicing on in Viewport 1, the entire thing gets cleared when Viewport 2 is rendered and the shadow split information is lost by the time the next frame comes around

@BastiaanOlij
Copy link
Contributor Author

@clayjohn Actually it's worse, if you turn on time slicing viewport 2 will overwrite the results of viewport 1.

So say Viewport 1 renders cascades 1 and 3, your shadow map now contains:

  • viewport 1s cascade 1
  • viewport 2s cascade 2
  • viewport 1s cascade 3
  • viewport 2s cascade 4

That'll give some funky results :)

If we do splits, we have to have a different system where multiple textures are possible.

@Calinou right now that doesn't even work. If you have 5 viewports, we render the directional shadows 5x (assuming all viewports are 3D and have cameras), each time updating it for the camera for that viewport. We just happen to reuse the same texture to save on memory.

@BastiaanOlij BastiaanOlij force-pushed the step_cascades branch 2 times, most recently from 870c902 to e46f3bc Compare April 25, 2023 09:16
@mrjustaguy
Copy link
Contributor

mrjustaguy commented Apr 25, 2023

Regarding some Documentation, would be great to note that rotating around really fast could result in momentary lack of directional shadows in the distant splits, especially at lower framerates, as some people might not imply that to be the case.

The Current Broken version (not the latest, still waiting on it to compile) though is hard to verify that it's reasonable to expect that issue appearing in real world scenarios though as the 3rd and 4th split are blinking a lot so hard to test, while 2nd split is too close, but looks just like opacity is cut in half, due to it rendering at a little above 60hz..
Kinda shows how running High Frame Rates are a pointless waste of energy as no shipped game I've ever seen have any serious issues visible, that would require multiple frames for every actually perceived frame, even at 30fps

Edit: just finished, and while no more blinking, the reprojection is broken (heavy tearing and Shadow Acne as camera moves) still preventing a proper test of the scenario.. also 8k and 16k shadow maps break

@BastiaanOlij
Copy link
Contributor Author

Thanks @mrjustaguy

I had a feeling we may have an issue with reprojection, I suspect we're already writing the new matrix for all cascades instead of skipping the ones we're not rendering. It's even possible that in a multi viewport scenario we're now getting matrices for the other viewport...

This part of Godot relies on WAY too much global state. What should be a simple change is turning into a monster of trying to figure out whats going on :) I think right now 80% of my time has been spend testing and 20% actually writing code :)

@BastiaanOlij
Copy link
Contributor Author

Btw, I'm less worries about heavy rotating, this type of movement will often hide imperfections especially since they are more distance.

Low framerate is a worry and in that scenario this system should be turned off.

@BastiaanOlij
Copy link
Contributor Author

Ok, I've moved some of the code earlier in the process so we're not updating the meta data around the shadows and using the wrong matrices. This actually cleaned up some of the code BUT in true Godot fashion this data is stored globally so if you have more then 1 viewport, it starts screwing up.

I'll have to rewrite that part of the system so the data is stored on viewport/light level, not just on light data. sigh

@BastiaanOlij
Copy link
Contributor Author

I also believe that the way we determine the frustum for the directional lights needs to improve, under certain light angles we're not using large parts of the shadow map, and the shadow maps are directionally locked which means that under many camera angles we get really ugly edges.

@BastiaanOlij
Copy link
Contributor Author

Implementation is now also working on mobile.

Will start looking at changing how we're storing the cascade info so viewports don't overwrite each others results.
Nearly done I hope...

@mrjustaguy
Copy link
Contributor

mrjustaguy commented Apr 27, 2023

8k and 16k still don't work, but 4k is now reprojecting fine, and you were right, it seems heavy rotating isn't producing any issues, from my testing that is..

edit: 8k and 16k are working, however had to resize the viewport for them to start rendering..

On the other hand however, objects in motion render it totally useless without blending splits due to heavy tearing (at 60 FPS).

On a side note, would it be possible to have object specific biasing? so that objects that move in distant cascades have a higher bias until the cascades update, to avoid self shadowing artifacts in motion..
Another option would be to try to render the shadow cascades of the objects in motion in distant cascades projected to them as if they didn't move, though unsure how much work that'd be

@BastiaanOlij
Copy link
Contributor Author

I wonder why you needed to resize, that shouldn't have an impact on the shadowmaps... weird..

I haven't tested yet with moving objects...

I did just solve the multi viewport issue, needs a cleanup step but will add that later. First need to get some lunch.

@BastiaanOlij
Copy link
Contributor Author

Hmmm, so its having a self shadow issue when moving away from the lightsource. Thats going to be tricky.

That said, I don't get why its using one of the later cascades, in this scenario the first cascade should cover a large part of the view frustum...

@mrjustaguy
Copy link
Contributor

mrjustaguy commented Apr 28, 2023

actually, this only happens for splits 2 3 and 4, with the 2nd one having the least visible issue, it's just that split 1 is fairly close in reality, compared to the movement speed.

as far as dealing with this well.. unless you can have shadows render on objects using their last valid transforms per given cascade update.. none really, I mean aside from just not rendering dynamic objects in the shadow map, or doing them separately (in which case probably all 4 splits can be rendered one after another, with a separate dynamic object pass that does all every frame and merging the two, like godotengine/godot-proposals#4635)

However, if splitting static and dynamic objects is the way to do this, it'd be best to simply totally separate dynamic and static directional shadows, allowing them to run at different resolutions and different cascade distances, like godotengine/godot-proposals#5841 essentially, just instead of per object, being for all dynamic objects vs all static objects.

@jcostello
Copy link
Contributor

@BastiaanOlij baking lights in this PR cause an error

handle_crash: Program crashed with signal 11
Engine version: Godot Engine v4.1.dev.custom_build (fe974f189ea2e62dd3122389f6241852c52d399d)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] /lib/x86_64-linux-gnu/libc.so.6(+0x42520) [0x7f7e50e42520] (??:0)
[2] /lib/x86_64-linux-gnu/libc.so.6(+0x1a0c7e) [0x7f7e50fa0c7e] (??:0)
[3] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0x1b8a176) [0x55b5e1b8a176] (??:0)
[4] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0x43698b5) [0x55b5e43698b5] (??:0)
[5] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0x4949caa) [0x55b5e4949caa] (??:0)
[6] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0x42be616) [0x55b5e42be616] (??:0)
[7] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0x342b5c1) [0x55b5e342b5c1] (??:0)
[8] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0x24a2ae7) [0x55b5e24a2ae7] (??:0)
[9] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0x24a365b) [0x55b5e24a365b] (??:0)
[10] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0xca740e) [0x55b5e0ca740e] (??:0)
[11] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0x507ec60) [0x55b5e507ec60] (??:0)
[12] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0x4d7e8e6) [0x55b5e4d7e8e6] (??:0)
[13] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0x508905c) [0x55b5e508905c] (??:0)
[14] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0x2fcb4f5) [0x55b5e2fcb4f5] (??:0)
[15] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0x2fcc5b1) [0x55b5e2fcc5b1] (??:0)
[16] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0x2fcc8d6) [0x55b5e2fcc8d6] (??:0)
[17] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0x2f5bb6a) [0x55b5e2f5bb6a] (??:0)
[18] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0x2f5ccf1) [0x55b5e2f5ccf1] (??:0)
[19] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0x2f75af6) [0x55b5e2f75af6] (??:0)
[20] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0x2f9af9d) [0x55b5e2f9af9d] (??:0)
[21] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0x2fb4cb6) [0x55b5e2fb4cb6] (??:0)
[22] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0xb269d4) [0x55b5e0b269d4] (??:0)
[23] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0x4d08da7) [0x55b5e4d08da7] (??:0)
[24] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0x4d0b792) [0x55b5e4d0b792] (??:0)
[25] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0xb27d4c) [0x55b5e0b27d4c] (??:0)
[26] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0xb01e30) [0x55b5e0b01e30] (??:0)
[27] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0xaf36ce) [0x55b5e0af36ce] (??:0)
[28] /lib/x86_64-linux-gnu/libc.so.6(+0x29d90) [0x7f7e50e29d90] (??:0)
[29] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80) [0x7f7e50e29e40] (??:0)
[30] /home/juan/dev/godot/bin/godot.linuxbsd.editor.x86_64(+0xaffd65) [0x55b5e0affd65] (??:0)
-- END OF BACKTRACE --
===========================

@BastiaanOlij
Copy link
Contributor Author

@mrjustaguy Clay looked into this a little further and indeed, it's very noticeable with the sun coming from behind the camera. When the sun points towards the camera the issue isn't noticeable.

Looking at other engines they seem to "solve" this by only stepping cascades 3+4 and only rendering shadows for static objects.

It made me wonder if we should keep this in the artists control and move our setting to the viewport instead of being a global setting (with the project settings being used as the default for the main viewport) and change the option to switch between a 2 step (1 + 2 + 3, 1 + 2 + 4) and 4 step (1 + 2, 1 + 3, 1 + 2, 1 +4) mode.

We can then make only rendering static objects an option as well. This will have to wait until we implement the static/dynamic split update for positional lights.

@BastiaanOlij
Copy link
Contributor Author

@jcostello any chance you can reproduce this with a debug version with proper debug symbols? That crash log sadly doesn't contain any usable information.

@Calinou
Copy link
Member

Calinou commented Apr 30, 2023

Looking at other engines they seem to "solve" this by only stepping cascades 3+4 and only rendering shadows for static objects.

Is there a point in stepping cascades if they're only rendering static objects in the first place? As far as I know, no updates should ever occur in those cascades (so the engine shouldn't even bother checking).

@jcostello
Copy link
Contributor

@jcostello any chance you can reproduce this with a debug version with proper debug symbols? That crash log sadly doesn't contain any usable information.

@BastiaanOlij this?

ERROR: Too many pixels for Image. Maximum is 16777216x16777216 = 268435456pixels .
at: initialize_data (core/io/image.cpp:2186)
ERROR: Too many pixels for Image. Maximum is 16777216x16777216 = 268435456pixels .
at: initialize_data (core/io/image.cpp:2186)
ERROR: Too many pixels for Image. Maximum is 16777216x16777216 = 268435456pixels .
at: initialize_data (core/io/image.cpp:2186)
ERROR: Condition "p_size < 0" is true. Returning: ERR_INVALID_PARAMETER
at: resize (./core/templates/cowdata.h:262)

Before on master was working fine for the scene

@BastiaanOlij
Copy link
Contributor Author

Is there a point in stepping cascades if they're only rendering static objects in the first place? As far as I know, no updates should ever occur in those cascades (so the engine shouldn't even bother checking).

All 4 cascades are rendered based on the view frustum of the camera, so if the camera moves the cascades need to be updated. Especially when the camera turns the center point of the later cascades can move by a bit (though for some reason I'm not sure if that's happening in our current implementation.

Right now Godot updates all 4 cascades every frame even regardless of whether the camera moves, we only have a system for our omni/spot lights where we only update the shadowmap if the light is moved.

@BastiaanOlij
Copy link
Contributor Author

@BastiaanOlij this?

ERROR: Too many pixels for Image. Maximum is 16777216x16777216 = 268435456pixels . at: initialize_data (core/io/image.cpp:2186) ERROR: Too many pixels for Image. Maximum is 16777216x16777216 = 268435456pixels . at: initialize_data (core/io/image.cpp:2186) ERROR: Too many pixels for Image. Maximum is 16777216x16777216 = 268435456pixels . at: initialize_data (core/io/image.cpp:2186) ERROR: Condition "p_size < 0" is true. Returning: ERR_INVALID_PARAMETER at: resize (./core/templates/cowdata.h:262)

Before on master was working fine for the scene

Owh wow, that looks like it's trying to allocate our shadow maps at some insane size... I'll have a look if I can find a cause, stepped cascades shouldn't be applied for lightmapping anyway.

@mrjustaguy
Copy link
Contributor

I guess the priority then would be to implement static & dynamic shadow maps (for both spot&omni and directional lights), and totally rethink this.

After that, probably makes sense for static directional lights to have the following options:

  1. Render all cascades every frame (highest stability of fps, most demanding)
  2. Render half the cascades every frame
  3. Render one cascade every frame (lowest stability of fps, least demanding)
    With a bias setting to determine how much a camera should change it's transform for re-rendering cascades, with 0 being always render even if the slightest change happens

Dynamic lights however, should render all cascades every frame, but be totally decoupled from static light settings, so can use different resolution of shadow maps, different cascade values, blur values etc. with an option to automatically match to static light settings that is on by default

Given that most of the geometry in a game is static, this would yield in significant benefits to shadow rendering, even more so vs the original idea of 2 cascades per frame, as you are able to only do 1 cascade per frame of static objects and all the cascades of dynamic objects (from 1 to 4 depending on settings) which will even with all 4 cascades typically be significantly cheaper compared to even one cascade of static objects.

Ofc, Omni and Spot Lights would greatly benefit from Static Shadow Mapping too in plenty of scenes, however a method should exist for force updating static shadows, in case the user wants to for some reason change objects from static to dynamic and vice versa.

@BastiaanOlij
Copy link
Contributor Author

@mrjustaguy I think splitting static/dynamic rendering of shadow maps for omni and spot lights will have the biggest impact for most games. The big challenge will be how we end up marking dynamic objects as dynamic..

@mrjustaguy
Copy link
Contributor

mrjustaguy commented May 1, 2023

there are several possible paths to go regarding that. The Two that are the most logical are:

  1. Manual per mesh instance (defaults at auto, but can be forced to static or dynamic)
  2. Automatic Based on how long passed since mesh last updated (example if hasn't moved in the last minute or some other user defined time to mark as static, and if it updates, mark as dynamic)

Both could likely coexist

@Calinou
Copy link
Member

Calinou commented May 1, 2023

Regarding splitting static and dynamic shadows, see this proposal: godotengine/godot-proposals#4635

@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented Jun 28, 2023

Some feedback from @reduz after talking with him yesterday. We discussed adding 3 extra options here to deal with the self shadow issue if stepped cascades is enabled:

  1. Only render static objects to the cascades, ignore dynamic objects
  2. Render static and dynamic objects to the first cascade, only render static objects to cascades 2, 3 and 4.
  3. Render static and dynamic objects to the first cascade, use split static/dynamic approach for cascades 2, 3 and 4

This does require finishing #77683 first.

@Calinou
Copy link
Member

Calinou commented Jun 28, 2023

  1. Only render static objects to the cascades, ignore dynamic objects
  2. Render static and dynamic objects to the first cascade, only render static objects to cascades 2, 3 and 4.

I'd consolidate those options under a single project setting that governs the maximum cascade dynamic objects can be rendered on:

  • None (No Shadows, Fastest)
  • 1st Cascade (Close Shadows, Faster)
  • 2nd Cascade (Medium Shadows, Fast)
  • 3rd Cascade (Far Shadows, Average)
  • 4th Cascade (Farthest Shadows, Slow)

This makes it easy for graphics settings menus to adjust those options behind the scenes, without requiring too much additional code.

The default would probably be 3rd Cascade (likely the sweet spot in terms of performance/visuals) or 4th Cascade (if we want to keep existing behavior).

@mrjustaguy
Copy link
Contributor

2nd split or 4th split for dynamic by default, as 4th cascade itself has very little geometry thanks to mesh LOD for dynamic objects as they're typically fairly small, so no point in cutting just 4th cascade, cutting 3rd and 4th however would make more of an impact, while still keeping dynamic shadows active for a usable distance.

Question is how will dynamic's fade out when they stop rendering at earlier cascades?

@Calinou
Copy link
Member

Calinou commented Jun 28, 2023

as 4th cascade itself has very little geometry thanks to mesh LOD for dynamic objects as they're typically fairly small

Godot currently doesn't cull objects from shadow maps entirely if they take a very small space on the shadow map (say, ≤ 2 pixels). This could be done in a separate PR based on the mesh's AABB size, with the size threshold being adjustable in the project settings.

Question is how will dynamic's fade out when they stop rendering at earlier cascades?

I don't see a way to do this if the scene shader only "sees" a single shadow map texture, with no distinction between static and dynamic shadows. I guess most AAA games just live with it.

@mrjustaguy
Copy link
Contributor

I guess blending splits on would get the effect fade out..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce shadow update frequency for distant real-time shadows
5 participants