Hello, my program keeps crashing all the time. The VC10++ debugger won't help me either. In my main I create a Player class and a Game class to handle logic.
And when I get into the game loop it sometimes crashes instantly, but often not. I send the address of the player class and an array of Platform classes to the game.update to do collision detections and other stuff.
The crash happens inside game.update at the player->move(&events);
main.cpp
game.h
game.cpp
Player.cpp
player.h
the here's the Pistol.h
Pistol.cpp
And the bullet class is quite small, so I doubt that's the problem but here it is anyways.
bullet.h
bullet.cpp
The VC10++ debugger won't help me either.
Then you need to learn to use it better. (It does sound like it helped you somewhat.)
Bullet ** bullets;
Bad idea. I don't see you checking for your magical number 12 either. That could very well crash. I.e. if you fire more than 12 bullets.
if(!bullets[bulletsShot]->isActive)
{
bullets[bulletsShot] = new Bullet(bulletVelocityX, bulletVelocityY, bulletType);
Memory leak. Remove the "new"... Or better yet: rethink how exactly you want to store your bullets first.
Oh the number 12 comes from magazineSize variable, forgot to edit that. I want all of the bullets to be separate objects, also when a bullet hit something it would get destroyed I just haven't gotten to that part yet.
But why is an array of objects a bad idea? The array size would be different depending on how big the magazine was, I had a for(int i = 0; i < magazineSise;i++) which would check if bullet[i] was already active, if it was then it would continue until it found one not active and create one there.
The reason I want the bullets to be objects is that it would make it easier to mess with them when they're in the air, also I will be adding different types of damage and other fun stuff.
The debugger did help me a bit, and I did learnt to use it a bit better the hours I've been trying to fix the crash. Thank you for your help, but it would be really helpful if someone would elaborate why it is a bad idea to use object array, was it because of the memory leak and if I fired more bullets than there was size for in the array?
I've never tried to have a class create an instance of some other class in the constructor, is that different in any way? I've used object arrays before and they've never been a problem before.
I would recommend using a std::vector to store your bullets instead of a multidimensional array. This won't put any restrictions on how many bullets can be fired but you can easily change this so you can't fire more bullets than what the magazine size allows.
There is too little code to exactly say what might be wrong. What does the backtrace of the debugger tell you?
void Pistol::fire() { if(!bullets[bulletsShot]->isActive) { bullets[bulletsShot] = new Bullet(bulletVelocityX, bulletVelocityY, bulletType); calcDamage(bullets[bulletsShot]->mass); std::cout << "Bam!" << std::endl; } bulletsShot++; };
This code could easily lead to out-of-bounds array access and cause memory corruption, which would result in the random crashes you've described. Add a check to make sure 'bulletsShot' is in the valid range.
There's nothing technically wrong with using arrays of objects or object pointers, but arrays in general can lead to bugs that are notoriously hard to track down because they let you silently corrupt memory if you're not careful. It may lead to random crashes at random parts of the program that have nothing to do with the source of the bug. As rule of thumb, use vectors unless you have a good reason not to. They offer additional safety and features. Note that vectors have a '[]' operator like arrays, but it doesn't check for out-of-bounds access and is as dangerous as plain array access. Use the 'at' method.
In any case, I think your design isn't optimal. When a bullet leaves the gun, it belongs to the world. The gun shouldn't be responsible for updating the bullet positions or for checking if they collide with anything. It's the job of the world object.
I have updated the Pistol.cpp to have the loop which will make sure that no out of bound business happens, and I've added much more code to the first post.
In the game.cpp the debugger gives me:
Unhandeled exception at 0x779e15de in Allegro.exe: 0xC0000005: Access violation reading location 0xbaadf025.
and then shows me that the next statement is p->update(), so that means it's something in p->move(&events) that is causing it to crash, which is where the gun->fire() is located and it crashes instantly when I press space which makes it execute gun->fire(). This is why I think the fault is located within the gun class, or the how I'm creating instances of classes inside constructors.
Also I will look into vectors.
Also about my design, the gun wont be responsible it will just create a bullet object and give it it's values after that it's game class which will be messing with the bullets trajectory and checking for collision, almost all the collision code is in the collision.h and .cpp but I do not think they are relevant right now, I will also add a separate class for controls so I can clean up the player class.
Stacktrace:
Oh, actually, I see the problem now.
When you create the bullet pointer array, it's filled with garbage. Then you treat that garbage like valid pointers and try to read the 'isActive' field, so you get a segfault.
What you're doing doesn't really make much sense.
Why does the ability to shoot new bullets depend on whether the old ones are alive?
Why are the bullet pointers not stored in a list in the game class with the rest of your objects? The gun has nothing to do with the bullets after they've been created.
If you really must insist to store the bullets in the gun object, why do you need an array of pointers? Just make an array of bullets and mark them all as dead initially. This will prevent your crash.
When you create the bullet pointer array, it's filled with garbage. Then you treat that garbage like valid pointers and try to read the 'isActive' field, so you get a segfault.
I was actually thinking about that, and I knew that the isActive would be filled with crap, but I thought at worst it would give me some false positives. I will immediately start to rewrite the Pistol class.
What you're doing doesn't really make much sense.
Why does the ability to shoot new bullets depend on whether the old ones are alive?
Why are the bullet pointers not stored in a list in the game class with the rest of your objects? The gun has nothing to do with the bullets after they've been created.
I wanted to check the the array so I wouldn't overwrite any of them, and I haven't gotten to the game class / collision detection part yet, right now I just want to make sure that it fires once that is done I'll remove the memory leak and add collision detection.
If you really must insist to store the bullets in the gun object, why do you need an array of pointers? Just make an array of bullets and mark them all as dead initially. This will prevent your crash.
Inexperience, I really don't know any other way to create new objects. If I did bullet = new bullet when he fired then all of the instances of it would be called bullet, and I'm not sure if the second time he fired it would crash, overwrite the first one or just not compile at all. Also, I think I got the idea from Unity when I was checking it out and playing one of the demos, when the player shot it would cycle through a list and when the bullets hit something they were marked as dead on the list.
I changed the pistol.fire() to and removed the bullet array.
void Pistol::fire() { Bullet bullet(bulletVelocityX, bulletVelocityY, bulletType); bulletsShot++; //this will be used for reloading if I decide to do that. calcDamage(bullet.mass); std::cout << "Bam!" << std::endl; };
It did fix the crashes, I'm not sure what it does. Is it creating a lot of bullet objects who are all called bullet? or is it creating one, then replacing it when the next one is created. Also, isn't this a memory leak aswell? And I will try to figure out a way to create and destroy the bullets in the Game class, it would make it much easier to deal with them.
Also, I wanted to have an abstract weapon class, where all the different weapons inherited from the weapons class, then I could have a weapon pointer point to the different weapon objects when the player changed weapons. Is there any problem with this?
I was actually thinking about that, and I knew that the isActive would be filled with crap, but I thought at worst it would give me some false positives. I will immediately start to rewrite the Pistol class.
It depends on whether the garbage pointer happens to point at a location that belongs to your program (in which case you'll read a random number) or another program (that's a segmentation fault and will make you crash).
Now, I don't really understand what you're talking about in the rest of your post, but what I'm trying to say is that a bullet is a game object like any other and should be treated as such. The gun just creates one, adds it to the big list of game objects and forgets about it. If you don't have a Game class yet that holds such a list, you should create one first because everything else depends on it.
I suck at explaining, I know
What I need to know is:
When it fires, then it creates a Bullet object, called bullet right?
But when it is fired the second time, what happens to the first Bullet object? They both have the same name, is it destroyed or are there two Bullet objects who are both called bullet.
When it fires, then it creates a Bullet object, called bullet right?
But when it is fired the second time, what happens to the first Bullet object? They both have the same name, is it destroyed or are there two Bullet objects who are both called bullet.
If you create an object inside a function without the 'new' keyword, it lives on the stack and dies as soon as you leave the function.
If you create an object inside a function with the 'new' keyword, you hold its address in a temporary pointer that dies as soon as you leave the function. The object itself lives but you can't do anything with it (not even destroy it). This is called a memory leak.
Currently, you keep pointers to the bullet objects you create inside an array that is a member of the Pistol class. This is bad because:
1. The pistol object doesn't need to refer to the bullets, so it shouldn't store pointers to them.
2. The game manager object that needs to refer to the bullets should be able to do that directly, not through the pistol object.
3. You should be able to create and destroy bullets freely, so the proper data type is a list.
In short, you should not store bullet pointers inside an array that belongs to the pistol object. You should store bullet pointers inside a general list of game entities that belongs to some kind of game entity manager.
A quick and dirty fix for your problem would be to create an array of objects, not object pointers.
However, I would strongly advise against actually doing that. It will stop your game from crashing, but it's just a temporary quick fix. Your real problem is a design problem.
Ofcourse! How could I have forgotten that everything made inside a function is local(unless theres some way to make it global), thanks! You reminded me of something very important, I will get to work at once. Thank you very much for all the help!