Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There is a known issue in Mitsuba that a reference to the Python object is leaked when a Python-defined plugin is instantiated.
The original reason we had to leak this reference was to keep the Python object alive beyond the scope of the initialization call in
python.h
. The problem is that due to this leak, the Python object is never deallocated and may end up in a partially invalid state (e.g., see the nested emitter example in #771).Ideally, we could fully transfer the ownership of the Python object to C++ and Mitsuba's
ref
object. This doesn't seem to be possible in pybind11 and as far as I can tell is a known limitation. It will be fixed in nanobind (see the docs).This PR implements a relatively straightforward fix that should work until the transition to Nanobind is complete. The solution that seems to work is to keep a Python object handle in a cleanup callback lambda, and then explicitly invoke the cleanup call when the object's reference count decreases from 2 to 1 (i.e., the only remaining reference is the Python object). This isn't very elegant but appears to solve the problem on my end.
Not sure if we want to merge this, but I thought I would share this since the transition to Nanobind might still take some time (?). The leaked reference ended up causing some unforeseen issues on my end, so maybe others can benefit from a fix as well.