[cairo] [PATCH] Path clipping

[Kristian Høgsberg]

Carl Worth wrote:
> On Fri, 03 Jun 2005 15:29:47 -0400, Kristian Høgsberg wrote:
> 
>>Here's a patch to implement clip-to-path functionality.
...
> 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.

I like this idea, and path clipping is exclusive with the other clipping 
models.  Unfortunately, the mask / region clipping cases aren't, so we 
can't untangle those paths entirely.  Adding the clip.mode field and 
splitting up _cairo_gstate_clip() should still make for a decent 
cleanup, though.

>>+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).

Alright, that's a good point.

>>+ * _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 ?

Okay, I guess that's what Keith was saying, too.

>>+_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?

We call surface->backend->intersect_clip_path() with a NULL clip path to 
clear whatever clip path currently set in the surface.  Then 
_set_clip_path() builds the new clip path by calling 
surface->backend->intersect_clip_path() with each of the paths.  But I 
guess you're just saying that this should be in cairo-gstate.c and not 
in cairo-surface.c, right?

>>--- 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).

Alright, I'll shuffle the functions around and write some documentation.

> 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?)

If you can compute intersections in the upper layer, the 'set' function 
is preferrable, which is the case for regions.  For general paths, we 
can't compute intersections, so we have to rely on the backend to have 
this capability and use the less optimal 'intersect' function 
iteratively to build the current clipping path whenever we need to set 
it.  I don't think it makes sense to make the region API use 
intersection, and libpixman doesn't support it anyway.

I'll send an updated patch.

Kristian