Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
68 commits
Select commit Hold shift + click to select a range
5218545
changes to dual measure quant, old stuff commented out for now
keptsecret Sep 4, 2025
f7525af
create query structs in beckmann ndf, removed query concepts, funcs r…
keptsecret Sep 5, 2025
c50db68
did the same for ggx ndf, some fixes to beckmann
keptsecret Sep 5, 2025
c49bedb
Merge branch 'master' into bxdf_fixes_cook_torrance
keptsecret Sep 8, 2025
319c954
fixes to ndf concept
keptsecret Sep 8, 2025
4eeacf1
moved eval, quotient/pdf into cook torrance base
keptsecret Sep 9, 2025
e7bc784
moved generate H into ndfs, generate impl in cook torrance base
keptsecret Sep 9, 2025
72226bb
use new cook torrance base in microfacet bxdfs
keptsecret Sep 9, 2025
f34b348
Merge branch 'master' into bxdf_fixes_cook_torrance
keptsecret Sep 10, 2025
ad13044
numerous typo bug fixes
keptsecret Sep 10, 2025
d8d2116
fixes to ggx ndf
keptsecret Sep 10, 2025
238b08e
fixed conductor fresnel naming ior as eta
keptsecret Sep 10, 2025
8bc707f
minor fixes to beckmann ndf, removed obsolete comments
keptsecret Sep 10, 2025
9fb28cf
added luminosity contribution hint to cook torrance bsdfs
keptsecret Sep 11, 2025
4cb8a0a
added aniso overloads to isotropic bxdf methods
keptsecret Sep 11, 2025
392dc31
checks for invalid generate sample
keptsecret Sep 11, 2025
02de86e
removed obsolete commented out stuff
keptsecret Sep 12, 2025
4a7f532
added cartesian-polar conversions from unit tests
keptsecret Sep 12, 2025
80c4f67
Merge branch 'fix_rotation_mat' into bxdf_fixes_cook_torrance
keptsecret Sep 15, 2025
6233bd1
Merge branch 'master' into bxdf_fixes_cook_torrance
keptsecret Sep 15, 2025
ce5fbac
vector hashes
keptsecret Sep 15, 2025
47e814b
Merge branch 'fix_rotation_mat' into bxdf_fixes_cook_torrance
keptsecret Sep 15, 2025
c983975
Merge branch 'master' into bxdf_fixes_cook_torrance
keptsecret Sep 16, 2025
0f2ee0b
fix fresnel usage in bxdf pdf
keptsecret Sep 17, 2025
b5f02e6
fix F calc in generate
keptsecret Sep 17, 2025
a111415
merge master, fix conflicts
keptsecret Sep 22, 2025
9655049
minor fixes to non cook torrance bxdf generate
keptsecret Sep 22, 2025
a1743d2
split out bxdf concept typdefs
keptsecret Sep 22, 2025
e6d663b
removed redundant thin_smooth_dielectric create
keptsecret Sep 22, 2025
340cee3
added and use notEqual spirv intrinsic
keptsecret Sep 22, 2025
a3733b1
moved polar coord stuff into its own file
keptsecret Sep 22, 2025
3e3589b
made smith functions return measureless
keptsecret Sep 22, 2025
4bdf199
use type alias macro from config
keptsecret Sep 22, 2025
4faecc3
adjust ndf concept, change fresnel conductor ior to eta
keptsecret Sep 23, 2025
407da2f
combine brdf/bsdf cook torrance into same struct, old stuff commented…
keptsecret Sep 23, 2025
638b8b5
moved duplicate code in eval, pdf, quotient_pdf into templated intera…
keptsecret Sep 23, 2025
c587820
some changes to fresnel, cook torrance base
keptsecret Sep 24, 2025
d438360
reverted ggx ndf to use optimizations
keptsecret Sep 24, 2025
5f49f11
pdf function checks for backfacing V
keptsecret Sep 25, 2025
627074b
reduced beckmann, ggx ndfs to single struct with enable_ifs
keptsecret Sep 25, 2025
3896231
put concepts back in for ndf impl
keptsecret Sep 26, 2025
91b39d5
ggx ndf determine clamp with template bool
keptsecret Sep 26, 2025
2a3cda3
moved A out of ndf base into generate base
keptsecret Sep 26, 2025
c9f9366
added create methods to ndfs, cook torrance base; slight changes to c…
keptsecret Sep 26, 2025
fd128e6
make beckmann + ggx bxdfs typedefs of cooktorrance base, functionalit…
keptsecret Sep 26, 2025
882375e
added flipSign func that copies sign of rhs
keptsecret Sep 29, 2025
b22d570
remove unused var
keptsecret Sep 29, 2025
c0586f5
moved cook torrance base into base folder, removed commented out in b…
keptsecret Sep 29, 2025
9eeb248
make lambertian + oren nayar bxdfs typedefs of corresponding base, fu…
keptsecret Sep 29, 2025
26c76e4
fix microfacet bxdf concept
keptsecret Sep 29, 2025
58e2a0b
fixes to cook torrance generate
keptsecret Sep 30, 2025
2a08728
templated ray_dir_info reflect/refract funcs
keptsecret Sep 30, 2025
b993e47
pdf should return inf when smooth cook torrance bxdf
keptsecret Sep 30, 2025
f3cb6ff
set pdf=0 in specific cases
keptsecret Oct 1, 2025
7389c9a
added checking for TIR in bsdfs
keptsecret Oct 1, 2025
ec5913b
change fresnel orientedeta based on NdotV, user needs to pass front f…
keptsecret Oct 2, 2025
bccdb0b
fixes flipSign and flipSignIfRHSNeg
keptsecret Oct 3, 2025
1e9e407
fix bsdf generate: LdotH and NdotL same sign, otherwise invalid sample
keptsecret Oct 3, 2025
dab51c3
cook torrance brdf returns invalid sample in cases, fix missing return
keptsecret Oct 3, 2025
53ab934
more util funcs for ray_dir_info, avoid setting vals directly
keptsecret Oct 3, 2025
804014f
added IsMicrofacet to bxdf_traits
keptsecret Oct 3, 2025
260f7a3
projectedSphere generate should take v by ref
keptsecret Oct 6, 2025
fd54ac4
removed vector std::hash because glm already implemented it
keptsecret Oct 6, 2025
5caf006
Merge branch 'master' into bxdf_fixes_cook_torrance
keptsecret Oct 6, 2025
7508b82
get orientedeta and rcp methods in fresnel + concept
keptsecret Oct 6, 2025
2b4a9c1
slight change to flipSignIfRHSNeg struct
keptsecret Oct 6, 2025
d3fa872
added two-sided fresnel concept
keptsecret Oct 6, 2025
5765e87
removed cook torrance param structs/create funcs
keptsecret Oct 6, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
340 changes: 340 additions & 0 deletions include/nbl/builtin/hlsl/bxdf/base/cook_torrance_base.hlsl
Original file line number Diff line number Diff line change
@@ -0,0 +1,340 @@
// Copyright (C) 2018-2025 - DevSH Graphics Programming Sp. z O.O.
// This file is part of the "Nabla Engine".
// For conditions of distribution and use, see copyright notice in nabla.h
#ifndef _NBL_BUILTIN_HLSL_BXDF_COOK_TORRANCE_INCLUDED_
#define _NBL_BUILTIN_HLSL_BXDF_COOK_TORRANCE_INCLUDED_

#include "nbl/builtin/hlsl/bxdf/common.hlsl"
#include "nbl/builtin/hlsl/bxdf/config.hlsl"
#include "nbl/builtin/hlsl/bxdf/ndf.hlsl"
#include "nbl/builtin/hlsl/bxdf/fresnel.hlsl"
#include "nbl/builtin/hlsl/bxdf/ndf/microfacet_to_light_transform.hlsl"

namespace nbl
{
namespace hlsl
{
namespace bxdf
{

namespace impl
{
template<typename T, typename U>
struct __implicit_promote;

template<typename T>
struct __implicit_promote<T,T>
{
static T __call(const T v)
{
return v;
}
};

template<typename T>
struct __implicit_promote<T,vector<typename vector_traits<T>::scalar_type, 1> >
{
static T __call(const vector<typename vector_traits<T>::scalar_type, 1> v)
{
return hlsl::promote<T>(v[0]);
}
};

template<class N, class F, bool IsBSDF>
struct quant_query_helper;

template<class N, class F>
struct quant_query_helper<N, F, true>
{
using quant_query_type = typename N::quant_query_type;

template<class C>
static quant_query_type __call(NBL_REF_ARG(N) ndf, NBL_CONST_REF_ARG(F) fresnel, NBL_CONST_REF_ARG(C) cache)
{
return ndf.template createQuantQuery<C>(cache, fresnel.orientedEta.value[0]);
}
};

template<class N, class F>
struct quant_query_helper<N, F, false>
{
using quant_query_type = typename N::quant_query_type;

template<class C>
static quant_query_type __call(NBL_REF_ARG(N) ndf, NBL_CONST_REF_ARG(F) fresnel, NBL_CONST_REF_ARG(C) cache)
{
typename N::scalar_type dummy;
return ndf.template createQuantQuery<C>(cache, dummy);
}
};

template<class F, bool IsBSDF>
struct check_TIR_helper;

template<class F>
struct check_TIR_helper<F, false>
{
template<class MicrofacetCache>
static bool __call(NBL_CONST_REF_ARG(F) fresnel, NBL_CONST_REF_ARG(MicrofacetCache) cache)
{
return true;
}
};

template<class F>
struct check_TIR_helper<F, true>
{
template<class MicrofacetCache>
static bool __call(NBL_CONST_REF_ARG(F) fresnel, NBL_CONST_REF_ARG(MicrofacetCache) cache)
{
return ComputeMicrofacetNormal<typename F::scalar_type>::isValidMicrofacet(cache.isTransmission(), cache.getVdotL(), cache.getAbsNdotH(), fresnel.getRefractionOrientedEta());
}
};

template<class F, bool IsBSDF NBL_STRUCT_CONSTRAINABLE>
struct getOrientedFresnel;

template<class F>
struct getOrientedFresnel<F, false>
{
static F __call(NBL_CONST_REF_ARG(F) fresnel, typename F::scalar_type NdotV)
{
// expect conductor fresnel
return fresnel;
}
};

template<class F>
NBL_PARTIAL_REQ_TOP(fresnel::TwoSidedFresnel<F>)
struct getOrientedFresnel<F, true NBL_PARTIAL_REQ_BOT(fresnel::TwoSidedFresnel<F>) >
{
using scalar_type = typename F::scalar_type;

static F __call(NBL_CONST_REF_ARG(F) fresnel, scalar_type NdotV)
{
return fresnel.getReorientedFresnel(NdotV);
}
};
}

// N (NDF), F (fresnel)
template<class Config, class N, class F NBL_PRIMARY_REQUIRES(config_concepts::MicrofacetConfiguration<Config> && ndf::NDF<N> && fresnel::Fresnel<F>)
struct SCookTorrance
{
MICROFACET_BXDF_CONFIG_TYPE_ALIASES(Config);

using this_t = SCookTorrance<Config, N, F>;
using quant_type = typename N::quant_type;
using ndf_type = N;
using fresnel_type = F;

NBL_CONSTEXPR_STATIC_INLINE bool IsAnisotropic = ndf_type::IsAnisotropic;
NBL_CONSTEXPR_STATIC_INLINE bool IsBSDF = ndf_type::NDFSurfaceType != ndf::MTT_REFLECT;

template<class Interaction, class MicrofacetCache>
spectral_type __eval(NBL_CONST_REF_ARG(sample_type) _sample, NBL_CONST_REF_ARG(Interaction) interaction, NBL_CONST_REF_ARG(MicrofacetCache) cache)
{
fresnel_type _f = impl::getOrientedFresnel<fresnel_type, IsBSDF>::__call(fresnel, interaction.getNdotV());
Copy link
Member Author

Choose a reason for hiding this comment

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

yeah just add a method to create a reoriented-self-copy to the fresnels.

Btw why did you make a struct with a specialization instead of

fresnel_type _f = fresnel;
NBL_IF_CONSTEXPR(IsBSDF)
    _f = ..special stuff here...;

Copy link
Contributor

Choose a reason for hiding this comment

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

Not constexpr in hlsl

const bool notTIR = impl::check_TIR_helper<fresnel_type, IsBSDF>::template __call<MicrofacetCache>(_f, cache);
if ((IsBSDF && notTIR) || (!IsBSDF && _sample.getNdotL() > numeric_limits<scalar_type>::min && interaction.getNdotV() > numeric_limits<scalar_type>::min))
Comment on lines +138 to +139
Copy link
Member Author

@devshgraphicsprogramming devshgraphicsprogramming Oct 3, 2025

Choose a reason for hiding this comment

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

its a bit cleaner to write

bool valid;
fresnel_type _f = fresnel;
NBL_IF_CONSTEXPR(IsBSDF)
{
    _f = ..special stuff here...;
   valid = impl::check_TIR_helper<fresnel_type, IsBSDF>::template __call<MicrofacetCache>(_f, cache);
}
else
   valid =  _sample.getNdotL() > numeric_limits<scalar_type>::min && interaction.getNdotV() > numeric_limits<scalar_type>::min;
if (!valid)
   return hlsl::promote<spectral_type>(0.0);

{
using quant_query_type = typename ndf_type::quant_query_type;
using g2g1_query_type = typename ndf_type::g2g1_query_type;
Copy link
Member Author

Choose a reason for hiding this comment

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

push to alias to where its actually needed/used


scalar_type dummy;
quant_query_type qq = ndf.template createQuantQuery<MicrofacetCache>(cache, dummy);

quant_type D = ndf.template D<sample_type, Interaction, MicrofacetCache>(qq, _sample, interaction, cache);
scalar_type DG = D.projectedLightMeasure;
if (D.microfacetMeasure < bit_cast<scalar_type>(numeric_limits<scalar_type>::infinity))
{
g2g1_query_type gq = ndf.template createG2G1Query<sample_type, Interaction>(_sample, interaction);
DG *= ndf.template correlated<sample_type, Interaction>(gq, _sample, interaction);
}
Comment on lines +148 to +153
Copy link
Member Author

Choose a reason for hiding this comment

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

hmm actually since Eval is not Quotient, it should return 0 whenever PDF would have been INF, and therefore whenever Eval is inf, you can just return 0

NBL_IF_CONSTEXPR(IsBSDF)
return impl::__implicit_promote<spectral_type, typename fresnel_type::vector_type>::__call(_f(hlsl::abs(cache.getVdotH()))) * DG;
else
return impl::__implicit_promote<spectral_type, typename fresnel_type::vector_type>::__call(_f(cache.getVdotH())) * DG;
Comment on lines +154 to +157
Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rewrite to

float clampedVdotH = cache.getVdotH();
NBL_IF_CONSTEXPR(IsBSDF)
   clampedVdotH = hlsl::abs(clampedVdotH);
return impl::__implicit_promote<spectral_type, typename fresnel_type::vector_type>::__call(_f(clampedVdotH)) * DG;

}
else
return hlsl::promote<spectral_type>(0.0);
}
template<typename C=bool_constant<!IsAnisotropic> >
enable_if_t<C::value && !IsAnisotropic, spectral_type> eval(NBL_CONST_REF_ARG(sample_type) _sample, NBL_CONST_REF_ARG(isotropic_interaction_type) interaction, NBL_CONST_REF_ARG(isocache_type) cache)
{
return __eval<isotropic_interaction_type, isocache_type>(_sample, interaction, cache);
}
spectral_type eval(NBL_CONST_REF_ARG(sample_type) _sample, NBL_CONST_REF_ARG(anisotropic_interaction_type) interaction, NBL_CONST_REF_ARG(anisocache_type) cache)
{
return __eval<anisotropic_interaction_type, anisocache_type>(_sample, interaction, cache);
}

template<typename C=bool_constant<!IsBSDF> >
enable_if_t<C::value && !IsBSDF, sample_type> generate(NBL_CONST_REF_ARG(anisotropic_interaction_type) interaction, const vector2_type u, NBL_REF_ARG(anisocache_type) cache)
{
ray_dir_info_type localV_raydir = interaction.getV().transform(interaction.getToTangentSpace());
const vector3_type localV = interaction.getTangentSpaceV();
const vector3_type localH = ndf.generateH(localV, u);

cache = anisocache_type::createForReflection(localV, localH);
Copy link
Member Author

Choose a reason for hiding this comment

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

btw here https://github.com/Devsh-Graphics-Programming/Nabla/blob/804014f28e7469036673169266a013f42e018f60/include/nbl/builtin/hlsl/bxdf/common.hlsl#L710-L724C6

I don't see what sets cache.iso_cache.VdotL ( should be double the angle VdotH so VdotL = 2.f*VdotH*VdotH-1.f)

the same problem arises with the next create function

static this_t create(
const vector3_type tangentSpaceV,
const vector3_type tangentSpaceH,
const bool transmitted,
NBL_CONST_REF_ARG(fresnel::OrientedEtaRcps<monochrome_type>) rcpOrientedEta
)
{
this_t retval = createForReflection(tangentSpaceV,tangentSpaceH);
if (transmitted)
{
Refract<scalar_type> r = Refract<scalar_type>::create(tangentSpaceV, tangentSpaceH);
retval.iso_cache.LdotH = r.getNdotT(rcpOrientedEta.value2[0]);
}
return retval;
}

LdotH is overwritten but VdotL, which means your TIR checks later won't work

Copy link
Member Author

@devshgraphicsprogramming devshgraphicsprogramming Oct 3, 2025

Choose a reason for hiding this comment

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

btw in the transmitted case, retval.iso_cache.VdotL = VdotH*(LdotH - rcpOrientedEta.value[0] + VdotH* rcpOrientedEta.value[0]);

If I computed Correctly.

struct reflect_wrapper
{
vector3_type operator()() NBL_CONST_MEMBER_FUNC
{
return r(VdotH);
}
bxdf::Reflect<scalar_type> r;
scalar_type VdotH;
};
reflect_wrapper r;
r.r = bxdf::Reflect<scalar_type>::create(localV, localH);
r.VdotH = cache.getVdotH();
Comment on lines +180 to +191
Copy link
Member Author

Choose a reason for hiding this comment

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

write a comment that we're doing all these gymnastics to have reflect use an externally computed VdotH such that it doesn't compute it itself again

ray_dir_info_type localL = localV_raydir.template reflect<reflect_wrapper>(r);
Copy link
Member Author

@devshgraphicsprogramming devshgraphicsprogramming Oct 3, 2025

Choose a reason for hiding this comment

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

the whole point of the reflect_wrapper was to avoid turning the reflection of V around H into a three step process:

  1. transforming fat V Ray-Dir into tangent space
  2. reflecting V around H to get fat L
  3. transforming tangent space fat L into original space of fat V

Note that we can do reflections and refractions in any space we like, so instead of bringing V to tangent and L back out of it, one can just bring H to fat V's space, and do the refraction using non-local V and H because cache.VdotH remains invariant under rotations.

(this is why we get our reflection wrapper to override the computation of VdotH, its not something compiler CSE can spot being identical)

This is not only 1 transformation instead of 2, but also a muuch cheaper transformation (always a 3x3 matrix mul with H a vector, not a transformation of a direction vector and its derivatives or covariance matrices).

Copy link
Member Author

@devshgraphicsprogramming devshgraphicsprogramming Oct 3, 2025

Choose a reason for hiding this comment

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

I know that creating a sample from worldspace L requires 3 dot products with TBN which is same as creating a sample from tangentspace L (TBN dot products already given) and transforming it into worldspace

HOWEVER:

  1. in isotropic BRDF usage, the TdotL and BdotL may be thrown away
  2. when derivatives and covariance matrices come, taking worldspace ray-dir info and doint 3 dot products will be cheaper

Also the NdotL computation could be overwritten by 2.f*cache.getVdotH()*localH.z - localV.z if you want (the quantitiy you check to see if the sample has invalid path).


// fail if samples have invalid paths
if (localL.getDirection().z < scalar_type(0.0)) // NdotL<0
localL.makeInvalid(); // should check if sample direction is invalid
Comment on lines +194 to +196
Copy link
Member Author

Choose a reason for hiding this comment

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

you can check this before reflecting the V into L, simply compute 2.f*cache.getVdotH()*localH.z > localV.z to proceed with returning a valid sample (otherwise return invalid sample without writing anything to it)

Copy link
Member Author

@devshgraphicsprogramming devshgraphicsprogramming Oct 3, 2025

Choose a reason for hiding this comment

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

btw a compiler's Common Subexpression Elimination pass should re-use the already computed 2.f*cache.getVdotH() later on when actually codegenning the reflection

may want to leave that as a comment before the if-statement


return sample_type::createFromTangentSpace(localL, interaction.getFromTangentSpace());
}
Comment on lines +172 to +199
Copy link
Member Author

Choose a reason for hiding this comment

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

btw you're not checking at all that the NdotV>minimum which will lead you to call the generateH function with samples below the hemisphere

this in turn for a rough NDF will result in some localL.getDirection() in the upper hemisphere (reflection off a microfacet in the upward direction) and not failing

Copy link
Member Author

@devshgraphicsprogramming devshgraphicsprogramming Oct 3, 2025

Choose a reason for hiding this comment

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

after you throw away !(NdotV>minimum) samples, you can assert(cache.getVdotH()>=0.f) as soon as its known with a comment that we require VNDF sampling

template<typename C=bool_constant<IsBSDF> >
enable_if_t<C::value && IsBSDF, sample_type> generate(NBL_CONST_REF_ARG(anisotropic_interaction_type) interaction, const vector3_type u, NBL_REF_ARG(anisocache_type) cache)
{
fresnel_type _f = impl::getOrientedFresnel<fresnel_type, IsBSDF>::__call(fresnel, interaction.getNdotV());
fresnel::OrientedEtaRcps<monochrome_type> rcpEta = _f.getOrientedEtaRcps();

ray_dir_info_type V = interaction.getV();
Copy link
Member Author

Choose a reason for hiding this comment

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

you don't need this till much later

const vector3_type localV = interaction.getTangentSpaceV();
const vector3_type upperHemisphereV = ieee754::flipSignIfRHSNegative<vector3_type>(localV, hlsl::promote<vector3_type>(interaction.getNdotV()));
Comment on lines +203 to +208
Copy link
Member Author

Choose a reason for hiding this comment

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

assign interaction.getNdotV() to a const variable, or better use localV.z in its stead

const vector3_type localH = ndf.generateH(upperHemisphereV, u.xy);
const vector3_type H = hlsl::mul(interaction.getFromTangentSpace(), localH);

const scalar_type VdotH = hlsl::dot(V.getDirection(), H);
Copy link
Member Author

Choose a reason for hiding this comment

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

compute this as dot(localV,localH) before computing H, it will shorten the instruction dependency cycle

Copy link
Member Author

@devshgraphicsprogramming devshgraphicsprogramming Oct 3, 2025

Choose a reason for hiding this comment

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

also as soon as you know VdotH you can check its sign's consistency with NdotV, and reject the sample if it hits a microfacet from a different side than the surface

You can assert it, VNDF sampling should guarantee this

const scalar_type reflectance = _f(hlsl::abs(VdotH))[0];

scalar_type rcpChoiceProb;
scalar_type z = u.z;
bool transmitted = math::partitionRandVariable(reflectance, z, rcpChoiceProb);

Refract<scalar_type> r = Refract<scalar_type>::create(V.getDirection(), H);
bxdf::ReflectRefract<scalar_type> rr;
rr.refract = r;
ray_dir_info_type L = V.reflectRefract(rr, transmitted, rcpEta.value[0]);
Comment on lines +219 to +222
Copy link
Member Author

Choose a reason for hiding this comment

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

use a wrapper same as reflect to override operator()(const bool doRefract, const scalar_type rcpOrientedEta) into actually calling operator()(const scalar_type NdotTorR, const scalar_type rcpOrientedEta) which the first parameter coming from cache.LdotH (N.B. the NdotI internal computation will CSE with your VdotH computation)


const vector3_type T = interaction.getT();
const vector3_type B = interaction.getB();
const vector3_type _N = interaction.getN();

// fail if samples have invalid paths
const vector3_type Ldir = L.getDirection();
const scalar_type LdotH = hlsl::dot(Ldir, H);
if ((ComputeMicrofacetNormal<scalar_type>::isTransmissionPath(VdotH, LdotH) != transmitted) || (LdotH * hlsl::dot(_N, Ldir) < scalar_type(0.0)))
L.makeInvalid(); // should check if sample direction is invalid
Comment on lines +229 to +232
Copy link
Member Author

@devshgraphicsprogramming devshgraphicsprogramming Oct 3, 2025

Choose a reason for hiding this comment

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

you actually want to make the isotropic part of the cache first (before L is computed) cause it computes VdotH and LdotH by itself without knowing the full L

continues https://github.com/Devsh-Graphics-Programming/Nabla/pull/930/files?diff=split&w=1#r2356451287

Copy link
Member Author

@devshgraphicsprogramming devshgraphicsprogramming Oct 3, 2025

Choose a reason for hiding this comment

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

The LdotH and (this doesn't need to be checked, see next comment) NdotL sign consistency could be checked right after computing transmitted

Copy link
Member Author

Choose a reason for hiding this comment

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

You're also checking the wrong thing, ComputeMicrofacetNormal<scalar_type>::isTransmissionPath(VdotH, LdotH) != transmitted will always be true, why?

VNDF sampling guarantees that VdotH will have a sign consistent with NdotV, and transmitted controls the sign of LdotH relative to VdotH by construction (you either reflect and end up with the same sign, or you refract and you get the opposite sign)

So what you should really be checking is isTransmissionPath(NdotV,NdotL) != transmitted and the NdotL should be computed in a friendly way similarly to this https://github.com/Devsh-Graphics-Programming/Nabla/pull/930/files/4faecc38efe93237e48b795402451366eeb1c5ba..804014f28e7469036673169266a013f42e018f60?diff=unified&w=1#r2401939155

Copy link
Member Author

Choose a reason for hiding this comment

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

all the above means that the (LdotH * hlsl::dot(_N, Ldir) < scalar_type(0.0)) check is redundant because when you know that

isTransmissionPath(NdotV,VdotH) === false // VNDF

and

isTransmissionPath(VdotH,LdotH) === transmitted // VNDF + construction from ReflectRefract

for sure, then

 (LdotH * hlsl::dot(_N, Ldir) < scalar_type(0.0)) === (isTransmissionPath(NdotV,NdotL) != transmitted)

Copy link
Member Author

Choose a reason for hiding this comment

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

what you should assert() though after you pass the isTransmissionPath(NdotV,NdotL) != transmitted check, is that

ComputeMicrofacetNormal::isValidMicrofacet(transmitted,cache.VdotL,localH.z,orientedEta)==true

else
cache = anisocache_type::create(VdotH, Ldir, H, T, B, _N, transmitted);
Copy link
Member Author

@devshgraphicsprogramming devshgraphicsprogramming Oct 3, 2025

Choose a reason for hiding this comment

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

use this instead

static this_t create(
const vector3_type tangentSpaceV,
const vector3_type tangentSpaceH,
const bool transmitted,
NBL_CONST_REF_ARG(fresnel::OrientedEtaRcps<monochrome_type>) rcpOrientedEta
)


return sample_type::create(L, T, B, _N);
Copy link
Member Author

Choose a reason for hiding this comment

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

same as for the BRDF generation, NdotL can be overwriten by whatever you had to compute to check that L is in the correct hemisphere as per transmitted and the sign of VdotN

}
template<typename C=bool_constant<!IsAnisotropic>, typename D=bool_constant<!IsBSDF> >
enable_if_t<C::value && !IsAnisotropic && D::value && !IsBSDF, sample_type> generate(NBL_CONST_REF_ARG(isotropic_interaction_type) interaction, const vector2_type u, NBL_REF_ARG(isocache_type) cache)
{
anisocache_type aniso_cache;
sample_type s = generate(anisotropic_interaction_type::create(interaction), u, aniso_cache);
cache = aniso_cache.iso_cache;
return s;
}
template<typename C=bool_constant<!IsAnisotropic>, typename D=bool_constant<IsBSDF> >
enable_if_t<C::value && !IsAnisotropic && D::value && IsBSDF, sample_type> generate(NBL_CONST_REF_ARG(isotropic_interaction_type) interaction, const vector3_type u, NBL_REF_ARG(isocache_type) cache)
{
anisocache_type aniso_cache;
sample_type s = generate(anisotropic_interaction_type::create(interaction), u, aniso_cache);
cache = aniso_cache.iso_cache;
return s;
}
Comment on lines +238 to +253
Copy link
Member Author

Choose a reason for hiding this comment

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

you could have just had one templated on V used to type u instead of D


template<class Interaction, class MicrofacetCache>
scalar_type __pdf(NBL_CONST_REF_ARG(sample_type) _sample, NBL_CONST_REF_ARG(Interaction) interaction, NBL_CONST_REF_ARG(MicrofacetCache) cache)
{
using quant_query_type = typename ndf_type::quant_query_type;
using dg1_query_type = typename ndf_type::dg1_query_type;

dg1_query_type dq = ndf.template createDG1Query<Interaction, MicrofacetCache>(interaction, cache);

fresnel_type _f = impl::getOrientedFresnel<fresnel_type, IsBSDF>::__call(fresnel, interaction.getNdotV());
Copy link
Member Author

Choose a reason for hiding this comment

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

you only need to reorient fresnels for BSDFs, see https://github.com/Devsh-Graphics-Programming/Nabla/pull/930/files#r2401699310

quant_query_type qq = impl::quant_query_helper<ndf_type, fresnel_type, IsBSDF>::template __call<MicrofacetCache>(ndf, _f, cache);
quant_type DG1 = ndf.template DG1<sample_type, Interaction>(dq, qq, _sample, interaction);

NBL_IF_CONSTEXPR(IsBSDF)
{
const scalar_type reflectance = _f(hlsl::abs(cache.getVdotH()))[0];
return hlsl::mix(reflectance, scalar_type(1.0) - reflectance, cache.isTransmission()) * DG1.projectedLightMeasure;
}
else
{
return DG1.projectedLightMeasure;
}
}
template<typename C=bool_constant<!IsAnisotropic> >
enable_if_t<C::value && !IsAnisotropic, scalar_type> pdf(NBL_CONST_REF_ARG(sample_type) _sample, NBL_CONST_REF_ARG(isotropic_interaction_type) interaction, NBL_CONST_REF_ARG(isocache_type) cache)
{
if (IsBSDF || (_sample.getNdotL() > numeric_limits<scalar_type>::min && interaction.getNdotV() > numeric_limits<scalar_type>::min))
{
scalar_type _pdf = __pdf<isotropic_interaction_type, isocache_type>(_sample, interaction, cache);
return hlsl::mix(scalar_type(0.0), _pdf, _pdf < bit_cast<scalar_type>(numeric_limits<scalar_type>::infinity));
}
else
return scalar_type(0.0);
}
scalar_type pdf(NBL_CONST_REF_ARG(sample_type) _sample, NBL_CONST_REF_ARG(anisotropic_interaction_type) interaction, NBL_CONST_REF_ARG(anisocache_type) cache)
{
if (IsBSDF || (_sample.getNdotL() > numeric_limits<scalar_type>::min && interaction.getNdotV() > numeric_limits<scalar_type>::min))
Comment on lines +280 to +290
Copy link
Member Author

Choose a reason for hiding this comment

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

checks are not complete for a BSDF (same problem as eval), not checking for TIR invalid microfacet or incosistent transmission with microfacets.

I'd really abstract the whole if (IsBSDF || (_sample.getNdotL() > numeric_limits<scalar_type>::min && interaction.getNdotV() > numeric_limits<scalar_type>::min)) into a pseduo-private method (prefixed with __) to use for eval and pdf

Copy link
Member Author

Choose a reason for hiding this comment

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

{
scalar_type _pdf = __pdf<anisotropic_interaction_type, anisocache_type>(_sample, interaction, cache);
return hlsl::mix(scalar_type(0.0), _pdf, _pdf < bit_cast<scalar_type>(numeric_limits<scalar_type>::infinity));
}
else
return scalar_type(0.0);
}

template<class Interaction, class MicrofacetCache>
quotient_pdf_type __quotient_and_pdf(NBL_CONST_REF_ARG(sample_type) _sample, NBL_CONST_REF_ARG(Interaction) interaction, NBL_CONST_REF_ARG(MicrofacetCache) cache)
{
scalar_type _pdf = __pdf<Interaction, MicrofacetCache>(_sample, interaction, cache);
fresnel_type _f = impl::getOrientedFresnel<fresnel_type, IsBSDF>::__call(fresnel, interaction.getNdotV());
Copy link
Member Author

Choose a reason for hiding this comment

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

only BSDFs need to re-orient


spectral_type quo = hlsl::promote<spectral_type>(0.0);
const bool notTIR = impl::check_TIR_helper<fresnel_type, IsBSDF>::template __call<MicrofacetCache>(_f, cache);
if (notTIR)
Comment on lines +306 to +307
Copy link
Member Author

Choose a reason for hiding this comment

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

quotient_and_pdf is meant to be only called on samples we generated, so I'd turn these checks into asserts (also different thing needs to be asserted, see https://github.com/Devsh-Graphics-Programming/Nabla/pull/930/files#r2401699310 )

{
using g2g1_query_type = typename N::g2g1_query_type;
g2g1_query_type gq = ndf.template createG2G1Query<sample_type, Interaction>(_sample, interaction);
scalar_type G2_over_G1 = ndf.template G2_over_G1<sample_type, Interaction, MicrofacetCache>(gq, _sample, interaction, cache);
Comment on lines +310 to +311
Copy link
Member Author

@devshgraphicsprogramming devshgraphicsprogramming Oct 3, 2025

Choose a reason for hiding this comment

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

gq is not needed if the NDF is smooth, cause G2_over_G1 will approach 1

an NDF is smooth if PDF = INF

NBL_IF_CONSTEXPR(IsBSDF)
quo = hlsl::promote<spectral_type>(G2_over_G1);
else
quo = _f(cache.getVdotH()) * G2_over_G1;
Copy link
Member Author

Choose a reason for hiding this comment

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

assert that the !(VdotH<0.f) for certainty

}

// set pdf=0 when quo=0 because we don't want to give high weight to sampling strategy that yields 0 contribution
_pdf = hlsl::mix(_pdf, scalar_type(0.0), hlsl::all(quo < hlsl::promote<spectral_type>(numeric_limits<scalar_type>::min)));
Comment on lines +318 to +319
Copy link
Member Author

Choose a reason for hiding this comment

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

quotient will never be 0 after the _sample.valid() is checked (valid microfacetness can be asserted), its only when a sample is invalid that you need to return {0,0}

return quotient_pdf_type::create(quo, _pdf);
}
template<typename C=bool_constant<!IsAnisotropic> >
enable_if_t<C::value && !IsAnisotropic, quotient_pdf_type> quotient_and_pdf(NBL_CONST_REF_ARG(sample_type) _sample, NBL_CONST_REF_ARG(isotropic_interaction_type) interaction, NBL_CONST_REF_ARG(isocache_type) cache)
{
return __quotient_and_pdf<isotropic_interaction_type, isocache_type>(_sample, interaction, cache);
}
quotient_pdf_type quotient_and_pdf(NBL_CONST_REF_ARG(sample_type) _sample, NBL_CONST_REF_ARG(anisotropic_interaction_type) interaction, NBL_CONST_REF_ARG(anisocache_type) cache)
{
return __quotient_and_pdf<anisotropic_interaction_type, anisocache_type>(_sample, interaction, cache);
}

ndf_type ndf;
fresnel_type fresnel; // always front-facing
};

}
}
}

#endif
Loading
Loading