• Advertisement
Sign in to follow this  

Collision function causing heap corruption?

This topic is 2336 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


bool collision_line( float x1, float y1, float x2, float y2, int ObjectId)
{
Vector2 vec1(x1, y1);
Vector2 vec2(x2, y2);
std::vector<GridCell*> cells;

if (vec2.x < vec1.x && vec2.y < vec1.y)
{
Vector2 tmp = vec1;
vec1 = vec2;
vec2 = tmp;
}

float d = floor( point_direction( vec1.x, vec1.y, vec2.x, vec2.y));
float length = GetDistance( vec1.x, vec1.y, vec2.x, vec2.y);
GridCell *last = NULL;
for (int l = 0; l < length; l += CELL_SIZE)
{
int px = floor(cos(degtorad(d)) * l);
int py = floor(sin(degtorad(d)) * l);

GridCell *cell = objectManager->GetGrid()->CellAt( px, py);
if (cell != NULL && cell != last)
{
cells.push_back( cell);
}
last = cell;
}

PMASK mask;
init_pmask( &mask, (int)(vec2.x - vec1.x), (int)(vec2.y - vec1.y));
fill_pmask( &mask, 0);

for (int x = 0; x < length; ++x)
{
int mx = floor(cos(degtorad(d)) * x);
int my = floor(sin(degtorad(d)) * x);
set_pmask_pixel( &mask, mx, my, 1);
}

for (int n = 0; n < cells.size(); ++n)
{
std::vector<Object*> list;
cells[n]->GetObjects( list);

if (list.empty()){ continue;}

for ( int i = 0; i < list.size(); ++i)
{
if (list->GetType() != ObjectId){ continue;}
Sprite *spr = reinterpret_cast<Sprite*>(renderEngine->_get_sprite(list->mask_index));
Rect r = list->rect;

if (check_pmask_collision( &mask, &spr->mask->mask, floor(vec1.x), floor(vec1.x), floor(list->x), floor(list->y)) == 1)
{
return true;
}
}
}

return false;
}


Its seems that when false is returned, it causes a heap corruption when cleaning up local variables. Any reason why?

Share this post


Link to post
Share on other sites
Advertisement
Sprite *spr = reinterpret_cast<Sprite*>(renderEngine->_get_sprite(list->mask_index));
Now there's a big red glowing fish waiting to be caught.

First step when looking for why your program has weird behaviour: [font="Courier New"]grep -r reinterpret_cast *.cpp[/font]

Share this post


Link to post
Share on other sites
It has something to do with the cells vector. Hers is the call stack if it helps:

msvcr100d.dll!_CrtIsValidHeapPointer(const void * pUserData) Line 2036 C++
msvcr100d.dll!_free_dbg_nolock(void * pUserData, int nBlockUse) Line 1322 + 0x9 bytes C++
msvcr100d.dll!_free_dbg(void * pUserData, int nBlockUse) Line 1265 + 0xd bytes C++
msvcr100d.dll!operator delete(void * pUserData) Line 54 + 0x10 bytes C++
SADE_ENGINE4.exe!std::allocator<GridCell *>::deallocate(GridCell * * _Ptr, unsigned int __formal) Line 182 + 0x9 bytes C++
SADE_ENGINE4.exe!std::vector<GridCell *,std::allocator<GridCell *> >::_Tidy() Line 1309 C++
SADE_ENGINE4.exe!std::vector<GridCell *,std::allocator<GridCell *> >::~vector<GridCell *,std::allocator<GridCell *> >() Line 706 C++
SADE_ENGINE4.exe!collision_line(float x1, float y1, float x2, float y2, int ObjectId) Line 152 + 0x16 bytes C++

Share this post


Link to post
Share on other sites
I would suspect something is trampling over your stack memory. What does the 'GetObjects' function look like and how does it populate the vector?

ps. The list.empty() check is redundant

Share this post


Link to post
Share on other sites

It has something to do with the cells vector. Hers is the call stack if it helps:
[/quote]
Heap corruption can end up being detected in places removed from the location that is actually doing the corruption.

Bregma's line of inquiry is worth following: why do you need that reinterpret_cast?

Share this post


Link to post
Share on other sites
I managed to get rid of that reinterpret_cast and a lot of others, but a heap corruption is still being detected. Can it be happening somewhere else before it is detected in my collision functions?


bool collision_line( float x1, float y1, float x2, float y2, int ObjectId)
{
Vector2 vec1(x1, y1);
Vector2 vec2(x2, y2);
std::vector<GridCell*> cells;
GridCell *last = NULL;
PMASK mask;
float d = 0;
float length = 0;

length = GetDistance( vec1.x, vec1.y, vec2.x, vec2.y);

if (vec2.x < vec1.x && vec2.y < vec1.y)
{
Vector2 tmp = vec1;
vec1 = vec2;
vec2 = tmp;
}

for (int l = 0; l < length; l += CELL_SIZE)
{
int px = floor(cos(degtorad(d)) * l);
int py = floor(sin(degtorad(d)) * l);

GridCell *cell = objectManager->GetGrid()->CellAt( px, py);
if (cell != NULL && cell != last)
{
cells.push_back( cell);
}
last = cell;
}

init_pmask( &mask, (int)(vec2.x - vec1.x), (int)(vec2.y - vec1.y));
fill_pmask( &mask, 0);

d = floor( point_direction( vec1.x, vec1.y, vec2.x, vec2.y));

for (int x = 0; x < length; ++x)
{
int mx = floor(cos(degtorad(d)) * x);
int my = floor(sin(degtorad(d)) * x);
set_pmask_pixel( &mask, mx, my, 1);
}

for (int n = 0; n < cells.size(); ++n)
{
std::vector<Object*> list;
cells[n]->GetObjects( list);

for ( int i = 0; i < list.size(); ++i)
{
if (list->GetType() != ObjectId){ continue;}

Sprite *spr = renderEngine->_get_sprite(list->mask_index);
Rect r = list->rect;

if (check_pmask_collision( &mask, &spr->mask->mask, floor(vec1.x), floor(vec1.y), floor(list->x), floor(list->y)) == 1)
{
return true;
}
}
}

return false;
}


Heres the GetObjects function. I commented out my old method to use std::copy instead.


void GridCell::GetObjects( std::vector<Object*> &list)
{
/*for (std::vector<Object*>::iterator i = mObjects.begin(); i != mObjects.end(); i++)
{
list.push_back( *i);
}*/

list.resize( mObjects.size());
std::copy( mObjects.begin(), mObjects.end(), list.begin());

}

Share this post


Link to post
Share on other sites

Can it be happening somewhere else before it is detected in my collision functions?
[/quote]
Yes.

Other places to investigate (in the code you've posted) are putting bounds checking assertions in CellAt() and doing bounds checking before calling the various pmask functions (unless this library handles this for you?).


Heres the GetObjects function. I commented out my old method to use std::copy instead.
[/quote]
You could use std::vector::assign here, or just return a vector (the compiler should implement RVO or NRVO).

Share this post


Link to post
Share on other sites
You probably want to add a few calls to assert(_CrtCheckMemory()); to the function. If that doesn't assert the heap is almost certainly not corrupted so you can manually binary search to find where the corruption is happening.

Share this post


Link to post
Share on other sites
After Copying/Pasting "#define return _ASSERT(_CrtCheckMemory()); return" to various .CPPs, The program breaks here:



void GridCell::GetObjects( std::vector<Object*> &list)
{
/*for (std::vector<Object*>::iterator i = mObjects.begin(); i != mObjects.end(); i++)
{
list.push_back( *i);
}

list.resize( mObjects.size());
std::copy( mObjects.begin(), mObjects.end(), list.begin());*/

list.assign( mObjects.begin(), mObjects.end());
return;<-------- Obviously
}


This is in the output window:
Heap entry 0294FFC0 has incorrect PreviousSize field (8000 instead of 000b)

Share this post


Link to post
Share on other sites
So, where does list come from - is it always a valid reference?

I suggest you scatter you heap checks throughout the calling functions. Once you discover the culprit, use a binary search of the function to locate the area that is corrupting the heap.

For example, assuming the failing call is in collision_vector for example, I might throw the checks before and after each loop body. When I identify the problem loop, I'd add the checks to between each line of the loop. Eventually you'll find where the problem is (there could be multiple locations though, so be vigilant!).

Share this post


Link to post
Share on other sites

const std::vector<Object* const> GridCell::GetObjects()
{
std::vector<Object* const> list;
list.assign( mObjects.begin(), mObjects.end());
return list;
}


I rewrote this function, but I'm still getting this error. the mObjects vector is fine and so is all the objects. For some reason list is getting corrupted.

Share this post


Link to post
Share on other sites
Although this may not solve your problem, for the sake of any level of efficiency at all, you should pass an std::vector as a reference to that function and return that same reference.


typedef std::vector<const Object *> ObjectList;




ObjectList & GridCell::GetObjects( ObjectList &list )
{
list.assign( mObjects.begin(), mObjects.end());
return list;
}



L. Spiro

Share this post


Link to post
Share on other sites
I don't see the need for the troublesome copying and/or moving from GridCell members to the output parameter that takes place in the GridCell::GetObjects method .
Can't you return read-only references or pointers to the original collection, or iterators of that collection, or the individual objects themselves? Even without memory corruption problems, these objects are owned by GridCell instances, and they shouldn't go anywhere else without a good reason.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement