Re: [PATCH] usp10: rewrite ScriptXtoCP

[Huw Davies]

Hi Aric,

Just some comments to keep the ball rolling on this.

In general I can now at least understand the algorithm, so it's a vast
improvement over the one it replaces :-)

On Mon, Jan 04, 2016 at 07:52:59AM -0600, Aric Stewart wrote:
> diff --git a/dlls/usp10/usp10.c b/dlls/usp10/usp10.c
> +/* Count the number of characters in a cluster */
> +static inline int get_cluster_charsize(const WORD *pwLogClust, int cChars, int index)
> +{
> - int size = 0;
> - int i;
> - for (i = 0; i < cChars; i++)
> -
>    if (pwLogClust[i] == index) size++;
> - return size;

Does it make sense to break out of this loop if (size && pwLogClust[i] != index) ?
> +}
> -
>
> +/*
> - To handle multi-glyph clusters we need to find all the glyphs that are
> - represented in by the cluster. This involves finding the glyph whose

"in by"
> - index is the cluster index as well as whose glyph indices are greater than
> - our cluster index but not part of a new cluster.
> -
> - Then we sum all those glyph's advances.

glyphs'
> +*/
> +static inline int get_cluster_advance(const int* piAdvance, const SCRIPT_VISATTR *psva, const WORD *pwLogClust, int cGlyphs, int cChars, int cluster, int direction)

Some of these lines are getting awfully long.
> +{
> - int glyph_start;
> - int glyph_end;
> - int i, advance;
> -
> - if (direction > 0)
> -
>    i = 0;
> - else
> -
>    i = (cChars - 1);
> -
> - for (glyph_start = -1, glyph_end = -1; i < cChars && i >= 0 && (glyph_start < 0 || glyph_end < 0); i+=direction)
> - {
> -
>    if (glyph_start < 0 && pwLogClust[i] != cluster) continue;
> -
>    if (pwLogClust[i] == cluster && glyph_start < 0) glyph_start = pwLogClust[i];
> -
>    if (glyph_start >= 0 && glyph_end < 0 && pwLogClust[i] != cluster) glyph_end = pwLogClust[i];
> - }
> - if (glyph_end < 0)
> - {
> -
>    if (direction > 0)
> -
>        glyph_end = cGlyphs;
> -
>    else /* Don't understand multi-glyph reversed clusters yet, do they occur for real? */

Should we add a FIXME for this?
> -
>        glyph_end = glyph_start + 1;
> - }
> -
> - /* Check for fClusterStart, finding this generally would mean a malformed set of data */
> - for (i = glyph_start+1; i< glyph_end; i++)
> - {
> -
>    if (psva[i].fClusterStart)
> -
>    {
> -
>        glyph_end = i;
> -
>        break;
> -
>    }
> - }
> -
> - for ( advance = 0, i = glyph_start; i < glyph_end; i++)

Odd leading space before advance.
> -
>    advance += piAdvance[i];
> -
> - return advance;
>
> +}
> -
> -
>
> /***********************************************************************
> -
>  ScriptXtoCP (USP10.@)
> -
> -
> - Basic algorithm :
> -
> - use piAdvance to find the cluster we are looking at
> -
> - Find the character that is the first character of the cluster
> -
> - That is our base piCP
> -
> - If the script snaps to cluster boundries (Hebrew, Indic, Thai) then we are good
> -
> - Otherwise if the cluster is larger than 1 glyph we need to determin how far through the cluster

determine.
> -
> - to advance the cursor.
> */
>
> HRESULT WINAPI ScriptXtoCP(int iX,
>                             int cChars,
> @@ -2587,16 +2655,10 @@ HRESULT WINAPI ScriptXtoCP(int iX,
>                             int *piCP,
>                             int *piTrailing)
>  {
> - int item;
> - float iPosX;
> - float iLastPosX;
> - int iSpecial = -1;
> - int iCluster = -1;
> - int clust_size = 1;
> - int cjump = 0;
> - int advance;
> - float special_size = 0.0;
> int direction = 1;
> -
>
> int iPosX;
> -
>
> int i;
> -
>
> int glyph_index, cluster_index;
>
> TRACE("(%d,%d,%d,%p,%p,%p,%p,%p,%p)\n",
>       iX, cChars, cGlyphs, pwLogClust, psva, piAdvance,
>
> @@ -2605,6 +2667,7 @@ HRESULT WINAPI ScriptXtoCP(int iX,
>      if (psa->fRTL && ! psa->fLogicalOrder)
>          direction = -1;
> - /* Looking for non-reversed clusters in a reversed string */
> if (direction<0)
> {
>     int max_clust = pwLogClust[0];
>
> @@ -2616,14 +2679,15 @@ HRESULT WINAPI ScriptXtoCP(int iX,
>              return S_OK;
>          }
> -
>    for (item=0; item < cChars; item++)
> -
>        if (pwLogClust[item] > max_clust)
> -
>    for (i=0; i< cChars; i++)
> -
>        if (pwLogClust[i] > max_clust)
>        {
>            ERR("We do not handle non reversed clusters properly\n");
>            break;
>        }
>
> }
> -
>
> /* Handle an iX < 0 */
> if (iX < 0)
> {
>     *piCP = -1;
>
> @@ -2631,102 +2695,134 @@ HRESULT WINAPI ScriptXtoCP(int iX,
>          return S_OK;
>      }
> - iPosX = iLastPosX = 0;
> - /* find the glyph_index based in iX */
> if (direction > 0)
> -
>    item = 0;
> - {
> -
>    for (glyph_index = -1, iPosX = iX; iPosX >=0 && glyph_index < cGlyphs; iPosX -= piAdvance[glyph_index+1], glyph_index++)
> -
>        ;
> - }
> else
> -
>    item = cChars - 1;
> - for (; iPosX <= iX && item < cChars && item >= 0; item+=direction)
> - {
> -
>    iLastPosX = iPosX;
> -
>    if (iSpecial == -1 &&
> -
>         (iCluster == -1 ||
> -
>          (iCluster != -1 &&
> -
>             ((direction > 0 && iCluster+clust_size <= item) ||
> -
>              (direction < 0 && iCluster-clust_size >= item))
> -
>          )
> -
>         )
> -
>        )
> - {
> -
>    for (glyph_index = -1, iPosX = iX; iPosX > 0 && glyph_index < cGlyphs; iPosX -= piAdvance[glyph_index+1], glyph_index++)
> -
>        ;
> - }
> -
> - TRACE("iPosX %i ->  glyph_index %i (%i)\n", iPosX, glyph_index, cGlyphs);
> -
> - *piTrailing = 0;
> - if (glyph_index >= 0 && glyph_index < cGlyphs)
> - {
> -
>    /* find the cluster */
> -
>    if (direction > 0 )
> -
>        for (i = 0, cluster_index = pwLogClust[0]; i < cChars && pwLogClust[i] <= glyph_index; cluster_index=pwLogClust[i++])
> -
>            ;
> -
>    else
> -
>        for (i = 0, cluster_index = pwLogClust[0]; i < cChars && pwLogClust[i] >= glyph_index; cluster_index=pwLogClust[i++])
> -
>            ;
> -
> -
>    TRACE("cluster_index %i\n", cluster_index);
> -
> -
>    if (direction < 0 && iPosX >= 0 && glyph_index != cluster_index)
>    {
> -
>        int check;
> -
>        int clust = pwLogClust[item];
> -
>        /* We are off the end of the string */
> -
>        *piCP = -1;
> -
>        *piTrailing = 1;
> -
>        return S_OK;
> -
>    }
>
> -
>        iCluster = -1;
> -
>        cjump = 0;
> -
>        clust_size = get_cluster_size(pwLogClust, cChars, item, direction,
> -
>                                      &iCluster, &check);
> -
>        advance = get_glyph_cluster_advance(piAdvance, psva, pwLogClust, cGlyphs, cChars, clust, direction);
> -
>    /* find the first character of the cluster */
> -
>    for (i = 0;  i < cChars && pwLogClust[i] != cluster_index; i++)
> -
>        ;
>
> -
>        if (check >= cChars && direction > 0)
> -
>        {
> -
>            int glyph;
> -
>            for (glyph = clust; glyph < cGlyphs; glyph++)
> -
>                special_size += get_glyph_cluster_advance(piAdvance, psva, pwLogClust, cGlyphs, cChars, glyph, direction);
> -
>            iSpecial = item;
> -
>            special_size /= (cChars - item);
> -
>            iPosX += special_size;
> -
>        }
> -
>        else
> -
>    TRACE("first char index %i\n",i);
> -
>    if (scriptInformation[psa->eScript].props.fNeedsCaretInfo)
> -
>    {
> -
>        /* Check trailing */
> -
>        if (glyph_index != cluster_index ||
> -
>            (direction > 0 && abs(iPosX) <= (piAdvance[glyph_index] / 2)) ||
> -
>            (direction < 0 && abs(iPosX) >= (piAdvance[glyph_index] / 2)))
> -
>            *piTrailing = get_cluster_charsize(pwLogClust,cChars,cluster_index);
> -
>    }
> -
>    else
> -
>    {
> -
>        int cluster_size;
> -
> -
>        cluster_size = get_cluster_charsize(pwLogClust, cChars, cluster_index);
> -
> -
>        if (cluster_size > 1)
>        {
> -
>            if (scriptInformation[psa->eScript].props.fNeedsCaretInfo)
> -
>            /* Be part way through the glyph cluster based on size and position */
> -
>            int cluster_advance = get_cluster_advance(piAdvance, psva, pwLogClust, cGlyphs, cChars, cluster_index, direction);
> -
>            double cluster_part_width = cluster_advance / (float)cluster_size;
> -
>            double adv;
> -
>            int part_index;
> -
> -
>            /* back up to the beginning of the cluster */
> -
>            for (adv = iPosX, part_index = cluster_index; part_index <= glyph_index; part_index++)
> -
>                adv += piAdvance[part_index];
> -
>            if (adv > iX) adv = iX;
> -
> -
>            TRACE("Multi-char cluster, no snap\n");
> -
>            TRACE("cluster size %i, pre-cluster iPosX %f\n",cluster_size, adv);
> -
>            TRACE("advance %i divides into %f per char\n", cluster_advance, cluster_part_width);
> -
>            if (direction > 0)
>            {
> -
>                if (!cjump)
> -
>                    iPosX += advance;
> -
>                cjump++;
> -
>                for (part_index = 0; adv >= 0; adv-=cluster_part_width, part_index++)
> -
>                    ;
> -
>                if (part_index) part_index--;
>            }
>            else
> -
>                iPosX += advance / (float)clust_size;
> -
>            {
> -
>                for (part_index = 0; adv > 0; adv-=cluster_part_width, part_index++)
> -
>                    ;
> -
>                if (part_index > cluster_size)
> -
>                {
> -
>                    adv += cluster_part_width;
> -
>                    part_index=cluster_size;
> -
>                }
> -
>            }
> -
> -
>            TRACE("base_char %i part_index %i, leftover advance %f\n",i, part_index, adv);
> -
> -
>            if (direction > 0)
> -
>                i += part_index;
> -
>            else
> -
>                i += (cluster_size - part_index);
> -
> -
>            /* Check trailing */
> -
>            if ((direction > 0 && fabs(adv) <= (cluster_part_width / 2.0)) ||
> -
>                (direction < 0 && adv && fabs(adv) >= (cluster_part_width / 2.0)))
> -
>                *piTrailing = 1;
>        }
> -
>    }
> -
>    else if (iSpecial != -1)
> -
>        iPosX += special_size;
> -
>    else /* (iCluster != -1) */
> -
>    {
> -
>        int adv = get_glyph_cluster_advance(piAdvance, psva, pwLogClust, cGlyphs, cChars, pwLogClust[iCluster], direction);
> -
>        if (scriptInformation[psa->eScript].props.fNeedsCaretInfo)
> -
>        else
>        {
> -
>            if (!cjump)
> -
>                iPosX += adv;
> -
>            cjump++;
> -
>            /* Check trailing */
> -
>            if ((direction > 0 && abs(iPosX) <= (piAdvance[glyph_index] / 2)) ||
> -
>                (direction < 0 && abs(iPosX) >= (piAdvance[glyph_index] / 2)))
> -
>                *piTrailing = 1;
>        }
> -
>        else
> -
>            iPosX += adv / (float)clust_size;
>    }
> }
> -
> - if (direction > 0)
> - {
> -
>    if (iPosX > iX)
> -
>        item--;
> -
>    if (item < cChars && ((iPosX - iLastPosX) / 2.0) + iX >= iPosX)
> -
>    {
> -
>        if (scriptInformation[psa->eScript].props.fNeedsCaretInfo && clust_size > 1)
> -
>            item+=(clust_size-1);
> -
>        *piTrailing = 1;
> -
>    }
> -
>    else
> -
>        *piTrailing = 0;
> - }
> else
> {
> -
>    if (iX == iLastPosX)
> -
>        item++;
> -
>    if (iX >= iLastPosX && iX <= iPosX)
> -
>        item++;
> -
>    TRACE("Point falls outside of string\n");
> -
>    if (glyph_index < 0)
> -
>        i = cChars-1;
> -
>    else /* (glyph_index >= cGlyphs) */
> -
>        i = cChars;
>
> -
>    if (iLastPosX == iX)
> -
>        *piTrailing = 0;
> -
>    else if (item < 0 || ((iLastPosX - iPosX) / 2.0) + iX <= iLastPosX)
> -
>    /* If not snaping in the reverse direction (such as Hebrew) Then 0
> -
>       point flow to the next character */
> -
>    if (direction < 0)
>    {
> -
>        if (scriptInformation[psa->eScript].props.fNeedsCaretInfo && clust_size > 1)
> -
>            item-=(clust_size-1);
> -
>        *piTrailing = 1;
> -
>        if (!scriptInformation[psa->eScript].props.fNeedsCaretInfo && abs(iPosX) == piAdvance[glyph_index])
> -
>            i++;
> -
>        else
> -
>            *piTrailing = 1;
>    }
> -
>    else
> -
>        *piTrailing = 0;
>
> }
> -
>
> *piCP = item;
> -
>
> *piCP = i;
>
> TRACE("*piCP=%d\n", *piCP);
> TRACE("*piTrailing=%d\n", *piTrailing);
>

[PATCH] usp10: rewrite ScriptXtoCP

[Aric Stewart]

On 1/15/16 5:35 AM, Huw Davies wrote:
> Hi Aric,
>
> Just some comments to keep the ball rolling on this.
>
> In general I can now at least understand the algorithm, so it's a vast
> improvement over the one it replaces :-)

Thanks for the feedback Huw.  English, it may be my native language but I still suck at it. :)

-aric