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

Volumetric spotlight #15046

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Volumetric spotlight #15046

wants to merge 7 commits into from

Conversation

ademola-lou
Copy link

This PR implements volumetric spotlight

Test: http://localhost:1338/#0BUGL8

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 29, 2024

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@sebavan sebavan requested a review from deltakosh April 29, 2024 21:16
@bjsplat
Copy link
Collaborator

bjsplat commented Apr 29, 2024

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 29, 2024

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 29, 2024

Visualization tests for WebGPU (Experimental)
Important - these might fail sporadically. This is an optional test.

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/15046/merge/testResults/webgpuplaywright/index.html

@ademola-lou ademola-lou requested a review from sebavan May 5, 2024 20:33
@ademola-lou
Copy link
Author

ademola-lou commented May 5, 2024

Yeah this should also fix the issue with the cone lookAt direction

Test PG: localhost:1338/#KIXHUK

Copy link
Contributor

@Popov72 Popov72 left a comment

Choose a reason for hiding this comment

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

I'd like to decouple SpotLight from VolumetricSpotLight as much as possible (better for class independence and tree shaking).

I don't think it's necessary to modify the SpotLight class. The user can simply do new VolumetricSpotLight(...) to create a volumetric spotlight. This would also get rid of the IVolumetricSpotLight interface.

I'd also like to change the class name, because if we implement a true volumetric light in the future, we'll probably want to use VolumetricSpotLight for the name... I'm not sure which name to use (SimpleVolumetricSpotLight? FakeVolumetricSpotLight?). Let's summon @bghgary, the master of names!

Finally, you need to add documentation to the class/methods to keep the CI happy :)

packages/dev/core/src/Lights/volumetricSpotLight.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Lights/volumetricSpotLight.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Lights/volumetricSpotLight.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Lights/volumetricSpotLight.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Lights/volumetricSpotLight.ts Outdated Show resolved Hide resolved
}

private _update(){
const lightPos = this.spotLight.position.clone();
Copy link
Contributor

Choose a reason for hiding this comment

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

As this function is called each frame, you should remove all the GC and not create new objects.

Use TmpVectors.Vector3[0], TmpVectors.Vector3[1], etc to hold temporary results.

packages/dev/core/src/Lights/spotLight.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Lights/volumetricSpotLight.ts Outdated Show resolved Hide resolved
@ademola-lou
Copy link
Author

ademola-lou commented May 10, 2024

new test PG: http://localhost:1338/#KIXHUK#3

also implemented soft intersection using depthRender

@bjsplat
Copy link
Collaborator

bjsplat commented May 10, 2024

Visualization tests for WebGPU (Experimental)
Important - these might fail sporadically. This is an optional test.

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/15046/merge/testResults/webgpuplaywright/index.html

import { ShaderMaterial } from "core/Materials/shaderMaterial";
import "../Shaders/simpleVolumetricSpot.fragment";
import "../Shaders/simpleVolumetricSpot.vertex";
import { Observer, RenderTargetTexture, TmpVectors } from "..";
Copy link
Member

Choose a reason for hiding this comment

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

it is not allowed to import from a folder, import every element from their respective files.

@sebavan sebavan marked this pull request as draft May 13, 2024 13:44
@sebavan
Copy link
Member

sebavan commented May 13, 2024

Marked as draft until the PR is ready. Would be great to have the format/lint/doc and at least one visual test.

Also it would be great to ensure it follows the various falloff types inverse squared vs linear and so on ?

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.

None yet

5 participants