Yet another segfault (a.k.a. Darkhog is a total noob)

[Darkhog]

In current version of my game, I've set up debug room (accessed via right control+left shift+d on main menu) so I can test things in safe environment. And now I'm happy I've did that. Why? Because I've got weird segfault related to collisions. Collision function is 100 ok (masked_collision,collide(TSprite,TSprite)), problem lies either in how animated sprites are collising or in some weird clashing TGameObject stuff. Can someone help me? Code is available on github: https://github.com/darkhog/SuperHeliLand

[User137]

Isn't this an infinitely recursive loop? (could be wrong, difficult to trace types without downloading whole project)

function TGameObject.isColliding(Sprite: TSprite): Boolean;
begin
Result:=false; //setting result, just in case
case ObjectMode of
omAnim: begin
if Animations.Items[CurrentAnim]<>nil then //safety check so we won't get segfault
//setting result
Result:=Animations.Items[CurrentAnim].IsColliding(Sprite);
end;
omSprite: begin
if Sprites.Items[CurrentSprite]<>nil then //safety check so we won't get segfault
//setting result
Result:=Sprites.Items[CurrentSprite].IsColliding(Sprite);
end;
end;
end;

Seems that it might not recurse. But in that code you could debug if CurrentAnim could go out of list boundaries.

[Darkhog]

No, function that is used is TGameObject.isColliding(GO:TGameObject). But thanks for suggestion, will try to check that.

[Darkhog]

Nope, checked if it is in range and still segfaults.

[wagenheimer]

Are you sure CurrentSprite is a valid index?

You should not do this, :), but try this, it will avoid anything wrong :



function TGameObject.isColliding(Sprite: TSprite): Boolean;
begin
Result:=false; //setting result, just in case

if (not Assigned(self)) or (not Assigned(Sprite)) then Exit;

case ObjectMode of
omAnim: begin
if (CurrentAnim>0) and (CurrentAnim<=Animations.Items.Count-1) then
if Assigned(Animations.Items[CurrentAnim]) then //safety check so we won't get segfault
//setting result
Result:=Animations.Items[CurrentAnim].IsColliding(Sprite);
end;
omSprite: begin
if (CurrentSprite>0) and (CurrentSprite<=Animations.Items.Count-1) then
if Sprites.Items[CurrentSprite]<>nil then //safety check so we won't get segfault
//setting result
Result:=Sprites.Items[CurrentSprite].IsColliding(Sprite);
end;
end;
end;

[Darkhog]

I'm telling you this is not the right function, you're looking at! It

is TGameObject.isColliding(GO:TGameObject).

But I've used this code and it still crashes.

[User137]

Had another quick look on the github. Are you calling right constructor for TAnimatedSprite?

constructor TGameObject.Create(mode: TObjectMode);
begin
if mode=omAnim then Animations:=TAnimatedSpriteList.create;
the .Create is TObject.Create, it seems. The .Create without parameters is not written to either TAnimatedSprite or TSprite.

[Darkhog]

*TAnimatedSpriteList. And for a while I've thought you're right, but TAnimatedSpriteList.create is defined on line 186 of Sprites.pas.

Anyway, it then would crash as soon as I'd enter TDebugState since collision is checked constantly and thus Animations is accessed constantly and it only crashes if I make both game objects touch each other.

It may be something with TAnimatedSprite class though.

[Darkhog]

Any help? I can't pinpoint the issue at all!

[SilverWarior]

Where can I get al_ogg.pas file. I cant compile your project without it.

[Darkhog]

It's in Allegro.pas distribution under (allegro.pas)\addons\OGG_Vorbis\lib

//edit: Also to access debug screen press right ctrl+left shift+d on main menu.

[phibermon]

You've got two 'isColliding' member functions as part of TGameObject but you've not overloaded them. This should give you an error in objfpc mode as far as I'm aware.

You're range checking 'CurrentAnim' but you're not range checking 'GO.CurrentAnim' which you also use. Not saying that's the issue but you should be meticulous about such things.

I'm getting the build environment setup so I can compile and debug this for you, will have you an answer asap

EDIT : You're also calling 'inherited create' from your TAnimatedSprite constructor but the parent class doesn't have it's constructor marked as virtual.

User137 is correct, you don't have 'Create' defined in the TAnimatedSprite interface but you do have it in the implementation section. As a result you're not calling that 'Create' in your Gameobject creation, instead it'll be calling whatever 'Create()' it next finds, probably TObject.

I don't mean this is an offensive manner, but I've learned some things today, I'd of said this would never compile! but there lies the executable :)

Edit : BAADF00D isn't just an amusing coincidence you know, look it up :)

Edit : You had your compiler optimization on the highest setting, this will prevent you from debugging properly.

[phibermon]

Ok found it and sort of fixed it.

Oh yeah sorry, I should tell you shouldn't I? ;)

procedure TSprite.UpdateMask;

if ActualMaskBitmap<>nil then al_destroy_bitmap(ActualMaskBitmap); //preventing memory leak

Replace with :

if ActualMaskBitmap<>nil then
begin
al_destroy_bitmap(ActualMaskBitmap);
ActualMaskBitmap := nil;
end;



-----


the actual error happens on the al_bitmap_mask_color calls in collide in masked_collision.pas :

function collide( Sprite1: TSprite; Sprite2: TSprite):boolean;

mask1 := al_bitmap_mask_color(sprite1.ActualMaskBitmap);
mask2 := al_bitmap_mask_color(sprite2.ActualMaskBitmap);


I noted on the crash that ActualMaskBitmap was an invalid reference (just junk data at the time of the call)

However this points to a much more serious problem as it would appear that you've got more than one thread executing code across your program at the same time, the error is actually appears to be happening because al_destroy_bitmap(ActualMaskBitmap); is called in UpdateMask and then before it can be created again, the collide function tries to use it with al_bitmap_mask_color.

My fix isn't a real fix and it can still crash inbetween these two calls :

al_destroy_bitmap(ActualMaskBitmap);
ActualMaskBitmap := nil;

before the pointer is set to nil. al_bitmap_mask_color must contain code that just breaks out if it gets a null pointer.

I'd reccomend that you stop the multi-thredded execution (find out how and where it's doing that, allegro is probably firing off events or something in a seperate thread, something like that)

and stop re-creating these masks all the time, you should store all the masks for all the frames as well.

if you *never* called al_destroy_bitmap() on these masks, IE don't call updatemasks, find some other way of doing it then you might find it never crashes. I should expect my 'fix' would eventually crash, it'd just be pure chance if the al_bitmap_mask_color function gets called before that reference is set to nil.

But unless you really know what you're doing, you're only going to hit even harder problems with multi-threadded execution as your code base grows in size.


EDIT: I can confirm that sooner or later (20 seconds or so of continual sprite intersection in my case but could literally be at any moment) it'll break again. I'd eat my hat if I'm wrong about the threadding. I shall post pictures too ;)


EDIT EDIT : you're using Allegro timers. They are seperate threads (at least on the windows platform, not in DOS). there's your source. You should use mutexes etc to synchronize access to shared resources such as these bitmap masks or otherwise stop using the timers.

[Darkhog]

User137 is correct, you don't have 'Create' defined in the TAnimatedSprite interface but you do have it in the implementation section. As a result you're not calling that 'Create' in your Gameobject creation, instead it'll be calling whatever 'Create()' it next finds, probably TObject.

Is he?


Had another quick look on the github. Are you calling right constructor for TAnimatedSprite?

constructor TGameObject.Create(mode: TObjectMode);
begin
if mode=omAnim then Animations:=TAnimatedSpriteList.create;
the .Create is TObject.Create, it seems. The .Create without parameters is not written to either TAnimatedSprite or TSprite.
Let's repeat code part, shall we?

constructor TGameObject.Create(mode: TObjectMode);
begin
if mode=omAnim then Animations:=TAnimatedSpriteList.create;
Yup, just as I thought. I'm not creating TAnimatedSprite, but TAnimatedSpriteList. Slight difference, you know? And guess what, it does have parameter-less constructor.

Also where did you get that BAADF00D from? It doesn't appear anywhere during compilation, debugging or in my posts...

And thanks for optimization tip, I've just turned it off.

//edit: Sorry that I might sound a bit rude, but I have today my birthday. No one gave me wishes except some automatic from websites and I didn't even get a cake. And yes, I can also exclude "surprise birthday party" as day is almost ending (23:22 or 11:22PM).

[User137]

No, i wasn't correct on my guess last time. Can you perhaps zip the source+media files, and attach to forum post? Would be easier to test with compiler...

[Darkhog]

You can clone my git repo... (https://github.com/darkhog/SuperHeliLand) It's pretty current one.

[User137]

Yeah found the download link :) Either you commented the bugs away, or fixed it, but i don't see problems in menu.
PS. phibermon edited his post with fixes.

[phibermon]

Happy birthday :D

It's your update() method in the main unit that's running in a separate thread via an Allegro timer, if you call that in your main thread loop instead then that'll fix your problem.

You are correct about the create defintion, so my apologies :) However if me and 137 made exactly the same mistake then it might suggest your code could benefit from some formatting.

I'm only teasing ;) I'm glad we found the issue, it's a great project and please keep the updates on progress coming!

[Murmandamus]

Just to echo something that was said earlier...

Whenever you use the condition:

If SomePointerReference <> Nil Then

it is much better (more portable, readable, etc) if you use this instead:

If Assigned(SomePointerReference) Then

It is an inline function/macro, so it shouldn't be less efficient.

[phibermon]

Just to echo something that was said earlier...

Whenever you use the condition:

If SomePointerReference <> Nil Then

it is much better (more portable, readable, etc) if you use this instead:

If Assigned(SomePointerReference) Then

It is an inline function/macro, so it shouldn't be less efficient.

Assigned() was created to allow coders to make clear distinctions between checking to see if a func pointer is nil and checking if the result of an assigned function is nil. so without Assigned : MyFunc=nil and MyFunc()=Nil and with assigned : Assigned(MyFunc) and MyFunc()=nil.

Probably best to use Assigned for readability, It's most likely an inline function however It's also an overloaded function so I believe there's run-time type checking going on, don't really know but probably is. If so it'd be technically faster to stick with nil comparisons. Maybe, I'm only assuming that type checking for overloaded functions is run-time for all types, I think it is for class types. Or maybe not, class instance reference being a pointer, maybe you can't have overloaded functions for different class types with the same calling conventions, meaning that overloading might be a compile time thing. Perhaps the compiler will optimize when the program only ever uses one version of the overloaded function. Maybe.

I'm probably giving this too much thought. I'm a bit bored today. In fact it'd probably just be quicker to fire up Lazarus and try overloading a function with different class types.

EDIT : yes you can overload with identical calling convention for different class types so it's Run Time type checking and nil comparisions are faster (At least in ObjFPC mode with FPC) for class references and pointers. Assuming of course that a nil comparison isn't also subject to run-time checks, which it probably isn't as it wouldn't get past the compile time checks.

I should imagine overloaded functions with different conventions are sorted out at compile time though.

[Murmandamus]

Assigned() was created to allow coders to make clear distinctions between checking to see if a func pointer is nil and checking if the result of an assigned function is nil. so without Assigned : MyFunc=nil and MyFunc()=Nil and with assigned : Assigned(MyFunc) and MyFunc()=nil.

The distinction is important when you do not explicitly use the Nil constant value. Assigned may have been originally intended for that, it became used generally for opaque pointer references, to maintain that opacity.


Probably best to use Assigned for readability, It's most likely an inline function however It's also an overloaded function so I believe there's run-time type checking going on, don't really know but probably is. If so it'd be technically faster to stick with nil comparisons. Maybe, I'm only assuming that type checking for overloaded functions is run-time for all types, I think it is for class types. Or maybe not, class instance reference being a pointer, maybe you can't have overloaded functions for different class types with the same calling conventions, meaning that overloading might be a compile time thing. Perhaps the compiler will optimize when the program only ever uses one version of the overloaded function. Maybe.

Assigned() is a built-in function handled by the compiler. I don't think it is overloaded, as there are no definitions to overload. It only works on types which ultimate derive from, or are identical to, the Pointer type. It shouldn't be any slower than a comparison to Nil, since that is what it is replaced with at compile-time, at least on the Windows platform.


I'm probably giving this too much thought. I'm a bit bored today. In fact it'd probably just be quicker to fire up Lazarus and try overloading a function with different class types.

EDIT : yes you can overload with identical calling convention for different class types so it's Run Time type checking and nil comparisions are faster (At least in ObjFPC mode with FPC) for class references and pointers. Assuming of course that a nil comparison isn't also subject to run-time checks, which it probably isn't as it wouldn't get past the compile time checks.

I should imagine overloaded functions with different conventions are sorted out at compile time though.


I have no idea what you are referring to with respect to Assigned. It is simply a built-in function which the compiler hard-codes to a Nil check. If you're not explicitly using Nil as a data value, or are dealing with opaque (pointer) types, it is more "proper" to use Assigned.

[phibermon]

Oh fair enough :) I was going off some page I googled in terms of Assigned which showed three definitions, just checked in FPC and yes it's built in.

http://stackoverflow.com/questions/4484002/assigned-vs-nil

May as well use assigned. doesn't make a blind bit of difference really. If you're dealing with method pointers to methods that return pointers/objects then using assigned will help you avoid making mistakes I suppose but if you were doing that and used pointers / nil comparisons a lot then you wouldn't make that mistake anyway. Well you'd hope :)

[Murmandamus]

Well, here's the point. For some implementations, Assigned() may not be just a simple Nil check. For example, let's say that the Foo implementation of FPC does a Nil check AND a check for "0xbaadf00d" (uninitialized data value) to determine if a pointer is valid. If you port your code to Foo, which just checks Nil with a conditional, it may cause an unintended side effect.

Practically, they are identical, but methodologically, they aren't. It goes back to the same reasons why it is bad to do pointer arithmetic, as in C-like languages... pointers need to be treated as opaque types, and depending on them being specific constant values is just generally a bad practice overall (and why Object Pascal is a much superior language than, say C++).

[Darkhog]

Yeah found the download link :) Either you commented the bugs away, or fixed it, but i don't see problems in menu.
PS. phibermon edited his post with fixes.
No, you need to access debug screen. It's right ctrl+left shift+d in main menu. Then, after you touch "other" mario with submarine, game'll crash.

[Darkhog]

Happy birthday :D

It's your update() method in the main unit that's running in a separate thread via an Allegro timer, if you call that in your main thread loop instead then that'll fix your problem.

You are correct about the create defintion, so my apologies :) However if me and 137 made exactly the same mistake then it might suggest your code could benefit from some formatting.

I'm only teasing ;) I'm glad we found the issue, it's a great project and please keep the updates on progress coming!
Well, I'd put it into main loop, if only I wouldn't need to call update EXACTLY 60 times per second, so game won't run too fast on more powerful machines than my own.

[Ñuño Martínez]

Not sure if I've understood everything you wrote. May be I've lost the thread, but there are a lot to read... :-[

Allegro.pas isn't thread-safe (4.x isn't, not sure about 5.x), and interruptions works with threads. So interruption procedures should be short and they shouldn't use a lot of variables. In most cases, just increasing/decreasing an integer variable or set a boolean flag is enough and will work. If you want something more complex you should use semaphores or something (as somebody suggested). About the speed, may be you should read this tutorial (http://allegro-pas.sourceforge.net/wiki/doku.php?id=tutorials:timers) I wrote some time ago.

About Allegro's objects (sound samples, bitmaps, etc.) Allegro.pas doesn't assign NIL to variables when destroying so you should do it by yourself. In some cases the best way is to create a wrapper CLASS and use constructor and destructor to manage them.

[Darkhog]

OK, I've caved in and now call update in main loop, with Delay(17) at end (basic Delay, from Crt unit with which you are most familiar with).

@Nuno, your solution is IMO too complex (with ticks), though I know Delay(17) isn't what I really need, as (1/60)*1000 isn't 17ms, more like 16.66666666666667, though 17 is closest integer approximation. So, at best, I'd need more precise Delay function.

//edit: I think I know how to customize tick approach to my needs.

//edit#2: YAY! IT WORKS!

Thank you guys so much! You're great help even if I sometimes don't appreciate your wisdom!

[Super Vegeta]

Delay(17) is NOT the way to go, since the game will wait the same amount after every frame. On one machine, calculations will take 3ms + 17ms delay = 20ms per frame = 50FPS, but a slower machine will take 8ms + 17ms delay = 25ms per frame = 40FPS, and the game will slow down. To have a steady FPS, what you need to do is:


Function GetMSecs():Comp;
begin Exit(TimeStampToMSecs(DateTimeToTimeStamp(Now()))) end;
This function uses standard functions from SysUtils and returns current time as 64bit int containing the amount of miliseconds since 30.12.1899.

In the game loop:


Time := getMSecs();
(* Do all the calculations, drawing, et cetera *)
While (getMSecs() - Time < DELAY_BETWEEN_FRAMES) do Delay(1)
This way, unless the machine REALLY slows down, to the point that calculating/rendering a frame takes longer than DELAY_BETWEEN_FRAMES, you're going to have a steady FPS.

Edit:
Oh, and also, SysUtils contains Sleep(), which does the same crt.Delay() does.

[Ñuño Martínez]

What Super Vegeta said.

But since you're using Allegro, you should use al_rest (http://allegro-pas.sourceforge.net/docs/4/allegro.html#al_rest) (instead of Sleep or Delay), and al_retrace_count (http://allegro-pas.sourceforge.net/docs/4/allegro.html#al_retrace_count) (instead of getMSecs, and note that it doesn't count in milliseconds). Don't forget to initialize the timer (call to al_install_timer on initialization).

[Darkhog]

It's fixed now...

I've adapted tick approach in Nuno's article and my main loop looks like that:


repeat while TicksInQueue>0 do
begin
if ((CurrentState<>nil) and (not quit)) then CurrentState.Update;
Dec(TicksInQueue);
end;
if not quit then CurrentState.BeforeDraw;
if not quit then CurrentState.Draw;
if not quit then CurrentState.Main;
until quit;
The only thing update function (timer) does now is to increase TicksInQueue.