defold / sample-lights-and-shadows

Example of how to achieve pixel perfect shadows in a 2D view
MIT License
31 stars 5 forks source link

Lights break if they don't all have the same radius #8

Closed haath closed 1 year ago

haath commented 1 year ago

Hello,

I noticed that since your optimizations in this commit lights now break if there is at least one light in the scene with a different radius than the rest. In the example below the blue light radius was set to 300 while the red one was left at 256. This from the current master branch with no other modifications.

image

This issue was not present in the commit before that one. In fact even in my PR, you can see in the gif that the radius is changing without issue. But what happened is that in your optimization commit you changed the default radius to 256, and that is the exact same value that I was animating the blue radius to in my showcase! (see this line) So when you were working on it the radius stayed unchanged, and this is probably why you didn't notice the bug.


I am still a newbie at the rendering stuff, nonetheless I tried some things to identify the issue. Here is what I noticed:

If you don't have the time to debug it yourself, feel free to share your guesses as to where the mistake might be, and I'll try my best to fix it!

britzl commented 1 year ago

Ah, wow, you're right. It is indeed broken. The problem is the shared occluder and shadowmap render targets when the size of the lights aren't the same. If each light has its own render targets it works. You can test this here:

https://github.com/defold/sample-lights-and-shadows/archive/refs/heads/fix-light-size.zip

I need to spend some time debugging the drawcalls to better understand why the previous approach didn't work. A quick and easy way to do this is to use Spector.js (Chome/Firefox plugin) in the browser to step through the frame and see each step of the render process.

haath commented 1 year ago

Ah darn, I went as far as to create a separate shadowmap target per light, but didn't think to also separate the occluder targets 😄

Thanks for the quick reply, I will experiment a bit to see if this way can be optimized somehow.

britzl commented 1 year ago

Cool. Let me know if the branch works as expected for you and then I'll merge it to master!

haath commented 1 year ago

Cool. Let me know if the branch works as expected for you and then I'll merge it to master!

It looks correct, thanks!

britzl commented 1 year ago

Thanks @haath ! I've merged to master now.