Hello,
two faults:
1. draw_sprite failure to find correct place
2. system_bitmap with subbitmaps crashes when you destroy the subbitmaps

From testing I've found that when you draw_sprite a subbitmap of a video or system bitmap onto a different system or video bitmap, it doubles the x/y and you end up with the wrong portion showing (this is following from my thread trying to explain things).Also, I've found through testing that if you have a subbitmap of a system bitmap when you try to destroy the subbitmaps you get a crash. Video and memory bitmaps are ok.

Attached is a sample piece of code and some windows exe's to show how subbitmaps onto video or system bitmaps don't work, and how system bitmaps that have subbitmaps crash the system.

As you can see from the images, 'original.jpg' is how it works fine with memory bitmaps, but when you use video/system bitmaps you get it wrong, as in 'doublingerror.jpg'. sheet.bmp is the source bitmap.

If you look closely you can see what is happening is it is doubling the x and y position when you blit a video/memory subbitmap onto a system/video bitmap.

1. if you compile as is, it will work.
2. if you uncomment the line: //#define nonmemorybitmap you will get the error
3. if you change the line 41 to system bitmap you get the crash.

masked blit is fine. The problem, I believe, is the bit of code in the windows acceleration source file for draw_sprite that tries to determine the correct place.

Should crash is really crashing:

Should fail isn't:

C:\Documents and Settings\Gull\Bureau\failure>gdb shouldfail.exe
GNU gdb 5.2.1
Copyright 2002 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "i686-pc-mingw32"...(no debugging symbols found)...
(gdb) run
Starting program: C:\Documents and Settings\Gull\Bureau\failure/shouldfail.exe

Program exited normally.

Should work is working:

C:\Documents and Settings\Gull\Bureau\failure>gdb shouldwork.exe
GNU gdb 5.2.1
Copyright 2002 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "i686-pc-mingw32"...(no debugging symbols found)...
(gdb) run
Starting program: C:\Documents and Settings\Gull\Bureau\failure/shouldwork.exe

Program exited normally.

ALSO:
I first try with dev-cpp, -Wall -g with no optimisation:
The code given works fine when compiled on my computer.
No matter if i comment or not the #define nonmemorybitmap

Conclusion: it should be a problem with your compiler,compiling flags,level of optimisation.
It's perhaps a MSVC issue.

I'm sorry that i cannot help you more, it's working fine with me. attached are the 2 compiled from my pc.

I let you test and post to see if it is different.

LAST EDIT BEFORE LEAVING WORK:

I test with all optimisation, no optimisation, ... all i compile with dev-cpp & gcc 3.4.2 & allegro 4.0.2 is working fine. MSVC issue.

$ g++ main.cpp `allegro-config --libs --static`
main.cpp:9: error: `main' must return `int'
main.cpp: In function `int main(...)':
main.cpp:84: error: return-statement with no value, in function declared with a 
   non-void return type

Fixed those.

I'm on Linux, and everything worked fine.

[edit]I've now had two other people that I know compile my code (not msvc) as I said and the same bug happens.[/edit]

GullraDriel: Firstly, the crash shouldn't be happing so that is a bug, secondly I am compiling with DevC++ and MSVC and both versions show the odd images (when I say fail I mean show the wrong images). I've had others test my devcpp exe and it fails again for them. I've also tried your 'workingbutshouldcrash' and while it doesn't crash (are you sure you enabled the system bitmap at line 41?) it does show the incorrect images - all image pairs should be the same. Did you not see this?

CGamesPlay, linux, the code I think is wrong is in the windows area so no surprises there.

Have you seen my jpegs? Is there any explanation?

I've found the genuine and demonstrable Allegro bug! As Neil Walker observes (elsewhere), ddraw_draw_sprite looks like this:

The problem relates to [x/y]_ofs, which seem to be used to hold the top left corner of the area of a bitmap that is actually in use. With ordinary bitmaps they are both set to 0. With sub bitmaps they are set somewhere else in order to only display a window within the source bitmap. The Allegro 2.x days of just fiddling with ->line pointers are obviously long behind.

The problem is that both ddraw_draw_sprite and ddraw_masked_blit apply the sub bitmap constraints. I believe (but cannot test, as still in OS X) that the following should fix the problem:

Note the only change, near the top:

      sx = 0;
      sy = 0;

Which relate to lines 155 and 156 of src/win/wddaccel.c of the Allegro source.

Please could someone test this theory?

Someone should find the link to Matthew's patch generator..

Found it: http://www.allegro.cc/dev/make-diff.php. The diff, as shown below, is attached. Should this patch make the grade, please acknowledge both Neil Walker & myself in the authors file as I only got to ddraw_draw_sprite following a message by him on the Retrospec mailing list.

Well done Thomas. What you're saying is x and y offsets are being called both by draw_sprite and masked_blit, thus my 'doubling' effect. This makes sense as only draw_sprite was not working.

So two more questions:

1. given draw_sprite eventually calls masked_blit for system/video bitmaps at the end of the function why do we need any of the clipping checks in draw_sprite? why not just call masked_blit with the know parameters?

2. there is still the demonstrative code of system bitmaps crashing when you delete any sub-bitmap of it, which is the second part of my tests.

It's only two lines :-) As soon as someone confirms it works I'll commit it. I'm sure it's correct anyway. Thanks to all involved.

If no one else volunteers, I'll recompile and see what happens. I have a poorly baby lying beside me, so who knows when I'll get stopped

However, what about my comment regarding the need for all the code in draw_sprite?

Sorry, yes, I've found the cause of that but am otherwise lost as I haven't traced create_sub_bitmap yet.

destroy_bitmap is at line 1418 of src/graphics.c. It has a special branch for anything that passes is_video_bitmap, which includes sub bitmaps. Specifically, starting on line 1423 of that same file:

On DirectX, gfx_driver->destroy_video_bitmap directly deletes the bitmap no questions asked. So the memory for the parent bitmap is destroyed and both it and all sub bitmaps of it become invalid. Obviously an is_sub_bitmap needs to be checked somewhere, but I have no idea whether that is meant to be the responsibility of the platform neutral code or the driver specific.

I have to admit that I couldn't see a purpose for it. Perhaps the change to making that function sit on top of masked_blit was a late addition to the code?

Thanks thomas. Video bitmaps are fine, it is only 'system' bitmaps that cause the crash when you destroy a sub-bitmap. Video and memory work fine.

This code is at line 1466.

Hmmm. Then I've misunderstood the code a little. But line 1471 of src/graphics.c jumps to line 656 of src/win/wddbmp.c, then on to line 299 which is an IDirectDrawSurface2_Release, freeing the memory of the parent bitmap.

Thomas, I came to the same conclusion regarding destroy_bitmap(). I think that if a subbitmap is created by platform-specific code, then they should be destroyed by the platform-specific code. If created by the fallback case (platform neutral) then they should be destroyed by the platform neutral code. This possibly requires a new BMP_ID_* flag to remember how the subbitmap was created.

edit: I think the pos<0 prevents the problem with video bitmaps.

The code change works, three cheers for Thomas

Attached is an executable that used to fail, and now it works. I've included the source code, the bitmap, the .lib and the new dll if anyone wants to try or use a new .lib/dll file.

To see it failing simply rename the .DLL so that the exe will use the original dll and watch it show the wrong graphics!

btw, before the code is checked in, does someone want to see if all the clipping code is necessary? if you call masked_blit directly it doesn't do any of that.

This example code can also be used to see that when you use a SYSTEM bitmap (follow the comments) it crashes when you have subbitmaps and try to delete a sub-bitmap.

ps, to celebrate I've upped my AXL_FRAMS project from 70% to 90%

Is it not enough to know the type of bitmap and if the vtable includes functions for creating and destroying them? It seems to me that if it does, they are used and if it does not, the platform-neutral code is used. Am I missing something?

Hip hip hurray!
This must be a problem that has been in Allegro since... well, forever.

Doesn't really matter, since it can be done in succesive commits anyway. In fact, I think fixing the bug and removing redundant code should be done in seperate commits anyway, since you may want to roll back one but not the other if it turns out that the code was not as redundant as you thought it would be.

Hmm, yes, but a bit ugly. What do you think of this patch? (untested)
All this is a bit theoretical (not the bug Neil pointed out), since no gfx or system drivers have non-null create_sub_bitmap methods anyway.

Patch committed, cheers.

Well, when draw_sprite decides it is system/video it calls masked_blit. The same masked_blit you can call directly. masked_blit does not do any clipping tests. Therefore either draw_sprite is doing extra work it shouldn't or masked_blit is not doing the checks it should.

As for the sub-bitmap fault in windows, here is some minimal code that recreates the crash on any windows system:

For a moment I was thinking you were talking about draw_sprite() and masked_blit() in general, but it's the Windows-specific hardware accelerated implementation, right?
In general, you cannot always use masked_blit() where you can use draw_sprite().

Regarding the tests, they do look a bit redundant (or missing in the case of ddraw_masked_blit). What happens if you remove them altogether? Is clipping still performed as it should? What about maksed_blit? Does that do proper clipping?

I was referring to the ddraw code, and as far as I can see masked_blit it does no clipping checks. Similarly as far as "In general, you cannot always use masked_blit() where you can use draw_sprite()." goes, as far as I can see for video/system bitmaps (don't know about the software _original function) draw_sprite is now just a wrapper for masked_blit.

Although DirectDraw supersets the Allegro clipping functionality and the DirectDraw driver creates a clipper it appears that there is no DirectDraw version of set_clip[_rect]. So it seems to me that ddraw_masked_blit should clip "incorrectly" based on a reading of the current manual but if it were "correct" then there would be no need for the clipping in ddraw_draw_sprite.

Can anyone verify that?

Yes, as far as I can see, I agree. However, I'd like to know what the code does in practice and if ddraw_masked_blit() actually works properly now (since it doesn't seem to do the clipping) and if the tests in ddraw_draw_sprite() are redundant or not. That will have to be tested by someone. Could you test these things?

In general it certainly isn't. draw_sprite() can be used to draw 8bpp sprites onto any colourdepth target bitmap, while masked_blit() only works between equal depth bitmaps. Since video bitmaps all have the same depth as the screen anyway, there is no 8->any special case there and the two should be similar (as they indeed are now in the DX driver). I don't actually know about system bitmaps (they don't really exist in *nix), but I think they're similar to video bitmaps where their colourdepth is concerned.

Yes, that was the impression I got.

What do you want testing exactly, just use masked_blit to draw a bitmap onto another but make the co-ordinates outside the target bitmap?

Use set_clip_rect to reduce the clipping rectangle to only a small portion of the target bitmap, then see whether draw_sprite and/or masked_blit with video/system bitmaps obeys that reduced rectangle. I may get a go on a Windows PC later, so I may have a go myself...

Don't worry, I'll test it later as I can easily check all permutations of video/system/memory bitmaps and sub-bitmaps in all resolutions, colour depths and windowed modes with the power of AXL

Don't forget the firepower of Billy!

Double power eh?

I created a fullscreen bitmap (video/memory/system) and did set_clip_rect(page1,50,50,250,300);

I then did some blit, draw_sprite and masked_blit of both subbitmaps and normal bitmaps over the screen. The only thing to show was the stuff in the clipping rectangle.

All was well. Hope that helps.

And if you remove teh clipping code from ddraw_draw_sprite()?

ok, I added a clipping region to my back buffer (as above) and then modified wdaccel.c to be:

And something quite strange happened. If sprite was a video/system bitmap, i.e. it follows the ddraw_masked_blit route (but no clipping code is now there) and the destination (bmp) was a memory bitmap, it honoured the clip that we set in our code for bmp. All well with the removal of the code then...

However, if bmp is a system or video bitmap (i.e. we are drawing a system or video bitmap onto a video or system bitmap), then the clipping region is ignored for draw_sprite (masked_blit is still ok).

So it appears to me that removing the clipping code should be fine, but something odd is going on in the video bitmap code, not related to draw_sprite?

Looks ok to me. Did anyone test it?

And if you leave the clipping code in?
It stands to reason that it's there because someone figured it was nescesary, so maybe this is why.

Possibly it is there for that reason, but when I had a quick look at the masked_blit function it didn't do any clipping checks. But I guess thinking about it I might have been looking at the wrong function as there are a few different blitting functions for each one (e.g. original, ddraw_, etc).

I'll check again tomorrow, unless someone else does it for me beforehand.