[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