Skip to content
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

Port BxDF functions to HLSL #475

Conversation

Crisspl
Copy link
Collaborator

@Crisspl Crisspl commented Mar 12, 2023

Description

I started from the state of this PR: #439 .
Im not intending to port anything besides bxdf/ (and stuff used by it).
Is there some issue/doc/anything with design for HLSL port? From the state of already done things in aforementioned PR, i see there was some design work done.

Testing

TODO list:

BRDF functions are done, BSDF not yet.
Everything touched by me is compiling fine, not tested in practice yet
@devshgraphicsprogramming Please look throughout the diff of my commit for TODO comments, maybe you can answer some questions

TODO:

  • BRDF functions
  • BSDF functions
  • rename remainder_and_pdf functions and use quotient_and_pdf struct for them
  • now doing concept stuff

nahiim and others added 30 commits November 7, 2022 07:42
but don't implement the caches yet
not sure we need to output `tangentSpaceL`, if we do, then I'll add overloads for it
… Nabla build system via subdirectory and find out that their BS isn't supposed to work being added with this method (wrong link/include paths, weird errors) - TODO: build it as external project
@devshgraphicsprogramming
Copy link
Member

@Crisspl can I get a DCO and statement you're not making this during work hours from you?

Also where do you work right now and is your employer ok with your after hours open source contributions?

Then I can get the review done right away

@Crisspl
Copy link
Collaborator Author

Crisspl commented Mar 13, 2023

@devshgraphicsprogramming I'm not the only author of commits within this PR, so is it enough if I sign off just my commits?

Currently working in QLOC, I've asked personally and it's ok to contribute to opensource, my official agreement with QLOC also doesn't prohibit cooperation with others unless it's market competition

@devshgraphicsprogramming
Copy link
Member

devshgraphicsprogramming commented Mar 17, 2023

So this is the convention we want to keep

BxDFs

template<class IncomingRayDirInfo>
struct BxDFName
{
    // each BxDF can choose float, vec3, etc
    using spectrum_t = some_type;
    using pdf_t = float; // most of the time
    
    //
    using sample_t = LightSample<IncomingRayDirInfo>;
    using interaction_t = surface_interactions::Anisotropic<IncomingRayDirInfo>;
    using cache_t = AnisotropicMicrofacetCache;

    //! NOTE: now all the extra `...` parameters of the following functions ARE IN THE STRUCT AS MEMBERS!
    
    // if the BxDF is isotropic, then use isotropic types for the `interaction` and `_cache` inheritance will make it work nicely
    spectrum_t eval(in sample_t _sample, in interaction_t interaction, in cache_t _cache);
    
    sample_t generate(in interaction_t interaction, inout float3 u);
    
    quotient_and_pdf<spectrum_t> quotient_and_pdf(in sample_t _sample, in interaction_t interaction, in cache_t _cache);
};

the individual BxDF structs can obviously have "extra" functions to help us define the 3 cardinal ones that are needed.

So for example instead of having a diffuse namespace and prefixing all functions with lambertian_cos, you could just have

namespace brdf
{
struct Lambertian;
}
namespace bsdf
{
struct Lambertian;
}

@devshgraphicsprogramming
Copy link
Member

devshgraphicsprogramming commented Mar 17, 2023

Dielectrics

Could look like this

template<class fresnel_t>
struct /*Thin,Smooth*/Dielectric
{
using spectrum_t = fresnel_t::spectrum_t;

fresnel_t frensel;

// ... and all the functions required to conform to BXDF concept
};

Where fresnel_t is a struct functor implementing frensel;

Cook Torrance BxDFs

The NDF

We can define NDFs as struct concepts too

template<typename scalar_t=float>
struct NDF
{
    // generation is always anisotropic
    template <class IncomingRayDirInfo>
    float3 generateH(in surface_interactions::Anisotropic<IncomingRayDirInfo> interaction, inout float3 u, out AnisotropicMicrofacetCache _cache);
    
    // isotropic versions
    scalar_t D(in float NdotH2) const;
    scalar_t Lambda(in float NdotX2) const;
    
    // anisotropic version overloads
    scalar_t D(in float TdotH2, in float BdotH2, in float NdotH2) const;
    scalar_t Lambda(in float TdotX2, in float BdotX2, in float NdotX2) const;
};

all stuff like roughness will be stored as optional members

NDF Traits

Instead of providing G1, G2, and G2/G1 for every NDF, we can make a trait

namespace bxdf::cook_torrance
{
// forward declaration so we can explicitly specialize, e.g. for GGX where optimized forms of the functions provided by the trait exist
template<class ndf_t>
struct ndf_traits;

namspace impl
{
template<class ndf_t>
struct ndf_traits
{
    using scalar_t = ndf_t::scalar_t;

    scalar_t G1(in float NdotX2) const {return scalar_t(1)/(scalar_t(1)+ndf.Lambda(NdotX2));}
    scalar_t G2(in float NdotV2, in float NdotL2) const {return scalar_t(1)/(scalar_t(1)+ndf.Lambda(NdotV2)+ndf.Lambda(NdotL2));}
    
    scalar_t G2_over_G1(in float NdotV2, in float NdotL2) const
    {
        const scalar_t lambdaV_plus_one = ndf.Lambda(NdotV2)+scalar_t(1);
        return lambdaV_plus_one/(ndf.Lambda(NdotL2)+lambdaV_plus_one);
    }
    
    // bits from `ndf/common.glsl`
    scalar_t dHdL(NDFcos, in float_t maxNdotV);
    
    // bits from `geom/smith/common.glsl`
    scalar_t VNDF(...) // and variants
    
    //! anisotropic variants omitted for brevity
    
    ndf_t ndf;
};
}

// default specialization
template<class ndf_t>
struct ndf_traits : impl::ndf_traits<ndf_t> {};
}

The BxDFs

Instead of providing G1, G2, and G2/G1 for every NDF, we can make a trait

namespace bxdf::brdf
{
template<class fresnel_t, class ndf_t>
struct CookTorrance
{
   fresnel_t fresnel;
   ndf_traits<ndf_t> ndf;
};
}

Complex BxDFs

Since HLSL2021 doesn't yet support Variadic Templates (and therefore tuple not variant can be constructed) and IIRC not even unions

But its possible to create the structs on the fly and rely on inlining to clean it all up.

Comment on lines +17 to +18

float3 diffuseFresnelCorrectionFactor(in vec3 n, in vec3 n2)

Choose a reason for hiding this comment

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

should probably make it

template<typename spectrum_t>
spectrum_t diffuseFresnelCorrectionFactor(in spectrum_t n, in spectrum_t n2)

Comment on lines 21 to 22
template <class fresnel_t, class ndf_t, class Sample, class Interaction, class MicrofacetCache>
struct CookTorrance : BxDFBase<float3, float, Sample, Interaction, MicrofacetCache>

Choose a reason for hiding this comment

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

why are we making Sample, Interaction and MicrofacetCache template parameters, I think only the RayDirDistribution ought to be one (which then goes into the interaction and sample)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but u dont know if u want isotropic or anisotropic interaction at this point

Choose a reason for hiding this comment

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

btw could we make the MicrofacetCache's have a using typedef for the surface_interaction (iso vs aniso) it wants?

Comment on lines 28 to 38
template <class IncomingRayDirInfo, class fresnel_t, class ndf_t>
struct IsotropicCookTorrance : CookTorrance<fresnel_t, ndf_t, LightSample<IncomingRayDirInfo>, surface_interactions::Isotropic<IncomingRayDirInfo>, IsotropicMicrofacetCache>
{

};

template <class IncomingRayDirInfo, class fresnel_t, class ndf_t>
struct AnisotropicCookTorrance : CookTorrance<fresnel_t, ndf_t, LightSample<IncomingRayDirInfo>, surface_interactions::Anisotropic<IncomingRayDirInfo>, AnisotropicMicrofacetCache>
{

};

Choose a reason for hiding this comment

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

Its a bit sad that HLSL202x doesn't have SFINAE, because we could have just made isotropic and anisotropic the same struct and enable_if-ed the anisotropic interaction overload

However we have inheritance, so we can make the base class Isotropic, and inherit to make the Anistropic and add that overload, not as clean but will do

Copy link
Collaborator Author

@Crisspl Crisspl Mar 23, 2023

Choose a reason for hiding this comment

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

but why? this completely messes up typedefs got from BxDFBase (always isotropic)

Choose a reason for hiding this comment

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

hmm, good observation.

The thing is that an anisotropic BxDF should always be usable as an isotropic one (thanks to inheritance).

Maybe the BxDFBase should be adjusted (sepearate typedefs for aniso and iso)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The thing is that an anisotropic BxDF should always be usable as an isotropic one (thanks to inheritance).

it kinda is, just pass the same values to ax and ay, it's just going longer way to compute the result. Which should be assumed, since you've intentionally chosen anisotropic (more generic) variant

// BxDFs must define such typenames:
using spectrum_t = Spectrum;
using pdf_t = Pdf;
using q_pdf_t = quotient_and_pdf<spectrum_t>;

Choose a reason for hiding this comment

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

improve my quotient_and_pdf to take a pdf_t

Comment on lines 440 to 442
using sample_t = Sample;
using interaction_t = Interaction;
using cache_t = MicrofacetCache;

Choose a reason for hiding this comment

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

sample should always be the same thing, cache should tell us the interaction (iso vs aniso)

using sample_t = LightSample<ray_distribution_t>;
using interaction_t = MicrofacetCache::interaction_t<ray_distribution_t>;
using cache_t = MicrofacetCache;

P.S. I'm not sure whether HLSL202x lets us define template aliases, please check add this into AnisotropicMicrofacetCache and see if DXC chokes on this when actually used:

public:

template<class ray_distribution_t>
using interaction_t = surface_interactions::Anisotropic<ray_distribution_t>;

if it doesn't then use similar trick on Isotropic

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure whether HLSL202x lets us define template aliases

i assumed it doesnt, i'll try to make sure
its a pity hlsl2021 has no spec XD the only way to check things is to write code and see what dxc spits out

Choose a reason for hiding this comment

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

Move fast, break things, no docs = the Nabla DirectX way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, HLSL supports templated using statements for typedefs/aliases

Comment on lines +451 to +452
* q_pdf_t quotient_and_pdf(in sample_t, in interaction_t, in cache_t);
*/

Choose a reason for hiding this comment

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

@sunho can you suggest some function signatures for AOVs, I'm thinking

using albedo_t = _albedo_t; // float3 default
using aov_transmittance_t = _aov_transmittance_t; // float3 default

/**
* Fresnels must define following member functions:
*
* spectrum_t operator()(...); // TODO is there some paremeter list that can be universal for all fresnels ever needed?

Choose a reason for hiding this comment

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

just the angle between a surface and a ray cosTheta

all the other stuff like IoR and so on will be member variables (this is why lambdas rule!)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so F0 in case of schlick would be a member as well right?
btw do you remember what f0 actually was?

Choose a reason for hiding this comment

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

so F0 in case of schlick would be a member as well right? btw do you remember what f0 actually was?

Yes.

ofc i remember, F0 is reflectance when the angle is 0 (dot product is 1)

Comment on lines 121 to 146
//
// VNDF functions
//
// Statics:
static scalar_t VNDF_static(in scalar_t d, in scalar_t lambda_V, in float maxNdotV, out onePlusLambda_V)
{
return geom_smith::VNDF_pdf_wo_clamps(d, lambda_V, maxNdotV, onePlusLambda_V);
}
static scalar_t VNDF_static(in scalar_t d, in scalar_t lambda_V, in float absNdotV, in bool transmitted, in float VdotH, in float LdotH, in float VdotHLdotH, in float orientedEta, in float reflectance, out float onePlusLambda_V)
{
return geom_smith::VNDF_pdf_wo_clamps(d, lambda_V.absNdotV, transmitted, VdotH, LdotH, VdotHLdotH, orientedEta, reflectance, onePlusLambda_V);
}
static scalar_t VNDF_static(in scalar_t d, in scalar_t G1_over_2NdotV)
{
return geom_smith::VNDF_pdf_wo_clamps(d, G1_over_2NdotV);
}

static scalar_t VNDF_fromLambda_impl(in scalar_t d, in scalar_t lambda, in float maxNdotV)
{
float dummy;
return VNDF_static(d, lambda, maxNdotV, dummy);
}
static scalar_t VNDF_fromG1_over_2NdotV_impl(in scalar_t d, in scalar_t G1_over_2NdotV)
{
return VNDF_static(d, G1_over_2NdotV);
}

Choose a reason for hiding this comment

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

inline as much as possible and get rid of the forwarding

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what do you mean by forwarding?

Choose a reason for hiding this comment

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

function with same parameters being called with no change just because its layering one API to another

Comment on lines 162 to 193
scalar_t VNDF(
in float NdotH2, in float NdotV2,
in float absNdotV, in bool transmitted, in float VdotH, in float LdotH, in float VdotHLdotH, in float orientedEta, in float reflectance, out float onePlusLambda_V)
{
const float d = ndf.D(NdotH2);
const float lambda = ndf.Lambda(NdotV2);

return VNDF_static(d, lambda, absNdotV, transmitted, VdotH, LdotH, VdotHLdotH, orientedEta, reflectance, onePlusLambda_V);
}

// VNDF anisotropic variants
scalar_t VNDF(
in float TdotH2, in float BdotH2, in float NdotH2,
in float TdotV2, in float BdotV2, in float NdotV2,
in float maxNdotV)
{
const float d = ndf.D(TdotH2, BdotH2, NdotH2);
const float lambda = ndf.Lambda(TdotV2, BdotV2, NdotV2);
return VNDF_fromLambda_impl(d, lambda, maxNdotV);
}
scalar_t VNDF(
in float TdotH2, in float BdotH2, in float NdotH2,
in float G1_over_2NdotV)
{
const float d = ndf.D(TdotH2, BdotH2, NdotH2);
return VNDF_fromG1_over_2NdotV_impl(d, G1_over_2NdotV);
}
scalar_t VNDF(
in float TdotH2, in float BdotH2, in float NdotH2,
in float TdotV2, in float BdotV2, in float NdotV2,
in float absNdotV, in bool transmitted, in float VdotH, in float LdotH, in float VdotHLdotH, in float orientedEta, in float reflectance, out float onePlusLambda_V)
{

Choose a reason for hiding this comment

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

if it makes sense, delcare parameter structs in the bxdf namespace for the ndf functions and gather some of these inputs together, right now its tedious to read and typo prone (many parameters have the same type, you'll die)

// forward declaration so we can explicitly specialize, e.g. for GGX where optimized forms of the functions provided by the trait exist
template<class ndf_t>
struct ndf_traits;

Choose a reason for hiding this comment

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

this header needs to be renamed to ndf.hlsl and be a level up, then in this very place you need to include all the other NDFs so their specializations have a chance to appear before the default one

Comment on lines 94 to 96
struct ndf_traits
{
using scalar_t = ndf_t::scalar_t;

Choose a reason for hiding this comment

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

how much of all this will the GGX specialization also use?

@devshgraphicsprogrammingjenkins
Copy link
Contributor

[CI]: Can one of the admins verify this patch?

@devshgraphicsprogramming devshgraphicsprogramming changed the base branch from master to give_credit September 19, 2023 16:51
@devshgraphicsprogramming devshgraphicsprogramming merged commit c81e7ab into Devsh-Graphics-Programming:give_credit Sep 19, 2023
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.

None yet

5 participants