Re: OpenType Patches

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




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