Virtual function does not work... (or i missed something(iprobably did))

Virtual function does not work... (or i missed something(iprobably did))

mumu1000

Member #16,842

April 2018

Well... this issue is so weird i don't know how to synthesize it...
I'm just gonna post the code here, and tell you what i've figured out so far.
Feel free to test it and tell me what output you get ! (Only Allegro lib and std used)

This is going to be some sort of space game, yet its more a test for me to learn than an actual game

main.cpp

#SelectExpand
  1#include <iostream>
  2#include <string>
  3#include <vector>
  4#include <sstream>
  5#include <chrono>
  6#include <thread>
  7#include "allegro5\allegro.h"
  8#include "allegro5\allegro_color.h"
  9#include "allegro5/allegro_primitives.h"
 10#include "Chunk.h"
 11#include <random>
 12#include "Planet.h"
 13#include "AstronomicalObject.h"
 14
 15void testDraw(AstronomicalObject *ast)
 16{
 17    ast->draw();
 18    if ( typeid(*ast) == typeid(Planet) )
 19    {
 20        std::cout<<"PLANET";
 21    }else
 22    {
 23        std::cout<<"imanobject";
 24    }
 25}
 26int main(int argc, char **argv)
 27{
 28    auto now = std::chrono::system_clock::now();
 29    auto now_ms = std::chrono::time_point_cast<std::chrono::milliseconds>(now);
 30    auto value = now_ms.time_since_epoch();
 31    unsigned int duration = value.count();
 32
 33    srand(duration);
 34    if (al_init() && al_init_primitives_addon())
 35    {
 36        std::cout<<"It Works";
 37    }else
 38    {
 39        std::cout << "Allegro Not Loaded";
 40        return 1;
 41    }
 42    al_set_new_display_flags(ALLEGRO_FRAMELESS);
 43    ALLEGRO_DISPLAY * mainDisplay = al_create_display(1800,1000);
 44    Chunk testChunk;
 45    Planet * testPlanet = new Planet(3,6);
 46    testPlanet ->draw();
 47    al_flip_display();
 48    std::this_thread::sleep_for(std::chrono::milliseconds(1000));
 49    for (int u = 0;u<5;u++)
 50    {
 51        testChunk = Chunk(true);
 52        std::cout<<testChunk.getAstroObjListSize();
 53        al_clear_to_color(al_map_rgb(20,20,20));
 54        for (unsigned int i = 0; i<(testChunk.getAstroObjListSize());i++)
 55        {
 56            testDraw(testChunk.getAstroObjBylistIndex(i));
 57        }
 58        al_flip_display();
 59        std::this_thread::sleep_for(std::chrono::milliseconds(1000));
 60
 61    }
 62
 63    //al_set_target_bitmap(nullptr);   //does some weird access violation
 64    //al_destroy_display(mainDisplay);
 65    al_shutdown_primitives_addon();
 66    return 0;
 67}

Chunk.h

#SelectExpand
  1#ifndef CHUNK_H
  2#define CHUNK_H
  3#include "AstronomicalObject.h"
  4#include "Planet.h"
  5#include <vector>
  6
  7
  8class Chunk
  9{
 10    public:
 11        Chunk();
 12        Chunk(bool ranGen);
 13        virtual ~Chunk();
 14        void addAstroObject(AstronomicalObject *toBeAdded);
 15        AstronomicalObject* getAstroObjBylistIndex(unsigned int x){if(x<m_listeContenu.size()){return m_listeContenu[x];}else{return nullptr;}};
 16        unsigned int getAstroObjListSize(){return m_listeContenu.size();};
 17    protected:
 18        std::vector<AstronomicalObject*> m_listeContenu;
 19    private:
 20};
 21
 22#endif // CHUNK_H

Chunk.cpp

#SelectExpand
  1#include "Chunk.h"
  2#include <math.h>
  3#include "iostream"
  4
  5Chunk::Chunk()
  6{
  7
  8}
  9Chunk::Chunk(bool ranGen)
 10{
 11    if(ranGen)
 12    {
 13        unsigned int numbOfObj = rand()%30;
 14        std::cout<<" number:"<< numbOfObj <<". ";
 15        for (unsigned int i = 0; i<numbOfObj;i++)
 16        {
 17            addAstroObject(new AstronomicalObject(rand()%320,rand()%320)); //multiple tries to make it work
 18            addAstroObject(new Planet(rand()%320,rand()%320));
 19            m_listeContenu.push_back(new Planet(rand()%320,rand()%320));
 20        }
 21    }
 22}
 23
 24Chunk::~Chunk()
 25{
 26    for (unsigned int i = 0;i<m_listeContenu.size();i++)
 27    {
 28        delete m_listeContenu[i];
 29    }
 30}
 31void Chunk::addAstroObject(AstronomicalObject * toBeAdded)
 32{
 33    m_listeContenu.push_back(toBeAdded);
 34}

AstronomicalObject.h

#SelectExpand
  1#ifndef ASTRONOMICALOBJECT_H
  2#define ASTRONOMICALOBJECT_H
  3#include "allegro5/color.h"
  4#include "allegro5/allegro_primitives.h"
  5#include <iostream>
  6
  7
  8class AstronomicalObject
  9{
 10    public:
 11
 12        AstronomicalObject();
 13        AstronomicalObject(float xPos, float yPos);
 14
 15        virtual ~AstronomicalObject();
 16        float const getXPos(){return m_xPos;};
 17        float const getYPos(){return m_yPos;};
 18        void setXPos(float xPos){m_xPos = xPos;};
 19        void setYPos(float yPos){m_yPos = yPos;};
 20        virtual void draw() const;
 21
 22    protected:
 23        float m_xPos;
 24        float m_yPos;
 25        bool m_isAPlanet;
 26    private:
 27};
 28
 29#endif // ASTRONOMICALOBJECT_H

AstronomicalObject.cpp

#SelectExpand
  1#include "AstronomicalObject.h"
  2
  3AstronomicalObject::AstronomicalObject()
  4{
  5    m_xPos = 0;
  6    m_yPos = 0;
  7    m_isAPlanet = false;
  8}
  9
 10AstronomicalObject::AstronomicalObject(float xPos, float yPos)
 11{
 12    m_xPos = xPos;
 13    m_yPos = yPos;
 14    m_isAPlanet = false;
 15}
 16
 17AstronomicalObject::~AstronomicalObject()
 18{
 19}
 20
 21void AstronomicalObject::draw() const
 22{
 23    if(!m_isAPlanet)
 24    {
 25        al_draw_circle(10+(m_xPos*3),10+(m_yPos*3),8,al_map_rgb(200,200,200),3);
 26    }
 27    else
 28    {
 29        al_draw_circle(10+(m_xPos*3),10+(m_yPos*3),8,al_map_rgb(20,200,20),3);
 30    }
 31}

Planet.h

#SelectExpand
  1#ifndef PLANET_H
  2#define PLANET_H
  3
  4#include <iostream>
  5#include "AstronomicalObject.h"
  6#include "allegro5/color.h"
  7#include "allegro5/allegro_primitives.h"
  8
  9
 10class Planet : public AstronomicalObject
 11{
 12    public:
 13        Planet();
 14        Planet(float xPos, float yPos);
 15        virtual ~Planet();
 16        virtual void draw() const;
 17
 18    protected:
 19        bool m_isWaterPresent;
 20
 21    private:
 22};
 23
 24#endif // PLANET_H

Planet.cpp

#SelectExpand
  1#include "Planet.h"
  2Planet::Planet() : AstronomicalObject()
  3{
  4    m_isWaterPresent = true;
  5    m_isAPlanet = true;
  6}
  7Planet::Planet(float xPos, float yPos)
  8{
  9    m_xPos = xPos;
 10    m_yPos = yPos;
 11    m_isAPlanet = true;
 12    m_isWaterPresent = false;
 13}
 14
 15Planet::~Planet()
 16{
 17
 18}
 19void Planet::draw() const
 20{
 21    std::cout<<"I am A Planet";
 22    al_draw_filled_circle(10+(m_xPos*3),10+(m_yPos*3),8,al_map_rgb(20,200,20));
 23}

Outcome:
i get a plain green circle on my display, as expected, then i get 5 screens with some green and grey hollow circles... the thing is i should get grey hollow circles and PLAIN green circles... Something is going wrong.

There we are, these are all the sources i've got.
What it seems is the virtual draw function from AstronomicalObject overriden by Planet does not do what its supposed to do (no runtime link resolution).
No matter whether the pointer points to a Planet or an AstronomicalObject, it will call the AstroObj version of the draw function.

I've spent more than 5 hours trying to debug and understand what goes wrong, but... it just makes no sense ! It i supposed to work, that's what virtual functions are made for !

I really hope you will discover where the issue come from, cuz i'm dispairing ! :'(

Thx for you help ! <3

PS: As you may have noticed, i'm quite new to programming, so maybe the issue is dumb easily fixable, and i'm just too bad to find out how to.

Edgar Reynaldo

Major Reynaldo

May 2007

If you want people to try your code, provide a zip file with the sources in it.

Eagle and Allegro 5 binaries | Older Allegro 4 and 5 binaries | Allegro 5 compile guide
King Piccolo will make it so Then I could just flip it with some sweet, sweet matrix math. In either case, I’ll ride this wagon until the wheels fall off. Thanks to those that keep it running

mumu1000

Member #16,842

April 2018

Of course !

EDIT: Sorry the link didnt work ! You have it attached on my next post.

There you go ! (Im working with code::blocks, and i'm using MinGW64 Compiler)

Edgar Reynaldo

Major Reynaldo

May 2007

Your link doesn't work.
{"name":"611634","src":"\/\/djungxnpq2nug.cloudfront.net\/image\/cache\/a\/6\/a626c7009a4e4585ac18f6a2a43739da.png","w":1100,"h":552,"tn":"\/\/djungxnpq2nug.cloudfront.net\/image\/cache\/a\/6\/a626c7009a4e4585ac18f6a2a43739da"}

Just attach the zip file of your source.

mumu1000

Member #16,842

April 2018

Nani ??

Well, there you go then :p

Edgar Reynaldo

Major Reynaldo

May 2007

Every time you assign testChunk to a new Chunk(true) object on the stack you reference destroyed memory.

        testChunk = Chunk(true);

This calls Chunk::operator=(const Chunk& c);. The default assignment operator merely copies the data, so your vector<AstronomicalObject*> gets copied. This is a shallow copy. Now you have two copies of the same astronomical objects. Then the temporary Chuck(true) object goes out of scope, deleting every single object that was in it. Now your testChunk object holds pointers to deleted memory. That is why it segfaults.

2. You're using 'const' wrong. It goes at the end of the function declaration before the semi-colon.

class X {
public :
   void function() const;/// I promise I won't change anything
};

mumu1000

Member #16,842

April 2018

Oh
My
God

I still dont understand why it didnt work (i will definitely think about what you said, and try to understand) but using a pointer to a Chunk object instead of the Chunk object itself solved the sh*t !

My main.cpp is now

#SelectExpand
  Chunk* testChunk;
  Planet * testPlanet = new Planet(3,6);
  testPlanet ->draw();
  al_flip_display();
  std::this_thread::sleep_for(std::chrono::milliseconds(1000));
  for (int u = 0;u<5;u++)
  {
      testChunk = new Chunk(true);
      std::cout<<testChunk->getAstroObjListSize();
      al_clear_to_color(al_map_rgb(20,20,20));
      for (unsigned int i = 0; i<(testChunk->getAstroObjListSize());i++)
      {
          testDraw(testChunk->getAstroObjBylistIndex(i));
      }
      al_flip_display();
      std::this_thread::sleep_for(std::chrono::milliseconds(1000));
      delete testChunk;
 18
  }

and the virtual function does its job !!

Edgar Reynaldo said:

Now your testChunk object holds pointers to deleted memory. That is why it segfaults.

The problem is not a crash (it doesnt crash, no segfault here), the problem was the wrong version of the draw() function was called.

Edgar Reynaldo said:

your vector<AstronomicalObject*> gets copied. This is a shallow copy. Now you have two copies of the same astronomical objects.

The AstronomicalObjects dont get copied, since vector<AstronomicalObject*> holds pointers, not actual objects. So when it copies, it copies the adress stored in the pointers too. The actual AstronomicalObject is still being referenced to by these copied pointers : no memory leaks here. Where am i wrong ?

When you do testChunk = Chunk(true) doesnt it implicitly calls delete on the previous content of testChunk ?
I mean if you have something like std::string myString = "Thank You Very Much For Your Help";
then you do myString = "REALLY"; it doesnt leak memory right ?

Edgar Reynaldo said:

You're using 'const' wrong. It goes at the end of the function declaration before the semi-colon.

My bad ! Thx !

Thank you so much anyway ! You saved my day once again !

Edgar Reynaldo

Major Reynaldo

May 2007

Mumu1000 said:

Edgar Reynaldo said:

Now your testChunk object holds pointers to deleted memory. That is why it segfaults.

The problem is not a crash (it doesnt crash, no segfault here), the problem was the wrong version of the draw() function was called.

I ran it through gdb, and it segfault'ed. Same code. That's what you get when you dereference bad memory. You get undefined behavior.

mumu1000 said:

Edgar Reynaldo said:

your vector<AstronomicalObject*> gets copied. This is a shallow copy. Now you have two copies of the same astronomical objects.

Yes, the pointers get copied. But when your temporary object goes out of scope, those pointers get deleted. This means any copies of those pointers now hold references to deleted memory, which will make it segfault.

Mumu1000 said:

but using a pointer to a Chunk object instead of the Chunk object itself solved the sh*t !

That's because you stopped copying your vector. You need to learn about scope and the stack and the heap. Learn about the difference between a shallow copy and a deep copy.

Look at this code :

#SelectExpand
   1
   2
   3
   4#include <cstdio>
   5
   6typedef unsigned int ID;
   7const ID BADID = ~0;
   8
   9ID GetNewId() {
  10   static ID id = 0;
  11   return id++;
  12}
  13
  14
  15
  16class Base {
  17protected :
  18   ID id;
  19public :
  20   Base() :
  21         id(GetNewId())
  22   {}
  23   virtual ~Base() {}
  24   virtual Base* Clone() {
  25      return new Base(*this);
  26   }
  27   ID GetId() const {return id;}
  28};
  29
  30
  31
  32class Derived : public Base {
  33public :
  34   virtual Base* Clone() {
  35      return new Derived(*this);
  36   }
  37};
  38
  39
  40
  41class BadOwner {
  42   Base* b;
  43
  44public :
  45
  46   BadOwner() :
  47         b(0)
  48   {}
  49
  50   BadOwner(Base* base) :
  51         b(base)
  52   {}
  53
  54   BadOwner(const BadOwner& bo) :
  55         b(0)
  56   {
  57      Free();
  58      b = bo.b;/// Shallow copy
  59   }
  60
  61   ~BadOwner() {Free();}
  62
  63   void Free() {
  64      if (b) {
  65         printf("BadOwner  : Deleting base %p\n" , b);
  66         delete b;
  67         b = 0;
  68      }
  69   }
  70};
  71
  72class GoodOwner {
  73   Base* b;
  74
  75public :
  76
  77   GoodOwner() :
  78         b(0)
  79   {}
  80
  81   GoodOwner(Base* base) :
  82         b(base)
  83   {}
  84
  85   GoodOwner(const GoodOwner& go) :
  86         b(0)
  87   {
  88      Free();
  89      if (go.b) {
  90         b = go.b->Clone();/// Deep copy
  91      }
  92   }
  93
  94   ~GoodOwner() {Free();}
  95
  96   void Free() {
  97      if (b) {
  98         printf("GoodOwner : Deleting base %p\n" , b);
  99         delete b;
 100         b = 0;
 101      }
 102   }
 103};
 104
 105
 106int main(int argc , char** argv) {
 107   
 108   (void)argc;
 109   (void)argv;
 110   
 111   {
 112      BadOwner b1(new Derived());
 113      BadOwner b2 = b1;
 114
 115      GoodOwner g1(new Derived());
 116      GoodOwner g2 = g1;
 117   }/// Crashes because b2 deletes b1.b after b1.b is already deleted when they go out of scope
 118   
 119   return 0;
 120}

If you run it you can see the output and that it crashes because it destroys the same memory twice. That is what you were doing when you copied your vector using a shallow copy.

GoodOwner : Deleting base 00ca1620
GoodOwner : Deleting base 00ca1600
BadOwner  : Deleting base 00ca2f30
BadOwner  : Deleting base 00ca2f30

mumu1000

Member #16,842

April 2018

Thank you very much ! I'll work on it.

Edit:

Edgar Reynaldo said:

Oh, so what you mean is when this temporary object goes out of scope it implicitly calls the destructor function of every object pointed by the pointers held in this temporary object ? (i'm trying to reformulate to be sure i have well understood what you said ^^) Is that correct ? (i really must look again at basic class definition and operator method overriding courts xD)

Edit Edit:

I'm stupid...
It just calls the destructor of the Chunk class, which i told to delete everything.........
Sorry, i've been kinda slow on this one xD

Edgar Reynaldo

Major Reynaldo

May 2007

mumu1000 said:

Oh, so what you mean is when this temporary object goes out of scope it implicitly calls the destructor function of every object pointed by the pointers held in this temporary object ? (i'm trying to reformulate to be sure i have well understood what you said ^^) Is that correct ? (i really must look again at basic class definition and operator method overriding courts xD)

To answer the specific point in bold, no. And yes, because you told it to. When a pointer goes out of scope, nothing happens. When an object goes out of scope, it's destructor is called. Which in this case, was Chunk::~Chunk, which deleted every pointer in the object vector.