mmp / pbrt-v2

Source code for the version of pbrt described in the second edition of "Physically Based Rendering"
http://pbrt.org
990 stars 343 forks source link

fix the uint8 overflow bug in recursiveBuild function #33

Closed ghost closed 10 years ago

ghost commented 10 years ago

bug description: LinearBVHNode uses nPrimitives(type: uint8_t) to store the number of elements in the leaf node, and assume nPrimitives == 0 indicates it is an interior node. However, in recursiveBuild function we use nPrimitives(type: uint32_t) and we don't check nPrimitives <= maxPrimsInNode when we try to build a leaf node. If, for example, exactly 256(or 512, 768, 1024...) primitives happen to have the same BBox center, they will be put in a single leaf node. When we flatten the BVHTree, due to the overflow issue, nPrimitives in LinearBVHNode for this leaves will be assigned 0, and they will be mistakenly treated as interior nodes.

How to fix it: in recursiveBuild function we build a leaf node in three cases: case 1: nPrimitive == 1 case 2: all the primitives have exactly the same BBox center case 3: nPrimitives <= maxPrimsInNode && minCost >= nPrimitives in SAH(line 355- 364) case 1 and 3 won't lead to overflow problem because it checks nPrimitives <= maxPrimsInNode(which is no greater than 255u). case 2 could be problematic, so we fix it by adding the condition nPrimitives <= maxPrimsInNode. If it is true, we use the previous code; otherwise, we split the primitives into two halves.

mmp commented 10 years ago

Thanks!!