[PATCH wayland 4/5] Add support for signed 24.8 decimal numbers

[Kristian Høgsberg]

On Wed, May 2, 2012 at 11:48 AM, Daniel Stone <daniel at fooishbar.org> wrote:
> Hi,
>
> On 2 May 2012 12:57, Pekka Paalanen <ppaalanen at gmail.com> wrote:
>> Hmm, any reason for not doing this instead?
>>        *p = (int32_t)trunc(d * 256.0)
>
> Wow is that ever embarrassing.
>
>> Also, any rationale in choosing trunc() instead of round() or
>> ceil/floor?
>>
>> trunc() makes
>> 0.9 -> 0
>> -0.9 -> 0
>> which means the length of "zero" is double the length of any other
>> number. No?
>
> Thanks for the review.  I've fixed this now to use trunc(foo * 256.0)
> or / 256.0 as appropriate, plus fixed up the sign issues.  There are
> some tests in there now for both negative and large numbers and they
> seem to work fine.
>
>> Since we use double as the API type, should there not be a check for
>> out-of-bounds values? Perhaps even #defined min and max values apps
>> could use for their own additional checks. And that raises the
>> question, should we have a
>>        typedef double signed_24_8;
>> which would seem odd at first, but make it apparent on the API
>> that you cannot pass just whatever doubles there?
>
> I'm not sure about the typedef, but I've at least put range handling
> into the marshalling code so it rejects too-large numbers.  Kristian?

I wasn't thinking that we'd convert to doubles at all.  We could just
make it be an int32_t or perhaps a wl_fixed_t typedef.  But the point
is that the canonical type of input coordinates is 24.8 fixed point,
not just in the protocol but also in the API.  There may be
implementation limitations on both sides (weston using GLfloats,
toolkits rounding to integers, for example), but those are
implementation decisions, not something we want to encode in the
library API.

A wl_fixed_t is probably the nicest way to go, and I think that we
should add support for it in the protocol xml.  This lets us generate
the right types in the stubs and the debug code can convert it to
double as it prints it, but on the wire and in marshalling it's just a
int32_t.

Kristian