-
Notifications
You must be signed in to change notification settings - Fork 66
Bxdf fixes cook torrance #930
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?
Conversation
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 cache.isValid(fresnel.getRefractionOrientedEta()); | ||
} | ||
}; |
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 think its possible to move these into NBL_IUF_CONSTEXPR
if you just made a struct which just concerns itself with
scalar_type orientedEta = impl::get_orientedEta_helper(fresnel); // return `fresnel.getRefractionOrientedEta()` if `SupportsTransmission` requiring a twosided fresnel, otherwise return uninitialized var
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.
well your getOrientedFrensel
already does what my suggested getOrientedEta_helper
should do, (minus a call to getRefractionOrientedEta()
NBL_IF_CONSTEXPR(IsBSDF) | ||
{ | ||
_f = impl::getOrientedFresnel<fresnel_type, IsBSDF>::__call(fresnel, interaction.getNdotV()); | ||
valid = impl::check_TIR_helper<fresnel_type, IsBSDF>::template __call<MicrofacetCache>(_f, cache); |
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 don't need check_TIR_helper
, just call the cache.isValid()
directly
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.
Do a getter for orientedEta and reoriented fresnel, cause thats the thing that actually differs in codegen, not for calling cache.isValid()
scalar_type dummy; | ||
quant_query_type qq = ndf.template createQuantQuery<MicrofacetCache>(cache, dummy); |
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 the dummy in place of?
ray_dir_info_type invalidL; | ||
invalidL.makeInvalid(); | ||
return sample_type::createFromTangentSpace(invalidL, interaction.getFromTangentSpace()); |
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 probably a faster way to make an invalid sample, require a createInvalid
factory
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) | ||
{ | ||
if (interaction.getNdotV() > numeric_limits<scalar_type>::min) |
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.
either rewrite the if
so its >
and put the valid case code inside, or assert that getNdotV
is not NaN, as it stands a NaN will proceed
L.makeInvalid(); // should check if sample direction is invalid | ||
|
||
const vector3_type T = interaction.getT(); | ||
const vector3_type B = interaction.getB(); | ||
const vector3_type _N = interaction.getN(); | ||
|
||
return sample_type::create(L, T, B, _N); |
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 should really be just a sample_type::createInvalid
and this vode with TBN should go into the valid if-case
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 the NdotL computation should be overwritten by 2.f*cache.getVdotH()*localH.z - localV.z
which you compute for the if-statement (the quantitiy you check to see if the sample has invalid path).
ray_dir_info_type L; | ||
if (scalar_type(2.0) * VdotH * localH.z > localV.z) // NdotL>0, compiler's Common Subexpression Elimination pass should re-use 2*VdotH later | ||
{ | ||
assert(VdotH >= scalar_type(0.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.
you can asset this as soon as VdotH
is known with a comment about VNDF sampling
// fail if samples have invalid paths | ||
const scalar_type NdotL = scalar_type(2.0) * VdotH * localH.z - localV.z; | ||
if ((ComputeMicrofacetNormal<scalar_type>::isTransmissionPath(NdotV, NdotL) != 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.
hoist this check all the way to when VdotH
and transmitted
is first calculated
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.
would be good to record all this in a comment #930 (comment)
const vector3_type upperHemisphereV = ieee754::flipSignIfRHSNegative<vector3_type>(localV, hlsl::promote<vector3_type>(NdotV)); | ||
const vector3_type localH = ndf.generateH(upperHemisphereV, u.xy); | ||
const scalar_type VdotH = hlsl::dot(localV, localH); | ||
const vector3_type H = hlsl::mul(interaction.getFromTangentSpace(), localH); |
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.
push calculation till last moment
const vector3_type B = interaction.getB(); | ||
|
||
// fail if samples have invalid paths | ||
const scalar_type NdotL = scalar_type(2.0) * VdotH * localH.z - localV.z; |
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 not a valid computation of NdotL for the BSDF, this is the reflection equation
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 follow
Nabla/include/nbl/builtin/hlsl/bxdf/fresnel.hlsl
Lines 273 to 281 in 069f499
vector_type operator()(const bool doRefract, const scalar_type rcpOrientedEta, NBL_REF_ARG(scalar_type) out_IdotTorR) NBL_CONST_MEMBER_FUNC | |
{ | |
scalar_type NdotI = getNdotR(); | |
const scalar_type a = hlsl::mix<scalar_type>(1.0f, rcpOrientedEta, doRefract); | |
const scalar_type b = NdotI * a + getNdotTorR(doRefract, rcpOrientedEta); | |
// assuming `I` is normalized | |
out_IdotTorR = NdotI * b - a; | |
return refract.N * b - refract.I * a; | |
} |
then using the NdotH==localH.z
, NdotV
and the LdotH
you already compute for cache.isValid()
you can compute NdotL
as:
const float viewShortenFactor = hlsl::mix<scalar_type>(1.0f, rcpOrientedEta, transmitted);
const float NdotL = localH.z * (VdotH*viewShortenFactor + LdotH) - NdotV * viewShortenFactor;
const vector3_type _N = interaction.getN(); | ||
Refract<scalar_type> r = Refract<scalar_type>::create(V.getDirection(), H); | ||
const scalar_type LdotH = hlsl::mix(VdotH, r.getNdotT(rcpEta.value2[0]), transmitted); | ||
cache = anisocache_type::createPartial(VdotH, LdotH, hlsl::dot(_N, H), transmitted, rcpEta); |
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 don't need to compute dot(_N,H)
its literally localH.z
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 assert that the cache.isValid()
right away after this (it should be, by construction)
return rr(NdotTorR, rcpOrientedEta); | ||
} | ||
bxdf::ReflectRefract<scalar_type> rr; |
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.
leave a comment that the call rr
makes to getNdotTorR
and mix
as well as a good part of the comuptations should CSE with our computation of NdotL
(which should be hoisted above the reflect/refract code)
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) |
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 think you can collapse the two methods if you use a REQUIRES(RequiredInteraction<Interaction> && RequiredCache<Cache>)
similarly for other methods
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); | ||
} |
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 could get rid of simple passthrough if you put a REQUIRES
on the __quotient_and_pdf
and removed the __
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)) | ||
{ | ||
using g2g1_query_type = typename ndf_type::g2g1_query_type; | ||
g2g1_query_type gq = ndf.template createG2G1Query<sample_type, Interaction>(_sample, interaction); | ||
DG *= ndf.template correlated<sample_type, Interaction>(gq, _sample, interaction); | ||
} |
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.
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
you could even add an NBL_REF_ARG(bool)
to the D
signature for a boolean side-channel of isInf
because it pays for the NDF to set that boolean as soon as possible (before a chain or multiplies and divides, compiler won't hoiust your INF checks before divisions by measure factors, etc.)
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)) | ||
{ | ||
using g2g1_query_type = typename ndf_type::g2g1_query_type; | ||
g2g1_query_type gq = ndf.template createG2G1Query<sample_type, Interaction>(_sample, interaction); | ||
DG *= ndf.template correlated<sample_type, Interaction>(gq, _sample, interaction); |
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 must let NDF provide a Dcorrelated
method, and if detected, use that instead of this, because we miss an optimization opportunity with GGX
We could actually do this in the following way
using quant_query_type = typename ndf_type::quant_query_type;
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))
{
using g2g1_query_type = typename ndf_type::g2g1_query_type;
g2g1_query_type gq = ndf.template createG2G1Query<sample_type, Interaction>(_sample, interaction);
DG *= ndf.template correlated<sample_type, Interaction>(gq, _sample, interaction);
}
// overwrites DG with NDF's `Dcorrelated` method, also `asserts` that the optimal method returns similar to above
impl::overwrite_DG<ndf_type,sample_type,Interaction,Cache>(/*NBL_REF_ARG*/DG,ndf,...);
continues #930 (comment)
using scalar_type = T; | ||
|
||
scalar_type getNdf() NBL_CONST_MEMBER_FUNC { return ndf; } | ||
scalar_type getG1over2NdotV() NBL_CONST_MEMBER_FUNC { return 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.
want to either comment or stick it in the name or the Query concept comment that its the wo_numerator
static scalar_type DG1(NBL_CONST_REF_ARG(Query) query) | ||
{ | ||
return scalar_type(0.5) * query.getNdf() * query.getG1over2NdotV(); |
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.
Query is not G1, this kinda seems to not make sense to me as a thing to return from DG1, I'm expecting NDF measure times the real G1 from a function called DG1
A rename might be in order, into DG1over4NdotV
?
…back into OrientedEta struct
retval.iso_cache.VdotL = hlsl::mix(scalar_type(2.0) * retval.iso_cache.VdotH * retval.iso_cache.VdotH - scalar_type(1.0), | ||
VdotH * (LdotH - rcpOrientedEta.value[0] + VdotH * rcpOrientedEta.value[0]), 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.
there's a more graceful way to compute this via mix
, see my other comment
} | ||
|
||
OrientedEtas<T> getRefractionOrientedEta() NBL_CONST_MEMBER_FUNC { return orientedEta; } | ||
scalar_type getRefractionOrientedEta() NBL_CONST_MEMBER_FUNC { return orientedEta.value[0]; } // expect T to be monochrome? |
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.
its a bit of a complicated topic, its possible to have RGB fresnel without dispersion by fixing the refraction Eta to be something else than the etas used to compute RGB reflectance or some sort of interpolation of them.
This is a good default cause the Etas wont differ that much anyway
fresnel::OrientedEtas<vector_type> orientedEta = fresnel::OrientedEtas<vector_type>::create(typename F::scalar_type(1.0), hlsl::promote<vector_type>(fresnel.getRefractionOrientedEta())); | ||
return cache.isValid(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.
why are you promoting to a vector!? Cache.isValid
MUST take the Eta as a scalar!!!!
template<class Interaction, class MicrofacetCache, typename C=bool_constant<!IsAnisotropic> NBL_FUNC_REQUIRES(RequiredInteraction<Interaction> && RequiredMicrofacetCache<MicrofacetCache>) | ||
enable_if_t<C::value && !IsAnisotropic, dg1_query_type> createDG1Query(NBL_CONST_REF_ARG(Interaction) interaction, NBL_CONST_REF_ARG(MicrofacetCache) cache) | ||
{ | ||
dg1_query_type dg1_query; | ||
dg1_query.ndf = __ndf_base.template D<MicrofacetCache>(cache); | ||
scalar_type clampedNdotV = interaction.getNdotV(_clamp); | ||
dg1_query.G1_over_2NdotV = base_type::G1_wo_numerator_devsh_part(clampedNdotV, __ndf_base.devsh_part(interaction.getNdotV2())); | ||
return dg1_query; | ||
} | ||
template<class LS, class Interaction, typename C=bool_constant<!IsAnisotropic> NBL_FUNC_REQUIRES(LightSample<LS> && RequiredInteraction<Interaction>) | ||
enable_if_t<C::value && !IsAnisotropic, g2g1_query_type> createG2G1Query(NBL_CONST_REF_ARG(LS) _sample, NBL_CONST_REF_ARG(Interaction) interaction) | ||
{ | ||
g2g1_query_type g2_query; | ||
g2_query.devsh_l = __ndf_base.devsh_part(_sample.getNdotL2()); | ||
g2_query.devsh_v = __ndf_base.devsh_part(interaction.getNdotV2()); | ||
return g2_query; | ||
} | ||
template<class Interaction, class MicrofacetCache, typename C=bool_constant<IsAnisotropic> NBL_FUNC_REQUIRES(RequiredInteraction<Interaction> && RequiredMicrofacetCache<MicrofacetCache>) | ||
enable_if_t<C::value && IsAnisotropic, dg1_query_type> createDG1Query(NBL_CONST_REF_ARG(Interaction) interaction, NBL_CONST_REF_ARG(MicrofacetCache) cache) | ||
{ | ||
dg1_query_type dg1_query; | ||
dg1_query.ndf = __ndf_base.template D<MicrofacetCache>(cache); | ||
scalar_type clampedNdotV = interaction.getNdotV(_clamp); | ||
dg1_query.G1_over_2NdotV = base_type::G1_wo_numerator_devsh_part(clampedNdotV, __ndf_base.devsh_part(interaction.getTdotV2(), interaction.getBdotV2(), interaction.getNdotV2())); | ||
return dg1_query; | ||
} |
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 made createDG1Query
use createG2G1Query
and grab the devsh_v
from the return value you could get away with only having one createDG1Query
method I think
template<class LS, class Interaction NBL_FUNC_REQUIRES(LightSample<LS> && RequiredInteraction<Interaction>) | ||
scalar_type correlated(NBL_CONST_REF_ARG(g2g1_query_type) query, NBL_CONST_REF_ARG(LS) _sample, NBL_CONST_REF_ARG(Interaction) interaction) | ||
{ | ||
return base_type::template correlated_wo_numerator<g2g1_query_type, LS, Interaction>(query, _sample, interaction); | ||
} | ||
|
||
template<class LS, class Interaction, class MicrofacetCache NBL_FUNC_REQUIRES(LightSample<LS> && RequiredInteraction<Interaction> && RequiredMicrofacetCache<MicrofacetCache>) | ||
scalar_type G2_over_G1(NBL_CONST_REF_ARG(g2g1_query_type) query, NBL_CONST_REF_ARG(LS) _sample, NBL_CONST_REF_ARG(Interaction) interaction, NBL_CONST_REF_ARG(MicrofacetCache) cache) | ||
{ | ||
return base_type::template G2_over_G1<g2g1_query_type, LS, Interaction, MicrofacetCache>(query, _sample, interaction, cache); | ||
} |
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.
as pointed out, they're just passthroughs, could implement the methords here cause they're identical between isotropic and anisotropic when done in terms of queries
dmq.microfacetMeasure = d; | ||
dmq.projectedLightMeasure = d * _sample.getNdotL(BxDFClampMode::BCM_MAX); |
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 wrong, the projectedLightMeasure
needs a full transform same as 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.
because its the PDF of V getting reflected through H which has a PDF of microfacetmeasure
{ | ||
scalar_type d = __ndf_base.template D<MicrofacetCache>(cache); | ||
quant_type dmq; | ||
dmq.microfacetMeasure = d; // note: microfacetMeasure/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.
huh? then the method shouldn't be called D
but D_over_2NdotV
but the comment is wrong, cause your __ndf_base.D
is actually the NDF, not anythingelse
scalar_type NdotL_over_denominator = _sample.getNdotL(BxDFClampMode::BCM_ABS); | ||
if (transmitted) | ||
NdotL_over_denominator *= -scalar_type(4.0) * VdotHLdotH / (VdotH_etaLdotH * VdotH_etaLdotH); | ||
dmq.projectedLightMeasure = d * NdotL_over_denominator; |
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.
wrong the denominator is 4 NdotV NdotL
, so NdotL over denominator must be 0.25 / NdotV(ABS)
Description
Continues #899 , #916 and #919
Testing
TODO list: