Sign in to follow this  

std::vector and pointers to objects in the vector?

This topic is 3865 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hi! What is the correct way of setting up pointers to objects in another vector ? Currently I have this:

class Zone
{
 //class data in here... 
};

std::vector<Zone*> zones;
std::vector<Zone*> visiblezones;

void init()
{
  for (int i=0; i < 100; i++)
  zones.push_back(new Zone());  // load all zones.
};

void render();
{
  visiblezones.clear();

  for (std::vector<Zone*>::iterator i=zones.begin(); i != zones.end(); i++)                             
  {
     visiblezones.push_back((*i));   //i.e based on visibility, add to visible.
  }

   //render all visiblezones.
};


void cleanup()
{
   for (std::vector<Zone*>::iterator i=zones.begin(); i != zones.end(); i++)                             
       delete *i; //free memory from 'new'

   zones.clear();
   visiblezones.clear();
};

void main()
{
   init();

   while(running) //do gameloop etc
   {
   render();
   }

   cleanup();
};


The above should execute, right? Yes it does, but if I try to drop all elements in "zones", and load a new set it fails. I've tried adding another "init()" after the cleanup(), with crashes and memory-leaks to follow... The zone-positions gets screwed up as well. Should I do "delete (&i)" instead? Doing so loads the new zone positions in the correct spot, but doesn't free up memory and leads to crashes. What I do feel is really weird is that if I do the whole procedure in a row, i.e declaring a new zones-vector, adding elements, binding activezones and then removing them all... no memory-leaks, no crashes! If I however let the zones-vector survive a couple of gameloops, it crashes at cleanup(). I've tried removing all functions that read from the zones-vector, but it makes no difference. What I need is the ability to completely drop the zones-"catalogue" and load a new set - something that doesn't work in the curent state... I'd really appreciate any help in this matter, since I've painted myself into a corner here.. ;) Let me know if anything is unclear about my question, and I'll try to explain further! Thanks!

Share this post


Link to post
Share on other sites
Except for the syntax errors, your code looks fine to me ;-). The error is probably somewhere else. My guess would be that you use invalidated iterators in another place, f***ing up the vector, but it's just a guess. Could you provide a minimal program that reproduces the crash?

Share this post


Link to post
Share on other sites

#include <vector>
#include <iostream>

class Zone
{
public:
Zone(void) { std::cout << "Zone created" << std::endl; }
~Zone(void) { std::cout << "Zone destroyed" << std::endl; }
};

std::vector<Zone*> zones;
std::vector<Zone*> visiblezones;

void init()
{
for (int i = 0; i < 100; ++i)
{
Zone* new_zone = new Zone();
zones.push_back(new_zone);
}
}

void render()
{
visiblezones.clear();

for (std::vector<Zone*>::iterator i=zones.begin(); i != zones.end(); ++i)
visiblezones.push_back(*i);
}

void cleanup()
{
for (std::vector<Zone*>::iterator i=zones.begin(); i != zones.end(); ++i)
delete *i;

zones.clear();
visiblezones.clear();
}

int main(int argc, char *argv[])
{
init();
render();
cleanup();

init();
render();
cleanup();
}



Works perfectly fine. I'm guessing your problem is with cleanup within the Zone class, or some place else in your program. I suggest you head over to Boost and use smart pointers instead [smile]


#include <vector>
#include <iostream>
#include <boost/shared_ptr.hpp>
#include <boost/weak_ptr.hpp>
#include <boost/bind.hpp>

class Zone
{
int id;
public:
Zone(int zone_id) : id(zone_id) { std::cout << "Zone created: " << id << std::endl; }
~Zone(void) { std::cout << "Zone destroyed: " << id << std::endl; }

void print_me(void) {std::cout << "Zone: " << id << std::endl; }
};

typedef boost::shared_ptr<Zone> ZonePtr;
typedef boost::weak_ptr<Zone> ZoneWeakPtr;


std::vector<ZonePtr> zones;
std::vector<ZoneWeakPtr> visiblezones;

typedef std::vector<ZonePtr>::size_type ZoneIndex;

void init()
{
for (int i = 0; i < 5; ++i)
{
ZonePtr new_zone(new Zone(i));
zones.push_back(new_zone);
}
}

void render()
{
for(ZoneIndex i = 0; i < zones.size(); ++i)
{
if (i % 2 == 0)
{
ZoneWeakPtr zone(zones.at(i));
visiblezones.push_back(zone);
}
}

std::cout << "Printing Zones..." << std::endl;
std::for_each(zones.begin(), zones.end(), boost::bind(&Zone::print_me, _1));

std::cout << "Printing Visible Zones..." << std::endl;
for (std::vector<ZoneWeakPtr>::iterator it = visiblezones.begin(); it != visiblezones.end(); ++it)
{
ZonePtr zone = it->lock();
zone->print_me();
}

visiblezones.clear();
}

void cleanup()
{
zones.clear();
}

int main(int argc, char *argv[])
{
init();
render();
cleanup();
}



Share this post


Link to post
Share on other sites
Quote:
Original post by Rasmadrak
Hi!

What is the correct way of setting up pointers to objects in another vector ?


There is no one correct way; it depends on how you have things organized.

But do keep in mind that "pointers to" is being too specific: it is generally a better idea to forget you ever heard of pointers when you start designing, and say "handles to" instead.

If the "another vector" in fact contains the objects, and its contents won't change (rather, if you will never erase elements from the vector or insert/push_back any in) once you do your first initialization, then you can just store pointers - or probably better, std::vector<T>::iterators - and everything is fine: the elements will never move in memory.

If you will add more elements but never remove (or reorder) them, you can use a plain old int as a handle. That is, an index into the vector ;) The reason you can't use a pointer here is that when the vector is resized to make room for more elements, it will copy its contents to a new part of memory

If you can remove elements, and also need to alias them and have the aliases still work after removing other elements, then... well, you're screwed if you try to use a std::vector normally. You could use pointers as a handle type if you used std::list for your container, though (because std::list puts its elements into "nodes" that are separately allocated and linked up across memory, it never needs to "move" any of its dynamically allocated storage when something is inserted or deleted). HOWEVER...

Quote:

Currently I have this:


In what you currently have, the elements in question aren't actually *in* your vector [smile] since you made both vectors hold pointers to Zone.

As the other posters noted, there's apparently nothing wrong here yet (except for a typo - a semicolon at the end of 'void render()' - though as written that shouldn't compile in most contexts anyway, and except that 'void main' is not legal C++ - you must return int), and the problem presumably lies within the Zone class itself. However, this design is somewhat difficult to look after. Using a smart pointer such as boost::shared_ptr will help a lot - you can avoid all the manual memory management that way. (However, note that with this approach, a Zone removed from one container would be kept "alive" until also removed from the other.)

But if we don't actually need that - if we just load a set of Zones and want the visiblezones to alias some subset of those, and we don't change the Zones again until we load a whole new set, at which time the visiblezones are also updated - then we don't need this level of complexity. Instead, we use the language as intended, by storing Zones *by value* in zones, and just aliasing them in the visiblezones. We can also make a wrapper for Zone-handles, so that calling code needn't break if we determine that we need a different handle type. Oh, and we can make a class to represent a set of zones, and bundle together all the setup/teardown work much more cleanly [smile]


class Zone
{
//class data in here...
};

class ZoneHandle {
Zone* ptr;
public:
ZoneHandle(const std::vector<Zone>& context, std::vector<Zone>::iterator pos) : ptr(&*pos) {}
Zone& operator*() { return *ptr; }
Zone* operator->() { return ptr; }
};

// Alternate implementation using an int underneath, so you get the concept:
//class ZoneHandle {
// int index;
// std::vector<Zone>* cptr;
// public:
// ZoneHandle(const std::vector<Zone>& context, std::vector<Zone>::iterator pos) : cptr(&context), index(std::distance(context.begin(), pos)) {}
// Zone& operator*() { return (*cptr)[index]; }
// Zone* operator->() { return &((*cptr)[index]); }
//};

class ZoneSet {
std::vector<Zone> zones;
std::vector<ZoneHandle> visiblezones;

public:
ZoneSet() : zones(100, Zone()) {}

void render() {
visiblezones.clear();
for (std::vector<Zone>::iterator i=zones.begin(); i != zones.end(); i++) {
if (is_visible(*i) {
visiblezones.push_back(ZoneHandle(zones, i));
}
}

// then do the rendering, treating ZoneHandle instances exactly as if they
// were pointers-to-Zone.
}

// We actually don't need a destructor etc.: we didn't allocate the Zones
// dynamically, and there's no need to .clear the vectors because they are
// about to die anyway, which will implicitly clear them.
};

int main() {
ZoneSet zs;

while(running) {
render();
if (timeToStartANewLevel()) {
zs = ZoneSet();
}
}
} // <-- no semicolon needed here; but it doesn't cause problems, just looks odd



However, are you really sure you need this separate vector of visible Zones, anyway? If you're re-creating it each time you render... why not just render each Zone right away when you discover that it's visible, instead of building up the vector and then going over that vector?

Share this post


Link to post
Share on other sites
Hi, and thanks for your time!

I will check through your answers more thoroughly when I've got a little more time, but to answer a couple of things shortly:

Quote:

However, are you really sure you need this separate vector of visible Zones, anyway? If you're re-creating it each time you render... why not just render each Zone right away when you discover that it's visible, instead of building up the vector and then going over that vector?


Yes, I need to have visible zones in a separate array since many more functions depend on this list. My current system only loops through all of the zones once per frame (maybe even less further on) and is allowing me to have tens of thousands of zones without any slowdowns.
That is, physics and graphic-intense functions only operate on visible zones.
And the list would not be destroyed each frame, zones that are out of range or unloaded gets removed from the list. Maybe I've overlooked something, but this part of my code works just fine so I didn't think it would cause any troubles.
Now that I think of it, "activezones" is probably a more fitting term.

The same system would also be used for 3d models, where I insert models into a catalogue and will be able to instance objects instead of reloading them where they are needed.


Quote:

I'm guessing your problem is with cleanup within the Zone class, or some place else in your program.


Could very well be, but I've gone to great extents trying to keep the various components separated or at least not dependent on each other.
I've tried creating (using 'new') and deleting Zones in a loop, and no memory-leaks are present, so I'm pretty sure the Zone's are destroyed completely. But as always, I'll doublecheck just to be sure.


Overall, I realize that redoing code from the top of your head isn't really the way to go when posting in a forum... It leads to embarrassing problems such as "void render();" and "void main()"... ;)

I'll try to rewrite some code and see if that helps, otherwise I'll be back soon.

Thanks again!

Share this post


Link to post
Share on other sites
Just newing and deleting a class iteratively isn't really a good test of its design WRT memory management. You need to make sure that copy-construction and assignment work properly, too.

Share this post


Link to post
Share on other sites

This topic is 3865 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this