Add testrender volumes #2053

ThyOwen · 2025-11-30T23:33:20Z

Description

Alongside the rest of the initialization for subpixel_radiance, an empty MediumStack is initialized. Its behavior is conceptually similar to the existing CompositeBSDF. Media are stored in a statically allocated char pool, with pointers maintained in mediums[MaxMediums]. In processing medium closures, either HomogeneousMedium or EmptyMedium are inserted into the medium stack. There is a simple priority handling scheme that will shuffle the incumbent medium pointers to give high priority mediums lower indices. They both inherit from a shared CRTP interface, Medium. When processing BSDF closures, intersections with MxDielectric or MxGeneralizedSchlick may only be added when priority < current_priority or are both 0 (the precious priority). Attached to the MediumStack, an integrate method transmittance samples each medium and scatters using the phase function of the highest priority medium. As of now, only HenyeyGreenstein is implemented and inherits from BSDF.

With this being both a sizable update and my first commit to the project, I’m more than happy to iterate on this.

Tests

render-mx-medium-glass, render-mx-medium-vdf, render-mx-medium-anisotropic with OptiX references of the latter two. The images for render-mx-generalized-schlick-glass and render-mx-dielectric-schlick-glass have been updated as well.

Checklist:

I have read the contribution guidelines.
I have updated the documentation, if applicable.
I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
My code follows the prevailing code style of this project. If I haven't
already run clang-format v17 before submitting, I definitely will look at
the CI test that runs clang-format and fix anything that it highlights as
being nonconforming.

lgritz · 2025-12-31T19:57:20Z

Hi, just checking up on this. It's still marked as draft, still needs a DCO sign-off on all commits, and it's failing some CI tests.

DCO -- the easiest fix is on your end to squash all the little commits into one, make sure that one has the "Signed-off-by: Your Name your@email", then do a force push to the same branch to replace what you have.

CI tests -- I suspect that some numerical differences between platforms simply require more than one reference output. I can help with this, if you can fix the other problems.

ThyOwen · 2026-01-15T06:41:22Z

Thanks for the follow-up and guidance. I’ve squashed the commits into one and signed off. Are there any other changes I should address aside from the CI issues?

aconty · 2026-01-16T12:13:21Z

src/testrender/shading.cpp

+OSL_HOSTDEVICE BSDF::Sample
+Medium::sample_phase_func_vrtl(const Vec3& wo, float rx, float ry, float rz) const
+{
+    return dispatch([&](auto medium) { return medium.sample_phase_func(wo, rx, ry, rz); });


Why not const auto& ?

Also, happy to see more people using this "static-virtual" trick. Sometimes when it doesn't like what you do the compile errors are nightmare-ish.

aconty · 2026-01-16T12:19:56Z

src/testrender/shading.h

+            scatter           = s.t < hit.t || scatter;
+            combined_sample.t = s.t < combined_sample.t ? s.t
+                                                        : combined_sample.t;
+        }


Interesting, mediums are combined? I thought they were exclusive with the priority. I must be missing something.

aconty · 2026-01-16T12:30:18Z

Very nice addition. I would just ask for clarification on the stack, priorities and policy. In my mind, an ice cube with medium A floating in water with medium B pushes the water away. So in the stack inside the ice you have

A (highest prio)
B

But only A is active. When you exit the ice your stack is only B, making the water active. No?

Alongside the rest of the initialization for subpixel_radiance, an empty MediumStack is initialized. Its behavior is conceptually similar to the existing CompositeBSDF. Media are stored in a statically allocated char pool, with pointers maintained in mediums[MaxMediums]. In processing medium closures, either HomogeneousMedium or EmptyMedium are inserted into the medium stack. There is a simple priority handling scheme that will shuffle the incumbent medium pointers to give high priority mediums lower indices. They both inherit from a shared CRTP interface, Medium. When processing BSDF closures, intersections with MxDielectric or MxGeneralizedSchlick may only be added when priority < current_priority or are both 0 (the precious priority). MediumStack exposes an integrate method that operates on aggregated parameters from all media sharing the same priority as the topmost medium; these aggregated parameters are stored on the stack and updated whenever media are added. A CDF built from these parameters is then sampled to select one of the overlapping media for scattering. At present, only Henyey–Greenstein phase functions are supported, implemented as a BSDF subclass. Signed-off-by: Owen O'Malley <theowen@email.com>

ThyOwen · 2026-01-25T01:58:18Z

Yes — This is true. I've made some changes.

There were two problems with what I had. In the ice/water example, inside the ice the stack contains A (highest priority) and B, but only A is active, so B no longer incorrectly contributes. Also, what I had before didn't track the entry order and just decremented depth by 1. Which in the case of A (highest), B(middle), and C(lowest). If the ray enters B, C, then A. After sorting it will be A, B, C. If the ray exists B first C will be the one taken off the stack. Now when the ray exists B. the pointer to B is nullified and C takes its place. Does this sound right?

Mediums are now only combined when they share the same priority as the topmost medium. When that happens, a CDF is computed and sampled for scattering. This CDF is recomputed whenever add_medium or pop_medium is called.
Also, I had the false-intersection logic backwards. Priority 0 now disables false intersections rather than enabling them.

A ring of volumes with the same priority.

The upper left spheres are the same and non precious. Upper right spheres are the same priority and are precious (priority == 0).

lgritz · 2026-01-25T02:01:29Z

I'll let @fpsunflower and @aconty comment in detail, but glancing and the results, I think this is a lot more along the lines of what we expect for this feature in a renderer.

lgritz · 2026-01-27T19:51:48Z

@ThyOwen Do you know what's up with the CI failures?

fpsunflower

The general setup seems like its going in the right direction - see comments below for some things I think should be checked though.

Also - I would make sure the code works with closures like glass than can both reflect and refract. At a high level it looks like the medium update happens when we hit the surface (depending on the backfacing flag). I would expect the medium stack to change only if we pick a direction that actually crosses from one side to another. I might have missed something though.

fpsunflower · 2026-01-29T19:32:44Z

src/testrender/shading.cpp

 };

+
+struct HenyeyGreenstein final : public BSDF {


There is already an implementation of this in the BSDL headers - is it possible to re-use that one?

fpsunflower · 2026-01-29T19:34:58Z

src/testrender/shading.cpp

+};
+
+struct EmptyMedium final : public Medium {
+    MediumParams params;


If even the empty medium needs to have MediumParams - maybe the field should be in the base class?

Do we even need a class hierarchy here given that this medium tracking is only ever going to support homogeneous media?

fpsunflower · 2026-01-29T19:40:29Z

src/testrender/shading.h

+                num_overlapping++;
+            }
+
+            if (num_overlapping > 1) {


I don't think this is needed. You can just merge the sigma_t's by summing them up and then sample from that summed value.

When sampling the phase function, you want to take into account all the different overlapping sigma_s's and pick one.

fpsunflower · 2026-01-29T19:41:44Z

src/testrender/shading.h

+
+        Vec3 rand_vol  = sampler.get();
+        float t_volume = -logf(1.0f - rand_vol.x)
+                         / current_params.avg_sigma_t();


It would be better to perform MIS among the 3 color channels. Sampling using the average will not work well if there is a strong difference between the channels.

fpsunflower · 2026-01-29T19:43:40Z

src/testrender/shading.h

+            weight = combined_albedo / tr;
+        } else {
+            tr     = transmittance(current_params.sigma_t, hit.t);
+            weight = Color3(1.0 / tr.x, 1.0 / tr.y, 1.0 / tr.z);


I might be missing something, but this doesn't look correct. The weight should be the transmittance divided by the PDF. This is computing the 1 / transmittance which seems odd.

fpsunflower · 2026-01-29T19:44:58Z

src/testrender/shading.h

+            tr       = transmittance(current_params.sigma_t, t_volume);
+            Color3 combined_albedo = current_params.sigma_s
+                                     / current_params.sigma_t;
+            weight = combined_albedo / tr;


I think it would be simpler to merge this block and the one bellow so all the scatter logic is together.

Like below, I'm not quite following the logic on how the weight is computed here.

The weight should be transmittance / pdf, not albedo / transmittance.

To include MIS between the color channels, you want to consider sampling from r,g,b for sigma_t independently. The weighting between these should depend on the combined sigma_s (so that channels that don't have any scattering will not contribute). This way - if you don't have any scattering - you won't generate any sample for transmittance and can pass straight through without an additional random decision.

fpsunflower · 2026-01-29T19:52:10Z

src/testrender/shading.h

+                                                                rand_phase.y,
+                                                                rand_phase.z);
+
+            if (phase_sample.pdf <= 0.0f) {


Did you see negative values in practice?

I would write the test as if (pdf > 0) { ... return true; } return false; to be defensive against any NaN values.

ThyOwen force-pushed the add-testrender-volumes branch from 528f6d6 to 64f56b6 Compare January 15, 2026 06:27

ThyOwen marked this pull request as ready for review January 15, 2026 06:40

lgritz requested review from aconty and fpsunflower January 16, 2026 08:43

aconty reviewed Jan 16, 2026

View reviewed changes

ThyOwen force-pushed the add-testrender-volumes branch from 64f56b6 to 99e45d5 Compare January 25, 2026 01:57

fpsunflower reviewed Jan 29, 2026

View reviewed changes

Add testrender volumes #2053

Are you sure you want to change the base?

Add testrender volumes #2053

Uh oh!

Conversation

ThyOwen commented Nov 30, 2025

Description

Tests

Checklist:

Uh oh!

lgritz commented Dec 31, 2025

Uh oh!

ThyOwen commented Jan 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aconty commented Jan 16, 2026

Uh oh!

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

Uh oh!

Uh oh!

lgritz commented Jan 25, 2026

Uh oh!

lgritz commented Jan 27, 2026

Uh oh!

fpsunflower left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ThyOwen commented Jan 25, 2026 •

edited

Loading