better support for render-to-texture: keep buffers around until texture is gone too

panda/src/display/config_display.cxx

@@ -87,12 +87,8 @@ ConfigVariableBool show_buffers
           "GraphicsWindows, if possible, so that their contents may be viewed "
           "interactively.  Handy during development of multipass algorithms."));
 
-// Temporarily false by default until this code proves to be more
-// robust on different graphics drivers.  In particular, it seems to
-// cause problems on Jason's ATI FireGL and on Joe's Compaq laptop so
-// far.
 ConfigVariableBool prefer_texture_buffer
-("prefer-texture-buffer", false,
+("prefer-texture-buffer", true,
  PRC_DESC("Set this true to make GraphicsOutput::make_texture_buffer() always "
           "try to create an offscreen buffer supporting render-to-texture, "
           "if the graphics card claims to be able to support this feature.  "
@@ -123,6 +119,16 @@ ConfigVariableBool prefer_single_buffer
           "false (since in that case the buffer can share a graphics context "
           "with the window)."));
 
+// Temporarily false by default until this code proves to be more
+// robust on different graphics drivers.  In particular, it seems to
+// cause problems on Jason's ATI FireGL and on Joe's Compaq laptop so
+// far.
+ConfigVariableBool support_render_texture
+("support-render-texture", false,
+ PRC_DESC("Set this true allow use of the render-to-a-texture feature, if it "
+          "is supported by your graphics card.  Without this enabled, "
+          "offscreen renders will be copied to a texture instead of directly "
+          "rendered there."));
 
 ConfigVariableBool copy_texture_inverted
 ("copy-texture-inverted", false,

panda/src/display/config_display.h

@@ -52,6 +52,7 @@ extern EXPCL_PANDA ConfigVariableBool prefer_texture_buffer;
 extern EXPCL_PANDA ConfigVariableBool prefer_parasite_buffer;
 extern EXPCL_PANDA ConfigVariableBool prefer_single_buffer;
 
+extern EXPCL_PANDA ConfigVariableBool support_render_texture;
 extern EXPCL_PANDA ConfigVariableBool copy_texture_inverted;
 extern EXPCL_PANDA ConfigVariableBool window_inverted;
 extern EXPCL_PANDA ConfigVariableBool depth_offset_decals;

panda/src/display/displayRegion.cxx

@@ -90,7 +90,7 @@ operator = (const DisplayRegion&) {
 ////////////////////////////////////////////////////////////////////
 DisplayRegion::
 ~DisplayRegion() {
-  set_camera(NodePath());
+  cleanup();
 
   // The window pointer should already have been cleared by the time
   // the DisplayRegion destructs (since the GraphicsOutput class keeps
@@ -98,6 +98,20 @@ DisplayRegion::
   nassertv(_window == (GraphicsOutput *)NULL);
 }
 
+////////////////////////////////////////////////////////////////////
+//     Function: DisplayRegion::cleanup
+//       Access: Public
+//  Description: Cleans up some pointers associated with the
+//               DisplayRegion to help reduce the chance of memory
+//               leaks due to circular reference counts.
+////////////////////////////////////////////////////////////////////
+void DisplayRegion::
+cleanup() {
+  set_camera(NodePath());
+
+  _cull_result = NULL;
+}
+
 ////////////////////////////////////////////////////////////////////
 //     Function: DisplayRegion::get_dimensions
 //       Access: Published

panda/src/display/displayRegion.h

@@ -59,6 +59,7 @@ private:
 
 public:
   ~DisplayRegion();
+  void cleanup();
 
   INLINE bool operator < (const DisplayRegion &other) const;
 

panda/src/display/graphicsOutput.I

@@ -223,7 +223,9 @@ clear_delete_flag() {
 ////////////////////////////////////////////////////////////////////
 INLINE bool GraphicsOutput::
 get_delete_flag() const {
-  return _delete_flag;
+  // We only delete the window or buffer automatically when it is
+  // no longer associated with a texture.
+  return _delete_flag && !_hold_texture.is_valid_pointer();
 }
 
 ////////////////////////////////////////////////////////////////////

panda/src/display/graphicsOutput.cxx

@@ -179,29 +179,6 @@ GraphicsOutput::
   _default_display_region = NULL;
 }
 
-////////////////////////////////////////////////////////////////////
-//     Function: GraphicsOutput::detach_texture
-//       Access: Published
-//  Description: Disassociates the texture from the GraphicsOutput.
-//               The texture will no longer be filled as the frame
-//               renders, and it may be used as an ordinary texture in
-//               its own right.  However, its contents may be
-//               undefined.
-////////////////////////////////////////////////////////////////////
-void GraphicsOutput::
-detach_texture() {
-  MutexHolder holder(_lock);
-
-  if (_rtm_mode == RTM_bind_texture && _gsg != (GraphicsStateGuardian *)NULL) {
-    _gsg->framebuffer_release_texture(this, get_texture());
-  }
-
-  _texture = NULL;
-  _rtm_mode = RTM_none;
-
-  set_inverted(window_inverted);
-}
-
 ////////////////////////////////////////////////////////////////////
 //     Function: GraphicsOutput::setup_render_texture
 //       Access: Published
@@ -239,10 +216,6 @@ void GraphicsOutput::
 setup_render_texture(Texture *tex, bool allow_bind, bool to_ram) {
   MutexHolder holder(_lock);
 
-  if (_rtm_mode == RTM_bind_texture && _gsg != (GraphicsStateGuardian *)NULL) {
-    _gsg->framebuffer_release_texture(this, get_texture());
-  }
-
   if (tex == (Texture *)NULL) {
     _texture = new Texture(get_name());
     _texture->set_wrap_u(Texture::WM_clamp);
@@ -364,6 +337,8 @@ remove_display_region(DisplayRegion *display_region) {
   TotalDisplayRegions::iterator dri =
     find(_total_display_regions.begin(), _total_display_regions.end(), drp);
   if (dri != _total_display_regions.end()) {
+    // Let's aggressively clean up the display region too.
+    display_region->cleanup();
     display_region->_window = NULL;
     _total_display_regions.erase(dri);
     if (display_region->is_active()) {
@@ -376,6 +351,32 @@ remove_display_region(DisplayRegion *display_region) {
   return false;
 }
 
+////////////////////////////////////////////////////////////////////
+//     Function: GraphicsOutput::remove_all_display_regions
+//       Access: Published
+//  Description: Removes all display regions from the window, except
+//               the default one that is created with the window.
+////////////////////////////////////////////////////////////////////
+void GraphicsOutput::
+remove_all_display_regions() {
+  MutexHolder holder(_lock);
+
+  TotalDisplayRegions::iterator dri;
+  for (dri = _total_display_regions.begin();
+       dri != _total_display_regions.end();
+       ++dri) {
+    DisplayRegion *display_region = (*dri);
+    if (display_region != _default_display_region) {
+      // Let's aggressively clean up the display region too.
+      display_region->cleanup();
+      display_region->_window = NULL;
+    }
+  }
+  _total_display_regions.clear();
+  _total_display_regions.push_back(_default_display_region);
+  _display_regions_stale = true;
+}
+
 ////////////////////////////////////////////////////////////////////
 //     Function: GraphicsOutput::get_num_display_regions
 //       Access: Published
@@ -517,7 +518,8 @@ make_texture_buffer(const string &name, int x_size, int y_size,
   }
   
   bool allow_bind = 
-    (prefer_texture_buffer && gsg->get_supports_render_texture() && !to_ram);
+    (prefer_texture_buffer && support_render_texture && 
+     gsg->get_supports_render_texture() && !to_ram);
 
   // If the user so indicated in the Config.prc file, try to create a
   // parasite buffer first.  We can only do this if the requested size
@@ -857,13 +859,38 @@ end_frame() {
     _flip_ready = true;
   }
 
-  if (_one_shot && !show_buffers) {
+  if (_one_shot) {
     // In one-shot mode, we request the GraphicsEngine to delete the
-    // window after we have rendered a frame.  But when show-buffers
+    // window after we have rendered a frame.
-    // mode is enabled, we don't do this, to give the user a chance to
-    // see the output.
     _active = false;
     _delete_flag = true;
+
+    // We have to be sure to remove all of the display regions
+    // immediately, so that circular reference counts can be cleared
+    // up (each display region keeps a pointer to a CullResult, which
+    // can hold all sorts of pointers).
+    remove_all_display_regions();
+
+
+    // If we were rendering directly to texture, we can't delete the
+    // buffer until the texture is gone too.
+    if (_rtm_mode == RTM_bind_texture) {
+      _hold_texture = _texture;
+    }
+
+    // Also, when show-buffers mode is enabled, we want to keep the
+    // window around until the user has a chance to see the texture.
+    // Same story: we'll hold it until the texture is gone.  In this
+    // case we also keep the active flag true, so the window will
+    // repaint when needed.
+    if (show_buffers) {
+      _hold_texture = _texture;
+      _active = true;
+    }
+
+    // We have to be sure to clear the _texture pointer, though, or
+    // we'll end up holding a reference to it forever.
+    _texture = NULL;
   }
 
   _cube_map_index = -1;

panda/src/display/graphicsOutput.h

@@ -34,6 +34,7 @@
 #include "filename.h"
 #include "drawMask.h"
 #include "pvector.h"
+#include "weakPointerTo.h"
 
 class PNMImage;
 
@@ -75,7 +76,6 @@ PUBLISHED:
 
   INLINE bool has_texture() const;  
   INLINE Texture *get_texture() const;  
-  void detach_texture();
   void setup_render_texture(Texture *tex, bool allow_bind, bool to_ram);
 
   INLINE int get_x_size() const;
@@ -102,6 +102,7 @@ PUBLISHED:
   INLINE DisplayRegion *make_display_region(float l, float r,
                                             float b, float t);
   bool remove_display_region(DisplayRegion *display_region);
+  void remove_all_display_regions();
 
   int get_num_display_regions() const;
   PT(DisplayRegion) get_display_region(int n) const;
@@ -193,6 +194,12 @@ protected:
   bool _inverted;
   bool _delete_flag;
 
+  // This weak pointer is used to keep track of whether the buffer's
+  // bound Texture has been deleted or not.  Until it has, we don't
+  // auto-close the buffer (since that would deallocate the memory
+  // associated with the texture).
+  WPT(Texture) _hold_texture;
+
 protected:
   Mutex _lock; 
   // protects _display_regions.

panda/src/display/graphicsStateGuardian.cxx

@@ -447,6 +447,9 @@ begin_scene() {
 ////////////////////////////////////////////////////////////////////
 void GraphicsStateGuardian::
 end_scene() {
+  // We should clear this pointer now, so that we don't keep unneeded
+  // reference counts dangling.
+  _scene_setup = NULL;
 }
 
 ////////////////////////////////////////////////////////////////////

panda/src/glstuff/glGraphicsStateGuardian_src.cxx

@@ -2347,6 +2347,16 @@ framebuffer_copy_to_texture(Texture *tex, int z, const DisplayRegion *dr,
   tex->set_x_size(w);
   tex->set_y_size(h);
 
+  if (tex->get_match_framebuffer_format()) {
+    FrameBufferProperties properties = get_properties();
+    int mode = properties.get_frame_buffer_mode();
+    if (mode & FrameBufferProperties::FM_alpha) {
+      tex->set_format(Texture::F_rgba);
+    } else {
+      tex->set_format(Texture::F_rgb);
+    }
+  }
+
   TextureContext *tc = tex->prepare_now(get_prepared_objects(), this);
   nassertv(tc != (TextureContext *)NULL);
   bind_texture(tc);

panda/src/grutil/multitexReducer.cxx

@@ -288,7 +288,6 @@ flatten(GraphicsOutput *window) {
     // Create a root node for the buffer's scene graph, and set up
     // some appropriate properties for it.
     NodePath render("buffer");
-    cam_node->set_scene(render);
     render.set_bin("unsorted", 0);
     render.set_depth_test(false);
     render.set_depth_write(false);

panda/src/pgraph/cullHandler.h

@@ -28,7 +28,7 @@
 // Description : This defines the abstract interface for an object
 //               that receives Geoms identified by the CullTraverser.
 //               By itself, it's not a particularly useful class; to
-//               use it, derive from it and redefine record_geom().
+//               use it, derive from it and redefine record_object().
 ////////////////////////////////////////////////////////////////////
 class EXPCL_PANDA CullHandler {
 public:

panda/src/pgraph/cullResult.h

@@ -73,9 +73,6 @@ private:
 
   typedef pvector< PT(CullBin) > Bins;
   Bins _bins;
-
-  typedef phash_set<CullResult *, pointer_hash> CullResults;
-  static CullResults _cull_results;
 };
 
 #include "cullResult.I"