mitsuba-renderer / mitsuba3

Mitsuba 3: A Retargetable Forward and Inverse Renderer
https://www.mitsuba-renderer.org/
Other
2.09k stars 246 forks source link

Use variant-specific registry domains #1362

Closed merlinND closed 2 weeks ago

merlinND commented 3 weeks ago

Description

Previously, we could easily segfault when performing a vcall if an object was previously created with another variant. For example, a spectral_polarized BSDF and an rgb BSDF.

Other than fixing the case below, I don't think this change could hurt since we never need or want instances from different variants to participate in the same vcalls (?).

Testing

import drjit as dr
import mitsuba as mi

def test01_test_variants_switching():
    mi.set_variant('cuda_ad_mono_polarized')
    # Unused, but registers objects into the registry
    scene1 = mi.load_dict(mi.cornell_box())

    mi.set_variant('cuda_ad_rgb')
    scene2 = mi.load_dict(mi.cornell_box())

    # Before this PR, this would segfault when e.g. collecting the results of the BSDF vcall,
    # because the `Spectrum` types of the BSDF instances were not compatible.
    image = mi.render(scene2)
    print(image)

if __name__ == "__main__":
    test01_test_variants_switching()

Checklist

merlinND commented 3 weeks ago

This ended up being a fair bit more complicated that I expected, because now that the domain name depends on the Float, Spectrum template arguments, it cannot directly be set in the call support macros from the Name macro parameter.

But because call_support<>::Domain is used as a static constexpr field in a few places, we still have to compute the domain name in a constexpr way. In C++17, it's possible to do constexpr string concatenation via std::array<>.

If we find a simpler solution (either to the original issue or any of the implementation steps of this PR), I would be all for it.

merlinND commented 3 weeks ago

Thanks for having a look @njroussel!

I updated the macros to use MI_ instead of MTS and used domain_() instead of __domain() (matching the other DrJit methods like abs_, sin_, etc).

I tried another implementation of the constexpr string concatenation (using more std::array<char, N>) that will hopefully work across compilers.

merlinND commented 3 weeks ago

CI fails look like:

merlinND commented 3 weeks ago

Using fewer intermediate static constexpr std::array variables I can get MSVC build without crash locally, let's see if it works on the CI as well.

Edit: CI is green!

merlinND commented 3 weeks ago

Hi @njroussel,

Does the current state of this PR (+ the smaller one on the DrJit side: https://github.com/mitsuba-renderer/drjit/pull/307) look good to you?