issues with a vector of pointers

Started by
6 comments, last by Zakwayda 13 years, 11 months ago
I am having trouble getting a few different things to work. I tend to encounter segfaults or "This application has requested the Runtime to terminate it in an unusual way." I have 2 stl vectors, one is a vector of objects, the other is a vector of pointers to objects. Note that the vectors are only set up on initialisation and are not changed during the game logic loop. (they store occlusion hierarchy links) Basically, the environment holds a vector of cell objects. Each cell object has a vector of pointers to other cell objects. Getting the data in place isn't causing me issues, but I have functions which are required to return pointers to cells - and this is where I am doing something wrong. I have extracted some of the suspect code:

class env_map{
public:
  std::vector<env_cell> cells; //list of environment's cells
  std::vector<env_cell>::iterator cellI; //iterator

  //takes point x,z and returns a pointer to the cell it resides within
  env_cell* findCell(float x,float z){
    for(cellI = cells.begin();cellI != cells.end();cellI++){
      //if point is inside the cell
      if(x<=currentCell.right && x>=currentCell.left && z>=currentCell.top && z<=currentCell.bottom){
        return iCell; //return pointer to the cell
      }
    }
    return &(*cells.begin()); //return cell 0 (could check for nearest later)
  }
};






This should loop through all the cells in the environment and return a pointer to whichever one the point is within. Another piece of code resides in the env_cell class:

class env_cell{
public:
  std::vector<env_cell*> visCells; //cells that can be seen from this cell
  std::vector<env_cell*>::iterator visCellsI; //visible cells iterator
  int nextCell;

  //returns a pointer to an adjacent cell which the point is within
  env_cell* findCell(float x,float z){
    env_cell * foundCell;
    for(visCellsI = visCells.begin();visCellsI != visCells.end();visCellsI++){
      //if point is inside the cell
      if(x <= (*visCellsI)->right && x >= (*visCellsI)->left && z <= (*visCellsI)->top && 
      z >= (*visCellsI)->bottom){
        foundCell = (*visCellsI);
        break;
      }
    }
    
    //if a cell was found
    if(foundCell){
      return foundCell; //return new cell
    } else {
      return this; //keep current cell assigned
    }
  }

  //adds a cell to the vector of cells in the map (only called during init)
  void addCell(float sTop,float sLeft,float sBottom,float sRight){
    cells.push_back(env_cell(nextCell,sTop,sLeft,sBottom,sRight));
    nextCell ++; //increase cellId counter
  }
};






Now, I obviously have a problem with the data flow or my dereferencing, but I don't properly understand the correct way. If I try to fix the issue I usually make the problem worse. Put it this way: I want my cells stored within a vector in the map class. Pointers returned by findCell in the cell class should reference the cells stored in the map class At the moment I am manually assigning which cells are visible to other cells - this would be loaded at map initialisation after being loaded from the map file (calculated in the editor during exportation or manually set) here is how I am doing that:

  //(camera location starts at 0,0) (4 values are top,left,bottom,right of cell)
  map->addCell(-64,-256,128,128); //in the 0th place in the cells vector
  map->addCell(32,128,96,256); //1st place

  //manually enter vis
  map->cells[0].visCells.push_back(&map->cells[1]);
  //7 more lines similar to the above line






i.e. # map is a pointer to an object env_map # cells is the vector of all the cell objects in the environment # visCells, within objects env_cell, is the vector of pointers to other visible cells from within that cell so what that line of code should do is add a pointer to cell in the 1st index in the cells vector to the array of visible cells from the cell in the 0th index in the cells vector. I know I am probably not doing many things the best way - but I still have a lot to learn. I appreciate comments on better practices for such a solution, but if my current logic will work I want to get it running so I can see where its pitfalls are. [Edited by - Bozebo on April 25, 2010 8:48:14 PM]
Advertisement
You should use BSP tree if you want precalculated PVS for your scene. However, I would suggest using octrees and performing occlusion tests in run-time like explained here.
I didn't look at all the code, but I see a potential problem here:
env_cell* findCell(float x,float z){    env_cell * foundCell;
You probably want:
    env_cell * foundCell = 0;
As is, foundCell is unitialized, which means that if no cell is found, the function may return this (as intended), or it may return an invalid pointer (which could easily lead to your program crashing or terminating unexpectedly).
Well, I am trying to implement PVS as such. But I don't know the specifics of it - only the basic idea: and that is what I am trying to implement. Also, after getting the pointers to visible rooms I will have a combined vector (or list if that is a better idea?) of all the renderable objects in the non-occluded environment, each entity to be rendered as a radius for their sphere bounding box and values to represent a cuboid bounding box - I have in-place the necessary features to implement occlusion tests but I must get my application stable and showing an example of large-scale occlusion first (ie, cells round corners are not drawn).

Thanks jyk, that stopped the appcrash (env_cell foundCell = this).
I might not be safe yet though, I need to work out what is causing some other bugs I am now aware of :(
Quote:Original post by jyk
I didn't look at all the code, but I see a potential problem here:
env_cell* findCell(float x,float z){    env_cell * foundCell;
You probably want:
    env_cell * foundCell = 0;
As is, foundCell is unitialized, which means that if no cell is found, the function may return this (as intended), or it may return an invalid pointer (which could easily lead to your program crashing or terminating unexpectedly).


... Better yet, initialize foundCell with this, and then return it unconditionally after the loop instead of checking it. Or even skip having that variable completely, and just return whatever is found when you find it. ;)

Also, don't store iterators as class members. You only need it inside the find function, so it should get declared there. This is just applying the usual principle of scoping variables as tightly as possible, but it's especially important with class instances to avoid wasting memory.

There are a few other subtle things you can do to clean up the code right away without having to learn the really advanced techniques. Please pay close attention to all the differences - in particular, the lack of comments, which are made unnecessary by other changes:

class environmentCell {  // ...public:  std::vector<environmentCell*> adjacentCells;  // returns a pointer to an adjacent cell surrounding the specified point.  // If none of the adjacent cells qualify, return this.  environmentCell* findCell(float x, float z) {    typedef typename std::vector<environmentCell*>::iterator iterator;    for (iterator it = adjacentCells.begin(), end = adjacentCells.end(); it != end; ++it) {      environmentCell* cell = *it;      if (x <= cell->right && x >= cell->left && z <= cell->top && z >= cell->bottom) {        return cell;      }    }        return this;  }};


By the way, this comment is worrisome:

Quote: //adds a cell to the vector of cells in the map (only called during init)


You should almost never have "two-phase construction" (an 'init' function that is called after the constructor). Constructors are for the construction work. If, on the other hand, the init() function is called to implement the constructor, it - and its helpers, in turn - should probably be private. But again, why is creating the object so complicated that you need to cut up the work into these sorts of functions? Maybe you actually need to cut up the class itself?
Quote:... Better yet, initialize foundCell with this, and then return it unconditionally after the loop instead of checking it. Or even skip having that variable completely, and just return whatever is found when you find it. ;)
Agreed, of course, but in that particular post I was just trying to point out the (likely) source of the error. (Clearly, there are better ways the code can be written, most of which won't involve a 'found cell' variable at all.)
Naturally. You ought to realize by now that I tend to let other people take care of that aspect of things and instead point out the other aspects of writing good code. :)
Quote:Original post by Zahlman
You ought to realize by now that I tend to let other people take care of that aspect of things and instead point out the other aspects of writing good code. :)
Haha, right. Oh well - I don't mind being one of those 'other people' :)

This topic is closed to new replies.

Advertisement