-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Initial clustered lights implementation (tiled clustering only) #16866
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
base: master
Are you sure you want to change the base?
Conversation
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
3 similar comments
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Some more resources about the subject:
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
19 similar comments
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16866/merge/index.html#WGZLGJ#4600 Links to test babylon tools with this snapshot: https://playground.babylonjs.com/?snapshot=refs/pull/16866/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/16866/merge#BCU1XR#0 |
Visualization tests for WebGPU |
WebGL2 visualization test reporter: |
WebGL2 visualization test reporter: |
Visualization tests for WebGPU |
WebGL2 visualization test reporter: |
Visualization tests for WebGPU |
WebGL2 visualization test reporter: |
Visualization tests for WebGPU |
WebGL2 visualization test reporter: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work!
I have a few additional comments:
- Removing lights from the scene has an unfortunate side effect: lights can no longer be edited in the inspector... I'm not sure how to fix that, though. We would probably need a specific UI for clustered lights.
- You should add one or more visualization tests for this new feature.
- I think a test is needed to verify that this also works when using custom render targets using cameras other than the main camera.
- Actually, this use case isn't implemented; it's something we'll need to make work.
- I think a test is needed to verify that this also works when using custom render targets using cameras other than the main camera.
- For PBR, much of the code in lightFragment.fx is duplicated in the
computeClusteredLighting
function, but apart from rewriting all the lightFragment code to handle one type of light from start to finish instead of interleaving the calculation of light components (diffuse, specular, etc.), I don't see how to do this. cc @sebavan - we'll need to make it work with frame graphs
- we'll need to make it work with node materials
For reference, the PG I used to test:
@@ -0,0 +1,17 @@ | |||
struct SpotLight { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be clearer to name it ClusteredLight
, and the function below getClusteredLight
, because light is not necessarily a spot (and we may support other types than spot/point in the future).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally named it ClusteredLight
but thought it was confusing since the whole clustering system (in JavaScript land) was named ClusteredLight
so I wanted to name it something else that still represented it as being just one of the lights in the clustered system
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the class in JS should be named something like ClusteredLightContainer
, as it's not a real light but a container for lights?
packages/dev/core/src/Shaders/ShadersInclude/lightsFragmentFunctions.fx
Outdated
Show resolved
Hide resolved
packages/dev/core/src/Shaders/ShadersInclude/pbrClusteredLightingFunctions.fx
Show resolved
Hide resolved
packages/dev/core/src/Shaders/ShadersInclude/pbrClusteredLightingFunctions.fx
Show resolved
Hide resolved
@matanui159 This is an amazing contrib !!! Thanks a ton for the review @Popov72 I think the main left over part is a better factorization of the lighting function. Subsequent follow up should be NME, and FrameGraphs |
There's also support for custom render targets that we need to take into account. |
It should work as long as the other cameras are added to the active camera list. Not sure the best way to support any render target would be, especially if we don't want to run it for things like shadow generation. |
There's still the depth clustering PR that I will be opening after this gets merged. |
btw these are the main playgrounds I've been using to test: |
WebGL2 visualization test reporter: |
Visualization tests for WebGPU |
Visualization tests for WebGPU |
WebGL2 visualization test reporter: |
The use case I'm speaking about above is the one where we create a http://localhost:1338/#20OAV9#15378 We can fix it by generating the mask texture in http://localhost:1338/#20OAV9#15377 We could add the code in the cc @sebavan |
Note that for WebGPU, you will probably need to sort the light on the GPU, otherwise depth buffer management will become quite complicated, as updating a storage buffer from the CPU to the GPU does not happen in the same timeframe as the execution of graphics commands. To illustrate this, if you have two renderings from two cameras that follow each other, we currently (WebGPU) proceed as follows:
This works because StorageBufferMask is updated by some graphics commands that are executed in the same command stream as the rendering commands (they are all executed when we do Now, if we add depth clustering and perform sorting on the CPU, we will need to copy the result to a storage buffer:
The problem is that the copy from the CPU to the storage buffer is done immediately, not when we execute |
I ran some more tests and encountered a few problems:
![]()
![]()
![]()
![]() |
Couldn't really find an easy way to adjust the disc to be shaped to a cone depending on angle its looked at. So yeah currently it just does the full disc. Depth clustering will help with this since further away or closer pixels will ignore the light |
The two where the mask is too large is I think is because we don't do any depth pre-pass so we can't limit the light to the intersection of the plane since the plane could be closer to the screen in which case it would be larger. |
Indeed, I should have drawn a sphere at the light location, with a ![]() We can see it's ok.
It looks like the problem comes from the fact that the disk we rasterize to find the clusters the light intersects with is perspective projected, meaning it will be smaller than what it should be. See http://localhost:1338/#20OAV9#15396: ![]() In this example, the yellow mesh is a sphere which represents the light (positionned at 0,0,0) and the red mesh is a disk mesh which represents the disk that will be rasterized to find the clusters the light intersects with. I took the disk creation from As you can see, the red disk is smaller than the yellow disk, because of the perspective transform. To fix it, I think you will have to calculate an offset to move the disk nearer to the camera (in the camera coordinate system, so a negative offset to |
Remaining issues
Implementation Details
When this work is more complete ill move all this over to the official docs, but for now just using this PR as a rough set of notes/thoughts/todos
The implementation is mainly inspired by these CoD slides: https://advances.realtimerendering.com/s2017/2017_Sig_Improved_Culling_final.pdf
The main idea is to split the clustering into separate X/Y "tiled" clustering AND a Z "depth" clustering, and then checking if the lights is in both the tile and the depth slice.
Tiled Clustering
This uses the rendering pipeline to draw meshes for each light and writes bits out to a light mask texture if there are pixels infront of the depth buffer (effectively finding lights that intersect the depth buffer).
The render target with the light mask texture and depth buffer has a resolution equal to the amount of tiles.
Depth pre-pass (removed)
All the scene geometry is rendered to a low resolution render target. Currently this is repeated per
ClusteredLight
, in future these depth texture might be able to get sharedStencil Pass (removed)
Draws all the light geometry with depthFunction=GREATER and sets stencil bits to 1.
This is combined with the next pass to only mark pixels that contain light geometry both behind AND infront of the depth buffer.
However this ended up causing issues due to the low-resolution aliased depth buffer where stencil bits weren't set for the edges of meshes.

In future we could maybe get around this by doing a post-process pass over the depth pre-pass to expand the max depth values out by one tile. Although for now this just adds extra complexity and we probably don't really need this pass.
Light Mask Pass
Each light mesh is drawn with a different index and any fragment pixels which pass the depth test (meaning the light is in front of the depth pre-pass) will set its bit in the light mask texture with an atomic OR.
Atleast thats the plan on WebGPU which isn't implemented yet. WebGL doesn't have support for storage textures or atomic operations, so we instead use a float buffer with additive blending.
To prevent accuracy issues with floating point values we ensure the mask doesn't go above the bits in the floating-point fraction, which limits us to 23 lights per clusteredlight on most systems.
Lighting pass
This light mask is then queried bit-by-bit in the lighting calculation to figure out which lights are in that "tile"
Light Meshes
All the light meshes are just spheres. To reduce the pixels set for spotlights they are modified in the vertex shader so positions on the sphere that are outside the spotlight angle are set to 0,0,0. This causes the sphere to end up having a more cone shape
Depth Clustering
TODO. not currently implemented. will be implemented in a different PR
CoD seems to do this in CPU which makes sense since its only a min/max index per slice so can be cheaply calculated and uploaded.
Not Implemented / Broken
Number.MAX_VALUE
seems to break ittransferToNodeMaterialEffect
I don't plan to support these kinda lights: