[cairo] [PATCH] Path clipping

[Carl Worth]

On Fri, 03 Jun 2005 15:29:47 -0400, Kristian Høgsberg wrote:
> Here's a patch to implement clip-to-path functionality.

Here are some comments from a high-level scan of the code.

>      /*
> -     * XXX add clip paths here
> +     * If the surface supports path clipping, we store the list of
> +     * clipping paths that has been set here as a linked list.
>       */
> +    cairo_clip_path_t *path;
>  } cairo_clip_t;

Is the usage of this field mutually exclusive with other fields in
cairo_clip_t? If so, it would be nice if the code made that more
clear. Perhaps a union here?

> +    status = _cairo_surface_can_clip_path (gstate->surface);

(Recognizing that the current patch follows _cairo_surface_can_clip_region),
I don't like the introduction of these cairo_surface predicate
functions to test the existence of backend functions. We don't use
them universally for other surface backend functions, (where we just
test for NULL before calling).

Maybe what we need here is some gstate->clip.mode that can be
initialized based on the existence of surface backend functions, (and
used to control access to the clip union data discussed above).

In general _cairo_gstate_clip is too long now, and I'd prefer to see
it broken out into its three major sub-functions. Again, this would
make the exclusivity between the various code paths much easier to
follow.

> +static cairo_status_t
> +_set_clip_path (cairo_surface_t *surface, cairo_clip_path_t *clip_path)

This is just the recursive part of the implementation of
_cairo_surface_set_clip_path, right? I think I'd prefer to have this
named something like _cairo_surface_set_clip_path_recursive to make
that more apparent, (as now, the only distinction between the two
names is the presence/absence of the _cairo_surface and that doesn't
seem sufficient to indicate what the difference is).

> + * _cairo_surface_set_clip_path:

I'd prefer to see the generic _cairo_surface functions match the
backend-specific functions in name and purpose. Here, we have
_cairo_surface_set_clip_path that calls repeatedly into
surface->backend->intersect_clip_path. Perhaps the repetition should
instead get moved up into gstate ?

> +_cairo_surface_set_clip_path (cairo_surface_t	*surface,
> +			      cairo_clip_path_t	*clip_path,
> +			      unsigned int	serial)
>  {
> -    surface->current_clip_serial = clip_serial;
> -    return surface->backend->clip_path (surface, path);
> +    cairo_status_t status;
> +
> +    if (surface->finished)
> +	return CAIRO_STATUS_SURFACE_FINISHED;
> +
> +    assert (surface->backend->intersect_clip_path != NULL);
> +
> +    status = surface->backend->intersect_clip_path (surface,
> +						    NULL,
> +						    CAIRO_FILL_RULE_WINDING,
> +						    0);
> +    if (!CAIRO_OK (status))
> +	return status;
> +
> +    status =  _set_clip_path (surface, clip_path);

The control flow here is not clear to me. Why does
_cairo_surface_set_clip_path call into
surface->backend->intersect_clip_path exactly once and then
_set_clip_path calls into it multiple times recursively? Can we
structure the code so that there is only one call into the backend
function?


> --- src/cairoint.h	26 May 2005 18:35:46 -0000	1.144
> +++ src/cairoint.h	3 Jun 2005 18:16:09 -0000
> +
> +    cairo_int_status_t
> +    (*intersect_clip_path)	(void			*dst,
> +				 cairo_path_fixed_t	*path,
> +				 cairo_fill_rule_t	fill_rule,
> +				 double			tolerance);
>     

The surface backend is getting really complicated. I don't want to
accept any new functions here without documentation.

In this case, I think this new function needs to appear next to
set_clip_region and both need be documented as part of this patch,
(which would help answer my original question about exclusivity).

In addition, is there a reason the region-based interface is based on
a "set" function while the path-based interface has an "intersect"
function. If the intersect approach is preferable, should it perhaps
not be applied to both interfaces? (And perhaps provide some code
sharing in the gstate layer above?)

-Carl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/cairo/attachments/20050613/09159a31/attachment.pgp