Skip to content

Hlsl bxdfs 3 #899

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

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

Hlsl bxdfs 3 #899

wants to merge 180 commits into from

Conversation

devshgraphicsprogramming
Copy link
Member

Description

Continuing #811 due to GH UI messing up diffs again

Testing

TODO list:

ser += T(-1.231739572450155) / ++y;
ser += T(0.1208650973866179e-2) / ++y;
ser += T(-0.5395239384953e-5) / ++y;
return -tmp + log_helper<T>::__call(T(2.5066282746310005) * ser / x);
Copy link
Member Author

Choose a reason for hiding this comment

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

we're using two logarithms here, I think the BTDF specialized Beta should just use Stirling's approximation and take care to throw out terms that cancel out and gather what can be gathered together

Comment on lines +248 to +249
NBL_PARTIAL_REQ_TOP(always_true<decltype(spirv::IEqual<Vectorial>(experimental::declval<Vectorial>(),experimental::declval<Vectorial>()))> && concepts::Vectorial<Vectorial> && concepts::Integral<Vectorial>)
struct equal_helper<Vectorial NBL_PARTIAL_REQ_BOT(always_true<decltype(spirv::IEqual<Vectorial>(experimental::declval<Vectorial>(),experimental::declval<Vectorial>()))> && concepts::Vectorial<Vectorial> && concepts::Integral<Vectorial>) >
Copy link
Member Author

Choose a reason for hiding this comment

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

to avoid clutter can't you make a spirv::IEqualISCallable ? same for the FOrdEqual

Comment on lines +44 to +47
#ifndef __HLSL_VERSION
template<typename T, typename U>
NBL_BOOL_CONCEPT MixIsCallable = always_true<decltype(glm::mix(declval<T>(), declval<T>(), declval<U>()))>;
#endif
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 said something about glm::mix being templated in a different way, so don't you need to call it with different template args ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just had to remove the explicit template args to glm::mix. Then I could call MixIsCallable<T,T> or MixIsCallable<T,U>.

template<typename T, typename U>
NBL_BOOL_CONCEPT MixCallingBuiltins =
#ifdef __HLSL_VERSION
spirv::FMixIsCallable<T> || spirv::SelectIsCallable<T,U>;
Copy link
Member Author

Choose a reason for hiding this comment

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

spirv::FMixIsCallable<T> ignores U, you need this to be is_same_v<T,U> && spirv::FMixIsCallable<T>

Comment on lines 448 to 463
requires (concepts::FloatingPointScalar<T> && (concepts::FloatingPointScalar<U> || concepts::BooleanScalar<U>))
requires (impl::MixIsCallable<T,U> /*&& (concepts::FloatingPointScalar<T> || concepts::FloatingPointVector<T>) && (concepts::FloatingPointScalar<U> || concepts::Boolean<U> || ((concepts::Vector<U> || concepts::FloatingPointVector<U>) && vector_traits<T>::Dimension==vector_traits<U>::Dimension))*/)
Copy link
Member Author

Choose a reason for hiding this comment

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

whats doing on with the comment ?

Comment on lines 179 to 182
vector_type operator()()
{
NdotI = hlsl::dot<vector_type>(N, I);
return N * 2.0f * getNdotI() - I;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

why isn't this return operator()(getNdotI()); ?

Comment on lines 226 to 290
scalar_type NdotI;
scalar_type rcpOrientedEta;
Copy link
Member Author

Choose a reason for hiding this comment

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

be consistent, either refract is allowed to keep the rcpOrientedEta, and then it lives in _refract for this struct, or both can't keep it

Comment on lines +76 to +83
scalar_type correlated(scalar_type a2, scalar_type NdotV2, scalar_type NdotL2)
{
scalar_type c2 = C2(NdotV2, a2);
scalar_type L_v = Lambda(c2);
c2 = C2(NdotL2, a2);
scalar_type L_l = Lambda(c2);
return G1(L_v + L_l);
}
Copy link
Member Author

@devshgraphicsprogramming devshgraphicsprogramming Jul 18, 2025

Choose a reason for hiding this comment

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

wait, isn't correlated 1/(1+lambda+lambda) ? something is off

ugh,. notatio n abuse please don't use G1 do shorthand scalar_type(1)/(scalar_type(1)+L_v+L_i) it just looks like a bug since correlated is called G2 in papers

);
}

scalar_type onePlusLambda_V;
Copy link
Member Author

Choose a reason for hiding this comment

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

why is this a member!?

Copy link
Member Author

Choose a reason for hiding this comment

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

rougness a2 is supposed to be a member!

Comment on lines +102 to +109
}
};

template<typename T>
NBL_PARTIAL_REQ_TOP(concepts::FloatingPointScalar<T>)
struct GGX<T,true NBL_PARTIAL_REQ_BOT(concepts::FloatingPointScalar<T>) >
{
using scalar_type = T;
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 keep things that don't vary with V and L as members, so roughness and things connected to it

a2, one_minus_a2 and so on

// trowbridge-reitz
scalar_type D(scalar_type a2, scalar_type NdotH2)
{
scalar_type denom = NdotH2 * (a2 - scalar_type(1.0)) + scalar_type(1.0);
Copy link
Member Author

Choose a reason for hiding this comment

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

this is scalar_type(1,0)-one_minus_a2*NdotH2

Comment on lines +34 to +45
scalar_type FVNDF(scalar_type fresnel_ndf, scalar_type G1_over_2NdotV, bool transmitted, scalar_type VdotH, scalar_type LdotH, scalar_type VdotHLdotH, scalar_type orientedEta)
{
scalar_type FNG = fresnel_ndf * G1_over_2NdotV;
scalar_type factor = scalar_type(0.5);
if (transmitted)
{
const scalar_type VdotH_etaLdotH = (VdotH + orientedEta * LdotH);
// VdotHLdotH is negative under transmission, so this factor is negative
factor *= -scalar_type(2.0) * VdotHLdotH / (VdotH_etaLdotH * VdotH_etaLdotH);
}
return FNG * factor;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

there's no need to have FVNDF in any NDF as the Fresnel is a completely orthogonal concept that doesn't cancel out with anything.

So just VNDF is needed, which is called DG1

Comment on lines +47 to +50
scalar_type DG1(scalar_type ndf, scalar_type G1_over_2NdotV)
{
return ndf * scalar_type(0.5) * G1_over_2NdotV;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

a reflective specialized overload shouldn't have its own code to implement, it should call the general function with constants and dummies

const scalar_type dummy = 0.f;
return DG1(ndf,G1_over2NdotV,false,dummy,dummy,dummy,dummy,dummy);

Comment on lines +54 to +55
scalar_type FN = hlsl::mix(reflectance, scalar_type(1.0) - reflectance, transmitted) * ndf;
return FVNDF(FN, G1_over_2NdotV, transmitted, VdotH, LdotH, VdotHLdotH, orientedEta);
Copy link
Member Author

Choose a reason for hiding this comment

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

DG1 doesn't care about fresnel, so reflectance should be omitted

Comment on lines 19 to 20
template<typename T, bool IsAnisotropic=false NBL_STRUCT_CONSTRAINABLE>
struct Beckmann;
Copy link
Member Author

Choose a reason for hiding this comment

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

these should be in the headers of the ndfs

Comment on lines 25 to 33
// common
namespace impl
{
template<class T, class U>
struct is_ggx : bool_constant<
is_same<T, GGX<U> >::value
is_same<T, GGX<U,false> >::value ||
is_same<T, GGX<U,true> >::value
> {};
}
Copy link
Member Author

Choose a reason for hiding this comment

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

this should be in ndf/ggx.hls


scalar_type D(scalar_type a2, scalar_type NdotH2)
{
scalar_type nom = exp<scalar_type>( (NdotH2 - scalar_type(1.0)) / (a2 * NdotH2) ); // exp(x) == exp2(x/log(2)) ?
Copy link
Member Author

Choose a reason for hiding this comment

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

continue #899 (comment)


// TODO: get beta from lgamma, see: https://www.cec.uchile.cl/cinetica/pcordero/MC_libros/NumericalRecipesinC.pdf

// TODO: use query_type for D, lambda, beta, DG1 when that's implemented
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 may need different query_type for different functions, because

D - only carea about half vector and normal dot products
DG1 - cares about D plus View vector dot products
G1 - only about lambda
Lambda - only about dot product of View or Light vector with normal
G2 over G1 - only about view and light sample lambdas

Comment on lines +99 to +172
template<typename T>
NBL_PARTIAL_REQ_TOP(concepts::FloatingPointScalar<T>)
struct Beckmann<T,true NBL_PARTIAL_REQ_BOT(concepts::FloatingPointScalar<T>) >
{
using scalar_type = T;

scalar_type D(scalar_type ax, scalar_type ay, scalar_type ax2, scalar_type ay2, scalar_type TdotH2, scalar_type BdotH2, scalar_type NdotH2)
{
scalar_type nom = exp<scalar_type>(-(TdotH2 / ax2 + BdotH2 / ay2) / NdotH2);
scalar_type denom = ax * ay * NdotH2 * NdotH2;
return numbers::inv_pi<scalar_type> * nom / denom;
}

scalar_type DG1(scalar_type ndf, scalar_type maxNdotV, scalar_type lambda_V)
{
Beckmann<T,false> beckmann;
scalar_type dg = beckmann.DG1(ndf, maxNdotV, lambda_V);
onePlusLambda_V = beckmann.onePlusLambda_V;
return dg;
}

scalar_type DG1(scalar_type ndf, scalar_type absNdotV, scalar_type lambda_V, bool transmitted, scalar_type VdotH, scalar_type LdotH, scalar_type VdotHLdotH, scalar_type orientedEta, scalar_type reflectance)
{
Beckmann<T,false> beckmann;
scalar_type dg = beckmann.DG1(ndf, absNdotV, lambda_V, transmitted, VdotH, LdotH, VdotHLdotH, orientedEta, reflectance);
onePlusLambda_V = beckmann.onePlusLambda_V;
return dg;
}

scalar_type G1(scalar_type lambda)
{
return scalar_type(1.0) / (scalar_type(1.0) + lambda);
}

scalar_type C2(scalar_type TdotX2, scalar_type BdotX2, scalar_type NdotX2, scalar_type ax2, scalar_type ay2)
{
return NdotX2 / (TdotX2 * ax2 + BdotX2 * ay2);
}

scalar_type Lambda(scalar_type c2)
{
scalar_type c = sqrt<scalar_type>(c2);
scalar_type nom = scalar_type(1.0) - scalar_type(1.259) * c + scalar_type(0.396) * c2;
scalar_type denom = scalar_type(2.181) * c2 + scalar_type(3.535) * c;
return hlsl::mix<scalar_type>(scalar_type(0.0), nom / denom, c < scalar_type(1.6));
}

scalar_type Lambda(scalar_type TdotX2, scalar_type BdotX2, scalar_type NdotX2, scalar_type ax2, scalar_type ay2)
{
return Lambda(C2(TdotX2, BdotX2, NdotX2, ax2, ay2));
}

scalar_type correlated(scalar_type ax2, scalar_type ay2, scalar_type TdotV2, scalar_type BdotV2, scalar_type NdotV2, scalar_type TdotL2, scalar_type BdotL2, scalar_type NdotL2)
{
scalar_type c2 = C2(TdotV2, BdotV2, NdotV2, ax2, ay2);
scalar_type L_v = Lambda(c2);
c2 = C2(TdotL2, BdotL2, NdotL2, ax2, ay2);
scalar_type L_l = Lambda(c2);
return G1(L_v + L_l);
}

scalar_type G2_over_G1(scalar_type ax2, scalar_type ay2, bool transmitted, scalar_type TdotL2, scalar_type BdotL2, scalar_type NdotL2, scalar_type lambdaV_plus_one)
{
scalar_type c2 = C2(TdotL2, BdotL2, NdotL2, ax2, ay2);
scalar_type lambdaL = Lambda(c2);
return hlsl::mix(
lambdaV_plus_one / (lambdaV_plus_one + lambdaL),
lambdaV_plus_one * hlsl::beta<scalar_type>(lambdaV_plus_one, scalar_type(1.0) + lambdaL),
transmitted
);
}

scalar_type onePlusLambda_V;
};
Copy link
Member Author

@devshgraphicsprogramming devshgraphicsprogramming Jul 18, 2025

Choose a reason for hiding this comment

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

in the ndf header make a concept (what functions each ndf is supposed to have)

And document the measures in which the methods are, because right now you're relying on me knowing that:

  • D is in Projected Half vector (microfacet)
  • DG1 is in Projected Solid Angle of the reflected or refracted light vector
  • G1 is agnostic
  • G2_over_G1 is same as G1

Comment on lines +58 to +71
scalar_type devsh_part(scalar_type NdotX2, scalar_type a2, scalar_type one_minus_a2)
{
return sqrt(a2 + one_minus_a2 * NdotX2);
}

scalar_type G1_wo_numerator(scalar_type NdotX, scalar_type NdotX2, scalar_type a2, scalar_type one_minus_a2)
{
return scalar_type(1.0) / (NdotX + devsh_part(NdotX2,a2,one_minus_a2));
}

scalar_type G1_wo_numerator(scalar_type NdotX, scalar_type devsh_part)
{
return scalar_type(1.0) / (NdotX + devsh_part);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

this thing definitely needs an argument struct

Comment on lines +88 to +92
return hlsl::mix(
lambdaV_plus_one / (lambdaV_plus_one + lambdaL),
lambdaV_plus_one * hlsl::beta<scalar_type>(lambdaV_plus_one, scalar_type(1.0) + lambdaL),
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 should do lambdaV_plus_one * hlsl::mix(reciprocal(lambda_V_plus_one+lambdaL),beta(),transmitted)

Comment on lines +73 to +81
scalar_type correlated_wo_numerator(scalar_type a2, scalar_type NdotV, scalar_type NdotV2, scalar_type NdotL, scalar_type NdotL2)
{
scalar_type one_minus_a2 = scalar_type(1.0) - a2;
scalar_type Vterm = NdotL * devsh_part(NdotV2, a2, one_minus_a2);
scalar_type Lterm = NdotV * devsh_part(NdotL2, a2, one_minus_a2);
return scalar_type(0.5) / (Vterm + Lterm);
}

scalar_type G2_over_G1(scalar_type a2, bool transmitted, scalar_type NdotV, scalar_type NdotV2, scalar_type NdotL, scalar_type NdotL2)
Copy link
Member Author

Choose a reason for hiding this comment

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

variable naming is important, when I see NdotV I think I'm allowed to feed negative dot product inside, which is probably not true and you want absNdotV

Also pepper your code with asserts like a2>=numeric_limits<scalar_type>::min and so on, because some of these functions blow up at certain singularities

Copy link
Member Author

Choose a reason for hiding this comment

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

also add overloads of G2_over_G1 for the reflection only case

Comment on lines +84 to +99
if (transmitted)
{
scalar_type one_minus_a2 = scalar_type(1.0) - a2;
scalar_type devsh_v = devsh_part(NdotV2, a2, one_minus_a2);
scalar_type L_v = scalar_type(0.5) * (devsh_v / NdotV - scalar_type(1.0));
scalar_type L_l = scalar_type(0.5) * (devsh_part(NdotL2, a2, one_minus_a2) / NdotL - scalar_type(1.0));
G2_over_G1 = hlsl::beta<scalar_type>(scalar_type(1.0) + L_l, scalar_type(1.0) + L_v);
G2_over_G1 *= scalar_type(1.0) + L_v;
}
else
{
scalar_type one_minus_a2 = scalar_type(1.0) - a2;
scalar_type devsh_v = devsh_part(NdotV2, a2, one_minus_a2);
G2_over_G1 = NdotL * (devsh_v + NdotV); // alternative `Vterm+NdotL*NdotV /// NdotL*NdotV could come as a parameter
G2_over_G1 /= NdotV * devsh_part(NdotL2, a2, one_minus_a2) + NdotL * devsh_v;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

one_minus_a2 is common (and should be a member)

devsh_v and devsh_l are the same for both branches and should be hoisted (or could come from the outside then you don't need the V2 and L2 squares)

Comment on lines +88 to +90
scalar_type L_v = scalar_type(0.5) * (devsh_v / NdotV - scalar_type(1.0));
scalar_type L_l = scalar_type(0.5) * (devsh_part(NdotL2, a2, one_minus_a2) / NdotL - scalar_type(1.0));
G2_over_G1 = hlsl::beta<scalar_type>(scalar_type(1.0) + L_l, scalar_type(1.0) + L_v);
Copy link
Member Author

Choose a reason for hiding this comment

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

if you're gonna add 1 to L_v and L_l, then just compute them right away turn that -1 into a +1

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 because of what division by NdotV and NdotlL does to the numerical stability, you need to check in this branch that they're both above some small threshold, otherwise return 0 before even calling any of the other functions

Comment on lines 175 to 257
static this_t create(NBL_CONST_REF_ARG(vector_type) I, NBL_CONST_REF_ARG(vector_type) N, scalar_type NdotI, scalar_type rcpOrientedEta)
static this_t create(NBL_CONST_REF_ARG(vector_type) I, NBL_CONST_REF_ARG(vector_type) N, scalar_type rcpOrientedEta)
{
this_t retval;
retval.I = I;
retval.N = N;
retval.NdotI = NdotI;
retval._refract = Refract<scalar_type>::create(I, N);
retval.rcpOrientedEta = rcpOrientedEta;
return retval;
}

static this_t create(NBL_CONST_REF_ARG(Refract<scalar_type>) refract, scalar_type rcpOrientedEta)
{
this_t retval;
retval.I = refract.I;
retval.N = refract.N;
retval.NdotI = refract.NdotI;
retval._refract = refract;
retval.rcpOrientedEta = rcpOrientedEta;
return retval;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

both are useless, siince I can just fill out the member myself

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.

2 participants