[RFC PATCH 1/5] d2d1: Store cubic bezier control points.

[Connor McAdams]

On Tue, Mar 17, 2020 at 09:04:10PM +0330, Henri Verbeet wrote:
> On Sun, 15 Mar 2020 at 23:13, Connor McAdams <conmanx360 at gmail.com> wrote:
> > +/* Flag bit for bezier vertices. */
> > +#define D2D_VERTEX_TYPE_SPLIT_BEZIER 0x40
> > +#define D2D_VERTEX_TYPE_BEZIER 0x80
> >  enum d2d_vertex_type
> >  {
> > -    D2D_VERTEX_TYPE_NONE,
> > -    D2D_VERTEX_TYPE_LINE,
> > -    D2D_VERTEX_TYPE_BEZIER,
> > -    D2D_VERTEX_TYPE_SPLIT_BEZIER,
> > +    D2D_VERTEX_TYPE_NONE = 0,
> > +    D2D_VERTEX_TYPE_LINE = 1,
> > +    D2D_VERTEX_TYPE_QUADRATIC_BEZIER = 2 | D2D_VERTEX_TYPE_BEZIER,
> > +    D2D_VERTEX_TYPE_CUBIC_BEZIER = 3 | D2D_VERTEX_TYPE_BEZIER,
> > +    D2D_VERTEX_TYPE_SPLIT_QUAD_BEZIER = 4 | D2D_VERTEX_TYPE_SPLIT_BEZIER,
> > +    D2D_VERTEX_TYPE_SPLIT_CUBIC_BEZIER = 5 | D2D_VERTEX_TYPE_SPLIT_BEZIER,
> >  };
> That's a bit ugly. If you really need to store extra information, you
> could replace the "vertex_types" array with something like the
> following:
> 
>     struct d2d_segment
>     {
>         uint16_t type;
>         uint16_t flags;
>     } *segments;
> 
> Or some variant. For what you're doing here though, it seems much
> easier to do something like the following:
> 
>     static BOOL d2d_vertex_type_is_bezier(enum d2d_vertex_type t)
>     {
>         return t == D2D_VERTEX_TYPE_QUADRATIC_BEZIER || t ==
> D2D_VERTEX_TYPE_CUBIC_BEZIER;
>     }
> 
Okay, I can change that. That does seem to be the cleaner way to do it.

> > @@ -57,6 +62,7 @@ struct d2d_segment_idx
> >      size_t figure_idx;
> >      size_t vertex_idx;
> >      size_t control_idx;
> > +    size_t bezier_idx;
> >  };
> >
> >  struct d2d_figure
> > @@ -70,6 +76,7 @@ struct d2d_figure
> >      D2D1_POINT_2F *bezier_controls;
> >      size_t bezier_controls_size;
> >      size_t bezier_control_count;
> > +    size_t bezier_control_points;
> >
> Are those really needed?
Yes, they become more useful in later patches where there are
triangulation structures per-bezier and not per-point, so knowing which
bezier control we are on and not which control point we are on becomes
necessary. I guess I could just introduce these changes in those patches
where it becomes used specifically, but it seemed to be more related to
this patch (in my opinion).
> > -static BOOL d2d_figure_insert_bezier_control(struct d2d_figure *figure, size_t idx, const D2D1_POINT_2F *p)
> > +static BOOL d2d_figure_insert_quadratic_bezier_control(struct d2d_figure *figure, size_t idx, const D2D1_POINT_2F *p)
> >  {
> >      if (!d2d_array_reserve((void **)&figure->bezier_controls, &figure->bezier_controls_size,
> > -            figure->bezier_control_count + 1, sizeof(*figure->bezier_controls)))
> > +            figure->bezier_control_points + 1, sizeof(*figure->bezier_controls)))
> >      {
> >          ERR("Failed to grow bezier controls array.\n");
> >          return FALSE;
> >      }
> >
> >      memmove(&figure->bezier_controls[idx + 1], &figure->bezier_controls[idx],
> > -            (figure->bezier_control_count - idx) * sizeof(*figure->bezier_controls));
> > +            (figure->bezier_control_points - idx) * sizeof(*figure->bezier_controls));
> >      figure->bezier_controls[idx] = *p;
> > +    ++figure->bezier_control_points;
> >      ++figure->bezier_control_count;
> >
> >      return TRUE;
> >  }
> >
> > -static BOOL d2d_figure_add_bezier_control(struct d2d_figure *figure, const D2D1_POINT_2F *p)
> > +static BOOL d2d_figure_add_quadratic_bezier_control(struct d2d_figure *figure, const D2D1_POINT_2F *p)
> >  {
> >      if (!d2d_array_reserve((void **)&figure->bezier_controls, &figure->bezier_controls_size,
> > -            figure->bezier_control_count + 1, sizeof(*figure->bezier_controls)))
> > +            figure->bezier_control_points + 1, sizeof(*figure->bezier_controls)))
> >      {
> >          ERR("Failed to grow bezier controls array.\n");
> >          return FALSE;
> >      }
> >
> > -    figure->bezier_controls[figure->bezier_control_count++] = *p;
> > +    figure->bezier_controls[figure->bezier_control_points++] = *p;
> > +    ++figure->bezier_control_count;
> >
> >      return TRUE;
> >  }
> These don't seem all that specific to quadratics.
They are specific to quadratics in that they're only introducing one new
control point. When cubics actually begin to be in use, there are
d2d_figure_add_cubic_bezier_control functions along with the quadratic
ones.

I guess I may be confused as to when to introduce certain things. I
could introduce the add_cubic functions in this patch, but then I'd have
to drop them all down to quadratics until they're actually beginning to
be used. I guess this patch already does that in some areas, so maybe
it'd make sense to just do them all like that.

If you have any suggestions, please let me know. Thanks for the review.