-
Notifications
You must be signed in to change notification settings - Fork 65
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
base: master
Are you sure you want to change the base?
Hlsl bxdfs 3 #899
Conversation
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); |
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.
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
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>) > |
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.
to avoid clutter can't you make a spirv::IEqualISCallable
? same for the FOrdEqual
#ifndef __HLSL_VERSION | ||
template<typename T, typename U> | ||
NBL_BOOL_CONCEPT MixIsCallable = always_true<decltype(glm::mix(declval<T>(), declval<T>(), declval<U>()))>; | ||
#endif |
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.
you said something about glm::mix
being templated in a different way, so don't you need to call it with different template args ?
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 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>; |
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.
spirv::FMixIsCallable<T>
ignores U, you need this to be is_same_v<T,U> && spirv::FMixIsCallable<T>
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))*/) |
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.
whats doing on with the comment ?
vector_type operator()() | ||
{ | ||
NdotI = hlsl::dot<vector_type>(N, I); | ||
return N * 2.0f * getNdotI() - I; | ||
} |
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.
why isn't this return operator()(getNdotI());
?
scalar_type NdotI; | ||
scalar_type rcpOrientedEta; |
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.
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
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); | ||
} |
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.
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; |
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.
why is this a member!?
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.
rougness a2
is supposed to be a member!
} | ||
}; | ||
|
||
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; |
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.
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); |
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.
this is scalar_type(1,0)-one_minus_a2*NdotH2
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; | ||
} |
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.
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
scalar_type DG1(scalar_type ndf, scalar_type G1_over_2NdotV) | ||
{ | ||
return ndf * scalar_type(0.5) * G1_over_2NdotV; | ||
} |
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.
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);
scalar_type FN = hlsl::mix(reflectance, scalar_type(1.0) - reflectance, transmitted) * ndf; | ||
return FVNDF(FN, G1_over_2NdotV, transmitted, VdotH, LdotH, VdotHLdotH, orientedEta); |
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.
DG1 doesn't care about fresnel, so reflectance should be omitted
template<typename T, bool IsAnisotropic=false NBL_STRUCT_CONSTRAINABLE> | ||
struct Beckmann; |
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.
these should be in the headers of the ndfs
// 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 | ||
> {}; | ||
} |
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.
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)) ? |
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.
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 |
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.
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
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; | ||
}; |
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.
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
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); | ||
} |
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.
this thing definitely needs an argument struct
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 | ||
); |
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.
you should do lambdaV_plus_one * hlsl::mix(reciprocal(lambda_V_plus_one+lambdaL),beta(),transmitted)
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) |
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.
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
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.
also add overloads of G2_over_G1
for the reflection only case
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; | ||
} |
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.
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)
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); |
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.
if you're gonna add 1 to L_v and L_l, then just compute them right away turn that -1 into a +1
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.
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
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; | ||
} |
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.
both are useless, siince I can just fill out the member myself
Description
Continuing #811 due to GH UI messing up diffs again
Testing
TODO list: