senoutouya / recastnavigation

Automatically exported from code.google.com/p/recastnavigation
zlib License
0 stars 0 forks source link

rcCreateHeightfield does not initialise "pools" and "freelist" #116

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
When you create a new rcHeightfield and pass it to "rcCreateHeightfield" for 
initialisation, it leaves the "pools" and "freelist" members uninitialised. 
This causes a crash in later functions (i.e. "allocSpan" crashes when checking 
for NULL freelist)

Original issue reported on code.google.com by armstron...@gmail.com on 24 Aug 2010 at 11:35

GoogleCodeExporter commented 9 years ago
Also, seems that "rcBuildCompactHeightfield" function should initialise the 
"dist" member to NULL in rcCompactHeightfield

Original comment by armstron...@gmail.com on 24 Aug 2010 at 11:40

GoogleCodeExporter commented 9 years ago
rcAllocHeightfield() does the initialization.

Original comment by memono...@gmail.com on 24 Aug 2010 at 1:26

GoogleCodeExporter commented 9 years ago
Ok - thanks. As a client I could, if I wanted, declare the rcHeightfield as a 
data member of another class:

class MyRecastClass
{
    recast::rcHeightfield m_heightfield;
};

...in this case the data will not be initialised properly. It might be a good 
idea to initialise the data in the "rcCreateHeightfield" function. Or add a 
ctor (but I know what you'll say to that! :)

Original comment by armstron...@gmail.com on 24 Aug 2010 at 1:59

GoogleCodeExporter commented 9 years ago
Yes, I say no. There needs to be connection between rcAlloc and rcFree. Recast 
allocates lotsa stuff in the generation functions which gets stored in 
rcHeightfield  & co.

I'm a broken records and say it again, C++ memory custom management is so 
utterly fucked up :)

So there has the be some logic to use rcFree to free it too. I hope you will 
like it more if/when build context includes alloc too.

Recast stuff is allocated and freed using the designated functions, the 
constructors and destructor will be removed (they were left ther by mistake). 
You should store pointer instead of struct.

Original comment by memono...@gmail.com on 24 Aug 2010 at 2:08

GoogleCodeExporter commented 9 years ago
Fair enough.

>>You should store pointer instead of struct.
You could, if you wanted to reinforce these ideas, make the struct's ctor/dtor 
private to stop people allocating these structs on the heap. That way they'd be 
forced to go via the rcAllocXXX and rcFreeXXX functions. This should make it 
clear to the client the requirement of the system.

Original comment by armstron...@gmail.com on 24 Aug 2010 at 2:14

GoogleCodeExporter commented 9 years ago
I'll keep it as is for the time being. As a lame attempt to make it a hint 
better, I moved the alloc/free function defs closer to the structs.

Original comment by memono...@gmail.com on 24 Aug 2010 at 6:05