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

Add support for area lights #8020

Open
wants to merge 1 commit into
base: ma/descriptor-sets
Choose a base branch
from

Conversation

haneulkimdev
Copy link
Contributor

This pull request introduces area light support in Filament, focusing primarily on rectangle lights. The implementation draws inspiration from Eric Heitz et al.'s "Real-Time Polygonal-Light Shading with Linearly Transformed Cosines" research (ACM SIGGRAPH 2016, https://eheitzresearch.wordpress.com/415-2/) and the approach used in Three.js.

In developing this feature, I prioritized smooth integration with Filament's existing lighting system over performance optimization.

I'm open to feedback and suggestions for improving this implementation. Thank you for reviewing my contribution to the Filament project.

@pixelflinger
Copy link
Collaborator

it will take a little while to review. thank you for the contribution.

@romainguy
Copy link
Collaborator

I'll take a look as well but I've explored LTC before and burning 2 samplers is most likely a no-go on mobile for us (plus the question of the binary size increase). There are other concerns I'll need to detail as well (the way this is written there will be an extra cost for all lights even non area lights which isn't something we want to pay for on mobile).

@pixelflinger
Copy link
Collaborator

A few comments echoing Romain's:

  • We already are out of samplers for feature level 2 and below. So this PR actually breaks existing materials; so we can't take it as-is.
  • Is there a way you could use a texture array instead of 2 textures, so that we'd save one sampler?
  • Regardless, I think our only option here is to make this a "feature level 3" feature. This means it needs to be only enabled with feature level 3 materials and attempting to use any of these API with an feature level 2 or less engine should fail.
  • this PR needs to come with a sample (ideally one with a imgui so we can play with the parameters of the area light)
  • and ideally, it would also come with a basic "smoke test" unit test (e.g. checking the feature level and basic functionality)
  • and finally, I would prefer if this change was made against the ma/descriptors-set branch, which we are planning to merge "soon", so we don't have to a painful merge.

@haneulkimdev haneulkimdev marked this pull request as ready for review September 2, 2024 02:07
@haneulkimdev haneulkimdev marked this pull request as draft September 2, 2024 02:11
@haneulkimdev
Copy link
Contributor Author

I apologize for the confusion. I accidentally marked this PR as ready for review and have now converted it back to draft status.

I appreciate the feedback from @pixelflinger. I'll be working on addressing the following points:

  1. Adjusting the feature to be compatible with feature level 3
  2. Exploring the possibility of using a texture array
  3. Adding a sample with imgui for parameter adjustment
  4. Including a basic smoke test
  5. Rebasing against the ma/descriptors-set branch

As I'm balancing this with my full-time job, updates may take some time. I'll do my best to work on these changes as efficiently as possible, but please understand if progress is not immediate.

I'll update the PR once these changes are implemented. Thank you for your patience, understanding, and input.

@haneulkimdev
Copy link
Contributor Author

I've updated this PR with the following changes:

  1. Switched to texture arrays
  2. Added a sample with imgui
  3. Rebased onto the ma/descriptors-set branch

Next, I'm trying to restrict this feature to only work with feature level 3. However, I'm not yet fully familiar with the codebase and I'm unsure which parts need to be modified. Could you provide some guidance or a starting point on how to properly restrict this feature to feature level 3 and handle lower feature levels?

@haneulkimdev haneulkimdev marked this pull request as ready for review October 4, 2024 02:26
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 this pull request may close these issues.

3 participants