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

Fix light culling mask behavior in Mobile and Compat renderers #98266

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

m-pranav-r
Copy link

@m-pranav-r m-pranav-r commented Oct 17, 2024

Fixes #94643. Added logic which checks the light's culling mask settings before proceeding with lighting calculations to wherever not present (namely the Forward-Mobile and Compatibility renderers). Just a simple port of the one already used in the Forward-Plus renderer.

Pictures for comparison:

MRP: Compatibility:
bug-compat
Forward+:
bug-fplus
Forward-Mobile:
bug-mobilee
Fixed: Compatibility:
fix-compat
Forward+:
fix-fplus
Forward-Mobile:
fix-mobile

(Also fixes the bug in editor mode too)

@m-pranav-r m-pranav-r requested a review from a team as a code owner October 17, 2024 14:23
@AThousandShips AThousandShips added this to the 4.4 milestone Oct 17, 2024
@AThousandShips AThousandShips added cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Oct 17, 2024
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, it works as expected in Mobile and Compatibility.

However, the solution used in Mobile is probably not the correct one, as it'd leave performance on the table since lights would not be culled on the CPU before being sent to the GPU. See comments in #86105, as it's the same fix as applied there (except it's applied in two locations in that PR).

I suggest looking at moving the culling to the CPU side in Mobile, as is already done in Compatibility in your PR. If you can't find a way to do this, then I suggest removing the Mobile changes so the Compatibility changes can be merged first.

@m-pranav-r
Copy link
Author

Oh I see! I'll look for a while and hope i find the correct fix but if not ill just do as you said and try again only with the compatibility fix. ty for the directions!

@clayjohn
Copy link
Member

I think in both cases the check should be inside of pair_light_instances() so that it is done at culling time and also you aren't pairing with lights that are masked out anyway.

@m-pranav-r
Copy link
Author

m-pranav-r commented Oct 19, 2024

(I really should have consulted this page again before making the changes, I think I added the culling logic for Mobile just before the pairing call but I'll revise it if need be.)

Shifted culling back to CPU so that the light instances indices wont be sent to the buffer at all. Also covers both spot and omni lights in Mobile now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release topic:rendering
Projects
None yet
4 participants