[cairo] Working on degenerate matrix problem, some questions

[Daniel Kraft]

Chris Wilson wrote:
> On Mon, 2008-03-24 at 17:22 +0000, Daniel Kraft wrote:
>  > BTW, how should I communicate patches like this
>  > to you?  Is it ok like this, should I append the diff directly to the
>  > mail text, use something other than the mailing list...?
> 
> [I'll expect Carl will be able to answer more completely. Indeed since
> this is a FAQ, I'd expected to be able to find some instructions on the
> wiki...]
> For any non-trivial series of patches, it's preferable to publish a git
> branch with your changes. Carl will set up a freedesktop.org account for
> you, from which you'll be able to host a repository, if you ask politely
> (and follow the instructions at
> http://freedesktop.org/wiki/AccountRequests). Given such a repo, for us
> to keep current with your work is as simple as:
> $ git remote add kraft git://somewhere.org/~dkraft/cairo # once
> $ git fetch kraft

Ah, thanks for the hint, I'll look at this now; and yes, you actually 
convinced me this will be much better to handle than working with patches...

>  > * At some points I am currently ignoring a cairo_status_t return value;
>  > that is, in functions already present with a void return type.  How can
>  > I set the error flag if an error occurs there without having to return
>  > the error code?  What should I do there?  (For instance, the gstate
>  > device-to-user transformations, IIRC)
> 
> Here you should add a cairo_status_t return and propagate the error code
> back to the caller, and there you will be able to set the error flag on
> the correct object. The functions are currently void as they only ever
> return SUCCESS so the code was simplified by removing the redundant
> conditionals.

Ok, I'll look at this.

> [snip]
> I'm still digesting the bulk of the patch, in particular whether it is
> actually a step in the correct direction. ;-) It does seem to avoid the
> initial problem of flagging an error for cairo_scale(0,0), but you've
> neither updated the test suite (test/invalid-matrix.c, checking that the
> error is not set immediately, but only on the invalid use) nor added new
> tests to check drawing with cairo_scale(0,0).

Actually, at the moment I didn't do much into the direction of 
supporting degenerate matrices; I just started this cairo_transform_t 
stuff and wanted to get some feedback about the general "looking" of my 
code.  I believe it is far from being any complete ;)  BTW thanks for 
the name of the testcase for me, I'll look at it!

> However, I did spot this:
>  > @@ -1140,14 +1091,20 @@ _cairo_gstate_stroke_extents (cairo_gstate_t 
>  *gstate,
>  >
>  >      _cairo_traps_init (&traps);
>  >
>  > -    status = _cairo_path_fixed_stroke_to_traps (path,
>  > -                        &gstate->stroke_style,
>  > -                        &gstate->ctm,
>  > -                        &gstate->ctm_inverse,
>  > -                        gstate->tolerance,
>  > -                        &traps);
>  > -    if (status == CAIRO_STATUS_SUCCESS) {
>  > -        _cairo_gstate_traps_extents_to_user_rectangle(gstate, 
> &traps, x1, y1, x2, y2);
>  > +    status = cairo_transform_get_inverse (&gstate->transform, 
> &matrix_inverse);
>  > +    if (status)
>  > +    {
>  > +        /* XXX: Transform */
>  > +        status = _cairo_path_fixed_stroke_to_traps (path,
>  > + &gstate->stroke_style,
>  > + &gstate->transform.matrix,
>  > +                                                    &matrix_inverse,
>  > +                                                    gstate->tolerance,
>  > +                                                    &traps);
>  > +        if (status == CAIRO_STATUS_SUCCESS) {
>  > +            _cairo_gstate_traps_extents_to_user_rectangle(gstate, 
> &traps,
>  > +                                                          x1, y1, 
> x2, y2);
>  > +        }
>  >      }
>  >
>  >      _cairo_traps_fini (&traps);
> 
> which looks wrong, as the bbox determination seems only to occur on the
> error path from cairo_transform_get_inverse().

You mean, if (status) should become if (status == CAIRO_STATUS_SUCCESS)? 
  If so, that's probably right and what I meant.  (I've just got the 
habit of using such if (variable) constructs, mainly from checking 
pointers I think; and I did not think about this probably, but I suppose 
if any !status should be right)

> [Hmm, that should be
> _cairo_transform_get_inverse(), unless you really are proposing that
> cairo exports the cairo_transform_t as public API.]

I see, so I should probably change it, right?

Daniel

-- 
Done:     Bar-Sam-Val-Wiz, Dwa-Elf-Hum-Orc, Cha-Law, Fem-Mal
Underway: Ran-Gno-Neu-Fem
To go:    Arc-Cav-Hea-Kni-Mon-Pri-Rog-Tou