Re: OpenType Patches

[Eric Mader <mader jtcsv com>]

At 07:48 AM 9/6/2002, Owen Taylor wrote:
> > +           PangoGlyphUnit x_adj = 0;
> > +           int back = i;
> >
> > -           glyphs->glyphs[i].geometry.x_offset += PANGO_UNITS_26_6 
> (outgpos[i].x_pos);
> > -           glyphs->glyphs[i].geometry.y_offset += PANGO_UNITS_26_6 
> (outgpos[i].y_pos);
> > +           while (outgpos[back].back != 0)
> > +             {
> > +               back  -= outgpos[back].back;
> > +               x_pos += outgpos[back].x_pos;
> > +               y_pos += outgpos[back].y_pos;
> > +               x_adj += glyphs->glyphs[back].geometry.width;
> > +             }
>
> Don't we have to take into account glyphs->glyphs[].geometry.width
> for _all_ the characters intermediate between the current character
> and the base character?

This change is based on my understanding of how OpenType positioning done 
in the FreeType test program ftstrtto.c. I think the back field means "this 
glyph was positioned to line up with the glyph n before it; if that one 
moves, this one needs to move too." It needs to chain because one accent 
could be positioned relative to a previous accent which could be positioned 
relative to a base character. The old code didn't do this chaining or 
position adjustments, just the width adjustment.

The code in ftstrtto.c doesn't mess with the widths at all; it positions 
the accents at the position of the base character plus [x_pos, y_pos]. (I 
tried to do exactly that in this patch, but the positions aren't set when 
this code runs...)

So, in theory, I think you're right about needing to look at all of the 
intermediate widths. On the other hand, all of the intermediate glyphs will 
be marks of one kind or another, so they should all have zero widths.

> > +           glyphs->glyphs[i].geometry.x_offset += 
> PANGO_UNITS_26_6(x_pos) - x_adj;
> > +           glyphs->glyphs[i].geometry.y_offset += PANGO_UNITS_26_6(y_pos);
> >
> > -           for (j = i - outgpos[i].back; j < i; j++)
> > -             glyphs->glyphs[i].geometry.x_offset -= 
> glyphs->glyphs[j].geometry.width;
> > -
> >             if (outgpos[i].new_advance)
> >               /* Can't set new x offset for marks, so just make sure 
> not to increase it.
> >                  Can do better than this by playing with ->x_offset. */
> > -             glyphs->glyphs[i].geometry.width = 0;
> > +             glyphs->glyphs[i].geometry.width  = 
> PANGO_UNITS_26_6(outgpos[i].x_advance);
>
> Do you understand the FIXME here? I don't, really. The comment applies
> to the '= 0', so it certainly isn't literally relevant with your changes.
>
> >             else
> >               glyphs->glyphs[i].geometry.width += 
> PANGO_UNITS_26_6(outgpos[i].x_advance);
> >           }

No, I don't understand the comment here. I was tempted to remove it, but 
figured I should leave it in case someone else knows what it means. My 
understanding of the new_advance flag is that it means that x_advance is a 
new advance width rather than an adjustment to the existing advance width. 
(This is done because it makes some of the GPOS calculations a bit simpler)

> In general these changes look like they make sense, though. (The
> LangSys part of the patch looks fine to commit.)

OK.

> Regards,
>                                         Owen

Regards,
Eric