Address path tracer merge leftover comments by keptsecret · Pull Request #991 · Devsh-Graphics-Programming/Nabla

keptsecret · 2026-01-22T08:05:01Z

Makes changes left from #966

…ical triangle

…nstead

…tion

keptsecret · 2026-02-02T03:22:59Z

hlsl/matrix_utils/transformation_matrix_utils.hlsl removed because of https://github.com/Devsh-Graphics-Programming/Nabla/pull/966/files#r2618467323

devshgraphicsprogramming · 2026-02-12T16:34:52Z

include/nbl/builtin/hlsl/sampling/projected_spherical_triangle.hlsl

+        return pdf * bilinear.backwardPdf(u);
    }

    scalar_type pdf(const vector3_type receiverNormal, bool receiverWasBSDF, const vector3_type L)


#966 (comment)

devshgraphicsprogramming · 2026-02-12T16:43:25Z

include/nbl/builtin/hlsl/bxdf/fresnel.hlsl

        NBL_UNROLL for (int m=1; m<=2; ++m)
        {
            Cm *= r123s;
            Sm  = hlsl::promote<vector_type>(2.0) * evalSensitivity(hlsl::promote<vector_type>(scalar_type(m))*D, hlsl::promote<vector_type>(scalar_type(m)) *(phi23s+phi21s));


at least awrite a TODO about hoisting this common subexpression

devshgraphicsprogramming · 2026-02-12T16:48:49Z

include/nbl/builtin/hlsl/sampling/projected_spherical_triangle.hlsl

only the sampling::SphericalTriangle is needed #966 (comment)

devshgraphicsprogramming · 2026-02-12T16:50:16Z

include/nbl/builtin/hlsl/sampling/projected_spherical_triangle.hlsl

most parameters should be members #966 (comment)

Also #966 (comment)

devshgraphicsprogramming · 2026-02-12T16:51:19Z

include/nbl/builtin/hlsl/sampling/projected_spherical_triangle.hlsl

        return retval;
    }

    vector4_type computeBilinearPatch(const vector3_type receiverNormal, bool isBSDF)


why is this returning vector4_type instead of sampling::bilinear

devshgraphicsprogramming · 2026-02-12T16:52:18Z

include/nbl/builtin/hlsl/sampling/projected_spherical_triangle.hlsl

    static ProjectedSphericalTriangle<T> create(NBL_CONST_REF_ARG(shapes::SphericalTriangle<T>) tri)
    {
        ProjectedSphericalTriangle<T> retval;
        retval.tri = tri;
        return retval;
    }


remove, see #966 (comment)

devshgraphicsprogramming · 2026-02-12T16:53:31Z

include/nbl/builtin/hlsl/sampling/projected_spherical_triangle.hlsl

        vector3_type cos_vertices, sin_vertices;
-        const scalar_type solidAngle = tri.solidAngleOfTriangle(cos_vertices, sin_vertices, cos_a, cos_c, csc_b, csc_c);
+        const scalar_type solidAngle = tri.solidAngle(cos_vertices, sin_vertices);


should be precomputed and stored as members #966 (comment)

devshgraphicsprogramming · 2026-02-12T16:54:22Z

include/nbl/builtin/hlsl/sampling/projected_spherical_triangle.hlsl

again function with bazillion parameters that are really local members

devshgraphicsprogramming · 2026-02-12T16:59:07Z

include/nbl/builtin/hlsl/shapes/spherical_rectangle.hlsl

    using matrix3x3_type = matrix<Scalar, 3, 3>;

-    static SphericalRectangle<scalar_type> create(const vector3_type observer, const vector3_type rectangleOrigin, const matrix3x3_type basis)
+    static SphericalRectangle<Scalar> create(const vector3_type rectangleOrigin, const vector3_type right, const vector3_type up)


why are you not taking CompressedSphericalRectangle as input instead of 3 variables ?

devshgraphicsprogramming · 2026-02-12T17:03:59Z

include/nbl/builtin/hlsl/shapes/spherical_triangle.hlsl

-        cos_vertices[2] = hlsl::dot<vector3_type>(awayFromEdgePlane0, awayFromEdgePlane1);
-        // TODO: above dot products are in the wrong order, either work out which is which, or try all 6 permutations till it works
-        cos_vertices = hlsl::clamp<vector3_type>((cos_sides - cos_sides.yzx * cos_sides.zxy) * csc_sides.yzx * csc_sides.zxy, hlsl::promote<vector3_type>(-1.0), hlsl::promote<vector3_type>(1.0));
+        cos_vertices = hlsl::clamp((cos_sides - cos_sides.yzx * cos_sides.zxy) * csc_sides.yzx * csc_sides.zxy, hlsl::promote<vector3_type>(-1.0), hlsl::promote<vector3_type>(1.0));


this should be in a common private method, its same as above

devshgraphicsprogramming · 2026-02-12T17:07:57Z

include/nbl/builtin/hlsl/shapes/spherical_rectangle.hlsl

+        retval.extents = vector2_type(hlsl::length(right), hlsl::length(up));
+        retval.basis[0] = right / retval.extents[0];
+        retval.basis[1] = up / retval.extents[1];
+        retval.basis[2] = hlsl::normalize(hlsl::cross(retval.basis[0], retval.basis[1]));


add assert that the absolute value of dot product of cross product args is >0.f

devshgraphicsprogramming · 2026-02-12T17:09:34Z

include/nbl/builtin/hlsl/shapes/spherical_rectangle.hlsl

+// The matrix should be a matrix<scalar_type,3,4> where the last column is the translation, a 3x3 matrix with a pre-transform translation (worldSpace rectangle origin to be subtracted).
+
+// You can compute it from an OBB matrix (as given by/to imguizmo to position a [0,1]^2 rectangle mesh where Z+ is the front face.
+


add the code snippet to ocomments

#966 (comment)

devshgraphicsprogramming · 2026-02-12T17:21:11Z

include/nbl/builtin/hlsl/sampling/spherical_rectangle.hlsl

the observer should be passed during create because while we can have our shape::SphericalRectangle and query its solid angle from multiple observes, the sole purpose of sampling::SphericalRectangle is to draw multiple samples

As much as possible should be precomputed, like the cosGamma etc,

#966 (comment)

devshgraphicsprogramming · 2026-02-12T22:27:31Z

include/nbl/builtin/hlsl/path_tracing/unidirectional.hlsl

+            // We don't allow non watertight transmitters in this renderer
+            bool validPath = nee_sample.getNdotL() > numeric_limits<scalar_type>::min && nee_sample.isValid();


this is to implement in your NEE system

include/nbl/builtin/hlsl/path_tracing/unidirectional.hlsl

devshgraphicsprogramming · 2026-02-12T22:29:06Z

include/nbl/builtin/hlsl/path_tracing/unidirectional.hlsl

+            bxdf::fresnel::OrientedEtas<monochrome_type> orientedEta = bxdf::fresnel::OrientedEtas<monochrome_type>::create(interaction.getNdotV(), hlsl::promote<monochrome_type>(monochromeEta));
+            anisocache_type _cache = anisocache_type::template create<anisotropic_interaction_type, sample_type>(interaction, nee_sample, orientedEta);
+            validPath = validPath && _cache.getAbsNdotH() >= 0.0;
+            bxdf.params.eta = monochromeEta;


this should be done in your NEE system, and return a pdf thats NaN or 0

probably 0 so we dont have issues with MIS weights

devshgraphicsprogramming · 2026-02-12T22:29:52Z

include/nbl/builtin/hlsl/path_tracing/unidirectional.hlsl

+            validPath = validPath && _cache.getAbsNdotH() >= 0.0;
+            bxdf.params.eta = monochromeEta;
+
+            if (neeContrib_pdf.pdf < numeric_limits<scalar_type>::max)


this seems off, you should be checking for pdf>0.f

devshgraphicsprogramming · 2026-02-12T22:32:22Z

include/nbl/builtin/hlsl/path_tracing/unidirectional.hlsl

+                    quotient_pdf_type bsdf_quotient_pdf = materialSystem.quotient_and_pdf(bxdf.materialType, bxdf.params, nee_sample, interaction.isotropic, _cache.iso_cache);
+                    neeContrib_pdf.quotient *= bxdf.albedo * throughput * bsdf_quotient_pdf.quotient;


why is there an albedo thing going on?

This should literally be nee.quotient *= materialSystem.eval*rcpChoiceProb;

you definitely don't want to call quotient, because that picks up contributions and pdfs from dirac lobes which you shouldn't be able to pick up with other samplers

devshgraphicsprogramming · 2026-02-12T22:42:21Z

include/nbl/builtin/hlsl/path_tracing/unidirectional.hlsl

+                    const scalar_type otherGenOverChoice = bsdf_quotient_pdf.pdf * rcpChoiceProb;
+                    const scalar_type otherGenOverLightAndChoice = otherGenOverChoice / bsdf_quotient_pdf.pdf;
+                    neeContrib_pdf.quotient *= otherGenOverChoice / (1.f + otherGenOverLightAndChoice * otherGenOverLightAndChoice);   // balance heuristic


This is completely wrong, the MIS weight should be a ratio of PDFs

generator^2/(generator^2+notGenerator^2)

which is

1/(1+(notGenerator/generator)^2)

You've used the BSDF's PDF as the GENERATOR! The MIS weights are wrong.

in the code this should have the form

const scalar_type otherGenOverLightAndChoice = bsdf.pdf * rcpChoiceProb/ neeContrib.pdf; nee.quotient /= (1.f+otherGenOverLightAndChoice*otherGenOverLightAndChoice);

~~and only get applied if (neeContrib_pdf.pdf < numeric_limits<scalar_type>::max) because when it -> INF you'll get the MIS weight going to 1.f~~ you can always apply, its numerically stable

include/nbl/builtin/hlsl/path_tracing/unidirectional.hlsl

devshgraphicsprogramming · 2026-02-12T23:00:16Z

include/nbl/builtin/hlsl/path_tracing/unidirectional.hlsl

+                    ray_type nee_ray;
+                    nee_ray.origin = intersection + nee_sample.getL().getDirection() * t * Tolerance<scalar_type>::getStart(depth);
+                    nee_ray.direction = nee_sample.getL().getDirection();
+                    nee_ray.intersectionT = t;


can package inside intersector.traceShadowRay

devshgraphicsprogramming · 2026-02-12T23:05:04Z

include/nbl/builtin/hlsl/path_tracing/unidirectional.hlsl

+                    nee_ray.origin = intersection + nee_sample.getL().getDirection() * t * Tolerance<scalar_type>::getStart(depth);
+                    nee_ray.direction = nee_sample.getL().getDirection();
+                    nee_ray.intersectionT = t;
+                    if (bsdf_quotient_pdf.pdf < numeric_limits<scalar_type>::max && getLuma(neeContrib_pdf.quotient) > lumaContributionThreshold && intersector_type::traceRay(nee_ray, scene).id == -1)


you shouldn't be checking bsdf_quotient_pdf.pdf < numeric_limits<scalar_type>::max here... eval should never return a huge pdf really (dirac lobes should be skipped both for eval and pdf of eval - they differ with quotient), you could assert it

btw the traceRay should be a traceShadowRay and it should take the objectID of what you intend to hit and return a floating point "fuzzy" shadow (because I can make it attenuate NEE rays when there are diffuse translucents or thindielectrics)

devshgraphicsprogramming · 2026-02-12T23:10:11Z

include/nbl/builtin/hlsl/path_tracing/unidirectional.hlsl

+            // example only uses isotropic bxdfs
+            // the value of the bsdf divided by the probability of the sample being generated
+            quotient_pdf_type bsdf_quotient_pdf = materialSystem.quotient_and_pdf(bxdf.materialType, bxdf.params, bsdf_sample, interaction.isotropic, _cache.iso_cache);
+            throughput *= bxdf.albedo * bsdf_quotient_pdf.quotient;


what's the albedo doing here!? its literally part of the material!

devshgraphicsprogramming · 2026-02-12T23:11:22Z

include/nbl/builtin/hlsl/path_tracing/unidirectional.hlsl

+        const float lumaThroughputThreshold = lumaContributionThreshold;
+        if (bxdfPdf > bxdfPdfThreshold && getLuma(throughput) > lumaThroughputThreshold)
+        {
+            ray.payload.throughput = throughput;


shouldn't be touching a member directly you should have a method to multiply in a new factor

devshgraphicsprogramming · 2026-02-12T23:12:31Z

include/nbl/builtin/hlsl/path_tracing/unidirectional.hlsl

+            // trace new ray
+            ray.origin = intersection + bxdfSample * (1.0/*kSceneSize*/) * Tolerance<scalar_type>::getStart(depth);
+            ray.direction = bxdfSample;
+            NBL_IF_CONSTEXPR (nee_type::IsPolygonMethodProjectedSolidAngle)
+            {
+                ray.normalAtOrigin = interaction.getN();
+                ray.wasBSDFAtOrigin = isBSDF;
+            }


this needs to be tucked inside your ray as a method or something similar

devshgraphicsprogramming · 2026-02-12T23:20:52Z

include/nbl/builtin/hlsl/path_tracing/unidirectional.hlsl

+            scalar_type otherTechniqueHeuristic = neeProbability / bxdfPdf; // numerically stable, don't touch
+            ray.payload.otherTechniqueHeuristic = otherTechniqueHeuristic * otherTechniqueHeuristic;


do as a method to set it, something with namign that connects with with foundEmissionMIS

devshgraphicsprogramming · 2026-02-15T04:49:30Z

Merge #989 into this PR

devshgraphicsprogramming · 2026-02-15T05:26:51Z

include/nbl/builtin/hlsl/bxdf/base/cook_torrance_base.hlsl


        const scalar_type LdotH = hlsl::mix(VdotH, ieee754::copySign(hlsl::sqrt(rcpEta.value2[0]*VdotH*VdotH + scalar_type(1.0) - rcpEta.value2[0]), -VdotH), transmitted);
+        if (hlsl::isnan(LdotH))
+            return sample_type::createInvalid();


do it like this then

scalar_type LdotH; if (transmitted) { float det = rcpEta.value2[0]*VdotH*VdotH + scalar_type(1.0) - rcpEta.value2[0]; if (det<0.f) return sample_type::createInvalid(); LdotH = ieee754::copySign(hlsl::sqrt(det), -VdotH); } else LdotH = VdotH;

devshgraphicsprogramming · 2026-02-15T05:32:05Z

include/nbl/builtin/hlsl/path_tracing/unidirectional.hlsl

            // We don't allow non watertight transmitters in this renderer
            bool validPath = nee_sample.getNdotL() > numeric_limits<scalar_type>::min && nee_sample.isValid();
            // but if we allowed non-watertight transmitters (single water surface), it would make sense just to apply this line by itself
            bxdf::fresnel::OrientedEtas<monochrome_type> orientedEta = bxdf::fresnel::OrientedEtas<monochrome_type>::create(interaction.getNdotV(), hlsl::promote<monochrome_type>(monochromeEta));
            anisocache_type _cache = anisocache_type::template create<anisotropic_interaction_type, sample_type>(interaction, nee_sample, orientedEta);
            validPath = validPath && _cache.getAbsNdotH() >= 0.0;
            bxdf.params.eta = monochromeEta;

            if (neeContrib_pdf.pdf < numeric_limits<scalar_type>::max)


shouldn't this be tucked inside the if statement here?

Also you can just assert the nee_sample.isValid() if the neeContrib.pdf>0.f

devshgraphicsprogramming · 2026-02-15T05:32:14Z

include/nbl/builtin/hlsl/path_tracing/unidirectional.hlsl

drop the _pdf suffix

devshgraphicsprogramming · 2026-02-15T05:41:52Z

include/nbl/builtin/hlsl/path_tracing/unidirectional.hlsl

+            measure_type emissive = nee.deferredEvalAndPdf(_pdf, scene, lightID, ray) * throughput;
+            scalar_type _pdfSq = hlsl::mix(_pdf, _pdf * _pdf, _pdf < numeric_limits<scalar_type>::max);
+            emissive /= (1.0 + _pdfSq * ray.payload.otherTechniqueHeuristic);
+            ray.payload.accumulation += emissive;


I think the reason you got NaN is because _pdfSq was Max*Max giving you Inf, and otherTechniqueHeuristic was 0

Its a bit weird that your Area Light's PDF is near Max or INF, because this can only happen when Solid Angle or Projected Solid Angle is absolutely tiny, or when during area sampling the distance is huge or the angle of the ray to emitter normal is near orthogonal.

Because you only support 2 generators in this Path Tracer, I can allow you to not square your PDFs until you compue their ratios (square the ratio at the end), this way you'll have more numerical precision.

If you had 4 generators in your MIS weight then you'd have one MUL extra than doing it the way we did it until now.

Because the BxDF sample was generated and we've arrived here, it must be that the BxDF PDF was larger than 0.f, so no division by 0.

The other technique heuristic is neeProbability / bxdfPdf .

What could have gone wrong ? Your BxDF could have been a delta ditribution or close to, so the PDF would have been huge.

Would that have mattered? Probably not because neeProbability would be between 0 and 1, so you're divising a finite number by a non-zero number, so you can get between 0 and not infinity.

However what can happen is that the otherTechniqnueHeuristic is 0 due to delta distribution or zero NEE probability, then in the code I gave #991 (comment)

the MIS condition should be

if (otherTechniqueHeuristic>min) if (const uint lightID=scene.getLightID(objectID); lightID)

instead of just

if (const uint lightID=scene.getLightID(objectID); lightID)

meaning there was nothing to MIS in the first place

you should also probably assert that the _pdf of the light is not zero outside of the if-statement

devshgraphicsprogramming · 2026-02-15T06:27:54Z

include/nbl/builtin/hlsl/path_tracing/unidirectional.hlsl

        if (bxdfPdf > bxdfPdfThreshold && getLuma(throughput) > lumaThroughputThreshold)
        {
            ray.payload.throughput = throughput;
            scalar_type otherTechniqueHeuristic = neeProbability / bxdfPdf; // numerically stable, don't touch


assert that otherTechniqueHeuristic is finite (less than infinity)

devshgraphicsprogramming · 2026-02-15T06:28:16Z

include/nbl/builtin/hlsl/path_tracing/unidirectional.hlsl

+        const scalar_type neeProbability = bxdf.getNEEProb();
        scalar_type rcpChoiceProb;
        sampling::PartitionRandVariable<scalar_type> partitionRandVariable;
        partitionRandVariable.leftProb = neeProbability;


assert neeProbability is between 0 and 1 inclusive

…t system

keptsecret added 14 commits January 20, 2026 15:56

changes to linear, bilinear, box muller for pdf and backward pdf

3d0c454

get polarized fresnel values by component

bea01f9

precompute some phase shift constants

5a36766

removed morton struct superseded by new one

d079f5b

changes to solid angle method name, simplified a lot of code in spher…

3f8fd48

…ical triangle

simplify call to iridescent helper through base struct

c06f199

removed redundant/unused variables from spherical triangle sample

407d0fd

spherical rectangle stores origin, extent, basis and takes observer i…

795b9fa

…nstead

added compressed spherical rectangle, comments for info of implementa…

f9b949e

…tion

removed unused code

140c5c6

path tracer (unidirectional) and concepts moved from example

2ce08d9

use correct quantized sequence decode

1a7b185

moved raygen out as basic ray gen with gaussian filter

3350351

merge master, fix conflicts

29910cf

keptsecret added 2 commits February 6, 2026 15:14

merge master, fix conflicts

a4a5a12

Merge branch 'master' into path_tracer_leftover_cleanup

ea38df1

devshgraphicsprogramming reviewed Feb 12, 2026

View reviewed changes

include/nbl/builtin/hlsl/path_tracing/unidirectional.hlsl Outdated Show resolved Hide resolved

devshgraphicsprogramming reviewed Feb 12, 2026

View reviewed changes

include/nbl/builtin/hlsl/path_tracing/unidirectional.hlsl Outdated Show resolved Hide resolved

devshgraphicsprogramming reviewed Feb 12, 2026

View reviewed changes

include/nbl/builtin/hlsl/path_tracing/unidirectional.hlsl Outdated Show resolved Hide resolved

devshgraphicsprogramming reviewed Feb 12, 2026

View reviewed changes

keptsecret added 6 commits February 13, 2026 10:47

moved nan check from ndotl to ldoth

5ed90fa

minor fixes because this is ref in hlsl and pointer in c++

b8f0c9f

revert linear back to old version because rcpDiff can be negative

78dcf93

fix conditionalAbsOrMax for vectors

cf936ed

refactor changes to various sampling functions so it runs

5e52a6f

some fixes to pt concept type names

86e3146

devshgraphicsprogramming reviewed Feb 15, 2026

View reviewed changes

include/nbl/builtin/hlsl/path_tracing/unidirectional.hlsl Outdated

Copy link

Member

devshgraphicsprogramming Feb 15, 2026

Choose a reason for hiding this comment

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

drop the _pdf suffix

devshgraphicsprogramming reviewed Feb 15, 2026

View reviewed changes

keptsecret added 3 commits February 16, 2026 14:24

type alias for id of material and light, also reduced arguments to ma…

94a5bfb

…t system

change nee to return a struct because of multiple return values

1227991

scene does the mapping from object id to light/mat id

676831e

		// The matrix should be a matrix<scalar_type,3,4> where the last column is the translation, a 3x3 matrix with a pre-transform translation (worldSpace rectangle origin to be subtracted).

		// You can compute it from an OBB matrix (as given by/to imguizmo to position a [0,1]^2 rectangle mesh where Z+ is the front face.

		// We don't allow non watertight transmitters in this renderer
		bool validPath = nee_sample.getNdotL() > numeric_limits<scalar_type>::min && nee_sample.isValid();

		quotient_pdf_type bsdf_quotient_pdf = materialSystem.quotient_and_pdf(bxdf.materialType, bxdf.params, nee_sample, interaction.isotropic, _cache.iso_cache);
		neeContrib_pdf.quotient = bxdf.albedo throughput * bsdf_quotient_pdf.quotient;

		scalar_type otherTechniqueHeuristic = neeProbability / bxdfPdf; // numerically stable, don't touch
		ray.payload.otherTechniqueHeuristic = otherTechniqueHeuristic * otherTechniqueHeuristic;

Conversation

keptsecret commented Jan 22, 2026

Uh oh!

keptsecret commented Feb 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devshgraphicsprogramming Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devshgraphicsprogramming Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devshgraphicsprogramming commented Feb 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devshgraphicsprogramming Feb 12, 2026 •

edited

Loading

devshgraphicsprogramming Feb 12, 2026 •

edited

Loading

devshgraphicsprogramming Feb 15, 2026 •

edited

Loading