Re: [PATCH v2 5/8] d3dx9: Implement ID3DXFont_GetGlyphData.

[Matteo Bruni]

On Mon, Jan 6, 2020 at 3:35 PM Sven Baars  wrote:
> Based on a patch by Tony Wasserka.
> Signed-off-by: Sven Baars
>
> dlls/d3dx9_36/font.c       | 64 ++++++++++++++++++++++++++++++++++++--
>  dlls/d3dx9_36/tests/core.c | 11 +++----
>  2 files changed, 66 insertions(+), 9 deletions(-)
>
> diff --git a/dlls/d3dx9_36/font.c b/dlls/d3dx9_36/font.c
> index fe645a51c0..e204ca3012 100644
> --- a/dlls/d3dx9_36/font.c
> +++ b/dlls/d3dx9_36/font.c
> @@ -166,12 +166,70 @@ static HDC WINAPI ID3DXFontImpl_GetDC(ID3DXFont *iface)
>      return This->hdc;
>  }
>
> +/************************************************************
> -
> - ID3DXFont_GetGlyphData
> -
> -
> -
> - Returns the internally stored texture and some info about
> -
> - the position of the requested glyph on that texture
> -
> -
> -
> - PARAMS
> -
> - glyph    [I] glyph
> -
> - texture  [O] length of the string
> -
> - blackbox [O] smallest rectangle that completely encloses the glyph on the texture
> -
> - cellinc  [O] offset from the baseline to the bottom of the glyph
> -
> -
> -
> - RETURNS
> -
> - Success: D3D_OK
> -
> - Failure: D3DERR_INVALIDCALL
> -
> -
>        D3DXERR_INVALIDDATA
> -
> -
> -
> - NOTES
> -
> - Glyphs which are passed to this function get preloaded, too
> -
> -
> - */
>
> static HRESULT WINAPI ID3DXFontImpl_GetGlyphData(ID3DXFont *iface, UINT glyph,
>          IDirect3DTexture9 **texture, RECT *blackbox, POINT *cellinc)
>  {
> - FIXME("iface %p, glyph %#x, texture %p, blackbox %p, cellinc %p stub!\n",
> -
>        iface, glyph, texture, blackbox, cellinc);
> - return E_NOTIMPL;
> - struct d3dx_font *This = impl_from_ID3DXFont(iface);
> - HRESULT hr;
> - int i;
> - TRACE("iface %p, glyph %#x, texture %p, blackbox %p, cellinc %p\n",
> -
>      iface, glyph, texture, blackbox, cellinc);

Please leave a blank line between variable declarations and the first statement.
> -
> - for (i = 0; i < This->glyph_count; i++)
> -
>    if (This->glyphs[i].id == glyph)
> -
>    {
> -
>        if (cellinc)
> -
>            *cellinc = This->glyphs[i].cellinc;
> -
>        if (blackbox)
> -
>            *blackbox = This->glyphs[i].blackbox;
> -
>        if (texture)
> -
>            *texture = This->glyphs[i].texture;
> -
>        if (texture && *texture)
> -
>            IDirect3DTexture9_AddRef(This->glyphs[i].texture);
> -
>        return D3D_OK;
> -
>    }
> -
> - hr = ID3DXFont_PreloadGlyphs(iface, glyph, glyph);
> - if (FAILED(hr))
> -
>    return hr;
> -
> - /* Try again */
> - for (i = 0; i < This->glyph_count; i++)
> -
>    if (This->glyphs[i].id == glyph)
> -
>    {
> -
>        if (cellinc)
> -
>            *cellinc = This->glyphs[i].cellinc;
> -
>        if (blackbox)
> -
>            *blackbox = This->glyphs[i].blackbox;
> -
>        if (texture)
> -
>            *texture = This->glyphs[i].texture;
> -
>        if (texture && *texture)
> -
>            IDirect3DTexture9_AddRef(This->glyphs[i].texture);
> -
>        return D3D_OK;
> -
>    }

A couple of things. Is this "try once, if not found try again"
justified by tests? Otherwise it seems easier to call PreloadGlyphs()
unconditionally (i.e. get rid of the first loop).
The other one is: given that looking up glyphs is going to be a
frequent operation, linearly searching an array every time isn't going
to be ideal. We probably want to use the RB tree from
include/wine/rbtree.h for storing the glyphs.