Hello.

I am trying to "git format-patch" for allegro5 and i have encounter a small
problem with trailing white space. My editor is set to remove the trailing
white space and i was so sure( Oh how wrong i was ! ) that every other editor
in the world does it. Well who the hell needs a bunch of white space characters
on an empty line.
And now when i formated the patch i noticed that the files i was working with
had this white space trailing on almost every empty line. The patch is now plagued
with lines like this:

Not only is the patch unnecessary large but it is also harder to look trough
and see what was really done.

Does allegro have any kind of guidelines regarding white space trailing in the code.
Should it be removed? Or should it be kept as the original author wrote it?

I am asking because it will be a real pain to return the white space in the files
to original state. My GIT-KUNG-FU is not that good to remove this kind of problem( if it is possible).
And my patch looks ugly.

Your best bet is to reapply your changes to a fresh source checkout with a different editor that doesn't eat white space.

We prefer the whitespace changes to be their own commits (although we rarely do them unless the whitespace is really bad). It's not particularly hard to split up a commit though:

First, undo the commit (this'll keep the changes):

git reset HEAD~1

Then use the interactive add mode.

git add -p <optionally put the files you meant to change here>

That'll put you in a mode where you'll go through every diff hunk. Press y if you want to stage it (i.e. the changes you really meant to do), or n if not. After that's done you can do:

git commit -m "Your commit message"

After that's done you'll still have your whitespace changes... I'd probably avoid committing them, but you can if you want.

If that gives you too much trouble, then don't worry too much we can do the split for you as well.

^ Excellent post SiegeLord. Interactive add is probably the best option here.

Edgar, I'm getting old too, but seriously learn something new and master such an important tool. Git can save you so much fuss if you just take a weekend to learn it. There's even a free/libre "book" available online to walk you through most of it. Beyond that, ask in #git on freenode any question and they'll steer you in the right direction and hold your hand if you need it. And when we're present we can help you in #allegro too.

On a side note: if you are viewing a patch with a Git command and the patch is full of white-space only noise you can add a -w option to suppress it. It won't help with merging such a patch, but it can at least help for reviewing it. UIs for viewing Git history should support such an option too (or should be complained to if it doesn't).

@bamccaig

I do use git, but I haven't really had the need for its more advanced features. I rarely need any more than git branch and git merge. If git can somehow magically remove whitespace changes from a commit that's cool and its news to me.

Thank you all.

I will try to get rid of the white space changes on my own for now. Improving my GIT-KUNG-FU along the way.
It will be propably a real pain, but on the other hand, a valuable experience.

Will post the result when i am satisfied with them.

You may be able to hand edit the patch and literally remove whole swaths of ws changes.

Ok. I have managed to get rid of the white space changes and i have compiled a
patch preview so that you can see what i am trying to do and if it is worth
to continue( if it is actually desired to make such changes ).

PATCH HERE

I have noticed a little differences between some bitmap drawing functions so i made those changes:

1:
al_draw_scaled_bitmap and al_draw_scaled_rotated_bitmap where one takes
absolute destination bitmap size and one takes scale value.

Made them both take scale value and to compensate for the change i added
another functions named al_draw_stretched_bitmap and al_draw_stretched_rotated_bitmap
that takes absolute destination bitmap size.

2:
al_draw_tinted_bitmap_region and al_draw_tinted_scaled_rotated_bitmap_region had
the tint argument in different order. Corrected that.

3:
Now only region drawing functions use source bitmap position and dimensions arguments.

4:
Made a function for every sane combination of Tinted, Scaled, Stretched and Rotated drawing.
Also made those functions with region variants.

5:
Added documentation for all available functions.

6:
Adjusted examples to use the new or changed drawing functions.

To compile the allegro with this preview patch you have to disable
WANT_DEMO and WANT_TESTS in cmake for now.
Still trying to figure out how the tests works for now.

Take a look and share your opinions please.

Woah man, are you from the future?

{"name":"611906","src":"\/\/djungxnpq2nug.cloudfront.net\/image\/cache\/7\/2\/72e00ab6eb7bf3b17b698d64d215eedd.png","w":1920,"h":1080,"tn":"\/\/djungxnpq2nug.cloudfront.net\/image\/cache\/7\/2\/72e00ab6eb7bf3b17b698d64d215eedd"}

I'm looking at your patch now...

A tiny bit of work has been done on the patch again. The demos and tests now compiles too.
Also managed to get the test_bitmaps2.ini to pass all the tests.
But the test_bitmaps.ini is much bigger beast and i still fully do not understand
whole test_driver.c so it fails some of the tests.
A few tips from someone who knows what is going on there would be really helpfull.

New patch preview can be found here.

@Edgar:
Seems like all that white space trailing in the patch also messed up my TTD ( a Time Travel Device, not a Tranport Tycoon Deluxe )
settings and the patch ended in the wrong time

Seems there are options for dealing with white space changes ;

Just to be clear - items 1 & 2 in your list are breaking changes to the Allegro API, are they?

Yes, they are. Unfortunately.

But then i could just name the new functions something like:

al_blit_bitmap
al_blit_tinted_bitmap
al_blit_stretched_rotated_bitmap_region

and so on to keep the current al_draw_bitmap routines untouched.

If we're going to make sweeping changes, I have some suggestions :

Make naming and parameters consistent (its hard enough to remember the parameters as it is) :

See how all the drawing routines start with the destination? And how the region drawing routines start with the source rectangle and then the destination?

After that, it's just a matter of adding the parameter named by the function to the argument list before the flags parameter.

Everything lines up, the parameters are accepted in the order given by the naming of the function, and it just makes things so much easier.

*The bonus of this naming scheme is that it doesn't require any changes to the ABI of Allegro. The current versions of drawing routines could be mapped to the new ones to maintain compatibility, while at the same time, creating consistency and order in the naming and use of these functions.

I'm still looking at your patch.

Thank you Edgar for your suggestion and wording my intention with this patch correctly.
As my english is self-taught, sometimes its hard to write down exactly what i want.

Yes. The whole point of this patch is to make the drawing functions naming and parameter list consistent.
But my propposed changes are actually breaking the compatibility that is why i like your naming of the functions better.
I am willing to completely rewrite it if we can come up with a reasonable naming and parameter order.

I am proposing to use your naming witch does not interfere with the current drawing functions:
al_draw_bitmap_all-sane-variations and al_draw_bitmap_region_all-sane-variations

And parameter order list:
bitmap, tint, sx, sy, sw, sh, cx, cy, dx, dy, dw or scalex, dh or scaley, angle, flags

Witch would make the longest function name:

al_draw_bitmap_region_tinted_stretched_rotated( bmp, tint, sx, sy, sw, sh, cx, cy, dx, dy, dw, dh, angle, flags );

But i am realy fine with anything that is consistent.

Current naming and parameter order list in my patch ( breaks compatibility ):

Okay, let me try to explain my naming scheme :

It goes like this (if it were C++, these would be the functions names, but since C doesn't allow function overloading this will have to do);

al_draw_bitmap
al_draw_bitmap_region

Which makes the first parameters the bmp , and then source and destination. After that you have either stretched, scaled, tinted, or rotated. Any version that is stretched or scaled should be the next part of the function name.

_stretched
_scaled

This makes the next parameter either the destination width and height, or the scale factor for x and y. This parameter is optional, and then the next ones are _tinted, _rotated, and _tinted_rotated in that order. And then the flags come last.

So it is :

al_draw_bitmap[_region][_stretched | _scaled] | [_tinted | _rotated | _tinted_rotated]

I think that's pretty easy to do, but you'd have to ask others for their opinion on this, I'm not a developer, but I can help you.

And in the end, there should basically only be one function that every one of those function calls. draw_bitmap should call draw_bitmap_region.

Or if performance is a concern I guess you have to write every single function. But I would try to avoid that.

#include <std_disclaimer.h>

Side question: Do... you actually need GIF support?

I mean, yeah yeah, lots of people on forums say that and they're often wrong and need to just STFU and answer the question...

But,

High-level question: do you really need GIF support? Are you using it for animations? Could you implement animations a different way? Because the only real use-case I can think of for GIF in >2010 is for an image editor or conversion tool and the thing is... Allegro is a bad candidate for that because Allegro is designed for games--not tools. It fails on all kinds of edge cases of images that if you were simply making a game, you'd already have complete control of file formats and just use a different file format, or the same format, but with a different internal header that works. That is, is Allegro fails on some variation of BMP/GIF/PNG, you simply convert all your files to be a workable format.

PNG is basically the only file format anyone uses anymore. Even BMP's are fat, and, may even be slower than PNG because compression increases the speed you can suck them from the hard drive while modern CPUs can easily run decompression. (That's the entire premise for NTFS compression, btw. And we've been using it since... the 2000's. Hard drives are slower than CPUs, so make the files smaller and decompress them once they hit the CPU.)

So I'm not sure if I'm illustrating my point effectively but if this is a game, you should be able to control your file formats and GIF doesn't add any features that other, better, formats don't already have. Except animation... for which I really wish APNG (animated PNG) would become a wildly accepted format. But for a game, there's almost zero situations (that I can think of) where you'd want to intentionally use the GIF format.

But if it's a tool (I'm speaking from experience here!) Allegro is going to bite you in the butt in strange places. Like loading BMP's which can have tons of different header styles and Allegro fails on some of them--even ones that GIMP outputs. Even if it's just one type, end-users aren't going to appreciate "It's not in my control, it's Allegro's fault." when your tool can't load every image on their computer. They just need it to work. And in that case, you should use a dedicated image loading library. Because Allegro is focused on making games, not making every edge-case image format work--unlike an image library.

Beautiful post I'm not sure it belongs in this thread though.

Ok. Here it is.

This is how the function names and parameter list looks like:

There are always trade-offs when adding new API. On one hand, you add new functionality that was not easy or possible before. On the other hand, you add complexity that makes it more difficult to learn and use it. Only when the first outweighs the second does should we consider adding it. I don't think it does in the case of this change.

Allegro already has 11 ways to draw a bitmap. This patch adds 11 more ways (or 22, since you're coming up with new names). That's up to 33 ways to draw a bitmap. What new functionality do you gain from this? It would appear that you're saving a person from occasionally writing dw / al_get_bitmap_width(bmp) or xscale * al_get_bitmap_width(bmp) if their requirements are mismatched with what Allegro expects. But, in return, you now need to know the difference between scaled and stretched; you need to understand exactly what it means for dw to be specified when angle is e.g. PI/3 (it clearly doesn't actually correspond to the destination width). And, of course, you need to spend time thinking about which variant to use given what values you have.

Is Allegro's current bitmap drawing API perfect? No; I for sure can appreciate the oddity of having al_draw_scaled_bitmap and al_draw_scaled_rotated_bitmap taking different methods of scaling. And why does al_draw_scaled_bitmap need to know the source width and height?

I actually know the reason why. It's because this is Allegro 5, and it just copied Allegro 4's API with a few renames. People often say that Allegro 5 is completely different than Allegro 4, but this is one of the places where the resemblance is clear. Maybe it should have done something different, but at this point it's 8 years too late.

8 years too late but never a day too soon to fix old mistakes.

Izual,

That's closer to what I had in mind, but there's an important problem. cx and cy should not be the second parameter in draw bitmap or the fifth in draw bitmap region. It should come after dw,dh.

So that you always have

al_draw_bitmap_X(bmp , dx , dy , ...)
al_draw_bitmap_scaled_X(bmp , dx , dy , dw , dh , ...)

and

al_draw_bitmap_region_X(bmp , sx , sy , sw , sh , dx , dy , ...)
al_draw_bitmap_region_scaled_X(bmp , sx , sy , sw , sh , dx , dy , dw , dh , ...)

Also, I have a question, are cx and cy pivot points in the source bitmap?

SiegeLord - the destination width and height would be assumed to be pre-rotation.

stretched vs scaled is obvious in their naming. Scaling uses factors and stretching involves dimensions.

Consistency is key. And with autocomplete these days you can type al_draw and then select which one you want and it will give you the parameters.

Anyway, this could all be hidden under ALLEGRO_UNSTABLE. And we could easily deprecate the old style drawing functions and have them forward themselves to the new ones. It could all happen rather painlessly. The only thing that would change is that you would get deprecation warnings when defining ALLEGRO_UNSTABLE.

My 2¢

lulul I'm embarassed. Don't post at 3 am folks.

@Edgar
Yes. cx,cy is a point on the source bitmap. This point is drawn at the dx,dy position and the bitmap is rotated around this point.
That is why i placed it together with the parameters that deal with the source bitmap. It seemed logical to me.

@SiegeLord
Thank you for your answear and stating the reasons. Just wanted to add a little bit to allready a great library.

With the availability of transforms, similar code can be easily written, but at the expense of a great deal of verbosity ;

The question is, do we want this :

Or do we want this :

al_draw_bitmap_scaled_tinted_rotated(bmp , cx , cy , scalex , scaley , al_map_rgb(0,96,255) , M_PI/2.0 , 0);

The choice seems clear.