Re: OpenType Patches

[Owen Taylor <otaylor redhat com>]

Eric Mader <mader jtcsv com> writes:

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

Yes, this made sense to me.
 
> 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...)

Are you just referring to the Xft bug, or is this something else?
 
> 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.

Since taking all the intermediate glyphs into account shouldn't be
hard, I'd rather we did that then count on all the intermediate
widths being zero.
 
> > > +           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)

Lets just kill the comment then; an inaccurate comment is worse than
no comment.

Regards,
                                        Owen