[Cogl] Updated patchset for depth-texture support

Neil Roberts neil at linux.intel.com
Thu Sep 6 10:26:00 PDT 2012


The patches look good to me with the proposed tweaks. I just have a few
comments:

• cogl_framebuffer_enable_depth_texture ignores its ‘enabled’ parameter
  and just always enables the depth texture. It also has an unusual name
  if it's meant to be setting a property (ie,
  cogl_framebuffer_set_depth_texture_enabled would be more common). As
  it's pretty unlikely that you're going to enable a depth texture and
  then later disable it before allocating the framebuffer, maybe we
  should just remove the enabled parameter and keep the name as it is.
  Then it's not setting a property but rather ‘enable’ is a verb.

• If we don't remove the enabled parameter then we at least need to
  tweak the type in the header to be CoglBool instead of gboolean.

• Enabling the depth texture only works for offscreen framebuffers and
  in fact the function asserts that the framebuffer is offscreen. This
  is however not documented so it seems reasonable that someone would
  try adding a depth texture to an onscreen buffer.

  It doesn't look like GL currently has any way to create an FBO with
  the color attachment set to render to an onscreen buffer. However I
  imagine there could eventually be an extension to enable this.
  glDrawBuffer sets state on the currently bound framebuffer not the
  global state. You could imagine an extension which allows
  glDrawBuffer(GL_BACK) to be set on an FBO instead of requiring it to
  be GL_COLOR_ATTACHMENT0.

  Given that it conceptually might make sense to have a depth texture on
  an onscreen buffer, I think we should make it so that it's not a
  programmer error to enable the depth texture on an onscreen but
  instead it should fail gracefully with a GError when the framebuffer
  is allocated.

• It looks like nothing frees the depth_texture. We should probably just
  insert a cogl_object_unref in cogl_framebuffer_free.

Regards,
- Neil


More information about the Cogl mailing list