[cairo] [PATCH] Path clipping

[Kristian Høgsberg]

Keith Packard wrote:
> On Fri, 2005-06-03 at 15:29 -0400, Kristian Høgsberg wrote:
> 
> +struct _cairo_clip_path {
> +    unsigned int       ref_count;
> +    cairo_path_fixed_t path;
> +    cairo_fill_rule_t  fill_rule;
> +    double             tolerance;
> +    cairo_clip_path_t  *prev;
> +};
> 
> I think tolerance should not be captured in this structure, rather the
> tolerance should come from the environment in which the clip is used.
> Right?

As discussed in IRC, I was mimicking the behaviour of the surface mask 
clipping, where the tolerance setting is used at cairo_clip() time when 
tesselating the clipping path.  We should probably document 
cairo_set_tolerance().

> +_cairo_surface_set_clip_path (cairo_surface_t  *surface,
> +                             cairo_clip_path_t *clip_path,
> +                             unsigned int      serial);
> 
> I assume this sets the whole mess at once and avoids having to call
> _cairo_surface_clip_path multiple times?  Is this a useful change?

I think it makes for a nice symmetry between 
cairo_surface_set_clip_region() and cairo_surface_set_clip_path():

     if (gstate->clip.path)
         return _cairo_surface_set_clip_path (surface,
                                              gstate->clip.path,
                                              gstate->clip.serial);

     if (gstate->clip.region)
         return _cairo_surface_set_clip_region (surface,
                                                gstate->clip.region,
                                                gstate->clip.serial);

and the code has to live somewhere... is there a reason not to put the 
code in cairo_surface_set_clip_path()?

> 
> -    if (surface->backend->clip_path) {
> -       status = surface->backend->clip_path (surface, NULL);
> +
> +    if (surface->backend->intersect_clip_path) {
> +       status = surface->backend->intersect_clip_path (surface,
> +                                                       NULL,
> +                                                       CAIRO_FILL_RULE_WINDING,
> +                                                       0);
>  
> and in this case, the call is 'intersect_clip_path' instead of
> 'clip_path'.  Is there some need to make the name longer?

'clip' is not a verb here.  'intersect' is the verb, it's the action 
performed by the function, 'clip_path' is the object it performs the 
action with.  It is similar to 'set' and 'clip_region' in 'set_clip_region'.

> And, the '0' at the end is marking tolerance, I think this is probably
> the right place to have tolerance set.  I think we can simply fetch it
> from the gstate above and pass it along, right?

Yeah, this would be a good place.

 > Oh, and I'd like to see 0.0 for double constants...

Sure.  Is this just a stylistic issue or is there some double promotion 
issue I'm not aware of?

> Should the backend see the private clip structure so that we don't have
> to pass fill_rule along here?

Is this a problem?  There's only four parameters to intersect_clip_path, 
and there's a couple of fields in cairo_clip_path_t that the backend has 
no use for.

cheers,
Kristian