AcademySoftwareFoundation / OpenShadingLanguage

Advanced shading language for production GI renderers
BSD 3-Clause "New" or "Revised" License
2.05k stars 347 forks source link

fix: Fix testrender GPU regression with bad destruction order #1814

Closed lgritz closed 1 month ago

lgritz commented 1 month ago

A few months back, PR #1733 seems to have switched the order that testrender destroys the shading system versus the renderer (services).

This made some subtle bugs that were only symptomatic for GPU renders, but it's because of the destructor order, where the shadingsystem's dtr still references the renderer, which cannot be destroyed yet.

The clue is that the SS's constructor takes the RS pointer as an argument. The RS, then, must have been constructed before the SS, and therefore we should expect it to be a requirement for the RS to outlast the lifetime of the SS. (Complex objects should be destroyed in the opposite order that they were constructed, if they contain references to each other.)

brechtvl commented 1 month ago

I guess this will fix the ASAN error that came back, matching what optixraytracer already does.

diff --git a/src/testrender/simpleraytracer.cpp b/src/testrender/simpleraytracer.cpp
index d9ae6806..3607c4cc 100644
--- a/src/testrender/simpleraytracer.cpp
+++ b/src/testrender/simpleraytracer.cpp
@@ -1100,5 +1100,10 @@ SimpleRaytracer::render(int xres, int yres)
 }

+void
+SimpleRaytracer::clear()
+{
+  shaders().clear();
+}

 OSL_NAMESPACE_EXIT
diff --git a/src/testrender/simpleraytracer.h b/src/testrender/simpleraytracer.h
index fbddf68c..e9cae486 100644
--- a/src/testrender/simpleraytracer.h
+++ b/src/testrender/simpleraytracer.h
@@ -82,7 +82,7 @@ public:
     virtual void prepare_render();
     virtual void warmup() {}
     virtual void render(int xres, int yres);
-    virtual void clear() {}
+    virtual void clear();

     // After render, get the pixels into pixelbuf, if they aren't already.
     virtual void finalize_pixel_buffer() {}