[Mesa-dev] [PATCH 1/1] mesa: stub the _mesa_StencilOp function.

[Oliver McFadden]

On Fri, May 18, 2012 at 11:17:00PM +0300, Oliver McFadden wrote:
> On Fri, May 18, 2012 at 11:27:24AM -0600, Brian Paul wrote:
> > On 05/18/2012 05:24 AM, Oliver McFadden wrote:
> > > The device driver function table only implements StencilOpSeparate(),
> > > let's remove the obsolete 'todo' comment and code duplication.
> > >
> > > Signed-off-by: Oliver McFadden<oliver.mcfadden at linux.intel.com>
> > > ---
> > >   src/mesa/drivers/common/meta.c |    4 +-
> > >   src/mesa/main/stencil.c        |   75 ++++++---------------------------------
> > >   2 files changed, 14 insertions(+), 65 deletions(-)
> > >
> > > diff --git a/src/mesa/drivers/common/meta.c b/src/mesa/drivers/common/meta.c
> > > index 95336fc..42bbf9d 100644
> > > --- a/src/mesa/drivers/common/meta.c
> > > +++ b/src/mesa/drivers/common/meta.c
> > > @@ -2360,12 +2360,12 @@ _mesa_meta_DrawPixels(struct gl_context *ctx,
> > >         _mesa_set_enable(ctx, GL_STENCIL_TEST, GL_TRUE);
> > >
> > >         /* set all stencil bits to 0 */
> > > -      _mesa_StencilOp(GL_REPLACE, GL_REPLACE, GL_REPLACE);
> > > +      _mesa_StencilOpSeparate(ctx->Stencil.ActiveFace, GL_REPLACE, GL_REPLACE, GL_REPLACE);
> > >         _mesa_StencilFunc(GL_ALWAYS, 0, 255);
> > >         _mesa_DrawArrays(GL_TRIANGLE_FAN, 0, 4);
> > >
> > >         /* set stencil bits to 1 where needed */
> > > -      _mesa_StencilOp(GL_KEEP, GL_KEEP, GL_REPLACE);
> > > +      _mesa_StencilOpSeparate(ctx->Stencil.ActiveFace, GL_KEEP, GL_KEEP, GL_REPLACE);
> > >
> > >         _mesa_BindProgram(GL_FRAGMENT_PROGRAM_ARB, drawpix->StencilFP);
> > >         _mesa_set_enable(ctx, GL_FRAGMENT_PROGRAM_ARB, GL_TRUE);
> > > diff --git a/src/mesa/main/stencil.c b/src/mesa/main/stencil.c
> > > index f47b57b..f051479 100644
> > > --- a/src/mesa/main/stencil.c
> > > +++ b/src/mesa/main/stencil.c
> > > @@ -314,70 +314,24 @@ _mesa_StencilMask( GLuint mask )
> > >    *
> > >    * Verifies the parameters and updates the respective fields in
> > >    * __struct gl_contextRec::Stencil. On change flushes the vertices and notifies the
> > > - * driver via the dd_function_table::StencilOp callback.
> > > + * driver via the dd_function_table::StencilOpSeparate callback.
> > >    */
> > >   void GLAPIENTRY
> > >   _mesa_StencilOp(GLenum fail, GLenum zfail, GLenum zpass)
> > >   {
> > >      GET_CURRENT_CONTEXT(ctx);
> > > -   const GLint face = ctx->Stencil.ActiveFace;
> > > -
> > > -   if (MESA_VERBOSE&  VERBOSE_API)
> > > -      _mesa_debug(ctx, "glStencilOp()\n");
> > > -
> > > -   ASSERT_OUTSIDE_BEGIN_END(ctx);
> > > -
> > > -   if (!validate_stencil_op(ctx, fail)) {
> > > -      _mesa_error(ctx, GL_INVALID_ENUM, "glStencilOp(sfail)");
> > > -      return;
> > > -   }
> > > -   if (!validate_stencil_op(ctx, zfail)) {
> > > -      _mesa_error(ctx, GL_INVALID_ENUM, "glStencilOp(zfail)");
> > > -      return;
> > > -   }
> > > -   if (!validate_stencil_op(ctx, zpass)) {
> > > -      _mesa_error(ctx, GL_INVALID_ENUM, "glStencilOp(zpass)");
> > > -      return;
> > > -   }
> > > -
> > > -   if (face != 0) {
> > 
> > Note: if face != 0, it must be 2 (there should be an assertion).  More 
> > below...
> > 
> > 
> > > -      /* only set active face state */
> > > -      if (ctx->Stencil.ZFailFunc[face] == zfail&&
> > > -          ctx->Stencil.ZPassFunc[face] == zpass&&
> > > -          ctx->Stencil.FailFunc[face] == fail)
> > > -         return;
> > > -      FLUSH_VERTICES(ctx, _NEW_STENCIL);
> > > -      ctx->Stencil.ZFailFunc[face] = zfail;
> > > -      ctx->Stencil.ZPassFunc[face] = zpass;
> > > -      ctx->Stencil.FailFunc[face] = fail;
> > > -
> > > -      /* Only propagate the change to the driver if EXT_stencil_two_side
> > > -       * is enabled.
> > > -       */
> > > -      if (ctx->Driver.StencilOpSeparate&&  ctx->Stencil.TestTwoSide) {
> > > -         ctx->Driver.StencilOpSeparate(ctx, GL_BACK, fail, zfail, zpass);
> > > -      }
> > > -   }
> > > -   else {
> > > -      /* set both front and back state */
> > > -      if (ctx->Stencil.ZFailFunc[0] == zfail&&
> > > -          ctx->Stencil.ZFailFunc[1] == zfail&&
> > > -          ctx->Stencil.ZPassFunc[0] == zpass&&
> > > -          ctx->Stencil.ZPassFunc[1] == zpass&&
> > > -          ctx->Stencil.FailFunc[0] == fail&&
> > > -          ctx->Stencil.FailFunc[1] == fail)
> > > -         return;
> > > -      FLUSH_VERTICES(ctx, _NEW_STENCIL);
> > > -      ctx->Stencil.ZFailFunc[0] = ctx->Stencil.ZFailFunc[1] = zfail;
> > > -      ctx->Stencil.ZPassFunc[0] = ctx->Stencil.ZPassFunc[1] = zpass;
> > > -      ctx->Stencil.FailFunc[0]  = ctx->Stencil.FailFunc[1]  = fail;
> > > -      if (ctx->Driver.StencilOpSeparate) {
> > > -         ctx->Driver.StencilOpSeparate(ctx,
> > > -				       ((ctx->Stencil.TestTwoSide)
> > > -					? GL_FRONT : GL_FRONT_AND_BACK),
> > > -                                       fail, zfail, zpass);
> > > +   GLenum face;
> > > +
> > > +   if (ctx->Stencil.TestTwoSide) {
> > > +      face = GL_FRONT_AND_BACK;
> > > +   } else {
> > > +      if (ctx->Stencil.ActiveFace == 0) {
> > > +	 face = GL_FRONT;
> > > +      } else {
> > > +	 face = GL_BACK;
> > >         }
> > >      }
> > > +   _mesa_StencilOpSeparate(face, fail, zfail, zpass);
> > >   }
> > 
> > I think there's something subtly different here.  As noted above, if 
> > face != 0 it must be 2 so we're setting the stencil[2] state.  But 
> > that never happens in _mesa_StencilOpSeparate().
> > 
> > It's all a bit confusing because two-sided stencil is treated 
> > differently between GL_EXT_stencil_two_side, GL_ATI_separate_stencil 
> > and OpenGL 2.0.
> > 
> > Maybe you can double-check this stuff.
> 
> OK. I'll check it on Monday and I agree it's a bit confusing. At least
> the few Piglit tests I manually executed passed, but that's by no means
> a comprehensive test suite.

Brian, you're right, this patch is buggy and I need to rewrite it; I
just checked with Doom 3 and with this patch the shadow volumes were
totally wrong.  Without it, they are fine.

Sorry for the spam!

I might come up with another (better tested) patch later but as you
mention, it's a bit tricky due to the differing behaviours of the
extensions and GL2 core.

> 
> -- 
> Oliver McFadden.
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev

-- 
Oliver McFadden.