API Design: Vertices lifetime

[DavidPeicho]

Hi!

The current version store the vertices slice in the following case:

if (allocatedBVHNodes < spaceNeeded) {
    verts = (bvhvec4*)vertices;
}

This means that it's technically possible to read garbage from a previous verts pointer. While this is a fixable bug, the lifetime of verts could be a bit improved since it's not directly clear from just the doc and the method declaration, that the vertices slice is kept around.

Pass Around Vertex Slice

Ideally, every function would just read the slice once, and not store it:

void BVH::Build(const bvhvec4* vertices, const unsigned primCount);
void BVH::Refit(const bvhvec4* vertices, const BVHLayout layout, const unsigned nodeIdx);
...

Makes it clear that vertices isn't bound to the bvh lifetime. It doesn't prevent user errors obviously:

bvh.Build(triangles, 36);
bvh.Refit(otherTriangles, 36); // Oops

This will be the case with the current solution as well.

SetVertices

Another solution could be to explicitly ask for the vertices?

void BVH::SetVertices(const bvhvec4* vertices, const unsigned primCount);

void BVH::Build() {
    FATAL_ERROR_IF(!verts, "BVH::Build(), no vertices (use SetVertices(vertices))" );
}

Let me know what you think, happy to contribute based on your preferences

[jbikker]

The current approach is indeed not as safe as can be. An intermediate solution would be to move the verts assignment outside the if statement, so that the pointer is refreshed even when there is enough space. The solution of sending verts to each function, as you propose, is much safer still - but that also means that Intersect and IsOccluded needs to receive this data, from a point in user code where it may be quite inconvenient to obtain a verts pointer, which may perhaps lead to a different but similar copy of the verts pointer. Another solution would be to actually copy the data pointed to by verts. Performance-wise this is not a huge issue - although it is of course wasteful, also in terms of memory use. Or, we could have additional checks on the verts pointer and the data it points to but only in debug mode. Perhaps with a CRC32 over the data?