Re: ok lets try this yet again

[Juan Lang]

As others have said, there are plenty of unnecessary changes, but you
probably need some pointing at them, so here goes:
> --- context.c    2008-06-26 13:52:57.000000000 -0400
> +++ context.c.change    2008-06-26 14:32:48.000000000 -0400
> @@ -116,13 +116,12 @@
>      int iPixelFormat=0;
>      short redBits, greenBits, blueBits, alphaBits, colorBits;
>      short depthBits=0, stencilBits=0;
> -

Unneeded.
>  int i = 0;
>  int nCfgs = This->adapter->nCfgs;
> -
>
> WineD3D_PixelFormat *cfgs = This->adapter->cfgs;
> -
>
> TRACE("ColorFormat=%s, DepthStencilFormat=%s, auxBuffers=%d,
>
> numSamples=%d, pbuffer=%d, findCompatible=%d\n",
> -
>      debug_d3dformat(ColorFormat),
>
> debug_d3dformat(DepthStencilFormat), auxBuffers, numSamples, pbuffer,
> findCompatible);

Why did you remove the trace?
> - WineD3D_PixelFormat *cfgs = This->adapter->cfgs;
> - BOOL exactDepthMatch = FALSE;  /*Changed june 23,08 */
> - PIXELFORMATDESCRIPTOR pfd;     /*Changed june 23,08 */

Don't put comments on the change date in the code, git history will
tell us all we need to know about that.
>  if(!getColorBits(ColorFormat, &redBits, &greenBits, &blueBits,
>
> &alphaBits, &colorBits)) {
>          ERR("Unable to get color bits for format %s (%#x)!\n",
> debug_d3dformat(ColorFormat), ColorFormat);
> @@ -145,84 +144,91 @@
>  DepthStencilFormat = WINED3DFMT_D24S8;
>
> - if(DepthStencilFormat) {
> -
>    getDepthStencilBits(DepthStencilFormat, &depthBits, &stencilBits);
> - }
>
> +/* Changed Section June 24,08 */
> -
>
> +#if 0
> - if(DepthStencilFormat)
> -
>  {
> - getDepthStencilBits(DepthStencilFormat, &depthBits, &stencilBits);
> - }
>
> +#endif
> -
>
> +/* Just call getDepthStencilBits as the above IF will always in this case
> be true */
> -
> - getDepthStencilBits(DepthStencilFormat, &depthBits, &stencilBits);

While you're right that this if is unnecessary,
1) putting a redundant copy if an #if 0 block is really wasteful, as
it doesn't give future developers any hint as to when, if ever, the
block should be removed.
2) The comment is also unnecessary, as "the above IF" won't refer to
anything once it's removed.
Just remove the redundant if and be done with it.

Also, this change is really a no-op, since the if is guaranteed to be
true.  As such it should probably be a separate patch from any other
change.
>  /* Find a pixel format which EXACTLY matches our requirements (except
>
> for depth) */
> - for(i=0; i<nCfgs; i++) {
> -
>    BOOL exactDepthMatch = TRUE;
> - for(i=0; i<nCfgs; i++)
> -
>  {

Why did you re-indent the brace?  Just leave it where it is, please.
>      cfgs = &This->adapter->cfgs[i];
> -
> -

Here you've added a bunch of white space.  Please don't do that.
>      /* For now only accept RGBA formats. Perhaps some day we will
>       * allow floating point formats for pbuffers. */
>      if(cfgs->iPixelType != WGL_TYPE_RGBA_ARB)
> -
>        continue;
> -
>       continue;

Another whitespace-only change.
>      /* In window mode (!pbuffer) we need a window drawable format and
>
> double buffering. */
>          if(!pbuffer && !(cfgs->windowDrawable && cfgs->doubleBuffer))
> -
>        continue;
> -
>       continue;

and again..
> -
>    /* We like to have aux buffers in backbuffer mode */
> -
>   /* We like to have aux buffers in backbuffer mode */

and again..
>      if(auxBuffers && !cfgs->auxBuffers)
> -
>        continue;
> -
>       continue;

and again.. getting the picture?
>      /* In pbuffer-mode we need a pbuffer-capable format but we don't
>
> want double buffering */
>          if(pbuffer && (!cfgs->pbufferDrawable || cfgs->doubleBuffer))
> -
>        continue;
> -
>         continue;

more..
> -
>    if(cfgs->redSize != redBits)
> -
>        continue;
> -
>    if(cfgs->greenSize != greenBits)
> -
>        continue;
> -
>    if(cfgs->blueSize != blueBits)
> -
>        continue;
> -
>    if(cfgs->alphaSize != alphaBits)
> -
>        continue;
> -
> -
>    /* We try to locate a format which matches our requirements
>
> exactly. In case of
> -
>     * depth it is no problem to emulate 16-bit using e.g. 24-bit, so
>
> accept that. */
> -
>    if(cfgs->depthSize < depthBits)
> -
>        continue;
> -
>    else if(cfgs->depthSize > depthBits)
> -
>        exactDepthMatch = FALSE;
> -
>    if ((cfgs->redSize != redBits) || (cfgs->greenSize != greenBits) ||
>
> (cfgs->blueSize != blueBits) || (cfgs->alphaSize != alphaBits))
> -
>         continue;
>
>    /* In all cases make sure the number of stencil bits matches our
>
> requirements
>           * even when we don't need stencil because it could affect
> performance EXCEPT
>           * on cards which don't offer depth formats without stencil like
> the i915 drivers
>           * on Linux. */
> -
>    if(stencilBits != cfgs->stencilSize &&
>
> !(This->adapter->brokenStencil && stencilBits <= cfgs->stencilSize))
> -
>        continue;
> -
>    if((stencilBits != cfgs->stencilSize) &&
>
> !((This->adapter->brokenStencil && stencilBits) <= cfgs->stencilSize))
> -
>         continue;

more..
>      /* Check multisampling support */
>      if(cfgs->numSamples != numSamples)
> -
>        continue;
> -
>         continue;

yet more..
> -
>    /* When we have passed all the checks then we have found a format
>
> which matches our
> -
>     * requirements. Note that we only check for a limit number of
>
> capabilities right now,
> -
>     * so there can easily be a dozen of pixel formats which appear to
>
> be the 'same' but
> -
>     * can still differ in things like multisampling, stereo, SRGB and
>
> other flags.
> -
>     */
> -
>    /* We try to locate a format which matches our requirements
>
> exactly. In case of
> -
>     * depth it is no problem to emulate 16-bit using e.g. 24-bit, so
>
> accept that. */
> -
>     if (cfgs->depthSize !=  depthBits)
> -
>        continue;
>
>    /* Exit the loop as we have found a format :) */
> -
>    if(exactDepthMatch) {
> -
>    if (exactDepthMatch)
> -
>       {

more...
> -
>        TRACE("Exact Depth Match\n");
>        iPixelFormat = cfgs->iPixelFormat;
>        break;
> -
>    } else if(!iPixelFormat) {
> -
>       }
> -
>    if (!iPixelFormat)

and again.  Removing the else is silly, as the if branch breaks,
guaranteeing we can only get here if the first if was not true.
> -
>      {
>        /* In the end we might end up with a format which doesn't
>
> exactly match our depth
>               * requirements. Accept the first format we found because
> formats with higher iPixelFormat
>               * values tend to have more extended capabilities (e.g.
> multisampling) which we don't need. */
> -
> -
>        TRACE("Emulating %d\n",cfgs->iPixelFormat);
>        iPixelFormat = cfgs->iPixelFormat;
> -
>    }
> -
>        break; /* Added June 24,08 */

Again, don't put a changelog in the code.
> -
>      }

More whitespace change.
>  }
>
>  /* When findCompatible is set and no suitable format was found, let
>
> ChoosePixelFormat choose a pixel format in order not to crash. */
> - if(!iPixelFormat && !findCompatible) {
> -
>
> +#if 0

If you want to remove something, just remove it.  Don't add #if 0 around it.
> +if (!iPixelFormat && !findCompatible)
> -
>  {
>    ERR("Can't find a suitable iPixelFormat\n");
>    return FALSE;
> - } else if(!iPixelFormat) {
> -
>    PIXELFORMATDESCRIPTOR pfd;
> -
> -
>  }
>
> +#endif
> -
> - if (!iPixelFormat)
> -
>  {
>    TRACE("Falling back to ChoosePixelFormat as we weren't able to find
>
> an exactly matching pixel format\n");
>          /* PixelFormat selection */
>          ZeroMemory(&pfd, sizeof(pfd));
> @@ -235,16 +241,17 @@
>          pfd.cDepthBits = depthBits;
>          pfd.cStencilBits = stencilBits;
>          pfd.iLayerType = PFD_MAIN_PLANE;
> -
> -

More whitespace changes..
>      iPixelFormat = ChoosePixelFormat(hdc, &pfd);
> -
>    if(!iPixelFormat) {
> -
>        /* If this happens something is very wrong as ChoosePixelFormat
>
> barely fails */
> -
>        ERR("Can't find a suitable iPixelFormat\n");
> -
>        return FALSE;
> -
>    }
> - }
> -
>    if (!iPixelFormat)
> -
>      {
> -
>       /* If this happens something is very wrong as ChoosePixelFormat
>
> barely fails */

And again..
> -
>       ERR("Can't find a suitable iPixelFormat\n");
> -
>       return FALSE;
> -
>      }
> -
>  }
>
> - TRACE("Found iPixelFormat=%d for ColorFormat=%s,
>
> DepthStencilFormat=%s\n", iPixelFormat, debug_d3dformat(ColorFormat),
> debug_d3dformat(DepthStencilFormat));
> - TRACE("Found iPixelFormat\n");

Why did you change that?
>  return iPixelFormat;
>
> }
>
> @@ -372,6 +379,7 @@
>      /* If we still don't have a pixel format, something is very wrong
>
> as ChoosePixelFormat barely fails */
>          if(!iPixelFormat) {
> -
>        TRACE("Choose Pixel Format Failed\n");
>        ERR("Can't find a suitable iPixelFormat\n");
>        return FALSE;
>    }

Clean up the unnecessary stuff and we'll have another look.  Thanks,
--Juan