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

Initial implementation for Area Lights #16078

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

Conversation

SergioRZMasson
Copy link
Contributor

Overview

This PR provides a first implementation for rectangular area lights. The implementation is based on reference repo https://github.com/selfshadow/ltc_code/ and uses the Linearly Transformed Cosines technique described by Eric Heitz, Jonathan Dupuy, Stephen Hill and David Neubelt`s paper in 2016 . This PR only supports single color rectangular area lights with texture support been introduced in a latter PR. This PR also does not support shadows for the RectAreaLight.

API

Besides the usual diffuse and specular colors the area light is defined by a position, a width and a height. Setting a rotation is possible by assigning a transform node as a parent for the light.

var light = new BABYLON.RectAreaLight("areaLight", position, width, height, scene);

By default shaders will use LTC data stored in the Babylon.js CDN. However, users can assign their own IAreaLightLTCProvider to scene.areaLightLTCProvider and use their own LTC textures.

Copy link
Contributor

@bghgary bghgary left a comment

Choose a reason for hiding this comment

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

I didn't review the shaders or the inspector. Some smallish comments on the rest.

packages/dev/core/src/Lights/LTC/ltcTextureTool.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Lights/LTC/ltcTextureTool.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Lights/LTC/ltcTextureTool.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Lights/LTC/ltcTextureTool.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Lights/LTC/ltcTextureTool.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Lights/areaLight.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Lights/areaLight.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Lights/rectAreaLight.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Lights/rectAreaLight.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Materials/standardMaterial.ts Outdated Show resolved Hide resolved
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.

LGTM!

packages/dev/core/src/Lights/LTC/ltcTextureTool.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Lights/LTC/ltcTextureTool.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Lights/areaLight.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Lights/rectAreaLight.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Lights/rectAreaLight.ts Show resolved Hide resolved
@bjsplat
Copy link
Collaborator

bjsplat commented Jan 17, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 17, 2025

Comment on lines 16 to 26
ltc1Texture: BaseTexture;

/**
* Linearly trasnformed cossine texture Fresnel Approximation.
* Linearly transformed cosine texture Fresnel Approximation.
*/
ltc2Texture: BaseTexture;

/**
* Promise to wait for Area Lights are ready.
*/
whenAreaLightsReady: Promise<void>;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a great pattern. It's possible for someone to get an undefined ltc texture with the contract. Consider returning the ltc textures in the Promise result and rename function to getTexturesAsync.

Copy link
Member

@sebavan sebavan left a comment

Choose a reason for hiding this comment

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

Great job !! Besides the comments, I am wondering what makes standard and PBR not look alike in the tests knowing they use the same maths ?

const _ltc1 = new Uint16Array(64 * 64 * 4);
const _ltc2 = new Uint16Array(64 * 64 * 4);

const ltcPath = Tools.GetBabylonScriptURL("https://assets.babylonjs.com/areaLights/areaLightsLTC.bin", true);
Copy link
Member

Choose a reason for hiding this comment

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

I think we should still support something through the script base url if we can ? this is the centralized way for our users to prevent external call.

}

public transferToNodeMaterialEffect(effect: Effect, lightDataUniformName: string) {
// TO DO: Implement this to add support for NME.
Copy link
Member

Choose a reason for hiding this comment

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

Next PR @SergioRZMasson ?

uniform sampler2D areaLightsLTC2Sampler;

lightingInfo computeAreaLighting(sampler2D ltc1, sampler2D ltc2, vec3 viewDirectionW, vec3 vNormal, vec3 vPosition, vec4 lightData, vec3 halfWidth, vec3 halfHeight, vec3 diffuseColor, vec3 specularColor, float roughness )
Copy link
Member

Choose a reason for hiding this comment

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

Can we add the reference to the paper we used just before the function ?

Copy link
Member

Choose a reason for hiding this comment

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

it also looks really similar to computeAreaPreLightingInfo ? nothing we should factorize ? the rest is splitted cause pbr and standard do not do the same.

// http://blog.selfshadow.com/publications/s2016-advances/s2016_ltc_fresnel.pdf
vec3 fresnel = ( specularColor * t2.x + ( vec3( 1.0 ) - specularColor ) * t2.y );
result.areaLightSpecular = specularColor * fresnel * LTCEvaluate( normal, viewDir, position, mInv, rectCoords );
Copy link
Member

Choose a reason for hiding this comment

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

I guess we should not do the mul by specularColor to be consistent with the diffuse part

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.

6 participants