Skip to content

Resampled Importance Sampling bxdfs#1027

Open
devshgraphicsprogramming wants to merge 41 commits intosampler-conceptsfrom
ris_bxdfs
Open

Resampled Importance Sampling bxdfs#1027
devshgraphicsprogramming wants to merge 41 commits intosampler-conceptsfrom
ris_bxdfs

Conversation

@devshgraphicsprogramming
Copy link
Member

Description

Testing

TODO list:

kevyuu and others added 24 commits March 14, 2026 10:44
…ession reciprocation (swapping of interface order)

Unfortunately I realized our THINDIELECTRIC correction is badly designed
Point Boost to the exact Wave one-line backport for emitted pragma newlines, remove the temporary local pragma workaround, and keep the remaining include-path fixes in Nabla.

This leaves the Wave pragma issue fixed at the dependency level while preserving the Nabla-side fixes for Windows backslash includes and single-leading-slash virtual includes.

Thanks to @Themperror for the additional pragma and include repros. Those made it straightforward to verify the dependency-level fix and drop the local workaround cleanly.
…dates

Fix remaining preprocess bugs and backport Wave pragma fix
…te-nsc-windows-x64-release-master

CI: Promote NSC channel to 66da590
@devshgraphicsprogramming devshgraphicsprogramming changed the title Ris bxdfs Resampled Importance Sampling bxdfs Mar 20, 2026
((NBL_CONCEPT_REQ_EXPR_RET_TYPE)((bxdf.eval_and_weight(_sample, aniso, anisocache)), ::nbl::hlsl::is_same_v, typename T::quotient_pdf_type))
((NBL_CONCEPT_REQ_EXPR_RET_TYPE)((bxdf.pdf(_sample, aniso, anisocache)), ::nbl::hlsl::is_same_v, typename T::scalar_type))
((NBL_CONCEPT_REQ_EXPR_RET_TYPE)((bxdf.quotient_and_pdf(_sample, aniso, anisocache)), ::nbl::hlsl::is_same_v, typename T::quotient_pdf_type))
((NBL_CONCEPT_REQ_EXPR_RET_TYPE)((bxdf.quotient_and_weight(_sample, aniso, anisocache)), ::nbl::hlsl::is_same_v, typename T::quotient_pdf_type))
Copy link
Member Author

Choose a reason for hiding this comment

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

lets make all BxDFs have a cache type which goes from generate to quotient_and_weight but then microfacet_bxdf_common would be the weird one that puts a further constraint that the anisocache_type needs to fit some concept for Cook Torrance to be useful

and allow a 2nd cache type just for eval_and_weight (the two can obviously be identical in practice, e.g. our base impl for now)

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 could actually get rid of microfacex_bdf_common and the iso_microfacet concept (basically the entire block in namespace impl ehere)

and just check that {Isotropic}MicrofacetB{R|S}DF = {Isotropic}B{R|S}DF<T> && (Isotropic ? CreatableIsotropicMicrofacetCache<typedef T::cache_type>:AnisotropicMicrofacetCache<typedef T::cache_type>);

Comment on lines +155 to +205
if (!system || outputPath.empty())
return false;

const auto parent = outputPath.parent_path();
if (!parent.empty() && !system->isDirectory(parent))
{
if (!system->createDirectory(parent))
return false;
}

auto tempPath = outputPath;
tempPath += ".tmp";
system->deleteFile(tempPath);

smart_refctd_ptr<IFile> outputFile;
{
ISystem::future_t<smart_refctd_ptr<IFile>> future;
system->createFile(future, tempPath, IFileBase::ECF_WRITE);
if (!future.wait())
return false;

auto lock = future.acquire();
if (!lock)
return false;
lock.move_into(outputFile);
}
if (!outputFile)
return false;

if (!content.empty())
{
IFile::success_t success;
outputFile->write(success, content.data(), 0ull, content.size());
if (!success)
{
outputFile = nullptr;
system->deleteFile(tempPath);
return false;
}
}
outputFile = nullptr;

system->deleteFile(outputPath);
const auto moveError = system->moveFileOrDirectory(tempPath, outputPath);
if (moveError)
{
system->deleteFile(tempPath);
return false;
}

return true;
Copy link
Member Author

Choose a reason for hiding this comment

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

@AnastaZIuk do you think you could add an option to createFile flags (or pack the path+ IFileBase flaggs into a strcut) to create all the directories needed on the way to the path ?

cold be useful

Copy link
Member

Choose a reason for hiding this comment

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

ay

Comment on lines +185 to +187
((NBL_CONCEPT_REQ_EXPR_RET_TYPE)((matsys.evalAndWeight(matid, _sample, aniso_inter)), ::nbl::hlsl::is_same_v, typename T::quotient_pdf_type))
((NBL_CONCEPT_REQ_EXPR_RET_TYPE)((matsys.generate(matid, aniso_inter, u, cache_)), ::nbl::hlsl::is_same_v, typename T::sample_type))
((NBL_CONCEPT_REQ_EXPR_RET_TYPE)((matsys.pdf(matid, _sample, aniso_inter)), ::nbl::hlsl::is_same_v, typename T::scalar_type))
((NBL_CONCEPT_REQ_EXPR_RET_TYPE)((matsys.quotient_and_pdf(matid, _sample, aniso_inter, cache_)), ::nbl::hlsl::is_same_v, typename T::quotient_pdf_type))
((NBL_CONCEPT_REQ_EXPR_RET_TYPE)((matsys.quotientAndWeight(matid, _sample, aniso_inter, cache_)), ::nbl::hlsl::is_same_v, typename T::quotient_pdf_type))
Copy link
Member Author

Choose a reason for hiding this comment

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

the typedefs returned should be the new value_and_weight types

Need a new file called value_and_weight which is essentially the same as value_and_pdf with similar typedefs
https://github.com/Devsh-Graphics-Programming/Nabla/blob/38f73af10a6047ede44d7822930c04ce2a395f9d/include/nbl/builtin/hlsl/sampling/value_and_pdf.hlsl

With the exception of providing the value() method!

Then implement the value_and_pdf (and rcpPdf) via composition (cause in HLSL inheritance is bound to fuck us over with templated functions) from _and_weight and only have it differ by providing a value() method

Comment on lines 170 to 171
using quant_query_type = typename ndf_type::quant_query_type;
quant_query_type qq = impl::quant_query_helper<ndf_type, fresnel_type, IsBSDF>::template __call<Interaction, MicrofacetCache>(ndf, _f, interaction, cache);
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've already computed this in __forwardPdf

Copy link
Member Author

@devshgraphicsprogramming devshgraphicsprogramming Mar 23, 2026

Choose a reason for hiding this comment

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

any way to making __forwardPdf spit this out for you?
Make make __forwardPdf<bool FromGenerator> returna struct

scalar_type pdf;
frensel_type orientedFresenel;
quant_query_type quantQuery;
[optional only when `IsBSDF && not fresnel_type::ReturnsMonochrome`] spectral_type reflectance;
[optional only when `IsBSDF && not fresnel_type::ReturnsMonochrome`] scalar_type scaled_reflectance ;

and doesn't abuse NBL_REF_ARG to return extra things

Also this way this whole block of code

retval.pdf = 0.f;
retval.orientedFresenel = __getOrientedFresnel(fresnel, interaction.getNdotV());
const bool cacheIsValid = __checkValid<Interaction, MicrofacetCache>(retval.orientedFresenel, _sample, interaction, cache);
assert(!FromGenerator || cacheIsValid);
if (FromGenerator ? _sample.isValid():cacheIsValid)
{
... fill the members with the old function body
}
return retval;

can replace the preludes in quotientAndWeight and evalAndWeight with just

if (pdfQuery.pdf==0.f)
   return ...;

up until the G2_over_G1 computation

Copy link
Member Author

Choose a reason for hiding this comment

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

or the __overwriteDG

Copy link
Member Author

@devshgraphicsprogramming devshgraphicsprogramming Mar 23, 2026

Choose a reason for hiding this comment

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

the quotient... function can just not use quantQueryits the same as using a dummy on a NBL_REF_ARG

Comment on lines 402 to 427
Copy link
Member Author

Choose a reason for hiding this comment

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

hmm we still get a pointless mul with 1.0 if the PDF is infinity, if you move this computation to after quo is constructed, we can turn the conditional G2_over_G1 = into quo *=

It does require though that you quo = promote<spectral_type>(1.0) for Monochrome BSDF, thats not a problem because you go contexpr expressions first, so they'll optimize out and the mul with unity will optimize out too

Just the way you have it now, won't

{
spectral_type reflectance;
const scalar_type scaled_reflectance = __getScaledReflectance(_f, interaction, hlsl::abs(cache.getVdotH()), cache.isTransmission(), reflectance);
quo = reflectance / scaled_reflectance * G2_over_G1;
Copy link
Member Author

Choose a reason for hiding this comment

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

__forwardPDF already computes these would be cool to return them

bool isInfinity;
scalar_type _pdf = __pdf<Interaction, MicrofacetCache>(_sample, interaction, cache, isInfinity);
scalar_type _pdf = __forwardPdf<Interaction, MicrofacetCache>(_sample, interaction, cache, isInfinity);
return hlsl::mix(_pdf, scalar_type(0.0), isInfinity);
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 incorrect, it should return INF for forward, it was only backward when it was not meant to (hence why I asked for the distinction)

Comment on lines 188 to 190
scalar_type clampedVdotH = cache.getVdotH();
NBL_IF_CONSTEXPR(IsBSDF)
clampedVdotH = hlsl::abs(clampedVdotH);
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 only need this to call const spectral_type reflectance = impl::__implicit_promote<spectral_type, typename fresnel_type::vector_type>::__call(_f(clampedVdotH));

which we already would have had __forwardPdf return to us because it needed to compute it

Copy link
Member Author

Choose a reason for hiding this comment

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

it doesn't compute it for you in the BRDF case, but you don't ened to have this whole tempvar and clamp it then

Comment on lines 191 to 196

spectral_type quo;
NBL_IF_CONSTEXPR(IsBSDF)
{
const spectral_type reflectance = impl::__implicit_promote<spectral_type, typename fresnel_type::vector_type>::__call(_f(clampedVdotH));
return hlsl::mix(reflectance, hlsl::promote<spectral_type>(1.0) - reflectance, cache.isTransmission()) * DG;
quo = hlsl::mix(reflectance, hlsl::promote<spectral_type>(1.0) - reflectance, cache.isTransmission()) * DG;
}
else
return impl::__implicit_promote<spectral_type, typename fresnel_type::vector_type>::__call(_f(clampedVdotH)) * DG;
quo = impl::__implicit_promote<spectral_type, typename fresnel_type::vector_type>::__call(_f(clampedVdotH)) * DG;

return quotient_pdf_type::create(quo, _pdf);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

in combination with previous comment it would simplify to

NBL_IF_CONSTEXPR(fresnel_type::IsBSDF)
{
   NBL_IF_CONSTEXPR(fresnel_type::ReturnsMonochrome)
   {
      quo =  pdfRetval.reflectance;
   }
   else
   {
      quo = mix(pdfRetval.reflectance,hlsl::promote<spectral_type>(1.0) - pdfRetval.reflectance, cache.isTransmission();
   }
}
else
   quo =impl::__implicit_promote<spectral_type, typename fresnel_type::vector_type>::__call( _f(cache.getVdotH()));
quo *= DG;

with DG mul done outside

@@ -902,6 +902,7 @@ NBL_CONCEPT_END(
((NBL_CONCEPT_REQ_TYPE)(T::sample_type))
((NBL_CONCEPT_REQ_TYPE)(T::spectral_type))
((NBL_CONCEPT_REQ_TYPE)(T::quotient_pdf_type))
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 now needs to be value_and_weight_type

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 require a cache_type which will be passed from generate to quotientAndWeight

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 require a cache_type which will be passed from generate to quotientAndWeight

done

Comment on lines 1068 to 1071
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 already check these elsewhere I think

struct Cache {};
using isocache_type = Cache;
using anisocache_type = Cache;
using evalcache_type = Cache;
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 should be no evalcache_type, its still same cache as for quotient and generate

But having an evalAndWeight with an overload that takes it should be a Cook Torrance / Microfacet thing only

((NBL_CONCEPT_REQ_TYPE)(T::anisotropic_interaction_type))
((NBL_CONCEPT_REQ_TYPE)(T::sample_type))
((NBL_CONCEPT_REQ_TYPE)(T::anisocache_type))
((NBL_CONCEPT_REQ_TYPE)(T::evalcache_type))
Copy link
Member Author

Choose a reason for hiding this comment

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

don't require a separate evaluation cache type

((NBL_CONCEPT_REQ_TYPE)(T::isotropic_interaction_type))
((NBL_CONCEPT_REQ_TYPE)(T::isocache_type))
((NBL_CONCEPT_REQ_EXPR_RET_TYPE)((bxdf.eval_and_weight(_sample, iso, isocache)), ::nbl::hlsl::is_same_v, typename T::quotient_pdf_type))
((NBL_CONCEPT_REQ_EXPR_RET_TYPE)((bxdf.evalAndWeight(_sample, iso, evalcache)), ::nbl::hlsl::is_same_v, typename T::quotient_pdf_type))
Copy link
Member Author

Choose a reason for hiding this comment

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

evalAndWeight taking the anisocache or isocache should be an extra overload in the imcrofacet BxDF concepts

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.

3 participants