Re: OpenType Patches

At 09:07 AM 9/6/2002, Owen Taylor wrote:
> 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?

No. all of the glyphs->glyphs[i].geometry.x_offset values seemed to be zero on entry to this function. I've just been looking through the code, and it seems to explicitly set them this way. (in indic_engine_shape in indic-xft.c - I think I copied this from arabic-xft.c but I don't remember for sure - the same code is there too...) I thought that the x_offset and y_offset values would have the default (x, y) position of each glyph. I don't see any code that does anything other than apply the adjustments, so I guess there's something I'm not seeing. Can someone enlighten me?

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

Sure, unless I can figure out where the x, y positions are; using the position of the base glyph directly seems like the best way to go. Using the widths of the intermediate glyphs seems like a bit of a hack, somehow...

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

OK. I was afraid I was missing something important. I suppose if the code seems to work, then I can be pretty sure that I wasn't :-)

Any rush on committing these changes? I'm a bit pressed for time at the moment and may not be able to get them for a bit.



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]