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

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

Closed
haath opened this issue Mar 5, 2023 · 5 comments · Fixed by #10
Closed

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

haath opened this issue Mar 5, 2023 · 5 comments · Fixed by #10

Comments

@haath
Copy link
Contributor

haath commented Mar 5, 2023

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:

  • No matter how many lights you add, and with what sizes, the last light in the list always gets drawn correctly. All the rest are the ones that are drawn wrong.
  • The above point made me think that maybe each light was messing up the other's 1xN shadowmap, so I tried to separate them and created one shadowmap render target per light. This didn't fix the issue.
  • For small differences in radius, it appears as if the light is offset both on the x and y axis by the same amount as the difference in radius. Not sure what is the significance of this.
  • For larger differences in radius I can't notice any pattern, the lights are completely messed up.
  • If a light is much larger than the other, it will either completely disappear, or stop casting shadows but stay intact at the correct position. (very confusing)
  • It doesn't matter whether the lights overlap or not. Even if I place them on completely different parts of the scene, the issue still occurs the same way.

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
Copy link
Contributor

britzl commented Mar 7, 2023

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
Copy link
Contributor Author

haath commented Mar 7, 2023

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
Copy link
Contributor

britzl commented Mar 7, 2023

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

@haath
Copy link
Contributor Author

haath commented Mar 8, 2023

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
Copy link
Contributor

britzl commented Mar 8, 2023

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

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

Successfully merging a pull request may close this issue.

2 participants