mkazhdan / PoissonRecon

Poisson Surface Reconstruction
MIT License
1.58k stars 426 forks source link

Cannot Run Code in Loop #36

Open andrewcomer opened 7 years ago

andrewcomer commented 7 years ago

I would like to be able to run the Poisson code in a loop over multiple meshes, but running more than once in the same process causes the program to crash.

Specifically, line 204 of Octree.inl fails because the variable _depthAndOffset is not set. inline int OctNode< NodeData >::depth( void ) const {return int( _depthAndOffset & DepthMask );}

What needs to be modified so that the code can be run in a loop?

markloyman commented 7 years ago

This problem was addressed (with a few possible solutions) at issue #22.

andrewcomer commented 7 years ago

Apologies, did not see that when searching the issues.

For the sake of completeness: Replacing the set() method in Allocator.h with the following allows for the code to be run more than once:

void set( int blockSize )
{
    if (this->blockSize != blockSize)
    {
        reset();
        this->blockSize = blockSize;
        index = -1;
        remains = 0;
    }
}

Is there any reason this has not been added to the code outright?

mkazhdan commented 7 years ago

While this could work, I am hesitant to incorporate the fix within the code because it doesn't reallocate the memory. (Nor would it give correct behavior if the second pass used a different memory block size.)

andrewcomer commented 7 years ago

So is moving OctNode< TreeNodeData >::SetAllocator(MEMORY_ALLOCATOR_BLOCK_SIZE); above Octree< Real > tree; a more viable solution since it does reallocate the memory?

markloyman commented 7 years ago

Just tested it (moving SetAllocator()). This solution works for me as well, and is indeed better than my previous suggestion.

But I think a more explicit protection should be incorporated in the code (in addition to the above).

Maybe at Octree.inl add a check to see if memory was already allocated:

template< class NodeData >
void OctNode< NodeData >::SetAllocator(int blockSize)
{
    if( (blockSize>0) && (false==MemoryAlreadyAllocated))
    {
        UseAlloc=1;
        MemoryAlreadyAllocated = true;
        NodeAllocator.set(blockSize);
    }
    else{UseAlloc=0;}
}

I guess UseAlloc could've been used for this, but I think adding a new variable for this is cleaner. This is what I currently plan to do in my code.