Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New tgmath #165

Open
wants to merge 43 commits into
base: master
Choose a base branch
from
Open

New tgmath #165

wants to merge 43 commits into from

Conversation

Przemog1
Copy link
Contributor

No description provided.

@devshgraphicsprogramming
Copy link
Member

devshgraphicsprogramming commented Jan 6, 2025

why do you have random things from sorakrit here instead of your own branch against his or master ?

I literally can't see whats yours and what's @keptsecret's.

Comment on lines 46 to 48
retval.T = nbl::hlsl::normalize<float32_t3>(rngFloat301(retval.rng));

retval.T = nbl::hlsl::normalize<float32_t3>(retval.T - nbl::hlsl::dot<float32_t3>(retval.T, retval.N) * retval.N); // gram schmidt

Choose a reason for hiding this comment

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

you can still generate T almost colinear to N and end up with numerical issues, personally I'd use the frisvad basis function + random rotation around N (only one random number needed)

Comment on lines 53 to 56
retval.eta = 1.3;// rngFloat01(retval.rng) + 1.0;
retval.ior = float32_t3x2(1.02, 1.0, // randomize at some point?
1.3, 2.0,
1.02, 1.0);

Choose a reason for hiding this comment

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

you can only have one:

  • eta for BSDFs (things that refract) because refraction can only be done monochrome (each wavelength refracts at different angle)
  • ior for BRDFs (things that reflect) because reflection is alawys the same

Also eta is just a ratio of two refractive indices, and using matrix<float32_T,3,2> is very bad semantically because it assumes we'll have 3 spectral buckets.

Ideally we should do tests in monochrome, so just a float eta should be enough or vector<float,2> ior

Copy link
Contributor

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

no dont keep it was a super bad idea, I also want to make everything templated so that vector<T,3> isn't implied for the colours and we could do other number of spectral buckets (1, 2 or 4 for spectral rendering)

SBxDFTestResources retval;

retval.rng = nbl::hlsl::Xoroshiro64Star::construct(seed);
retval.u = float32_t3(rngFloat01(retval.rng), rngFloat01(retval.rng), 0.0);

Choose a reason for hiding this comment

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

the third random number is sometimes necessary to discern discrete BxDF branches:

  • reflection vs refraction in e.g. glass
  • diffuse vs specular in e.g. plastic
  • choosing a leaf BxDF in a mixture/tree of BxDFs

Comment on lines 60 to 69
ray_dir_info_t dV(int axis)
{
float32_t3 d = (float32_t3)0.0;
d[axis] += h;
ray_dir_info_t retval;
retval.direction = V.direction + d;
return retval;
}

float h = 0.001;

Choose a reason for hiding this comment

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

  1. what do you need dV for?
  2. this is not the way to vary V parameter

@@ -78,6 +80,9 @@ struct SBxDFTestResources
float32_t2 alpha;
float eta;
float32_t3x2 ior;

float32_t3 eta2; // what is this?

Choose a reason for hiding this comment

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

eta squared, compute on the fly, don't leave stuff like this here

Comment on lines 351 to 354
t.meta.result = t.test();
t.meta.testName = "u offset";
return t.meta;
}

Choose a reason for hiding this comment

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

setting those thousands of times will be expensive, function should be void and not return anything, callback instead of meta

Comment on lines 342 to 345
static STestMeta run(uint32_t2 seed)
{
this_t t;
t.init(seed);

Choose a reason for hiding this comment

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

I'd make the function take uint32_t as an argument, then run a hash function (wide not deep)
https://rene.ruhr/gfx/gpuhash/

This will make it easy to invoke multiple run(uint32_t invocationID) in parallel independently, be it on CPU with for loop or GPU with gl_GlobalInvocationID.x

Comment on lines 358 to 376
template<class BxDF, bool aniso = false>
struct TestVOffset : TestBxDF<BxDF>
{
using base_t = TestBase<BxDF>;
using this_t = TestVOffset<BxDF, aniso>;

float32_t4 test()
{
sample_t s, sx, sy, sz;
quotient_pdf_t pdf;
float32_t3 brdf;
aniso_cache cache, dummy;

iso_interaction isointerx = iso_interaction::create(base_t::rc.dV(0), base_t::rc.N);
aniso_interaction anisointerx = aniso_interaction::create(isointerx, base_t::rc.T, base_t::rc.B);
iso_interaction isointery = iso_interaction::create(base_t::rc.dV(1), base_t::rc.N);
aniso_interaction anisointery = aniso_interaction::create(isointery, base_t::rc.T, base_t::rc.B);
iso_interaction isointerz = iso_interaction::create(base_t::rc.dV(2), base_t::rc.N);
aniso_interaction anisointerz = aniso_interaction::create(isointerz, base_t::rc.T, base_t::rc.B);

Choose a reason for hiding this comment

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

there's no point to this test existing, the test I've outlined ONLY makes sense for varying (differentiating) the 2D random number xi, because thats the uniform distribution that must be stretched and squeezed to form the PDF of the importance sampler


#include "app_resources/tests.hlsl"

#define ASSERT_ZERO(x) (assert(all<bool32_t4>((x.result) < float32_t4(1e-3))));

Choose a reason for hiding this comment

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

use a nice callback

Comment on lines 28 to 42
const uint32_t2 state = uint32_t2(12u, 69u); // (12u, 69u)

// test u offset, 2 axis
ASSERT_ZERO((TestUOffset<bxdf::reflection::SLambertianBxDF<sample_t, iso_interaction, aniso_interaction>>::run(state)));
ASSERT_ZERO((TestUOffset<bxdf::reflection::SOrenNayarBxDF<sample_t, iso_interaction, aniso_interaction>>::run(state)));
printResult(TestUOffset<bxdf::reflection::SBeckmannBxDF<sample_t, iso_cache, aniso_cache>,false>::run(state));
ASSERT_ZERO((TestUOffset<bxdf::reflection::SBeckmannBxDF<sample_t, iso_cache, aniso_cache>,true>::run(state)));
printResult(TestUOffset<bxdf::reflection::SGGXBxDF<sample_t, iso_cache, aniso_cache>,false>::run(state));
ASSERT_ZERO((TestUOffset<bxdf::reflection::SGGXBxDF<sample_t, iso_cache, aniso_cache>,true>::run(state)));

ASSERT_ZERO((TestUOffset<bxdf::transmission::SLambertianBxDF<sample_t, iso_interaction, aniso_interaction>>::run(state)));
printResult(TestUOffset<bxdf::transmission::SBeckmannDielectricBxDF<sample_t, iso_cache, aniso_cache>,false>::run(state));
ASSERT_ZERO((TestUOffset<bxdf::transmission::SBeckmannDielectricBxDF<sample_t, iso_cache, aniso_cache>,true>::run(state)));
printResult(TestUOffset<bxdf::transmission::SGGXDielectricBxDF<sample_t, iso_cache, aniso_cache>,false>::run(state));
printResult(TestUOffset<bxdf::transmission::SGGXDielectricBxDF<sample_t, iso_cache, aniso_cache>,true>::run(state));

Choose a reason for hiding this comment

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

you need to run the tests thousands of times with different seeds, I'd use for_each over a ranges::iota_view with par_unseq policy to perform these tests on all CPU cores at once.
https://godbolt.org/z/xvxE7jzE3

this connects with https://github.com/Devsh-Graphics-Programming/Nabla-Examples-and-Tests/pull/165/files#r1906914911

Comment on lines 288 to 313
if NBL_CONSTEXPR_FUNC (is_basic_brdf_v<BxDF>)
{
s = base_t::bxdf.generate(base_t::anisointer, base_t::rc.u.xy);
sx = base_t::bxdf.generate(base_t::anisointer, base_t::rc.u.xy + float32_t2(base_t::rc.h,0));
sy = base_t::bxdf.generate(base_t::anisointer, base_t::rc.u.xy + float32_t2(0,base_t::rc.h));
}
if NBL_CONSTEXPR_FUNC (is_microfacet_brdf_v<BxDF>)
{
s = base_t::bxdf.generate(base_t::anisointer, base_t::rc.u.xy, cache);
sx = base_t::bxdf.generate(base_t::anisointer, base_t::rc.u.xy + float32_t2(base_t::rc.h,0), dummy);
sy = base_t::bxdf.generate(base_t::anisointer, base_t::rc.u.xy + float32_t2(0,base_t::rc.h), dummy);
}
if NBL_CONSTEXPR_FUNC (is_basic_bsdf_v<BxDF>)
{
s = base_t::bxdf.generate(base_t::anisointer, base_t::rc.u);
sx = base_t::bxdf.generate(base_t::anisointer, base_t::rc.u + float32_t3(base_t::rc.h,0,0));
sy = base_t::bxdf.generate(base_t::anisointer, base_t::rc.u + float32_t3(0,base_t::rc.h,0));
}
if NBL_CONSTEXPR_FUNC (is_microfacet_bsdf_v<BxDF>)
{
s = base_t::bxdf.generate(base_t::anisointer, base_t::rc.u, cache);
float32_t3 ux = base_t::rc.u + float32_t3(base_t::rc.h,0,0);
sx = base_t::bxdf.generate(base_t::anisointer, ux, dummy);
float32_t3 uy = base_t::rc.u + float32_t3(0,base_t::rc.h,0);
sy = base_t::bxdf.generate(base_t::anisointer, uy, dummy);
}

Choose a reason for hiding this comment

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

h should be a parameter of each individual specialized BxDF test, because its specific to the differential test and also may need to change depending on the BxDF getting tested

Choose a reason for hiding this comment

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

also maybe compute the perturbed u above for readability

float32_t2x2 m = float32_t2x2(sx.TdotL - s.TdotL, sy.TdotL - s.TdotL, sx.BdotL - s.BdotL, sy.BdotL - s.BdotL);
float det = nbl::hlsl::determinant<float32_t2x2>(m);

return float32_t4(nbl::hlsl::abs<float32_t3>(pdf.value() - brdf), nbl::hlsl::abs<float>(det*pdf.pdf/s.NdotL) * 0.5);
Copy link
Member

Choose a reason for hiding this comment

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

this needs to be done better semantically with a callback, one thing for each individually.

Alos pdf.value() does not equal brdf in general!!!!!

P.s. the *0.5 thing on the PDF to Jacobian ratio was a thing for visualization, you should really have a callback which gets triggered if the value is too far from 1.0

Comment on lines 333 to 334
}

Choose a reason for hiding this comment

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

you need to skip checking anything that produces an "impossible" s, so if eval or quotient is 0, you can skip the rest of the function (but not before you check pdf and quotient for 0 and INF respectively - the checks also check for NaN)

add more simple tests for:

  1. pdf>0 because something you generated cannot have 0 probability of getting generated
  2. quotient<INF always because our importance sampler's job is to prevent that!
  3. positivity, all pdf, quotient, and eval need to be >=0
  4. recprocity (eval must be equal if we swap L and V around, make a method/function to swap L and V around in s, the interaction, the bxdf itself and the cache)

Leave lots of comments.

Choose a reason for hiding this comment

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

Also when pdf is close to INF (above some super high threshold) you need to back off the last test you have here

Comment on lines 79 to 80
retval.V.direction = nbl::hlsl::normalize<float32_t3>(projected_hemisphere_generate<float>(rngUniformDist<float32_t2>(retval.rng)));
retval.N = nbl::hlsl::normalize<float32_t3>(projected_hemisphere_generate<float>(rngUniformDist<float32_t2>(retval.rng)));

Choose a reason for hiding this comment

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

not projected hemisphere, just hemisphere.

Also not hemisphere but full sphere, because its good to test that BRDFs handle invalid (below hemisphere) correctly

Comment on lines 25 to 30
uint32_t pcg_hash(uint32_t v)
{
uint32_t state = v * 747796405u + 2891336453u;
uint32_t word = ((state >> ((state >> 28u) + 4u)) ^ state) * 277803737u;
return (word >> 22u) ^ word;
}

Choose a reason for hiding this comment

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

this is useful in its own header like xoroshiro is

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