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

…mek's remove_core_matrix branch, added create from mat3x3

…c cast to/from truncated quat

…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

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

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

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

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)

for next sampling PR, to rework in line with concepts of #1001

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

ok sampling PR

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)

ok sampling PR

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)

ok sampling PR

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

ok sampling PR

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

devshgraphicsprogramming · 2026-02-24T14:33:17Z

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

+
+    scalar_type bxdfPdfThreshold;
+    scalar_type lumaContributionThreshold; // OETF smallest perceptible value
+    measure_type spectralTypeToLumaCoeffs;


measure_type is NOT the same thing as spectral_type I'll want to return normals, albedo and other things from the measure

@keptsecret still a TODO

devshgraphicsprogramming · 2026-02-24T14:34:43Z

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

+        scalar_type rcpChoiceProb;
+        sampling::PartitionRandVariable<scalar_type> partitionRandVariable;
+        partitionRandVariable.leftProb = neeProbability;
+        assert(neeProbability >= 0.0 && neeProbability <= 1.0)


missing ; at EOL

@keptsecret you still have a missing semicolon after the asseert

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

devshgraphicsprogramming · 2026-02-24T14:50:17Z

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

+            // We don't allow non watertight transmitters in this renderer
+            // but if we allowed non-watertight transmitters (single water surface), it would make sense just to apply this line by itself
+            const scalar_type monochromeEta = materialSystem.setMonochromeEta(matID, throughputCIE_Y);
+            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);
+
+            if (neeContrib.pdf > scalar_type(0.0))
+            {
+                // example only uses isotropic bxdfs
+                quotient_pdf_type bsdf_quotient_pdf = materialSystem.quotient_and_pdf(matID, nee_sample, interaction.isotropic, _cache.iso_cache);


this whole clump of code is super weird, it wont generalize to multi-layer materials, etc.

the cache shouldn't be exposed here at all (your material system in userspace could keep a cache around as a hidden variable)

The cache only applies to cook torrance materials and those are a high specialization.

You're abusing a quotient_and_pdf to just give you the pdf of a light sample, you shouldf be using pdf and eval separately instead.

Our dirac delta BxDFs will return Quotient of 1 and PDF of INF always!

Simply because the quotient still has dirac deltas in it, and the pdf does too, if you're doing NEE, you're supposed to completely remove those from the lighting integral - otherwise if you accidentally hit a Dirac Delta lobe the MIS weight for NEE goes to 0 which means that any non-dirac (such as plastic material which has diffuse mixed by fresnel with specular) lobes won't get any NEE at all!

also this can cause your problems with MIS weights not liking the NEE PDF being high!

https://discord.com/channels/593902898015109131/1184857002040631416/1325736444848766996

after leave comment that we'll need an eval_and_mis_weight https://discord.com/channels/593902898015109131/1400024201682747512/1400024205050908783

https://discord.com/channels/593902898015109131/1400024201682747512/1472484733710499970

Ok you can let the material system define a cache_type but then its geenrate must initialize it and pass it to quotient_and_pdf

@keptsecret next item on the agenda

devshgraphicsprogramming · 2026-02-24T14:57:26Z

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

+                // example only uses isotropic bxdfs
+                quotient_pdf_type bsdf_quotient_pdf = materialSystem.quotient_and_pdf(matID, nee_sample, interaction.isotropic, _cache.iso_cache);
+                neeContrib.quotient *= materialSystem.eval(matID, nee_sample, interaction.isotropic, _cache.iso_cache) * rcpChoiceProb;
+                const scalar_type otherGenOverLightAndChoice = bsdf_quotient_pdf.pdf * rcpChoiceProb / neeContrib.pdf;


pass full aniso interaction, let the material system deal with only using the isotropic part

Also your PDF is mismatched.

@keptsecret still using the wrong PDF to pair with eval

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

…n interaction instead of just isBSDF

devshgraphicsprogramming · 2026-02-25T15:17:35Z

include/nbl/builtin/hlsl/path_tracing/concepts.hlsl

+namespace impl
+{
+struct DummyInteraction {};
+}
+
 #define NBL_CONCEPT_NAME Ray
 #define NBL_CONCEPT_TPLT_PRM_KINDS (typename)
 #define NBL_CONCEPT_TPLT_PRM_NAMES (T)
 #define NBL_CONCEPT_PARAM_0 (ray, T)
 #define NBL_CONCEPT_PARAM_1 (v, typename T::vector3_type)
-#define NBL_CONCEPT_PARAM_2 (b, bool)
+#define NBL_CONCEPT_PARAM_2 (interaction, impl::DummyInteraction)


not sure how an empty struct tests any better than a real default aniso interaction

I didn't want to include the bxdf/common.hlsl in here.
But could change to surface_interaction::SIsotropic if that's better?

devshgraphicsprogramming · 2026-02-25T15:26:33Z

include/nbl/builtin/hlsl/path_tracing/concepts.hlsl

-    ((NBL_CONCEPT_REQ_EXPR_RET_TYPE)((scene.getNormal(id, intersectP)), ::nbl::hlsl::is_same_v, typename T::vector3_type))
+    ((NBL_CONCEPT_REQ_EXPR_RET_TYPE)((scene.template getInteraction<impl::DummyRay>(id, intersectP, ray)), ::nbl::hlsl::is_same_v, typename T::interaction_type))
 );
+#undef ray


why are you still adamant on the scene giving you an interaction and not the closestHit trace ray of the intersector !?

You do realize that in general thats not possible with a ray, intersection position and an object ID ?

(well or possible, but you'd need to re-intersect the BLAS just for that)

The scene already does give a closest hit ray. This is for filling out the interaction member of the closest hit return struct.
Regardless, I've now made the scene handle all of creating the closest hit return struct.

devshgraphicsprogramming · 2026-02-25T15:30:38Z

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

+            vector3_type direction = bxdfSample;
+            ray.init(origin, direction);
+            ray.template initInteraction<anisotropic_interaction_type>(interaction);
+            ray.setT(1.0/*kSceneSize*/);


huh? why 1.0 ?

devshgraphicsprogramming · 2026-02-25T15:32:25Z

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

+                quotient_pdf_type bsdf_quotient_pdf = materialSystem.quotient_and_pdf(matID, nee_sample, iso_interaction, _cache.iso_cache);
+                neeContrib.quotient *= materialSystem.eval(matID, nee_sample, iso_interaction, _cache.iso_cache) * rcpChoiceProb;


material system shall always take aniso interaction

devshgraphicsprogramming · 2026-02-25T15:32:33Z

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

-            quotient_pdf_type bsdf_quotient_pdf = materialSystem.quotient_and_pdf(matID, bsdf_sample, interaction.isotropic, _cache.iso_cache);
+            quotient_pdf_type bsdf_quotient_pdf = materialSystem.quotient_and_pdf(matID, bsdf_sample, iso_interaction, _cache.iso_cache);


material system shall always take aniso interaction

devshgraphicsprogramming · 2026-02-25T15:32:58Z

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

    {
        anisotropic_interaction_type interaction = intersectData.getInteraction();
-        isotropic_interaction_type iso_interaction = interaction.isotropic;
+        isotropic_interaction_type iso_interaction = interaction.getIsotropic();


in essence this should be an unused variable and removed

devshgraphicsprogramming · 2026-02-25T15:47:52Z

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

-        const vector3_type throughputCIE_Y = hlsl::normalize(spectralTypeToLumaCoeffs * throughput);
+        measure_type throughputCIE_Y = spectralTypeToLumaCoeffs * throughput;
+        {
+            scalar_type sum_throughput = throughputCIE_Y[0];
+            NBL_UNROLL for (uint16_t i = 1; i < vector_traits<measure_type>::Dimension; i++)
+                sum_throughput += throughputCIE_Y[i];
+            throughputCIE_Y /= sum_throughput;
+        }


cool, now this needs to in the interaction

devshgraphicsprogramming · 2026-02-25T15:53:17Z

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

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


does this update your thoroughput in the payload as well?

Then rename the function to updateThroughputAndMISWeights

devshgraphicsprogramming · 2026-02-25T15:55:02Z

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

            vector3_type direction = bxdfSample;
-            ray.initData(origin, direction, interaction.getN(), isBSDF);
+            ray.init(origin, direction);
+            ray.template initInteraction<anisotropic_interaction_type>(interaction);


setInteraction maybe would be a better name ?

devshgraphicsprogramming · 2026-02-25T15:56:01Z

include/nbl/builtin/hlsl/rwmc/SplattingParameters.hlsl

+    int32_t PackedRcpLog2BaseAndLog2BaseRoot;
+    float32_t BrightSampleLumaBias;


why do ou have two members now !?

devshgraphicsprogramming · 2026-02-25T21:43:14Z

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

+                nee_ray.init(origin, direction);
+                nee_ray.template initInteraction<anisotropic_interaction_type>(interaction);
                nee_ray.setT(t);
                tolerance_method_type::template adjust<ray_type>(nee_ray, direction, depth);


isn't the direction already in the ray ?

you need to give it the geometric normal (not the interaction which has the shading normal) if you want to know why read the PBRT book, but offsetting the origin by geo normal keeps your t modification small, while offsetting along the ray can be potentially unbounded with a surface-grazing ray if you want to not self-intersect

devshgraphicsprogramming · 2026-02-25T21:44:41Z

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

-                rayAlive = closestHitProgram(d, sampleIndex, ray, intersection);
+            continuePath = intersection.foundHit();
+            if (continuePath)
+                continuePath &= closestHitProgram(d, sampleIndex, ray, intersection);


you don't even need a &= just a = will do, you're in the if statement, so its already true

devshgraphicsprogramming · 2026-02-25T22:07:36Z

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

-                materialSystem, scene, intersectP, interaction,
-                isBSDF, eps0, depth
+                scene, materialSystem, intersectP, interaction,
+                eps0, depth


you don't need depth

…e internally to use iso

…rsected ray and gives intersection

keptsecret and others added 19 commits December 1, 2025 16:15

changed quaternion struct name, brought in quaternion stuff from prze…

9be65a0

…mek's remove_core_matrix branch, added create from mat3x3

_static_create quaternion to matrix, create quaternion from axis-angle

3e03467

added more quaternion funcs: transformVec, inverse, normalize + stati…

34d1385

…c cast to/from truncated quat

Modified LoadableImage and ResolveAccessorBase concepts

61540d6

Small RWMC fixes

6953879

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/sampling/projected_spherical_triangle.hlsl Outdated Show resolved Hide resolved

devshgraphicsprogramming reviewed Feb 12, 2026

View reviewed changes

include/nbl/builtin/hlsl/bxdf/fresnel.hlsl Show resolved Hide resolved

devshgraphicsprogramming reviewed Feb 12, 2026

View reviewed changes

include/nbl/builtin/hlsl/sampling/projected_spherical_triangle.hlsl Outdated Show resolved Hide resolved

devshgraphicsprogramming reviewed Feb 12, 2026

View reviewed changes

devshgraphicsprogramming reviewed Feb 24, 2026

View reviewed changes

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

devshgraphicsprogramming reviewed Feb 24, 2026

View reviewed changes

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

devshgraphicsprogramming reviewed Feb 24, 2026

View reviewed changes

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

keptsecret added 6 commits February 25, 2026 11:05

scene returns an interaction, instead of just normal

4ceb3f2

minor fixes to spherical rectangle stuff

917acc5

remove unused variable in packed splatting params

4ea58c8

minor changes to concept for intersector, nee; document concept symbols

885e58f

changes to ray methods to separate interaction from init, nee takes a…

9aeb80c

…n interaction instead of just isBSDF

fix normalize throughputCIE, minor changes to method names

575f15b

devshgraphicsprogramming reviewed Feb 25, 2026

View reviewed changes

keptsecret added 4 commits February 26, 2026 12:31

use nbl this macro for fresnel iridescent that uses this

b5a4feb

added some asserts, material system should take aniso types and decid…

120e179

…e internally to use iso

correct pdf method for mat system, added shouldDoMIS for ray payload

5e243d9

added some comments regarding future use of MIS, scene takes and inte…

40d585c

…rsected ray and gives intersection

		quotient_pdf_type bsdf_quotient_pdf = materialSystem.quotient_and_pdf(matID, nee_sample, iso_interaction, _cache.iso_cache);
		neeContrib.quotient = materialSystem.eval(matID, nee_sample, iso_interaction, _cache.iso_cache) rcpChoiceProb;

		int32_t PackedRcpLog2BaseAndLog2BaseRoot;
		float32_t BrightSampleLumaBias;

Conversation

keptsecret commented Jan 22, 2026

Uh oh!

keptsecret commented Feb 2, 2026

Uh oh!

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!

devshgraphicsprogramming Feb 24, 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 24, 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!

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!

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!

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!

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 25, 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!

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

Uh oh!

Choose a reason for hiding this comment

devshgraphicsprogramming Feb 24, 2026 •

edited

Loading

devshgraphicsprogramming Feb 24, 2026 •

edited

Loading

devshgraphicsprogramming Feb 25, 2026 •

edited

Loading

devshgraphicsprogramming Feb 25, 2026 •

edited

Loading