-
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?
Changes from 3 commits
5218545
f7525af
c50db68
c49bedb
319c954
4eeacf1
e7bc784
72226bb
f34b348
ad13044
d8d2116
238b08e
8bc707f
9fb28cf
4cb8a0a
392dc31
02de86e
4a7f532
80c4f67
6233bd1
ce5fbac
47e814b
c983975
0f2ee0b
b5f02e6
a111415
9655049
a1743d2
e6d663b
340cee3
a3733b1
3e3589b
4bdf199
4faecc3
407da2f
638b8b5
c587820
d438360
5f49f11
627074b
3896231
91b39d5
2a3cda3
c9f9366
fd128e6
882375e
b22d570
c0586f5
9eeb248
26c76e4
58e2a0b
2a08728
b993e47
f3cb6ff
7389c9a
ec5913b
bccdb0b
1e9e407
dab51c3
53ab934
804014f
260f7a3
fd54ac4
5caf006
7508b82
2b4a9c1
d3fa872
5765e87
23763e2
069f499
22c4918
dcc2464
7aa071b
97e5ce3
6272281
b55bdef
444b656
971e140
6a0e28d
f93772e
3519866
2a7cf4b
79aea6d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -84,10 +84,13 @@ struct check_TIR_helper<F, false> | |||||||||||||||||||
template<class F> | ||||||||||||||||||||
struct check_TIR_helper<F, true> | ||||||||||||||||||||
{ | ||||||||||||||||||||
using vector_type = typename F::vector_type; // expect monochrome | ||||||||||||||||||||
|
||||||||||||||||||||
template<class MicrofacetCache> | ||||||||||||||||||||
static bool __call(NBL_CONST_REF_ARG(F) fresnel, NBL_CONST_REF_ARG(MicrofacetCache) cache) | ||||||||||||||||||||
{ | ||||||||||||||||||||
return cache.isValid(fresnel.getRefractionOrientedEta()); | ||||||||||||||||||||
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); | ||||||||||||||||||||
} | ||||||||||||||||||||
}; | ||||||||||||||||||||
|
||||||||||||||||||||
|
@@ -131,33 +134,40 @@ struct SCookTorrance | |||||||||||||||||||
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> | ||||||||||||||||||||
static bool __checkValid(NBL_CONST_REF_ARG(fresnel_type) f, NBL_CONST_REF_ARG(sample_type) _sample, NBL_CONST_REF_ARG(Interaction) interaction, NBL_CONST_REF_ARG(MicrofacetCache) cache) | ||||||||||||||||||||
|
const bool notTIR = impl::check_TIR_helper<fresnel_type, IsBSDF>::template __call<MicrofacetCache>(_f, cache); | |
assert(notTIR); |
then there's no need to have an impl::check_TIR_helper
at all
Outdated
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?
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.
comment please
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 erm, since you're using this for BRDF and BSDF, this can't be dummy, needs to be the actual refractive index thing I think!
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.
Should've been replaced with the quant_query_helper
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)
Outdated
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
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've now made the valid case return invalid!?
Outdated
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 on the sample concept
Outdated
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
Outdated
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
Outdated
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)
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)
Outdated
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;
Outdated
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.
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
Outdated
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 use conditional_t
directly in a function signature, there's no need to add another template parameter T
P.S. also try to use NBL_FUNC_REQUIRES instead of the enable_if_t
Outdated
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 don't passIsBSDF
to impl::getOrientedFresnel
or skip the NBL_IF_CONSTEXPR
devshgraphicsprogramming marked this conversation as resolved.
Show resolved
Hide resolved
Outdated
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.
add a comment why
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!!!!Uh oh!
There was an error while loading. Please reload this page.
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.
The
ComputeMicrofacetNormal::isValid
thatcache.isValid
uses is called withOrientedEta
type. Does that need to take eta (and rcp_eta) as scalar(s) then? Or just move construction ofOrientedEta
intocache.isValid
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.
OrientedEta<scalar_type> or
scalar_type
yes