[Xr] Dealing with groups in Xr

[Owen Taylor]

On Tue, 2003-04-22 at 14:43, Carl Worth wrote:
> Long ago, on a mailing list far, far away
> On Nov 21, Owen Taylor wrote:
>  >  * It looks like the temporary surface created for XrPushGroup
>  >    will always have the same format as the parent surface;
>  >    I don't think this is right ...
>  > 
>  >  * I think it's probably necessary to be able to XrPushgroup
>  >    with a restricted size; the overhead of creating a full
>  >    size temporary pixmap could be sunstantial.
>  >  
>  >  * It looks to me like on XrPopGroup, the group is composited
>  >    with the current alpha at the end of drawing the group,
>  >    rather than the alpha before pushing the group; I think this
>  >    is wrong - operations inside PushGroup/PopGroup shouldn't  
>  >    affect drawing onto the parent surface.
> 
> Clearly, the Push/PopGroup functionality had a lot of problems.
> 
> I've now dropped these functions from Xr.
> 
> The new surface management support provides mechanism for replacing
> calls to the old functions. The idiom is a change from:
> 
> 	XrPushGroup (xrs);
> 
> 	/* Drawing operations for the group ... */
> 
> 	/* Setup group compositing state (eg. set alpha) ... */
> 
> 	XrPopGroup (xrs);
> 
> to:
> 
> 	XrSurface *group;
> 
> 	group = XrSurfaceCreateNextTo (XrGetTargetSurface (xrs),
> 				       XrFormatARGB32, width, height);
> 	XrSave (xrs);
> 
> 	XrSetTargetSurface (xrs, group);
> 
> 	/* Drawing operations for the group ... */
> 
> 	XrRestore (xrs);
> 
> 	/* Setup group compositing state (eg. set alpha) ... */
> 
> 	XrShowSurface (xrs, group, 0, 0, width, height);
> 
> This approach should address the three points made by Owen above. And
> I feel pretty good about the mechanism enabled here.
> 
> Obviously, the fix comes at a cost of increased verbosity, (sometimes
> a bit worse than shown since the "Setup group compositing state" may
> need to be wrapped in a Save/Restore block). Perhaps there's room for
> a convenience function in here somewhere to make a common case less
> painful.
>
> The xrtest module in CVS contains Owen's original xrknockout program
> with the above modification applied three times. It works, but it
> seems less readable to me.

I took the liberty of committing a couple of tweaks - mostly
variable names and comments; I switched one of the temporary
surfaces to XrFormatA8, since that was sufficient.

Question I have - don't the temporary surfaces need to be 
freed?

I think the new stuff is definitely good in the sense of adding
more power and generality. I'm not sure that it really makes the
code less readable, except for making it longer. 

The only thing I can think of to reduce the verbosity would be
move back a bit in the direction you had before by introducing
the idea of a "temporary surface" that is part of the state.

 XrStartTemp (xrs, XrFormatARGB32, x, y, width, height);
 
 /* Drawing operations for the group ... */

 XrEndTemp (xrs);

 /* Setup group compositing state (eg. set alpha) ... */

 XrShowTemp (xrs);

I've appended the main draw() function of the knockout example
done in this fashion; I think it reads fairly well, though the
lack of names for the temporary surfaces makes it less 
self-documenting than the explicit-surface example.

Legibility could be improved at the expense of potential
efficiency by removing the x,y,width,height arguments
from XrStartTemp.

Regards,
                                         Owen

static void
draw (XrState *r,
      int      width,
      int      height)
{
    /* Fill the background */
    double radius = 0.5 * (width < height ? width : height) - 10;
    double xc = width / 2.;
    double yc = height / 2.;

    fill_checks (r, 0, 0, width, height);

    if (XrStartTemp (x, XrFormatARGB32, 0, 0, width, height)) {
        /* Draw a black circle on the overlay
         */
        XrSetRGBColor (r, 0., 0., 0.);
        oval_path (r, xc, yc, radius, radius);
        XrFill (r);

        /* Draw 3 circles to the punch surface, then cut
         * that out of the main circle in the overlay
         */
        if (XrStartTemp (r, XrFormatARGB8, 0, 0, width, height)) {
            draw_3circles (r, xc, yc, radius);

            XrEndTemp (r);
            XrSetOperator (r, XrOperatorOutReverse);
            XrShowTemp (r);
        }

        /* Now draw the 3 circles in a subgroup again
         * at half intensity, and use OperatorAdd to join up
         * without seams.
         */
        if (XrStartTemp (xrs, XrFormatARGB32, 0, 0, width, height)) {
            XrSetAlpha (r, 0.5);
            XrSetOperator (r, XrOperatorOver);
            draw_3circles (r, xc, yc, radius);

            XrEndTemp (r);
            XrSetOperator (r, XrOperatorAdd);
            XrShowTemp (r);
        }

        XrEndTemp (r);
        XrShowTemp (r);
    }
}