Program crashes when Canon ball reaches "water"

[MongKong]

So i made a simple game where i protect my island from incoming enemies and at the start u control ship and when you press space you shoot a canon ball and all goes well until my canon ball reaches the point where i delete it.
I get a segmentation error which i believe means that im trying to reach memory that no longer exists, but still have no idea what is wrong...

and these are the errors i get:

error.jpg
link: https://ddgobkiprc33d.cloudfront.net...b6759bc53e.png

QGraphicsItem::x

graphiitem.jpg
link: https://ddgobkiprc33d.cloudfront.net...5b5438eff2.png

function inside Canon Class moveCanonBall()

mvoecanoin.jpg
link: https://ddgobkiprc33d.cloudfront.net...92fc602e76.png

and this

this.jpg
link: https://ddgobkiprc33d.cloudfront.net...e9062b1a01.png

This is Canon's header file:

#ifndef CANON_H
#define CANON_H
 
#include <QGraphicsItem>
#include <QObject>
#include <QGraphicsPathItem>
#include <QTimer>
 
class Canon: public QObject, public QGraphicsPixmapItem{
    Q_OBJECT
public:
    Canon(QGraphicsItem *parent=0);
    ~Canon();
    QTimer *time;
    static bool getCanonBallGone();
    static bool setCanonBallGoneToFalse();
    static bool setCanonBallGoneToTrue();
private:
    int direction;
    double xPl;
    double yPl;
    double newX;
    double moveByX;
    double moveByY;
    bool moveDown;
    double help;
    bool ballHitEnd;
    static bool CanonBallGone;
    int splashLasting;
 
public slots:
    void moveCanonBall();
};
 
#endif // CANON_H

#ifndef CANON_H
#define CANON_H

#include <QGraphicsItem>
#include <QObject>
#include <QGraphicsPathItem>
#include <QTimer>

class Canon: public QObject, public QGraphicsPixmapItem{
    Q_OBJECT
public:
    Canon(QGraphicsItem *parent=0);
    ~Canon();
    QTimer *time;
    static bool getCanonBallGone();
    static bool setCanonBallGoneToFalse();
    static bool setCanonBallGoneToTrue();
private:
    int direction;
    double xPl;
    double yPl;
    double newX;
    double moveByX;
    double moveByY;
    bool moveDown;
    double help;
    bool ballHitEnd;
    static bool CanonBallGone;
    int splashLasting;

public slots:
    void moveCanonBall();
};

#endif // CANON_H

To copy to clipboard, switch view to plain text mode 

And this i canon's .cpp file:

#include "canon.h"
#include "game.h"
#include "math.h"
#include "soundeffects.h"
#include <QGraphicsPixmapItem>
#include <QPixmap>
#include <QObject>
#include <QTimer>
#include <QDebug>
#include <QMediaPlayer>
 
#include "soundeffects.h"
 
extern Game *game;
 
bool Canon::CanonBallGone=false;
 
Canon::Canon(QGraphicsItem *parent): QGraphicsPixmapItem(parent){
 
    ballHitEnd = false;
 
    setPixmap(QPixmap(":/Images/Canon_1.png")); // 30x25
    SoundEffects::canonBallShot(1);
    game->scene->addItem(this);
 
    // bullet cords
    moveByY = 1.0;
    moveDown = false;
    splashLasting = 0;
 
    // shrani direction katero player trenutno is facing // saves the direction which the player is currently facing
    // (left==3, right ==1)
    direction = game->player->getLeftOrRightDirection();
    if(direction == 1){
        xPl = game->player->x()+83;
        yPl = game->player->y()+42;
        newX = game->player->x()+83;
    }else if(direction == 3){
        xPl = game->player->x()-18;
        yPl = game->player->y()+42;
        newX = game->player->x()-18;
    }
 
    time = new QTimer();
    connect(time,SIGNAL(timeout()),this,SLOT(moveCanonBall()));
    time->start(5);
}
 
Canon::~Canon(){
    delete time;
}
 
bool Canon::getCanonBallGone(){
    return CanonBallGone;
}
 
bool Canon::setCanonBallGoneToFalse(){
    CanonBallGone = false;
}
 
bool Canon::setCanonBallGoneToTrue(){
    CanonBallGone = true;
}
 
 
void Canon::moveCanonBall(){
 
    if(direction==1 && ballHitEnd==false){
        // s pomocjo balisticne enacbe zracunamo y  // with a help of this function we calculate y  ( parabolic movement of the canon ball )
        help = (0.5*(x()-xPl)*tan(0.6)-(0.5 *pow((x()-xPl),2))
                / (2*pow(20,2)*pow(cos(0.6),2))+yPl)-yPl;
        moveByY = yPl - help;
        setY(moveByY);
        setPos(x(),y());
 
        // povecujemo x za 1.5  // incrementing x by 1.5
        setX(x()+1.5);
 
        // ce je canon ball v vodi spremenimo sliko v splash sliko  // if canon ball hits its final destination we change picture to a splash 
        if(y() >= yPl+32){
 
            ballHitEnd = true;
            setPixmap(QPixmap(":/Images/Splash_1.png"));
        }
 
    }
 
 
    else if(direction==3 && ballHitEnd == false){
 
        help = (0.5*(newX-xPl)*tan(0.6)-(0.5 *pow((newX-xPl),2))
                / (2*pow(20,2)*pow(cos(0.6),2))+yPl)-yPl;
        moveByY = yPl - help;
        setY(moveByY);
        setPos(x(),y());
 
        // povecujemo x za 1.5 // incrementing x by 1.5
        newX = newX + 1.5;
        setX(x()-1.5);
 
        // ce je canon ball v vodi spremenimo sliko v splash sliko // if canon ball hits its final destination we change picture to a splash
        if(y() >= yPl+32){
 
            ballHitEnd = true;
            setPixmap(QPixmap(":/Images/Splash_2.png"));
        }
    }
 
 
 
    // slika ostane nekaj sekund nato se canon sporsti s pomnilnika // picture stays as a splash for couple of microseconds then we delete the canon ball from memory
    if(ballHitEnd == true){
 
        splashLasting++;
 
        if(splashLasting == 15){
            game->scene->removeItem(this);
            CanonBallGone=true;
            delete this;
       }
    }
    // ce je canon ball sou izven scene ga sporstimo iz pomnilnika // if canon ball is out of the scene we remove it  from the scene and delete it
    if(x()<(0-30) || x()>game->getW()+30){
        qDebug() << "deleted canonball";
        game->scene->removeItem(this);
        delete time;
        CanonBallGone=true;
        delete this;
    }
}

#include "canon.h"
#include "game.h"
#include "math.h"
#include "soundeffects.h"
#include <QGraphicsPixmapItem>
#include <QPixmap>
#include <QObject>
#include <QTimer>
#include <QDebug>
#include <QMediaPlayer>

#include "soundeffects.h"

extern Game *game;

bool Canon::CanonBallGone=false;

Canon::Canon(QGraphicsItem *parent): QGraphicsPixmapItem(parent){

    ballHitEnd = false;

    setPixmap(QPixmap(":/Images/Canon_1.png")); // 30x25
    SoundEffects::canonBallShot(1);
    game->scene->addItem(this);

    // bullet cords
    moveByY = 1.0;
    moveDown = false;
    splashLasting = 0;

    // shrani direction katero player trenutno is facing // saves the direction which the player is currently facing
    // (left==3, right ==1)
    direction = game->player->getLeftOrRightDirection();
    if(direction == 1){
        xPl = game->player->x()+83;
        yPl = game->player->y()+42;
        newX = game->player->x()+83;
    }else if(direction == 3){
        xPl = game->player->x()-18;
        yPl = game->player->y()+42;
        newX = game->player->x()-18;
    }

    time = new QTimer();
    connect(time,SIGNAL(timeout()),this,SLOT(moveCanonBall()));
    time->start(5);
}

Canon::~Canon(){
    delete time;
}

bool Canon::getCanonBallGone(){
    return CanonBallGone;
}

bool Canon::setCanonBallGoneToFalse(){
    CanonBallGone = false;
}

bool Canon::setCanonBallGoneToTrue(){
    CanonBallGone = true;
}


void Canon::moveCanonBall(){

    if(direction==1 && ballHitEnd==false){
        // s pomocjo balisticne enacbe zracunamo y  // with a help of this function we calculate y  ( parabolic movement of the canon ball )
        help = (0.5*(x()-xPl)*tan(0.6)-(0.5 *pow((x()-xPl),2))
                / (2*pow(20,2)*pow(cos(0.6),2))+yPl)-yPl;
        moveByY = yPl - help;
        setY(moveByY);
        setPos(x(),y());

        // povecujemo x za 1.5  // incrementing x by 1.5
        setX(x()+1.5);

        // ce je canon ball v vodi spremenimo sliko v splash sliko  // if canon ball hits its final destination we change picture to a splash 
        if(y() >= yPl+32){

            ballHitEnd = true;
            setPixmap(QPixmap(":/Images/Splash_1.png"));
        }

    }


    else if(direction==3 && ballHitEnd == false){

        help = (0.5*(newX-xPl)*tan(0.6)-(0.5 *pow((newX-xPl),2))
                / (2*pow(20,2)*pow(cos(0.6),2))+yPl)-yPl;
        moveByY = yPl - help;
        setY(moveByY);
        setPos(x(),y());

        // povecujemo x za 1.5 // incrementing x by 1.5
        newX = newX + 1.5;
        setX(x()-1.5);

        // ce je canon ball v vodi spremenimo sliko v splash sliko // if canon ball hits its final destination we change picture to a splash
        if(y() >= yPl+32){

            ballHitEnd = true;
            setPixmap(QPixmap(":/Images/Splash_2.png"));
        }
    }



    // slika ostane nekaj sekund nato se canon sporsti s pomnilnika // picture stays as a splash for couple of microseconds then we delete the canon ball from memory
    if(ballHitEnd == true){

        splashLasting++;

        if(splashLasting == 15){
            game->scene->removeItem(this);
            CanonBallGone=true;
            delete this;
       }
    }
    // ce je canon ball sou izven scene ga sporstimo iz pomnilnika // if canon ball is out of the scene we remove it  from the scene and delete it
    if(x()<(0-30) || x()>game->getW()+30){
        qDebug() << "deleted canonball";
        game->scene->removeItem(this);
        delete time;
        CanonBallGone=true;
        delete this;
    }
}

To copy to clipboard, switch view to plain text mode 

[d_stranz]

In line 126 of canon.cpp, you delete "time", and in line 128 you delete "this". The destructor for Canon also deletes "time", so you are deleting it twice so that is probably what causes the crash. It could also be caused by the cannonball exploding when it hits the water.

In any case, you should never delete an object instance while you are still executing methods for that instance. That isn't a good design in any case, but here since Canon is derived from QObject, you could replace calls to "delete this" with a call to "this->deleteLater()", which schedules the instance for safe deletion once control returns to the event loop.

I have no idea why you make "CanonBallGone" a static member of the Canon class. By making it static, there is only one class-wide instance of this Boolean, and it applies to ALL cannonballs in the scene.

[MongKong]

Oh damn...i must've forgotten about the destructor.... I had the same problem in a Bullet class aswell and this seemed to fix it aswell !!!
Thanks for the advice and about the static member, yeah i put it in there because i wanted my ship to fire a canon ball when CanonBallGone == true; that means player can only shoot one canon ball at once, he can make the next shot after the first canon ball has been deleted ( when it lands in water )
Thank you very much, everything is working great now !!!

[d_stranz]

that means player can only shoot one canon ball at once, he can make the next shot after the first canon ball has been deleted

There are better ways to do this. For example, all classes based on QObject have a QObject::destroyed() signal that is emitted in the QObject destructor. You can use this signal to detect when your cannonball is being destroyed instead of relying of a static (global) variable:

void Ship::fireCannonball()
{
    if ( canFireCannon() )  
    {
        Canon * cannonball = new Canon( this );
        connect( canonball, &QObject::destroyed, this, &Ship::onCannonballDestroyed );
        cannonballInFlight = true;
    }
}
 
void Ship::onCannonballDestroyed( QObject * )  // ignore "obj" argument since we know it is a cannonball
{
    cannonballInFlight = false;  // allows a new ball to be fired now
}
 
bool  Ship::canFireCannon()
{
   return ( cannonballInFlight == false );  // cannonballInFlight is a member variable of Ship, initialized to false
}

void Ship::fireCannonball()
{
    if ( canFireCannon() )  
    {
        Canon * cannonball = new Canon( this );
        connect( canonball, &QObject::destroyed, this, &Ship::onCannonballDestroyed );
        cannonballInFlight = true;
    }
}

void Ship::onCannonballDestroyed( QObject * )  // ignore "obj" argument since we know it is a cannonball
{
    cannonballInFlight = false;  // allows a new ball to be fired now
}

bool  Ship::canFireCannon()
{
   return ( cannonballInFlight == false );  // cannonballInFlight is a member variable of Ship, initialized to false
}

To copy to clipboard, switch view to plain text mode 

By making cannonballInFlight a member of Ship (instead of a global variable in Canon), you can have any number of ships firing cannonballs at the same time. Each Ship only has to check that it is allowed to fire another cannonball before it fires again. The last cannonball that it fired tells it when it is gone using the destroyed() signal, so the ship doesn't really have to do anything except wait for that after the cannonball is fired.

[MongKong]

Oh wow, that looks much better and easier to do, thank you !! Tho may i just ask, What is the point of having & operator at the beginning of QObject::destroyed and Ship:: onCannonBallDestroyed ? And why do you have to call onCannonballDestroyed using scope, if the method is part of the class Ship?

[d_stranz]

What is the point of having & operator at the beginning of QObject:: destroyed and Ship:: onCannonBallDestroyed

connect( canonball, &QObject::destroyed, this, &Ship::onCannonballDestroyed );

connect( canonball, &QObject::destroyed, this, &Ship::onCannonballDestroyed );

To copy to clipboard, switch view to plain text mode 

This is the new, type-safe form of QObject::connect(), which uses the "pointer to member function" style of giving the signal and slot pair. "&QObject:: destroyed" is the address of (pointer to) the "destroyed" signal (a member function) of the QObject class.

It is better to use this new style instead of the old one:

connect( cannonball, SIGNAL( destroyed( QObject * ), this, SLOT( onCannonballDestroyed( QObject * ) ) );

connect( cannonball, SIGNAL( destroyed( QObject * ), this, SLOT( onCannonballDestroyed( QObject * ) ) );

To copy to clipboard, switch view to plain text mode 

because the new style will not compile if you give the wrong arguments. The old style will compile, but will fail at run time if you give it the wrong arguments. That isn't so good, because unless you read the messages coming from the runtime output in the debugger, you will not know that the connect() failed and will wonder why your app isn't handling the signals in the way you think it should be.

[MongKong]

Oh i see, then i believe this works with timers aswell and timeout() method?

[anda_skoa]

Just as added info: the life time of a QObject can also be tracked with a QPointer

class Ship : public QObject
{
private:
    QPointer<Canon> m_cannonBall;
};

class Ship : public QObject
{
private:
    QPointer<Canon> m_cannonBall;
};

To copy to clipboard, switch view to plain text mode 

void Ship::fireCannonball()
{
    if ( canFireCannon() )  
    {
        m_cannonBall = new Canon( this );
    }
}
 
bool  Ship::canFireCannon()
{
   return m_cannonBall.isNull();
}

void Ship::fireCannonball()
{
    if ( canFireCannon() )  
    {
        m_cannonBall = new Canon( this );
    }
}

bool  Ship::canFireCannon()
{
   return m_cannonBall.isNull();
}

To copy to clipboard, switch view to plain text mode 

I.e. a QPointer tracks the QObject it is holding and resets itself to nullptr if the object is deleted

Cheers,
_

[d_stranz]

Thanks, anda_skoa. I've learned something new. QPointer<> is a bit more useful in this circumstance and leads to less code and overhead.

As I understand the docs, QPointer<> does not do reference counting in the way that std:: shared_ptr<> does. Also unlike shared_ptr, QPointer is not responsible for the lifetime of its object pointer. This also implies that you could potentially have several QPointer instances wrapping the same QObject pointer, correct? Presumably all of them connect to the same QObject:: destroyed() signal, and would simply set their internal copy of the pointer to null.

I will have to think of a good use case for this.

Oh i see, then i believe this works with timers aswell and timeout() method?

connect( myTimer, &QTimer::timeout, this, &ThisClass::onTimeout );

connect( myTimer, &QTimer::timeout, this, &ThisClass::onTimeout );

To copy to clipboard, switch view to plain text mode 

(Of course, substitute the real name of your class for "ThisClass")

connect( myTimer, &QTimer::timeout, [=]() { cannonball->deleteLater(); } );

connect( myTimer, &QTimer::timeout, [=]() { cannonball->deleteLater(); } );

To copy to clipboard, switch view to plain text mode 

Works with lambdas, too.

[anda_skoa]

Thanks, anda_skoa. I've learned something new.

As I understand the docs, QPointer<> does not do reference counting in the way that std:: shared_ptr<> does.
Also unlike shared_ptr, QPointer is not responsible for the lifetime of its object pointer.

In terms of smart pointers you can think of QPointer as a weak-pointer, created from a shared-pointer inside the object itself.

This also implies that you could potentially have several QPointer instances wrapping the same QObject pointer, correct?

Yes, correct.

Presumably all of them connect to the same QObject:: destroyed() signal, and would simply set their internal copy of the pointer to null.

It is a bit more sophisticated than that

Originally the implementation used some Qt internal functions to register the QPointer instances with the objects they were holding and the object would then notify the pointers as part of their destruction code.
I.e. without going through the signal mechansim (as that would have required QPointer to be a QObject).

The internals of the implementation changed and now literally uses a QWeakPointer for storing and monitoring.

I will have to think of a good use case for this.

Super useful in many circumstances.

Cheers,
_

[MongKong]

d_stranz and anda_skoa thank you both for help and advices, i appreciate it a lot thank you again !!!!!