Created concepts for samplers, added quotient_and_pdf variants to satisfy the concepts#1001
Created concepts for samplers, added quotient_and_pdf variants to satisfy the concepts#1001karimsayedre wants to merge 39 commits intomasterfrom
Conversation
…isfy the concepts
…concepts - Move codomain_and_*Pdf and domain_and_*Pdf structs into their own warp_and_pdf.hlsl header - Keeping quotient_and_pdf.hlsl focused on importance sampling quotients for BxDFs - Add SampleWithPDF, SampleWithRcpPDF, and SampleWithDensity concepts to validate sample types - Used concept composition (NBL_CONCEPT_REQ_TYPE_ALIAS_CONCEPT) to build ResamplableSampler on TractableSampler and BijectiveSampler on ResamplableSampler
… projected/spherical triangle
| retval.r0 = hlsl::mul(rect.basis, rect.origin - observer); | ||
| const vector4_type denorm_n_z = vector4_type(-retval.r0.y, retval.r0.x + rect.extents.x, retval.r0.y + rect.extents.y, -retval.r0.x); | ||
| const vector4_type n_z = denorm_n_z / hlsl::sqrt<vector4_type>(hlsl::promote<vector4_type>(retval.r0.z * retval.r0.z) + denorm_n_z * denorm_n_z); | ||
| retval.cosGamma = vector4_type( | ||
| -n_z[0] * n_z[1], | ||
| -n_z[1] * n_z[2], | ||
| -n_z[2] * n_z[3], | ||
| -n_z[3] * n_z[0] | ||
| ); |
There was a problem hiding this comment.
everything up to here is duplicate computation with shapes::SPhericalRectangle::solidAngle
| math::sincos_accumulator<scalar_type> angle_adder = math::sincos_accumulator<scalar_type>::create(cosGamma[0]); | ||
| angle_adder.addCosine(cosGamma[1]); | ||
| math::sincos_accumulator<scalar_type> angle_adder = math::sincos_accumulator<scalar_type>::create(retval.cosGamma[0]); | ||
| angle_adder.addCosine(retval.cosGamma[1]); | ||
| scalar_type p = angle_adder.getSumofArccos(); | ||
| angle_adder = math::sincos_accumulator<scalar_type>::create(cosGamma[2]); | ||
| angle_adder.addCosine(cosGamma[3]); | ||
| angle_adder = math::sincos_accumulator<scalar_type>::create(retval.cosGamma[2]); | ||
| angle_adder.addCosine(retval.cosGamma[3]); | ||
| scalar_type q = angle_adder.getSumofArccos(); | ||
|
|
||
| const scalar_type k = scalar_type(2.0) * numbers::pi<scalar_type> - q; | ||
| const scalar_type b0 = n_z[0]; | ||
| const scalar_type b1 = n_z[2]; | ||
| S = p + q - scalar_type(2.0) * numbers::pi<scalar_type>; | ||
|
|
||
| const scalar_type CLAMP_EPS = 1e-5; | ||
| retval.solidAngle = p + q - scalar_type(2.0) * numbers::pi<scalar_type>; |
There was a problem hiding this comment.
you can use rect.solidAngle() and then not use the solidAngle member and recompute it from these
There was a problem hiding this comment.
also why is q not a member, you compute it and then don't use it!
| math::sincos_accumulator<scalar_type> angle_adder = math::sincos_accumulator<scalar_type>::create(cosGamma[2]); | ||
| angle_adder.addCosine(cosGamma[3]); | ||
| scalar_type q = angle_adder.getSumofArccos(); |
There was a problem hiding this comment.
q is already computed in create
| const scalar_type yv = hlsl::mix(r1.y, (hv * d) / hlsl::sqrt<scalar_type>(scalar_type(1.0) - hv2), hv2 < scalar_type(1.0) - CLAMP_EPS); | ||
| const scalar_type yv = hlsl::mix(r1.y, (hv * d) / hlsl::sqrt<scalar_type>(scalar_type(1.0) - hv2), hv2 < scalar_type(1.0) - ClampEps); | ||
|
|
||
| cache.pdf = scalar_type(1.0) / solidAngle; |
| retval.rect = rect; | ||
| return retval; | ||
| } | ||
| density_type pdf; |
There was a problem hiding this comment.
make cache empty
| scalar_type b0; | ||
| scalar_type b1; | ||
| vector3_type r0; | ||
| vector3_type r1; |
There was a problem hiding this comment.
cosGamma don't need to be there, only k, b0, b1 and b0squared
| const scalar_type au = u.x * solidAngle + k; | ||
| const scalar_type fu = (hlsl::cos<scalar_type>(au) * b0 - b1) / hlsl::sin<scalar_type>(au); | ||
| const scalar_type cu_2 = hlsl::max<scalar_type>(fu * fu + b0 * b0, 1.f); // forces `cu` to be in [-1,1] | ||
| const scalar_type cu = ieee754::flipSignIfRHSNegative<scalar_type>(scalar_type(1.0) / hlsl::sqrt<scalar_type>(cu_2), fu); |
There was a problem hiding this comment.
use inversesqrt instead of scalar_type(1.0) / hlsl::sqrt<scalar_type>(cu_2)
There was a problem hiding this comment.
also cu*sign(fu) can possibly be faster than integer bit tricks, try to profile
There was a problem hiding this comment.
also fix the naming, cu_2 is actually rcpCu_2 from what I see (but carried over from my code) double check with the paper
|
|
||
| const scalar_type au = uv.x * S + k; | ||
| const scalar_type au = u.x * solidAngle + k; | ||
| const scalar_type fu = (hlsl::cos<scalar_type>(au) * b0 - b1) / hlsl::sin<scalar_type>(au); |
There was a problem hiding this comment.
read the paper, or deduce if au can't be more than PI or less than 0
if indeed 0 <= au <=PI then you can use inversesqrt(1.f-cos*cos) instead of division by sin and profile/benchmark
| const scalar_type cu_2 = hlsl::max<scalar_type>(fu * fu + b0 * b0, 1.f); // forces `cu` to be in [-1,1] | ||
| const scalar_type cu = ieee754::flipSignIfRHSNegative<scalar_type>(scalar_type(1.0) / hlsl::sqrt<scalar_type>(cu_2), fu); | ||
|
|
||
| scalar_type xu = -(cu * r0.z) / hlsl::sqrt<scalar_type>(scalar_type(1.0) - cu * cu); |
There was a problem hiding this comment.
I see an opportunity for algebraic optimization
r0.z * sign(negFu) * inversesqrt(rcpCu_2 - 1.0);
Note that rcpCu_2 is probably current cu_2 because thats wrongly named
this way you should get dif of the cu variable alltogether
and compute negFu instead of fu by reversing the order of the subtraction in current fu equation
(note that the latter cu_2 computation uses fu*fu which stacks constant regardless of sign of fu )
| r0.z = -hlsl::abs(r0.z); | ||
| vector3_type r1 = r0 + vector3_type(rect.extents.x, rect.extents.y, 0); | ||
| retval.r0 = hlsl::promote<vector3_type>(-hlsl::abs(retval.r0.z)); |
There was a problem hiding this comment.
I'm 99% sure this change broke everything and the solid angle rectangle sampler is broken
you're assigning -abs(r0.z) to all 3 components
it was just supposed to be r0.z = ieee754::negativeAbs(r0.z) I think, thats all
There was a problem hiding this comment.
could also just abs(r0.z) and correct everything that uses r0.z to flip its sign
|
|
||
| scalar_type xu = -(cu * r0.z) / hlsl::sqrt<scalar_type>(scalar_type(1.0) - cu * cu); | ||
| xu = hlsl::clamp<scalar_type>(xu, r0.x, r1.x); // avoid Infs | ||
| const scalar_type d_2 = xu * xu + r0.z * r0.z; |
There was a problem hiding this comment.
could be computed as r0_z_squared*(1/(rcpCu_2-1)+1) and the r0_z_squared kept as a member
Actually algebra-ing more
d_2 = r0_z_squared*(rcpCu_2*rcpCu_2-rcpCu_2)
or
d_2 = r0_z_squared*rcpCu_2*(rcpCu_2-1)
| @@ -74,18 +90,45 @@ | |||
|
|
|||
| const scalar_type h0 = r0.y / hlsl::sqrt<scalar_type>(d_2 + r0.y * r0.y); | |||
| const scalar_type h1 = r1.y / hlsl::sqrt<scalar_type>(d_2 + r1.y * r1.y); | |||
There was a problem hiding this comment.
looks like you should also keep a r0_y_squared and r1_y_squared
btw everthing that happens to h0 and h1 here, is sampling a line's projection on the sphere, useful if you want to make the rectangle sampling robust to being thing (it would involve flipping the local X and Y axes of the rectangle / basis rows such that the y-direction is always longer)
There was a problem hiding this comment.
also use inversesqrt here
|
|
||
| struct cache_type | ||
| { | ||
| density_type pdf; |
There was a problem hiding this comment.
pdf is constant, have an empty cache
|
|
||
| vector3_type retval = tri_vertices[1]; | ||
| const scalar_type cosBC_s = nbl::hlsl::dot(C_s, tri_vertices[1]); | ||
| const scalar_type csc_b_s = 1.0 / nbl::hlsl::sqrt(1.0 - cosBC_s * cosBC_s); |
There was a problem hiding this comment.
can we use inversesqrt without messing up the precision ?
| domain_type _generateInverse(const codomain_type L) | ||
| { | ||
| const scalar_type cosAngleAlongBC_s = nbl::hlsl::dot(L, tri_vertices[1]); | ||
| const scalar_type csc_a_ = 1.0 / nbl::hlsl::sqrt(1.0 - cosAngleAlongBC_s * cosAngleAlongBC_s); |
There was a problem hiding this comment.
inversesqrt without messing up precision ?
| angle_adder.addCosine(cosGamma[1]); | ||
| angle_adder.addCosine(cosGamma[2]); | ||
| angle_adder.addCosine(cosGamma[3]); | ||
| return angle_adder.getSumofArccos() - scalar_type(2.0) * numbers::pi<float>; |
There was a problem hiding this comment.
template the sincos_accumulator::getSumofArccos which acos implementation it should use (let default template arg be impl::acos_helper) and try some fast inverse trig functions
| math::sincos_accumulator<scalar_type> angle_adder = math::sincos_accumulator<scalar_type>::create(cosA, sinA); | ||
| angle_adder.addAngle(cosB_, sinB_); | ||
| angle_adder.addAngle(cosC_, sinC_); | ||
| const scalar_type subTriSolidAngleRatio = (angle_adder.getSumofArccos() - numbers::pi<scalar_type>) * (scalar_type(1.0) / solidAngle); |
There was a problem hiding this comment.
lets store rcpSolidAngle instead of `solidAngle
| // 1 ULP below 1.0, ensures (1.0 - cosBC_s) is strictly positive in float | ||
| const scalar_type one_below_one = bit_cast<scalar_type>(bit_cast<uint_type>(scalar_type(1)) - uint_type(1)); |
There was a problem hiding this comment.
don't we have a ieee754 function for this ?
There was a problem hiding this comment.
just wrote this in `ieee754.hlsl`, looks good?
There was a problem hiding this comment.
yes great, you should probably require that T is scalar
maaaybe make a nextTowardZero then you don't need care about sign or negativity
| { | ||
| const scalar_type cosAngleAlongAC = ((v_ * q - u_ * p) * cosA - v_) / ((v_ * p + u_ * q) * sinA); | ||
| if (nbl::hlsl::abs(cosAngleAlongAC) < 1.f) | ||
| C_s += math::quaternion<scalar_type>::slerp_delta(tri_vertices[0], tri_vertices[2] * triCscB, cosAngleAlongAC); |
There was a problem hiding this comment.
store tri_vertices[2] * triCscB instead of tri_vertices[2] and triCscB
There was a problem hiding this comment.
the triCscB < numeric_limits<scalar_type>::max check can be done with cosA<1.f-epsilon instead
There was a problem hiding this comment.
leave a comment that there's plenty of opportunity for optimization given that first two arguments to slerp_delta are constant
| const scalar_type cosAngleAlongBC_s = nbl::hlsl::clamp(scalar_type(1.0) + cosBC_s * u.y - u.y, scalar_type(-1.0), scalar_type(1.0)); | ||
| if (nbl::hlsl::abs(cosAngleAlongBC_s) < scalar_type(1.0)) |
There was a problem hiding this comment.
you don't need the clamp, you have an abs(...)<1.f right before you use it for anything
| vector3_type tri_vertices[3]; | ||
| scalar_type triCosC; | ||
| scalar_type triCscB; | ||
| scalar_type triCscC; |
There was a problem hiding this comment.
take a template whether you want the SphericalTriangle to be bijective (have a generateInverse method) and implement the Bijective in terms of Resamplable & Tractable + the triCscC member
|
|
||
| weight_type backwardWeight(const vector3_type L) | ||
| { | ||
| return backwardPdf(L); |
There was a problem hiding this comment.
now here's our time to shine!
The reason for this whole damn refactor and distrinction between PDF and MIS weight
you can simply return abs(L.z)*triangleProjectedSolidAngle for the weight!
You would need to compute the triangleProjectedSolidAngle during create though.
There was a problem hiding this comment.
I'd implenent backwardWeight in terms of forwardWeight (can fill unused cache variables with garbage)
| retval.sphtri = sampling::SphericalTriangle<T>::create(tri); | ||
| return retval; | ||
| } | ||
| density_type pdf; |
There was a problem hiding this comment.
you want this to be basically only L_z for the weight functions and bilinear cache to get the forward PDF
| // NOTE: produces a degenerate (all-zero) bilinear patch when the receiver normal faces away | ||
| // from all three triangle vertices, resulting in NaN PDFs (0 * inf). Callers must ensure | ||
| // at least one vertex has positive projection onto the receiver normal. | ||
| Bilinear<scalar_type> computeBilinearPatch() |
There was a problem hiding this comment.
this is why this sampler shouldn't hand out a PDF but only weights
There was a problem hiding this comment.
add a __ prefix to the function
There was a problem hiding this comment.
also why aren't you storing the bilinear patch as a member and re-creating it on the fly?
the receiver normal doesn't chance between calls to generate
There was a problem hiding this comment.
Also where's a static create method and creation params ?
| const vector2_type u = sphtri.generateInverse(L); | ||
| Bilinear<scalar_type> bilinear = computeBilinearPatch(); | ||
| return pdf * bilinear.backwardPdf(u); | ||
| } |
There was a problem hiding this comment.
this function is not needed at all
| density_type forwardPdf(const cache_type cache) | ||
| { | ||
| const scalar_type cos_c = sphtri.tri.cos_sides[2]; | ||
| const scalar_type csc_b = sphtri.tri.csc_sides[1]; | ||
| const scalar_type solidAngle = sphtri.tri.solidAngle(); | ||
| return generate(rcpPdf, solidAngle, cos_c, csc_b, receiverNormal, isBSDF, u); | ||
| return cache.pdf; |
There was a problem hiding this comment.
we'd get the cache.blinearCache and query the PDF of that and multiply it with sphtri.rcpSolidAngle
|
|
||
| sampling::SphericalTriangle<T> sphtri; | ||
| vector3_type receiverNormal; | ||
| bool receiverWasBSDF; |
There was a problem hiding this comment.
don't need receiverNormal and receiverWasBSDF members, need bilinearPatch and rcpProjSolidAngle instead
There was a problem hiding this comment.
so its useful to have a create (which computes bilinear for you from receiverNormal and wasBSDF) but not the kind I told before to remove
There was a problem hiding this comment.
# Conflicts: # examples_tests # include/nbl/builtin/hlsl/shapes/spherical_triangle.hlsl
3 participants
Examples PR
Notes:
quotient_and_pdf()methods inUniformHemisphere,UniformSphere,ProjectedHemisphere, andProjectedSphereshadow the struct typesampling::quotient_and_pdf<Q, P>fromquotient_and_pdf.hlsl. DXC can't resolve the return type because the method name takes precedence over the struct name during lookup. Fixed by fully qualifying with::nbl::hlsl::sampling::quotient_and_pdf<U, T>.