create_bitmap() problem.
A J

why should this crash allegro ?
it crashes on the free(bitmap) inside of destroy_bitmap()
it says "Damage after normal block"
which sounds like memory corruption.

and yes i know, im requesting 0,0 size bitmap.

WinXP, alg 4.1.20b4 MSVC7.1 (2003 .net)

BITMAP* p = create_bitmap( 0, 0 );
if ( p )
  destroy_bitmap( p );

HoHo

In graphics.c I found two lines of code:

bitmap = malloc(sizeof(BITMAP) + (sizeof(char *) * height));
bitmap->dat = malloc(width * height * BYTES_PER_PIXEL(color_depth));

If you really don't allocate space for the ->dat then first malloc doesn't even allocate space for the ->dat pointer and effectively bitmap will only get sizeof(BITMAP) bytes of memory and not (sizeof(BITMAP)-sizeof(BITMAP->dat) as it should. For the last four bytes in BITMAP, no memory will get allocated and everything that is written there will probably overwrite something useful.

That means you can't use Allegro with zero-pixel bitmaps. You should be able to create bitmaps of height==1 and width==0

[edit]

Of cource I might be wrong, I just saw the functions code for the first time in my life.

Elias

Try to compile in debug mode.

I.e., there is this at the beginning of the function:

ASSERT(width >= 0);
ASSERT(height > 0);

So when run, your program should exit and point you to the line in the source where the problem happened. Using ASSERT simply is the way Allegro currently uses for runtime errors (instead of making the function fail and set an error code or raise an exception or whatever).

In any case, a 0x0 bitmap is not allowed, which the documentation specifically states: http://alleg.sourceforge.net/onlinedocs/en/alleg009.html#create_bitmap

A J

if you accidently make a 0x0 sized bitmap, and no bitmap is created, then shouldn't create_bitmap() return NULL.
else, you would be lead to beleive you have a bitmap that needs to
get destroy_bitmap() which current crashes.

is there any reason why a create_bitmap(0,0) should return a valid pointer?

HoHo

to make it a bit faster?

I think there should be a way to create bitmaps of 0x0 sizem adding another check shouldn't be so slow that it is only done with debug versions.

allthough this would probably make all image-saving functions a bit more difficult because I'm not sure if the formats even support 0x0 images. Also I think some other functions assume that bitmaps have some image-data with them(e.g 3d rasterizing perhaps)

A J

what the hell are you even considering 0x0 bitmaps as worth creating ?

HoHo
Quote:

what the hell are you even considering 0x0 bitmaps as worth creating ?

Why the hell would anyone even try creating 0x0 bitmaps ;)

Now that I thought of it a bit more I came to conclusion that it would probably be a lot easier to add the check to user side code, not in allegro.

A J

the point of libs is to do as much work for you as possible.
lib should do it. there is no reason it shouldn't.

Michael Jensen

really, would adding if ((w<1) || (h<1)){return(NULL);} to the beginning of create_bitmap() slow it down that much?

(it seems that passing in a negative height could cause all sorts of problems!)

Kitty Cat

I think ASSERT'ing both w and h would be enough. Creating a 0-sized bitmap is considered an error, AFAIK, and no sane person would do it. So, if it does happen, it would be a bug in the code, which instead of just simply returning NULL, an ASSERT in debug mode would catch more effectively.

However, I do think both width and height need to be 1 or greater. If the width is allowed to be 0, you'll still get a 0-sized bitmap and have the same problems.

A J

Why is there so much resistance to having a library function do some sanity checking, what is the point of a library ?

Why would anyone want to create a zero sized bitmap, can anyone give me an example ?

Until BOTH these questions get answered, the only conclusion i can come too is that some of you are resisting for the sake of it.

allegro is a library, its not an application, it needs to SERVE the applications/programmers needs, not the other way around.

spellcaster

The lib will do the Sanity check. As pointed out above, they'll add an ASSERT. So, what's the problem?

HoHo
A J said:

Why would anyone want to create a zero sized bitmap, can anyone give me an example ?

You tell us. It was you who started this thread because you had a problem with that. I have never before heard anyone having the same problem.

Elias

As also pointed out above, there already is an ASSERT.

spellcaster

Yes, but width should be asserted to be > 0 as well ;)

Elias

Ah, yes, could change this. But a width of 0 at least won't crash of do anything unexpected, so that's why who added the ASSERT made them as they are.

A J

the problem is, create_bitmap(0,0) returns a valid pointer, that when passed to destroy_bitmap() crashes.
if i have a valid pointer, why does destroy_bitmap() crash ?

spellcaster

Compile your program in debug mode. Do you still have the problem?

Michael Jensen

What is ASSERT? I agree with AJ, if the library gives lets you create a bitmap, and returns a valid bitmap without errors, why should it crash on destruction?

But, maybe destroy_bitmap() (since that's where it crashes) should be fixed? Since destroy_bitmap usually isn't called until shutdown, and you can always check bmp->w/bmp->h

I wonder, can you blit to/from the 0 sized bitmap? putpixel? If all of the graphics routines crash while using it, then create_bitmap() should return NULL, but if the only problem is destroy_bitmap, then that's where it should be fixed; regardless of why you'd create one, if it lets you create something you should be able to destroy it!

spellcaster

It doesn't return a valid bitmap. That's the reason for the crash.
create_bitmap() uses a contract. Validation of this contract takes place only if you compile against the debug version of allegro.

Michael Jensen

That's part of my point. If it can't create a valid bitmap it should return NULL, not allocate the wrong amount of memory and return an unusable pointer that can't be deallocated via destroy_bitmap. Just b/c there s no apparant use doesn't mean there is absolutley no use.

Elias

It doesn't return NULL in debug mode.

A J

release build, this causes a crash.

BITMAP* p = create_bitmap(0,0);
if ( NULL != p )
  destroy_bitmap( p );

As we seem to be having some trouble communicating exactly why this is considered OK, can i get a simplified bullet point list of reasons why it is OK for a library to crash destroying objects it made?

Thomas Harte
Quote:

As we seem to be having some trouble communicating exactly why this is considered OK, can i get a simplified bullet point list of reasons why it is OK for a library to crash destroying objects it made?

Because of the general rule that as Allegro is built with speed critical applications in mind, functions do not have to check every single possible error condition and are allowed to crash if you pass wildly unexpected parameters. For example, it is allowed to crash on the following:

persp_project(1, 1, 0, &x, &y);

From that point of view, the question is not "why it is OK for a library to crash destroying objects it made?" but "why should Allegro consider (0, 0) passed to create_bitmap to be sufficiently extraordinary that it may not handle the possibility of error?"

I guess the current dev answer is that this sort of thing is better checked by the user or that it will occur to so few people as to make it prima facie sufficiently extraordinary.

Personally, I agree that a check should be incorporated that requested dimensions are both larger than 0 as Allegro only purports to be able to handle images, not data structures representing "no image".

Bob
Quote:

As we seem to be having some trouble communicating exactly why this is considered OK, can i get a simplified bullet point list of reasons why it is OK for a library to crash destroying objects it made?

Read up on Design by Contract. This is what Allegro implements. It's a perfectly valid and widely used methodology.

spunit262

The docs never said not to pass 0,0 to create_bitmap, so your point is mutt.

Carrus85

It is kindof a trade off... you can have the "contractual agreements" enforced all of the time, or only enforce them under development (to ensure proper execution). For the most part, the latter is the method that makes the most sense, because errors like this shouldn't creep up in a release-mode project (thus, you should compile your program in debug mode until all assertions are met (pre and post conditions)), then you should be safe to compile in release mode. Unless of course this is a very security critical application, than keeping the pre and post condition checking in release mode is prudent. Since most games are performance critcal, not security critical, it makes sense that these conditions are only checked during development, not continuously during runtime.

At least that is how I learned it in College CNS classes...

Thomas Harte
Quote:

The docs never said not to pass 0,0 to create_bitmap, so your point is mutt.

They also don't say not to pass a casted BITMAP pointer to play_sample. So the docs not saying something is not enough to say that "your point is mutt". At least is isn't based on my guess at what you mean by that, i.e. that mutt means void.

Richard Phipps

For speed critical functions (say the getpixel, putpixel type ones) then I agree that the checking may be a concern. But I don't know of any programmer who would do a create and destroy_bitmap call in a inner loop. Just the malloc alone must take much more time than the double check on the width and height.

For that reason, I would add the checks.

spunit262
Quote:

They also don't say not to pass a casted BITMAP pointer to play_sample. So the docs not saying something is not enough to say that "your point is mutt". At least is isn't based on my guess at what you mean by that, i.e. that mutt means void.

No one with a brain would do that, but there are zero size arrays so who's to say you can't make a zero size bitmap.
please use your brain before making counter arguments.</annoyed>

HoHo
Quote:

The docs never said not to pass 0,0 to create_bitmap, so your point is mutt.

You are reading the wrong documantation :)

As was reported earlier, docs do say that 0x0 bitmap is not allowed:
The minimum height of the BITMAP must be 1 and width can't be negative.

That means height must be 1, 2, 3, ..., a_big_number and width must be in 0, 1, 2, ..., a_big_number.

Thomas Harte
Quote:

No one with a brain would do that, but there are zero size arrays so who's to say you can't make a zero size bitmap.
please use your brain before making counter arguments.

I'll use my brain when you start saying what you mean. Your sentence was "The docs never said not to pass 0,0 to create_bitmap, so your point is mutt." which implies to me merely that points are 'mutt' if they rely on people assuming things that aren't in the docs. I suggest you try a sentence such as "This isn't obviously going to cause a problem and the docs never said not to pass 0,0 to create_bitmap, so your point is void." should you ever want to make the same point again.

Bob
Quote:

which implies to me merely that points are 'mutt'

I think he meant 'moot'

spunit262

That's to long a stentes for me. I like to be short and to the point, witch sometime makes me sound rude. Ok. If your going to be overly picky end it with </jk>, or some thing like that.

ReyBrujo

In the past, passing a NULL pointer to a %s argument in a printf call would crash the application. Nowadays I think it prints (null) instead. I would think processor speed is so good that using some checks inside the functions are not a big performance hit. However, programmers become lazy and stop checking because it is the library task.

At work we had this heated argument. In one side, everytime a friend got a NULL parameter which was not logically allowed, he would throw an error that would end crashing the application. In the other side, friends prefer handling the NULL pointer, in example, returning the function immediately, and dismissing the call. And the discussion was basically, the clients get pissed off because we make the application crash in order to get a full home made trackback and spot which check failed (isolating the problem and fixing in either the code or the client database), but if we didn't, we never learn the client's database had this problem which was carried over during months until it became so critical the database would take several weeks of intense working just to fix it.

Since Allegro is an amateur-for-amateur game programming, I believe checks are good. Allegro is usually the first or second game library a programmer would try, and it should be as newbie friendly as possible. If that includes checking for invalid parameters, so be it.

Richard Phipps

Couldn't you pass an error message to an internal string, set a flag, return without crashing and then in a seperate thread check for an error in the background and print out the full error details?

Or call a function pointer (if not NULL) from within each function if an error occured and passing it the error text. That would let people use custom error reporting functions..

ReyBrujo

Maybe in debug mode. But most will just want the application to finish with the error message, just as most people want the default X behaviour is to close the window, as much asking if you really want to.

Kitty Cat
Quote:

In the past, passing a NULL pointer to a %s argument in a printf call would crash the application. Nowadays I think it prints (null) instead.

I think it depends on the optimization settings and the compiler. I rememberfor one game I was making, DJGPP would print "(null)", but under Linux, with a newer GCC, it would crash.

Thomas Harte
Quote:

I think he meant 'moot'

In which case I had entirely the wrong idea about what he was saying and all misunderstandings are entirely my fault. Apologies all round! Obviously there is a gulf of difference between a void point (one which is clearly invalid) and a moot one (one which is sufficiently borderline to be arguable either way).

Quote:

Since Allegro is an amateur-for-amateur game programming, I believe checks are good. Allegro is usually the first or second game library a programmer would try, and it should be as newbie friendly as possible. If that includes checking for invalid parameters, so be it.

I agree with this, especially since there can't be a reasonable developer on the planet trying to get decent performance out of any loop which involves a malloc?

A J

given create_bitmap() is not speed critical, is there any harm in adding the check.
i also like the point ReyBrujo made, that its often a noobs 1st choice of game lib, so lets make it as helpful as possible.

can we set a errno number for this condition, so the user after receiving a NULL ptr can check it to find out why.

Kitty Cat
Quote:

given create_bitmap() is not speed critical, is there any harm in adding the check

Personally, I wouldn't mind a check for both to be > 0, but I really would like those assert's to remain. So, like:

// ASSERT in debug, so a debugger will catch the problem
ASSERT(w > 0);
ASSERT(h > 0);

// Otherwise, make sure we'll be allocating something
if(w < 1 || h < 1)
    return NULL;
...

Evert

Just a quick round of answers before I roll into bed. These are my personal views after skimming through the thread with a throbbing headache and a sore throat so I apologise beforehand if I insult anyone.

The docs clearly say that you cannot create a 0x0 size bitmap. If you try to do so anyway, the consequences are for you. Perhaps it can be argued that destroy_bitmap() should not crash or create_bitmap() should return NULL instead, but since the user code is in error in this case I personally don't think this is proper behavior for the library. Changing this in the way suggested will make code run normally (without errors) in release mode and it will cause a crash in debug mode. This is not acceptable.

Allegro's policy is to do pedantic sanity checks only in debug mode and at least for 4.2, this is not going to change. How failures and problems should be dealt with after 4.2 is a different discussion.

No, passing NULL to a %s format specifier in printf() is not ok. If the result is the insertion of the string (null) this is non-standard behavior (granted, it's non-standard behavior used by many compilers, but it's still non-standard). We've had a few patches to Allegro that solved crashes because the library assumed it did.

Kitty Cat
Quote:

Perhaps it can be argued that destroy_bitmap() should not crash or create_bitmap() should return NULL instead, but since the user code is in error in this case I personally don't think this is proper behavior for the library.

I'm not totally sure if returning an invalid bitmap should be allowed either, though. IMO, it should either return a bitmap with a NULL dat field, or a NULL struct. Neither should cause problems when passing the result to destroy_bitmap, so this would always work:
destroy_bitmap(create_bitmap(w, h));

A J
Quote:

will make code run normally (without errors) in release mode and it will cause a crash in debug mode. This is not acceptable.

kittycats suggestion, of leaving the asserts in but including the check after the asserts, would allow debug & release modes to act consistantly, whilst offering warnings to users in debug mode via the asserts.

Should all non-speed critical functions check input parameters for sanity ?

[edit]
create_bitmap() docs say:
"Return value: Returns a pointer to the created bitmap, or NULL if the bitmap could not be created."

to me the "or" word implies the 1st half of the sentence is the opposite to the end, the end being NULL if not created, therefore the start of the sentence says non-NULL if created. So if a bitmap is created successfully (non-NULL ptr), then it should be able to feed that ptr to destroy_bitmap() without a hard crash.

GullRaDriel

you all should know what you code.

create_bitmap crash if size is 0x0 ? so don't use a 0x0 size.

it is evil to think that library develloper must have a thought to everything.

error checking is also the job of the user of the lib.

Excuse me, but if you're too dumb to not do yourself some check, you 'll loose a lot of time on non-initialised pointer & else when debugging.

Aren't you checking , when doing A=1/B , if B isn't zero ? i hope you do.

Kitty Cat

BTW

Quote:

Changing this in the way suggested will make code run normally (without errors) in release mode and it will cause a crash in debug mode. This is not acceptable.

Returning a NULL bitmap is considered an error (it's not wanted behavior).. means something went wrong and the bitmap couldn't be made. I can't think of a situation where a returned NULL bitmap doesn't indicate a problem. However, by returning NULL in release, it allows the program to handle it more gracefully, while in debug mode the ASSERT's would throw the problem to the debugger.

EDIT:

Quote:

create_bitmap crash if size is 0x0 ? so don't use a 0x0 size.

create_bitmap doesn't crash. It crashes when you try to call destroy_bitmap on a 0-sized bitmap. And no one's saying 0x0 bitmaps should be allowed.. we're just determining what the best method of dealing with it would be (return an error with a NULL bitmap so the program can handle the problem, or let it create an invalid bitmap that will crash the program later).

Quote:

it is evil to think that library develloper must have a thought to everything.

Which is why we're bringing it up, since someone else found it. :)

Quote:

error checking is also the job of the user of the lib.

What should be checked by the user, and what should be checked by the lib? In non-speed-critical functions, such as this, it should be safe to assume the lib will make sure it won't cause a situation to allow itself to crash later.

Quote:

Excuse me, but if you're too dumb to not do yourself some check, you 'll loose a lot of time on non-initialised pointer & else when debugging.

When your projects get larger and larger, it becomes increasingly harder to keep track of everything. Situations that couldn't happen before could happen now, but you wouldn't realize it until it's too late. For non-speed critical functions, it'd be best to let the lib check and save on having all of its users program the exact same safety checks (or not, and risk questions of why the program crashes, and time wasting hunting uncaught problems).

Quote:

Aren't you checking , when doing A=1/B , if B isn't zero ? i hope you do.

Not in situations where I can't conceive B being 0. And if it is, it'll crash, I'll debug, and catch the problem. Something like this would be easy since it crashes right where the problem is. But with a 0-sized bitmap, you may not get a crash until the program closes, even if the problem is when the program begins.

Elias

I agree, we could change this for create_bitmap, especially since it would be consistent to load_bitmap to return NULL in case it fails.

But in general, what is a speed critical function?

If I do:

bmp = create_bitmap(w, h);
line(bmp, 0, 0, 10, 10, 0);

Then it will crash now in line 2. line certainly isn't a speed critical function (adding a check for NULL takes no time at all compared to all the checks it already has to do). So, this really opens a can of worms..

I'd say, we should leave the ASSERTs in 4.2.0, they are a perfectly good way of error checking.

After 4.2.0, we could implement something like OpenGL, where every function returns an error code. Or maybe even better, provide a global error callback which is called for errors (in which you can e.g. raise an exception - or in C you could use longjmp to jump to a user exception handler).

GullRaDriel

it is speed critical if you do this:

while(do_draw){
line(...)
}

an error check will slow the draw loop.

Elias

Not really, just because you use it in a loop doesn't make it speed critical, else create_bitmap would be as well:

while(do_draw){
bmp = create_bitmap(...);
line(bmp, ...);
blit(..., bmp, ...);
destroy_bitmap(bmp);
}

ReyBrujo

create_bitmap is so slow that adding a check there is depreciable.

GullRaDriel

Elias: you're re-creating your bitmap each loop ?

Using line in a loop is , i think, normal.
Using create_bitmap inside a loop is, i think , EVIL (the way you post)

EDIT: i mean in a drawing loop

Elias

I can do 2937779 create/destroy_bitmap pairs with a 1x1 bitmap in a second, and can do only 44555 lines from 0/0 to 1000/1000 in a second. So now, which one is speed critical? :)

Of course, both times, the additional check would not change it noticeable at all I think.

So, just want to demonstrate that it's hard to decide what is speed critical. OpenGL has error checking for every single function, so we could have it too after 4.2.0. For 4.2.0, ASSERT is simply the way that was chosen for error handling, and since it works perfectly well, no need to change it.

[Edit:] Temporary bitmaps in the draw loop are needed for example if you want a stretch_trans_blit, or stretched_printf, or things like that.

Peter Wang

So the conclusion should be that assertions should never be disabled unless absolutely critical for speed. Then create_bitmap(0,0) would always abort regardless of whether you use the release or debug version of Allegro. Great!

GullRaDriel

:p

Thomas Harte

[

Quote:

Temporary bitmaps in the draw loop are needed for example if you want a stretch_trans_blit, or stretched_printf, or things like that.

Temporary bitmaps aren't necessary, they're used there because it makes implementation easier and/or are the fastest method of doing what is required. It's a circular argument. If bitmap creation were substantially slower then (hopefully) it wouldn't be done in things like stretch_trans_blit and stretched_printf (really - 'stretched'? Is someone actually on a mission to create the least orthogonal API?).

Besides which, anything that relies on malloc relies on OS services, so speed is highly dependent on OS. Perhaps this is (another) reason why us OS X'ers see about 1/4 the frame rate of our PC cousins?

Elias

I wasn't talking about Allegro functions there, it was just an example why you might need create_bitmap in a draw loop (to support my argument that this can't be a criterion what needs error checking and what not).

A J

so are we in agreement create_bitmap() is not speed critical ?

asserts and a check is the best option ?

Kitty Cat
Quote:

I wasn't talking about Allegro functions there, it was just an example why you might need create_bitmap in a draw loop (to support my argument that this can't be a criterion what needs error checking and what not).

You'd only create the bitmap when you need to, and recycle it for later. In the loop, you'd check that the bitmap exists, and that it's big enough. If not, you make a big enough bitmap, and leave it around for the next iteration. Yeah, this may cause the first iteration to lag, but it's a one-time deal.. the bitmap will stay around so you can use it as-is on subsequent run-throughs (no additional mallocs necessary). I actually did this when I was working with Allegro to do translucent (normal and additive) software polygons.

Quote:

I can do 2937779 create/destroy_bitmap pairs with a 1x1 bitmap in a second, and can do only 44555 lines from 0/0 to 1000/1000 in a second. So now, which one is speed critical?

line is still speed critical, even if it ends up being slower. The idea is that you can use line multiple times in quick succession per frame render. The faster the better. Creating a bitmap, however, doesn't typcially get used outside of initialization (or in the manner I outlined above) where speed isn't as much of an issue.. that a few extra if checks aren't going to cause speed problems for a program.

Carrus85

Yeah, I myself was wondering why you wouldn't just create a "working" bitmap outside of the loop, and just recycle it... (and destroy/create it again if the size changes for some reason). :)

Michael Jensen

While I think Elias's line argument is kind of bogus, I really don't think checking for NULL in drawing functions would lead to much of a slower program, in all of my draw loops, functions that take a bitmap as an argument or call create_bitmap etc I always check for null, not that I expect any of them to fail (except incase of a logic error, or file not found error) If I remove the checks my program speed difference is competley unnoticable, plus it's safer to leave them in.

Quote:

No one with a brain would do that, but there are zero size arrays so who's to say you can't make a zero size bitmap.
please use your brain before making counter arguments.</annoyed>

if a zero sized bitmap is allowed then it's destroyal should also be allowed. And saying "no one with a brain would do that" is a completely worthless argument, that having been said, no one with a brain would have posted what you have! :P

Quote:

Changing this in the way suggested will make code run normally (without errors) in release mode and it will cause a crash in debug mode. This is not acceptable.

No it wouldn't.

Quote:

so this would always work:
destroy_bitmap(create_bitmap(w, h));

not if create bitmap returns null -- or did I miss something? are null checks being added to everything? Actually, adding a null check to destroy bitmap isn't a bad idea, I don't think I've evern seen anyone sanley calling destroy_bitmap(bmp) without first checking if bmp is null, and if create_bitmap is not speed critical, why should destroy_bitmap be? it's usually only called when shutting down!

Quote:

Aren't you checking , when doing A=1/B , if B isn't zero ? i hope you do.

I usually don't perform much division unless I'm using doubles... (it's Ok to divide a double by 0, and actually useful.)

Quote:

After 4.2.0, we could implement something like OpenGL, where every function returns an error code. Or maybe even better, provide a global error callback which is called for errors (in which you can e.g. raise an exception - or in C you could use longjmp to jump to a user exception handler).

User-Defined error handlers would be SWEET!

As far as create bitmap goes, to me it makes more sense to let the lib do the checking because it's one if statement, and the create_bitmap could return faster in rare instances where it can pre-determine that it doesn't even have to call malloc. If the user was required to do this, a code bloat could occur, it makes sense to put the check at the base-level so that there's absolutley no chance of it accidently not getting checked for.

Edit:

Quote:

Besides which, anything that relies on malloc relies on OS services, so speed is highly dependent on OS. Perhaps this is (another) reason why us OS X'ers see about 1/4 the frame rate of our PC cousins?

If it is, it would be neat if allegro could allocate a chunk of memory at the beginning of the program and preformed it's own memory managment/allocation on that chunk to bypass the OS's speed issue...

Kitty Cat
Quote:

not if create bitmap returns null -- or did I miss something?

AFAIK, destroy_bitmap does nothing with a NULL pointer. I simply returns. IMO, anything that create_* returns, destroy_* should accept without problems.

Evert
Quote:

if a zero sized bitmap is allowed then it's destroyal should also be allowed.

Yes. Thing is, they're not allowed.

Quote:

I don't think I've evern seen anyone sanley calling destroy_bitmap(bmp) without first checking if bmp is null

In that case, you've never seen my code. create/destroy behaves like alloc/free: it does nothing with a NULL pointer.

Quote:

If the user was required to do this, a code bloat could occur, it makes sense to put the check at the base-level so that there's absolutley no chance of it accidently not getting checked for.

It is the user's responsibility to pass valid, sane, arguments to a function. The library should, in my opinion, do sanity checks in debug mode but not in release mode. Compare the speed difference between the library in debug mode and release mode. It could be minor on your machine and for your programme, but that doesn't mean it would be for everyone.

Quote:

IMO, anything that create_* returns, destroy_* should accept without problems.

Yes, no argument there.

Thomas Harte
Quote:

I really don't think checking for NULL in drawing functions would lead to much of a slower program

It shouldn't make much odds at all, it's presumably the difference between:

LD register, pointer value

And:

LD register, pointer value
BRA.Z .return_code

Especially with branch prediction very effectively preventing any sort of stall in close repeated calls it isn't much of a difference.

Quote:

If it is, it would be neat if allegro could allocate a chunk of memory at the beginning of the program and preformed it's own memory managment/allocation on that chunk to bypass the OS's speed issue...

Firstly, it's wild speculation that this particular thing is a speed issue, I was just making the point that most of the time, the way things are implemented on Allegro is dictated by quick tests or by programmer intuition. Since a lot of the non-DOS/Windows versions seem to be very slow then this possibly needs addressing.

Secondly, Allegro is already a 90% bloat library for most people that being DOS-centric in design eats 100% CPU at runtime, can it really justify taking an unnecessarily gigantic chunk of memory as well?

Quote:

It is the user's responsibility to pass valid, sane, arguments to a function. The library should, in my opinion, do sanity checks in debug mode but not in release mode.

Then you definitely want to look at functions such as get_camera_matrix. Expect the user to be able to pass orthogonal 'up' and 'front' vectors or keep things normalised at their end? Nah, lets just force two sqrts and a bunch of dot products on them.

EDIT: probably the smartest thing to do instead of a function by function disassembly is to work out all the anti-patterns in Allegro? I'm new to this, but I definitely spot abstraction inversion, accidental complexity, busy waiting, code momentum, continuous obsolescence, creeping featurism, procedural code and the aura of a stovepipe system.

But I assume 4.3 will be a clean sweep of much of this, or at least an API that allows a clean sweep?

A J

Thomas can you give 1 example in allegro, of each of those anti-patterns you mentioned ? or are you just a warm-body ;)

Thomas Harte
Quote:

Thomas can you give 1 example in allegro, of each of those anti-patterns you mentioned ? or are you just a warm-body

Can do! I'd never actually heard of this stuff before, but discovered it yesterday while searching for the Gang of Four, a historical political grouping here in the UK. Turns out another Gang of Four wrote a book that expounds most of the terms on that page. Point is, I may well be misunderstanding or misapplying any quantity of the group. Anyway...

abstraction inversion
The bundles and sets of code most of us have self written and squirreled away for screen update, automatically switching between memory buffering, page flipping, etc. A potential argument is that this isn't abstraction inversion because the user is merely collecting together the hardware resources that Allegro abstracts, but when you look at the code for things like the OS X port you discover, for example, that Allegro doesn't tap into any hardware for page flipping but emulates it in software.

accidental complexity
Wikipedia quote: "accidental complexity may be caused by misunderstanding of problems, ineffective planning...". I assert that Allegro has had what amounts to ineffective planning due to its "good idea for DOS, now let's port it!" approach. Complexity is introduced by the array of different video techniques, as described above, as well as the way that FONTs are not composed of normal bitmaps except in 16/24bit mode, the distinction between a VOICE and an AUDIOSTREAM, etc

busy waiting
Demonstrated by almost every Allegro application that attempts to be frame rate indepdendent.

code momentum
Created by things like rotate_stretch_pivot_bitmap as compared to the two other functions rotate_pivot_bitmap and stretch_bitmap. Which are all hypothetical because I don't have the Allegro manual to hand, but you get the point. Compare and contrast with OpenGL or DirectDraw, the former of which uses modal parameters and the latter uses, for the limited functions it has, states attached to objects.

continuous obsolescence
Industry trends have brought Windows to the forefront, the Mac back as a significant player for hobbyist developers and invented Linux as a useable gaming platform in the time since Allegro was first implemented. At the minute much more work seems to go into getting every piece of Allegro to work on every platform it is meant to support than in improving the API, etc. See the huge number of OS X problems being reported (admittedly mostly by me) - 8bit desktop mode not working, joystick input unreliable if you want to allow the user to redefine keys, etc

creeping featurism
Possibly the least controversial of my list. Allegro is historically very guilty of this, although it seems to have stopped lately. So I withdraw this one as a going concern and apologise for raising it.

procedural code
Which in this case means "building indivisible, monolithic solutions to problems by not factoring out commonalities" - see examples of "code momentum" above.

stovepipe system
Defined as "a legacy system which cannot be upgraded or refactored and which must be built around until it can be replaced entirely". See the abandonment of Allegro 5.0 and instead the idea of building a new API over the existing one then gradually filtering out the old stuff.

Aside: it would be very interesting to make a virtual middle layer Allegro DLL that simply flagged which functions were used then passed calls on to the real DLL and then to run a whole bunch of Allegro programs through it and find out what proportion of the API is actually in common use. I may try that one day if I ever return to the Windows fold.

Evert

This is somewhat sidetracking the original topic, and I don't think we need another discussion on what things need to be changed in Allegro. ;) We're aware of the problems, but we're only a few people working on this in our spare time. Beside which, 4.2 needs to be out of the way first.

Quote:

Complexity is introduced by the array of different video techniques, as described above, as well as the way that FONTs are not composed of normal bitmaps except in 16/24bit mode

Actually, I 8 bit fonts are normal BITMAPs as well. Only monochrome fonts are different, which is not that surprising considering that Allegro doesn't support monochrome bitmaps.

Quote:

At the minute much more work seems to go into getting every piece of Allegro to work on every platform it is meant to support than in improving the API

As I'm sure you're aware, that's because we want to get 4.2 out. API improvements are post 4.2

Quote:

See the huge number of OS X problems being reported (admittedly mostly by me)

Yes, and do keep them coming! Afterall, we want to fix those (although it's rather hard with most of us not having a Mac).

Quote:

Defined as "a legacy system which cannot be upgraded or refactored and which must be built around until it can be replaced entirely". See the abandonment of Allegro 5.0 and instead the idea of building a new API over the existing one then gradually filtering out the old stuff.

Not sure if you followed why this is, but the reason the `rewrite Allegro 5 from scratch' idea was dropped is that there is simply not enough manpower available to do it. No one has the time for a big undertaking like that.

Thomas Harte
Quote:

This is somewhat sidetracking the original topic

I disagree, the topic had already begun "the current implementation decisions affecting Allegro vs the ones we'd like to see"!

Anyway, since all of the concerns are already known or to be addressed after 4.2, things are looking up and as you point out, there isn't much utility in discussing them again.

Just one question, the answer to which I'm sure has been made clear 1,000 times but which I never seem to grasp for very long: where can the API specs for 4.3 be found? Or are they the same as 5.0 was, but without a full rewrite underneath?

Evert
Quote:

where can the API specs for 4.3 be found? Or are they the same as 5.0 was, but without a full rewrite underneath?

They're the same as for 5.0, the rationale being that a lot of work and thought was put into those and it'd be a waste not to use it, or to just start over and reinvent all that was considered before.
That said, as things are implemented, issues may pop up that make it nescessary to discuss some things again. Some parts of the 5.0 spec may be so incomplete that we need to discuss them anyway.

About the `rewrite underneath', that is actually part of the plan, but it won't be a rewrite from scratch. Peter [Wang] has worked on the event subsystem, which replaces what was previously in place. There are also new keyboard drivers. I've started on the graphics API, but it still lives on top of the old drivers. Eventually, those will have to go.
The internals will be rewritten, just not as one big chunk and it won't be done from scratch.
Right now, what has been done can really only be seen by checking the CVS commit logs, which isn't really ideal. We'll have to move to a detailed TODO/DONE list... after 4.2. :)

Elias
Quote:

abstraction inversion
accidental complexity

Yes, that's all the reason why there will be a new version.

Quote:

busy waiting

This has two reasons. One is the current polling nature - so the only way around it is to insert rest(1). The other is that many programs simply don't put in rest(1) on purpose, fearing it will cause worse performance. The events system already implemented in new_api_branch solves this.

Quote:

code momentum
Created by things like rotate_stretch_pivot_bitmap as compared to the two other functions rotate_pivot_bitmap and stretch_bitmap. Which are all hypothetical because I don't have the Allegro manual to hand, but you get the point. Compare and contrast with OpenGL or DirectDraw, the former of which uses modal parameters and the latter uses, for the limited functions it has, states attached to objects.

I don't understand.. you mean, the API has too many functions?

Quote:

continuous obsolescence

As Evert pointed out, in the time of feature freeze before the next stable release after several years it really is wise to do nothing except fixing the existing things.

Quote:

creeping featurism

The problem is, many included things make it easier to use. But there haven't been any much new features added in a long time. Is there any new feature in 4.2.0 we didn't have in 4.0.0? I fully would support a move towards making Allegro more modular though.. even if we then have to package up all the modules again as a single package for anyone to be happy with it.

Quote:

procedural code

I don't see this. There's the long standing bitmap/sprite API problem, and some historic things like the 3D functions, to which this may apply - but the core drivers follow a very clean architecture.

Quote:

stovepipe system
Defined as "a legacy system which cannot be upgraded or refactored and which must be built around until it can be replaced entirely". See the

Well, what is happening is, core parts are upgrade in new_api_branch, and the other code is constantly refactored.. so per your definition this doesn't apply to Allegro.

Quote:

Just one question, the answer to which I'm sure has been made clear 1,000 times but which I never seem to grasp for very long: where can the API specs for 4.3 be found? Or are they the same as 5.0 was, but without a full rewrite underneath?

I'd like there to be a list of things which need to be done, and who is working on what.. my attemps can be seen in the wiki: http://awiki.strangesoft.net/bin/view/Main/AllegroDev

There's not much there yet, but I guess that can be contributed to everyone being busy with 4.2.0 :) I will make sure to document what I want to work and am currently working on there once 4.2.0 is out. Or maybe 4.2.0 is just an excuse for my laziness, we'll see..

Michael Jensen
Quote:

Firstly, it's wild speculation that this particular thing is a speed issue,

I got that part.

Quote:

Secondly, Allegro is already a 90% bloat library for most people that being DOS-centric in design eats 100% CPU at runtime, can it really justify taking an unnecessarily gigantic chunk of memory as well?

Why not? If people ask we could use Pacman as a scapegoat.

Quote:

Yes. Thing is, they're not allowed.

I get that part too, but the person I was responding too didn't; he said something along the lines of "If it lets you do it..." and I said something along the lines of "well if it lets us do it, then it should work"

Quote:

Yes, no argument there.

Umm if destroy_bitmap should destroy anything create_bitmap makes then create_bitmap should return NULL on passing (0,0) but you seem to be disagreeing on that item... not sure... -- And I was unaware of NULL not crashing destroy_bitmap...

A J
Quote:

it would be very interesting to make a virtual middle layer Allegro DLL ... find out what proportion of the API is actually in common use

.

my allegro C++ wrapper does func call counting, i get a nice chart at teh bottom of the log file that tells me which functions get (ab)used.

Quote:

don't put in rest(1) on purpose, fearing it will cause worse performance. The events system already implemented in new_api_branch solves this

"solves" or forces ?

Evert
Quote:

it would be very interesting to make a virtual middle layer Allegro DLL ... find out what proportion of the API is actually in common use

I'd just use a profiler. That's (part of) what it's for, afterall.

Elias
Quote:

"solves" or forces ?

Solves. You can always busy loop and poll for events with an event based API. It just would be stupid.

Quote:

I'd just use a profiler. That's (part of) what it's for, afterall.

Do we provide a profiling version for MSVC? It indeed sounds like AJ simply wrote his own profiler..

Evert
Quote:

Do we provide a profiling version for MSVC?

Not sure. I don't think so, but it shouldn't matter for call counts.
If we don't, then how do people do speed comparisons between MinGW and MSVC? Allegro's test programme?

By the way, Elias, is reply-by-email working for you? It stopped working for me a while back, but it might be a problem on my end (didn't have time to look into it yet).

Elias
2005-08-20 at 22:31 +0000, Evert said:

Quote:
Do we provide a profiling version for MSVC?
Not sure. I don't think so, but it shouldn't matter for call counts.
If we don't, then how do people do speed comparisons between MinGW and
MSVC? Allegro's test programme?

Yes, I've never seen profiler comparisons. It wouldn't be the best thing anyway, since each profiler produces overhead of its own. Some things really take much longer with the profile time, in my experience.

AJ: Do you know if MSVC supports profiling?

And related, I actually have no idea how well the debug version works in MSVC. Can you actually step through Allegro and view variable values and so on, like you can with gdb?

Quote:

By the way, Elias, is reply-by-email working for you? It stopped
working for me a while back, but it might be a problem on my end
(didn't have time to look into it yet).

Not sure, I mostly click the "Go to post" link - unless I mistake it as an [AD] post and then just reply. This is an email reply, in case it arrives.

A J

for a long time i have wondered about the profiling build for MSVC, but never looked into it. i figured that what would i do with such information if i had it, so i never bothered.

my own C++ allegro wrapper that does func call counting, was not its intended purpose, the function call counting was just a side effect of a method i was using to send real time function call entry times to a seperate analysis app i wrote to measure timing.

i have never bothered to look at allegro internal variables in the MSVC debugger, i presume you can.

HoHo
Quote:

Yes, I've never seen profiler comparisons. It wouldn't be the best thing anyway, since each profiler produces overhead of its own. Some things really take much longer with the profile time, in my experience.

I think I've heard from somewhere that MSVC licence forbids you to compare the compilers quality/speed with other compilers, at least in public. Thats the reason why there are so little comparisons of MSVC vs anything.

Evert
Quote:

I think I've heard from somewhere that MSVC licence forbids you to compare the compilers quality/speed with other compilers, at least in public.

What?!??!!?
That's just too much. I'll belief it when I see it.

gnolam
Evert said:

Quote:

I think I've heard from somewhere that MSVC licence forbids you to compare the compilers quality/speed with other compilers, at least in public.

What?!??!!?
That's just too much. I'll belief it when I see it.

http://msdn.microsoft.com/vstudio/productinfo/faq/default.aspx

"Q. Can I benchmark the Microsoft .NET Framework?", near the bottom of the page.

A J

what of the create_bitmap(0,0) problem, anything going to be done about it?

BITMAP* p = create_bitmap( 0,0 );

debug build msvc7 doesn't stop, doesn't do its assert thing, doesnt make an error, run it in debug mode, it just returns a !0 ptr.

:-/

Michael Jensen
Quote:

I think I've heard from somewhere that MSVC licence forbids you to compare the compilers quality/speed with other compilers, at least in public.

Quote:

http://msdn.microsoft.com/vstudio/productinfo/faq/default.aspx

"Q. Can I benchmark the Microsoft .NET Framework?", near the bottom of the page.

.NET Framework != MSVC Compiler

.NET Frame is more of a "library" + Set of standards for the JIT Compiler; .NET apps are compiled into IL code (byte code basically) and when the app is run, the JIT Compiler compiles/caches/runs each part of the bytecode as it's needed, the only effeciency gained by this method is that the code can be optimized for every platform it's run on, the drawbacks are obvious.

A J

Micheal Jensen, your off topic.

ReyBrujo
A J said:

Micheal Jensen, your off topic.

Yes, it is Michael's off topic post :P

Sorry, couldn't resist.

Evert

Actually, I think this thread has served its purpose too...

Thread #519665. Printed from Allegro.cc