Re: [RFC] Framebuffer rotation patch

[Michal Suchanek <hramrach@centrum.cz>]

On 11 February 2010 11:19, Michal Suchanek <hramrach@centrum.cz> wrote:
> 2010/2/11 Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com>:
>> Michal Suchanek wrote:
>>> Hello
>>>
>>> Sending a preliminary framebuffer rotation patch.
>>>
>>> You can use videotest to see 4 tiles rotated from the same bitmap data.
>>>
>>>
>> +char leaf_data[] = { 0x00, 0x0f, 0xe0, 0x00,
>> +                     0x00, 0x7f, 0xfc, 0x00,
>> +                     0x01, 0xff, 0xff, 0x00,
>> +                     0x03, 0xff, 0xff, 0x80,
>> This is a blob. Could it be generated automatically at build time?
>
> How less of a blob it would be if it was included as a bitmap?
>
> This is just a shape used for the video tests.
>
> There are some X tools for working with bitmaps/pixmaps which could
> generate this from a viewable file but they are usually not available
> on Windows and other non-X platforms AFAIK.
>
>> +
>> +  sans = grub_font_get ("Helvetica Bold 14");
>> Please use free font rather than Helvetica
>
> I am pretty sure I did not choose the fonts, they must have been there
> before I started with modifications to the tests.
>
>> Could you split addition of videotests from the rest of the patch?
>> -    unsigned int x;
>> -    unsigned int y;
>> -    unsigned int width;
>> -    unsigned int height;
>> +    int x;
>> +    int y;
>> +    int width;
>> +    int height;
>> Why do you need negative values?
>
> I don't need the negative values everywhere but this change was to
> align the typing so that casts are not required.
>
>> +/* Supported operations are simple and easy to understand.
>> + * MIRROR  |  swap image across (around) the vertical axis
>> + * FLIP    -  swap image across the horizontal axis - upside down
>> + * SWAP    /  swap image across the x=y axis - swap the x and y coordinates
>> It's just a D_8 group. Could you add a comment to functions what they do
>> in group theoretical sense? It would make the code easier to follow and
>
> Obviously it is a group, any set of transforms closed on composition is.
>
> And according to Wikipedia it's actually called D4.
>
>> compute transformations for mathematicians. Perhaps another
>> representation of D_8 would result in more efficient code?
>
> If you have clearer explanation or more efficient code please share.
>

Let me elaborate a bit more on this one.

The provided explanation should make it possible for anybody who
understands plain English to also understand the operations used and
how they interact should they know group theory in English, any other
language, or not at all.

I guess mentioning the English mathematical terms where applicable and
where I can figure them out should not that much of a problem, as well
as checking that the transform functions are sufficiently described.

Still the fact that is used (and explained) is that the operations I,
- and / generate the group and in addition you can get all members of
the group if you compose a subset of these generators in a fixed order
which is trivially true for comutative groups but not so for a
non-comutative group such as this one.

Thanks

Michal