views:

139

answers:

4

Hello, I'm porting some code to another structure:

class EnvironObject
{
   protected:
      vector<float> mX, mY, mXSpeed, mYSpeed;
      int mMaxObjects;

   public:
      virtual void init(int maxObjects);
      virtual void setLimit(int limit);
      virtual int getLimit();
      virtual void update(float arg) = 0;
};

void EnvironObject::setLimit(int limit)
{
   mMaxObjects = limit;

   mX.resize(limit, 0); mY.resize(limit, 0);
   mXSpeed.resize(limit, 0); mY.resize(limit, 0);
}

int EnvironObject::getLimit()
{
   return mMaxObjects;
}

void EnvironObject::init(int maxObjects)
{
    mX = mY = mXSpeed = mYSpeed = std::vector<float>(mMaxObjects);

    fill(mX.begin(), mX.end(), 0);
    fill(mY.begin(), mY.end(), 0);
    fill(mXSpeed.begin(), mXSpeed.end(), 0);
    fill(mYSpeed.begin(), mYSpeed.end(), 0);

    /*mX.reserve(mMaxObjects * 1.5); mY.reserve(mMaxObjects * 1.5);
    mXSpeed.reserve(mMaxObjects * 1.5); mYSpeed.reserve(mMaxObjects * 1.5);*/

    mMaxObjects = maxObjects;
}

This is some basic class, now it's usage:

class Rain : public EnvironObject
{
    public:
        Rain(int maxDrops = 150);
        void update(float windPower);
};

Rain::Rain(int maxDrops)
{
    srand(time(NULL));

    IEnvironObject::init(maxDrops);
}

void Rain::update(float windPower)
{
    for (int i=0; i < mMaxObjects; i++)
    {
       mX[i] += mXSpeed[i];
       mY[i] += mYSpeed[i];

       mXSpeed[i] += windPower;
       mYSpeed[i] += G;

   // Drawing
    }
}

The objects Rain creates with default constructor (so, each array is 150 elements size) and then I'm calling setLimit(50). The problem is that code fails almost each running with exception:

terminate called after throwing an instance of 'std::bad_alloc'

And sometimes it segfaults at line:

mY[i] += mYSpeed[i];

I can't image what could it be, because the code is old and it worked. The new one is only base class.

And when I'm looking at RAM usage when starting app, I see almost +600 mb!

+7  A: 

Look again at that function of yours:

void EnvironObject::init(int maxObjects)
{
    mX = mY = mXSpeed = mYSpeed = std::vector<float>(mMaxObjects);
    //                                               ^
    // ...

    mMaxObjects = maxObjects;
}

You're using a not yet initialized variable.

A big problem with your class is that you are doing what's called two-phase construction. Your class EnvironObject has a compiler-supplied default constructor that creates an object with random values for all POD types (mMaxObjects). Users then need to call the init() method to really initialize the object. But that's what constructors are there for!

void EnvironObject::EnvironObject(int maxObjects)
  : mMaxObjects(maxObjects)
  , mX(maxObjects), mY(maxObjects), mXSpeed(maxObjects), mYSpeed(maxObjects)
{
    /* these aren't necessary, std::vector automatically does this
    fill(mX.begin(), mX.end(), 0);
    fill(mY.begin(), mY.end(), 0);
    fill(mXSpeed.begin(), mXSpeed.end(), 0);
    fill(mYSpeed.begin(), mYSpeed.end(), 0);
    */
}

Derived classes can then use this constructor:

Rain::Rain(int maxDrops)
 : EnvironObject(maxDrops)
{
    srand(time(NULL));
}

Regarding this crash in the subscription mY[i] += mYSpeed[i]:

This might happen when you are calling this function through a pointer that's pointing to nowhere.

sbi
Thanks for the reply.
Ockonal
Oh, this is great asnwer.
Ockonal
Btw, would it be better to remove 4 arrays and make structure with that fields. And make vector with structure?
Ockonal
@Ockonal: Yes, that's certainly better (though not much in terms of memory) or it wouldn't have been up-voted so far. (I had up-voted Reinderien's answer, too, BTW.)
sbi
+5  A: 

You're using mMaxObjects in init() before initializing it. So it has a random value.

void EnvironObject::init(int maxObjects) 
{ 
   mX = mY = mXSpeed = mYSpeed = std::vector<float>(mMaxObjects);  // you mean maxObjects here
jdv
+4  A: 

I think you want to replace

void EnvironObject::init(int maxObjects)
{
    mX = mY = mXSpeed = mYSpeed = std::vector<float>(mMaxObjects);

with

void EnvironObject::init(int maxObjects)
{
    mX = mY = mXSpeed = mYSpeed = std::vector<float>(maxObjects);

Notice the replacement of mMaxObject to maxObjects in the vector creation.

extraneon
+4  A: 

One comment, though it won't likely fix your memory error, is that since the fields mX, mY, mXSpeed, and mYSpeed seem related and the vectors are all the same size, you should consider merging them into one structure with four members, and having a single vector containing several of those structure instances.

Reinderien
Thanks for the tip ;)
Ockonal
@Reinderien: it depends actually, separating the various fields makes sense if you plan on working on one at a time, this way you don't fill your cache with values you don't care for. Can't recall the name of this programming strategy though...
Matthieu M.