Skip to content

Comments

Address path tracer merge leftover comments#991

Open
keptsecret wants to merge 58 commits intomasterfrom
path_tracer_leftover_cleanup
Open

Address path tracer merge leftover comments#991
keptsecret wants to merge 58 commits intomasterfrom
path_tracer_leftover_cleanup

Conversation

@keptsecret
Copy link
Contributor

Makes changes left from #966

@keptsecret
Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

most parameters should be members #966 (comment)

Choose a reason for hiding this comment

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

return retval;
}

vector4_type computeBilinearPatch(const vector3_type receiverNormal, bool isBSDF)

Choose a reason for hiding this comment

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

why is this returning vector4_type instead of sampling::bilinear

Comment on lines 29 to 34
static ProjectedSphericalTriangle<T> create(NBL_CONST_REF_ARG(shapes::SphericalTriangle<T>) tri)
{
ProjectedSphericalTriangle<T> retval;
retval.tri = tri;
return retval;
}

Choose a reason for hiding this comment

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

remove, see #966 (comment)

Comment on lines 67 to 68
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);

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

again function with bazillion parameters that are really local members

// TODO: probably will only work with isotropic surfaces, need to do aniso
bool closestHitProgram(uint32_t depth, uint32_t _sample, NBL_REF_ARG(ray_type) ray, NBL_CONST_REF_ARG(scene_type) scene)
// TODO: will only work with isotropic surfaces, need to do aniso
bool closestHitProgram(uint32_t depth, uint32_t _sample, NBL_REF_ARG(ray_type) ray, NBL_CONST_REF_ARG(intersect_data_type) intersectData, NBL_CONST_REF_ARG(scene_type) scene)

Choose a reason for hiding this comment

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

you're storing the scene, so why are you passing it by reference to the function !?

ray_dir_info_type V;
V.setDirection(-ray.direction);
isotropic_interaction_type iso_interaction = isotropic_interaction_type::create(V, N);
const vector3_type intersection = intersectData.intersection;

Choose a reason for hiding this comment

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

METHOD

anisotropic_interaction_type interaction = anisotropic_interaction_type::create(iso_interaction);
interaction.isotropic.luminosityContributionHint = colorspace::scRGBtoXYZ[1];

vector3_type throughput = ray.payload.throughput;

Choose a reason for hiding this comment

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

METHOD

// emissive
const uint32_t lightID = glsl::bitfieldExtract(bsdfLightIDs, 16, 16);
if (lightID != light_type::INVALID_ID)
typename scene_type::mat_light_id_type matLightID = scene.getMatLightIDs(ray.objectID);

Choose a reason for hiding this comment

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

why is what you hit stored in the ray!?

Please make the ray be a CONST reference, this stuff belongs in the intersection data!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we still add to ray.payload.accumulation throughout though

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)
if (bsdf_quotient_pdf.pdf < numeric_limits<scalar_type>::max && getLuma(neeContrib_pdf.quotient) > lumaContributionThreshold && !intersector_type::traceRay(nee_ray, scene).foundHit)

Choose a reason for hiding this comment

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

you want an anyhit ray, and you want the anyhit ray return type to have an operator scalar_t() to give you a shadow percentage

Comment on lines 153 to 156
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;

Choose a reason for hiding this comment

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

I'd make a Tolerance<scalar_type>::adjust(&ray,geoNormalAtOrigin,depth); so it can do whatever internally, and you call like this

ray_type nee_ray;
nee_ray.origin = intersection;
nee_ray.direction = nee_sample.getL().getDirection();
nee_ray.intersectionT = t;
Tolerance<scalar_type>::adjust(ray,geoNormalAtOrigin,depth);

ray_type ray = rayGen.generate(uvw);
ray.initPayload();

nee.scene = scene;

Choose a reason for hiding this comment

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

okay, because of the copying, I'll allow passing the scene as NBL_CONST_REF_ARG to nee's methods as the first parameter

ray.normalAtOrigin = interaction.getN();
ray.wasBSDFAtOrigin = isBSDF;
}
vector3_type origin = intersection + bxdfSample * (1.0/*kSceneSize*/) * Tolerance<scalar_type>::getStart(depth);

Choose a reason for hiding this comment

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

do the tolerance stuff with adjust as per previous comment

vector3_type origin = intersection + bxdfSample * (1.0/*kSceneSize*/) * Tolerance<scalar_type>::getStart(depth);
vector3_type direction = bxdfSample;

ray.initData(origin, direction, interaction.getN(), isBSDF);

Choose a reason for hiding this comment

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

pass whole interaction and something about the material syustem like ID or other stuff instead of just passing the isBSDF

@keptsecret keptsecret mentioned this pull request Feb 20, 2026
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.

4 participants